feat(server): error feedback channels and structured logging - #456
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds anomaly and guidance reporting, safe offset mapping, compile-command provenance tracking, config issue diagnostics, structured LSP errors, and broader test coverage for anomalies and closed-document behavior. ChangesServer diagnostics and anomaly handling
Sequence Diagram(s)None Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/server/compiler/indexer.cpp (1)
821-834:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the fallback-source gate ahead of the module prebuild.
Lines 823-825 still run
workspace.compile_graph->compile(server_path_id)before the newCommandSource::Fallbackcheck. For module interface units with no real compile command, background indexing now skips the final worker request on Line 833, but it still pays for the PCM build first. That undermines the new “skip fallback-sourced files” rule on the heaviest path.Suggested reorder
- // For module interface units, compile their PCM (and transitive deps) - // first so the stateless worker has the artifacts it needs. - if(workspace.compile_graph && workspace.path_to_module.contains(server_path_id)) { - co_await workspace.compile_graph->compile(server_path_id); - } - worker::BuildParams params; params.kind = worker::BuildKind::Index; params.file = file_path; - // Bulk background indexing sticks to real commands; synthesized fallback - // commands would fill the index with guesses. - if(compiler.fill_compile_args(file_path, params.directory, params.arguments, nullptr) == - CommandSource::Fallback) + auto source = + compiler.fill_compile_args(file_path, params.directory, params.arguments, nullptr); + // Bulk background indexing sticks to real commands; synthesized fallback + // commands would fill the index with guesses. + if(source == CommandSource::Fallback) co_return; + + // For module interface units, compile their PCM (and transitive deps) + // first so the stateless worker has the artifacts it needs. + if(workspace.compile_graph && workspace.path_to_module.contains(server_path_id)) { + co_await workspace.compile_graph->compile(server_path_id); + }🤖 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/indexer.cpp` around lines 821 - 834, The fallback-source gate should run before triggering module prebuild to avoid building PCMs for files that will be skipped; move the call that checks compiler.fill_compile_args(file_path, params.directory, params.arguments, nullptr) against CommandSource::Fallback to precede the workspace.compile_graph->compile(server_path_id) call so that if the command source is Fallback you co_return early and do not invoke workspace.compile_graph->compile(server_path_id); keep the existing worker::BuildParams setup (params.kind, params.file) but only create/use them after the fallback check or ensure the fallback check uses file_path/server_path_id as it does now.src/server/service/agent_client.cpp (1)
195-206:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t drop compile-command provenance at the agent boundary.
This handler now always succeeds, but it discards the
CommandSourcereturned byfill_compile_args(...). That means agentic callers cannot distinguish an exact CDB command from an inferred/fallback one anymore, even though this PR adds provenance specifically to track that difference. If the old error-on-miss behavior is intentionally gone,CompileCommandResultneeds to carry the source; otherwise keep rejectingFallbackhere.🤖 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/service/agent_client.cpp` around lines 195 - 206, The handler passed to peer.on_request currently calls srv.compiler.fill_compile_args(params.path, directory, arguments) and drops the returned CommandSource, so CompileCommandResult loses provenance; update the handler to capture the CommandSource from fill_compile_args and include it in the response (e.g., add a source field to CompileCommandResult and populate it from the returned CommandSource), or if the original behavior should still reject inferred commands, check the returned CommandSource and return a failure for CommandSource::Fallback instead of always succeeding; adjust the code paths around CompileCommandResult, CompileCommandParams, and srv.compiler.fill_compile_args accordingly.src/server/worker/stateless_worker.cpp (1)
218-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
has_user_errorsfor index compile failures too.
handle_build_pch()andhandle_build_pcm()now distinguish user diagnostics from infrastructure failures, buthandle_index()still returns a bare failure result. That leaveshas_user_errorsfalse for normal source-level index failures, so broken user code can be reported downstream as an anomaly instead of a regular compile problem.Suggested fix
static worker::BuildResult handle_index(const worker::BuildParams& params) { ScopedTimer timer; @@ auto unit = compile(cp); if(!unit.completed()) { - LOG_WARN("Index failed: file={}, {}ms", params.file, timer.ms()); - return {false, "Index compilation failed"}; + auto errors = collect_errors(unit); + LOG_WARN("Index failed: file={}, {}ms, errors=[{}]", params.file, timer.ms(), errors); + worker::BuildResult result; + result.success = false; + result.error = errors.empty() ? "Index compilation failed" : errors; + result.has_user_errors = !errors.empty(); + return result; }🤖 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/worker/stateless_worker.cpp` around lines 218 - 221, handle_index currently returns a plain failure when compile(cp) yields !unit.completed(), which loses the user-vs-infrastructure distinction; update the failure path to propagate the unit's user-error flag (as done in handle_build_pch()/handle_build_pcm()) by checking unit.has_user_errors() (or the equivalent user-diagnostics accessor) and returning a result that sets has_user_errors accordingly instead of always leaving it false, and log/include that flag in the returned failure from handle_index and in the LOG_WARN message to mirror the other handlers.
🧹 Nitpick comments (1)
src/server/compiler/compiler.cpp (1)
99-99: 💤 Low valueDiscarded
CommandSourcereturn value.
fill_compile_argsnow returns aCommandSourceto indicate how the compile command was resolved, but this call site ignores the return value. While the PCM build path may not need to inject guidance diagnostics (PCMs come from module sources that likely have CDB entries), silently discarding the result loses potentially useful provenance information for debugging or future use.Consider capturing the return value for logging consistency with other call sites, or add a comment explaining why it's intentionally ignored here.
🤖 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` at line 99, The call to fill_compile_args(file_path, bp.directory, bp.arguments) discards its CommandSource return value; capture and use that return (e.g., auto src = fill_compile_args(...)) or explicitly document why it's ignored to preserve provenance; update the call site in compiler.cpp to store the returned CommandSource and either log it (consistent with other sites) or add a clear comment referencing fill_compile_args and CommandSource explaining why the value is intentionally unused.
🤖 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/feature/feature.h`:
- Around line 29-35: The to_position function currently returns a default (0,0)
on mapping failure which breaks downstream unsigned delta math; change its
contract to return std::optional<protocol::Position> (i.e., inline auto
to_position(...) -> std::optional<protocol::Position>), log the anomaly via
LOG_ANOMALY(PositionMapFail, ...) and return std::nullopt on failure instead of
protocol::Position{.line=0,.character=0}; then update callers (notably the
consumer in semantic_tokens.cpp around the relative encoder) to check the
optional, drop or skip the affected artifact when mapping fails, or otherwise
handle the error path so no unsigned underflow occurs.
In `@src/server/workspace/config.cpp`:
- Around line 207-210: The function Config::load_from_workspace should clear the
out-parameter loaded_path on entry to satisfy the header contract that it “stays
empty” when no config is found; update the start of Config::load_from_workspace
to check if loaded_path is non-null and call loaded_path->clear() so any
previous value is removed before the function searches for candidates and only
set when a candidate is found.
In `@src/support/anomaly.cpp`:
- Around line 17-19: notify_hook and testing_trap are accessed concurrently
(written by set_notify_hook(), set_anomaly_trap_for_testing(),
reset_anomaly_for_testing() and read by report_anomaly()/report_guidance()),
causing a data race; protect these process-wide hooks with a mutex: add a
std::mutex (or std::shared_mutex) guarding the hook variables, acquire the lock
when mutating them in
set_notify_hook()/set_anomaly_trap_for_testing()/reset_anomaly_for_testing(),
and in report_anomaly()/report_guidance() take the lock, copy the std::function
into a local variable, release the lock, then invoke the copied callable outside
the lock to avoid holding the mutex during callback execution.
In `@tests/integration/compilation/test_staleness.py`:
- Around line 343-346: The test currently awaits
client.text_document_hover_async(...) inside pytest.raises without a timeout and
can hang; wrap the awaited call in asyncio.wait_for(...) with a short timeout
(e.g., 1s) so the assertion fails fast if no response is returned. Update the
block around client.text_document_hover_async(HoverParams(...)) to await
asyncio.wait_for(client.text_document_hover_async(...), timeout=1) and add an
asyncio import if not present.
In `@tests/replay.py`:
- Around line 150-163: The reader_loop currently only captures
"window/logMessage" entries and misses worker-only anomalies written to the
workspace logs; update the replay flow to, when display_ws resolves to a
workspace path, perform the same scan of the workspace ".clice/logs" files
(reuse the existing log-file scan logic) before making the final PASS/FAIL
decision so worker-side anomalies are collected into the anomalies list;
specifically, after or alongside reader_loop (and in the analogous spot
referenced by the other occurrence), call the log-file scanner and append any
found anomalies to the anomalies list so they are considered when deciding
PASS/FAIL.
In `@tests/unit/support/anomaly_tests.cpp`:
- Around line 14-32: The destructor of AnomalyCapture currently unconditionally
clears globals instead of restoring previous state; update the constructor to
capture the existing notify hook and anomaly trap (via logging::set_notify_hook
and logging::set_anomaly_trap_for_testing accessors) into members (e.g.,
prev_notify_hook, prev_anomaly_trap) and then in ~AnomalyCapture restore them
(call logging::set_notify_hook(prev_notify_hook) and
logging::set_anomaly_trap_for_testing(prev_anomaly_trap)) while still restoring
logging::options.level and calling logging::reset_anomaly_for_testing as
appropriate; alternatively, if restoring previous callbacks is impossible,
change the struct comment and name to clearly document that it overwrites global
hooks for the test duration.
In `@tests/unit/test/tester.h`:
- Around line 100-103: The helper to_local_range currently dereferences unit and
the results of converter.to_offset(range.start/end) unconditionally; change it
to fail tests explicitly instead of crashing by asserting unit is non-null and
that converter.to_offset(range.start) and converter.to_offset(range.end) return
values before dereferencing (e.g., use test assertions like
ASSERT_TRUE/EXPECT_TRUE or return std::optional<LocalSourceRange>), then
construct and return LocalSourceRange from the validated offsets; reference the
to_local_range function, the unit pointer, and
converter.to_offset(range.start)/converter.to_offset(range.end) locations when
making the checks.
---
Outside diff comments:
In `@src/server/compiler/indexer.cpp`:
- Around line 821-834: The fallback-source gate should run before triggering
module prebuild to avoid building PCMs for files that will be skipped; move the
call that checks compiler.fill_compile_args(file_path, params.directory,
params.arguments, nullptr) against CommandSource::Fallback to precede the
workspace.compile_graph->compile(server_path_id) call so that if the command
source is Fallback you co_return early and do not invoke
workspace.compile_graph->compile(server_path_id); keep the existing
worker::BuildParams setup (params.kind, params.file) but only create/use them
after the fallback check or ensure the fallback check uses
file_path/server_path_id as it does now.
In `@src/server/service/agent_client.cpp`:
- Around line 195-206: The handler passed to peer.on_request currently calls
srv.compiler.fill_compile_args(params.path, directory, arguments) and drops the
returned CommandSource, so CompileCommandResult loses provenance; update the
handler to capture the CommandSource from fill_compile_args and include it in
the response (e.g., add a source field to CompileCommandResult and populate it
from the returned CommandSource), or if the original behavior should still
reject inferred commands, check the returned CommandSource and return a failure
for CommandSource::Fallback instead of always succeeding; adjust the code paths
around CompileCommandResult, CompileCommandParams, and
srv.compiler.fill_compile_args accordingly.
In `@src/server/worker/stateless_worker.cpp`:
- Around line 218-221: handle_index currently returns a plain failure when
compile(cp) yields !unit.completed(), which loses the user-vs-infrastructure
distinction; update the failure path to propagate the unit's user-error flag (as
done in handle_build_pch()/handle_build_pcm()) by checking
unit.has_user_errors() (or the equivalent user-diagnostics accessor) and
returning a result that sets has_user_errors accordingly instead of always
leaving it false, and log/include that flag in the returned failure from
handle_index and in the LOG_WARN message to mirror the other handlers.
---
Nitpick comments:
In `@src/server/compiler/compiler.cpp`:
- Line 99: The call to fill_compile_args(file_path, bp.directory, bp.arguments)
discards its CommandSource return value; capture and use that return (e.g., auto
src = fill_compile_args(...)) or explicitly document why it's ignored to
preserve provenance; update the call site in compiler.cpp to store the returned
CommandSource and either log it (consistent with other sites) or add a clear
comment referencing fill_compile_args and CommandSource explaining why the value
is intentionally unused.
🪄 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: 017bb9e9-2234-408b-a1dc-fde1bdc1e898
📒 Files selected for processing (47)
cmake/package.cmakesrc/compile/diagnostic.cppsrc/feature/code_completion.cppsrc/feature/diagnostics.cppsrc/feature/feature.hsrc/feature/folding_ranges.cppsrc/feature/formatting.cppsrc/feature/inlay_hints.cppsrc/feature/semantic_tokens.cppsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/compiler/indexer.cppsrc/server/protocol/worker.hsrc/server/service/agent_client.cppsrc/server/service/lsp_client.cppsrc/server/service/lsp_client.hsrc/server/service/master_server.cppsrc/server/service/master_server.hsrc/server/worker/stateless_worker.cppsrc/server/worker/worker_pool.cppsrc/server/workspace/config.cppsrc/server/workspace/config.hsrc/support/anomaly.cppsrc/support/anomaly.htests/conftest.pytests/integration/agentic/test_agentic.pytests/integration/agentic/test_cli.pytests/integration/compilation/test_persistent_cache.pytests/integration/compilation/test_staleness.pytests/integration/features/test_guidance_diagnostics.pytests/integration/features/test_index.pytests/integration/features/test_server.pytests/integration/lifecycle/test_anomaly.pytests/integration/lifecycle/test_config.pytests/integration/lifecycle/test_file_operation.pytests/integration/modules/test_modules.pytests/integration/stress/test_rapid_edit.pytests/integration/utils/assertions.pytests/integration/utils/client.pytests/integration/utils/wait.pytests/pytest.initests/replay.pytests/unit/feature/document_link_tests.cpptests/unit/feature/folding_range_tests.cpptests/unit/server/config_tests.cpptests/unit/support/anomaly_tests.cpptests/unit/test/tester.h
💤 Files with no reviewable changes (1)
- tests/unit/feature/document_link_tests.cpp
16bit-ykiko
force-pushed
the
feat/server-feedback
branch
from
June 29, 2026 13:46
340f9dd to
c41956d
Compare
LOG_ANOMALY(id, ...) is a soft assertion for internal invariants: Debug builds abort after logging (CLICE_ANOMALY_NO_TRAP escapes for tests), Release builds log with a stable [anomaly:<id>] marker, forward it to a notify hook (master: window/logMessage) and continue. Reports are rate-limited per ID; the gate runs before format arguments are evaluated, so suppressed reports cost nothing. LOG_GUIDANCE(...) is the user-actionable channel ([guidance] marker, Warning logMessage) for situations that are not clice bugs. Unit tests lock the lazy-evaluation contract, rate limiting, markers and the trap mock point.
Reimplements the intent of #436 on current main (superseding it): - fill_compile_args() reports a CommandSource (CdbExact / IncludeGraph / Inferred / Fallback) and emits a per-file decision log (tiers tried, tier hit, command hash). Real CDB entries are now distinguished from the synthesized default command via has_entry(), which also revives the automatic include-graph header context tier that the synthesis had shadowed. - Guessed commands whose diagnostics contain file-not-found errors get a file-top Warning guidance diagnostic (source: clice, code links the setup guide, resolving the DiagnosticSource::Clice TODO). Exact CDB matches never get it. - clice.toml problems become diagnostics on the config file's URI with line/column from the TOML decoder: decode failures as Error (defaults apply), unknown keys as Warning via a strict second decode pass. initializationOptions now overlay BEFORE apply_defaults so derived fields (logging_dir, index_dir) follow overridden values. - Feature requests on closed documents and unresolvable hierarchy items return LSP errors instead of silent null; client-provided positions and ranges that fail to map return InvalidParams. Empty hierarchy and workspace-symbol results return empty arrays instead of null. - Internal failures report anomalies: unexpected PCH/PCM build failures (user-code errors are distinguished via BuildResult.has_user_errors), compile/query IPC failures, worker crash/spawn failures, and internally-produced offsets failing to map (feature::to_position now clamps instead of dereferencing an empty optional). - Master pushes anomaly/guidance to the client via window/logMessage; workers only log (their files are scanned by tests).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c41956dc99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- CliceClient records window/logMessage; assert_no_anomaly() runs in every client/agentic fixture teardown, scanning both notifications and master/worker log files (opt-out: @pytest.mark.allow_anomaly). - replay.py fails a trace when an [anomaly:] message is pushed. - E2E: fallback-command guidance diagnostic lifecycle (appears without CDB, gone after providing one), clice.toml type/unknown-key diagnostics with line/column and clearing after a fix, worker-crash anomaly reporting (Linux), error responses for requests on closed or never-opened documents. - Named timing constants (MTIME_GRANULARITY/SETTLE_TIME/IDLE_TIMEOUT) replace hardcoded sleeps; workspace fixture cleans up after tests; log messages are dumped when a test fails. - Tester::to_local_range() replaces per-file duplicates.
Pre-PR review findings: - publish_config_diagnostics groups issues by their own file URI; a malformed clice.toml no longer renders its error on the next config candidate that loaded. - A failed switchContext header context now falls through to the real CDB entry before reaching the synthesized fallback. - Header-context host selection filters hosts through has_entry(); lookup() never returns empty (it synthesizes), so the old emptiness checks were dead and hosts without real entries were mislabeled as IncludeGraph. - Style: string_view message parameters, llvm::join, doc comments.
- All seven AnomalyIds fire through the macro once (marker names are wire-stable for the integration greps). - InvalidParams for an out-of-range hover position; error response for an unresolvable call hierarchy item. - Column assertions for config issue locations. - assert_no_anomaly in the agentic local fixtures and make_client-based tests that bypass the client fixture teardown.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/server/compiler/indexer.cpp (1)
935-948: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck the command source before the module prebuild step.
index_one()only returns onCommandSource::Fallbackaftercompile_graph->compile(server_path_id). That path callsfill_compile_args()again inCompiler::init_compile_graph()and can still build/cache a PCM from synthesized args, so background indexing still leaves guessed artifacts behind for files this PR intended to skip.Suggested reorder
- // For module interface units, compile their PCM (and transitive deps) - // first so the stateless worker has the artifacts it needs. - if(workspace.compile_graph && workspace.path_to_module.contains(server_path_id)) { - co_await workspace.compile_graph->compile(server_path_id); - } - worker::BuildParams params; params.kind = worker::BuildKind::Index; params.file = file_path; - // Bulk background indexing sticks to real commands; synthesized fallback - // commands would fill the index with guesses. - if(compiler.fill_compile_args(file_path, params.directory, params.arguments, nullptr) == - CommandSource::Fallback) + auto source = + compiler.fill_compile_args(file_path, params.directory, params.arguments, nullptr); + if(source == CommandSource::Fallback) co_return; + + // For module interface units, compile their PCM (and transitive deps) + // first so the stateless worker has the artifacts it needs. + if(workspace.compile_graph && workspace.path_to_module.contains(server_path_id)) { + co_await workspace.compile_graph->compile(server_path_id); + }🤖 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/indexer.cpp` around lines 935 - 948, Move the CommandSource check in index_one() ahead of the module prebuild step so synthesized commands are rejected before any PCM work starts. Specifically, call Compiler::fill_compile_args() and return early on CommandSource::Fallback before invoking workspace.compile_graph->compile(server_path_id), since compile_graph/init_compile_graph can otherwise still cache artifacts from guessed args. Keep the existing module-interface handling in index_one() and preserve the current worker::BuildParams setup after the early-exit check.src/server/compiler/compiler.cpp (2)
581-596: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winVerify the cached PCH file still exists before reusing it.
The new reuse predicate no longer checks
st.pathon disk. If.clice/cache/pchis cleaned or the file is evicted manually, we skip rebuilding and hand the worker a dead path instead.Suggested fix
if(auto it = workspace.pch_cache.find(path_id); it != workspace.pch_cache.end()) { auto& st = it->second; if(st.hash == preamble_hash && !st.path.empty() && + llvm::sys::fs::exists(st.path) && !deps_changed(workspace.path_pool, st.deps)) { st.bound = bound; session.pch_ref = Session::PCHRef{path_id, preamble_hash, bound}; co_return true; } @@ - co_return workspace.pch_cache.count(path_id) && !workspace.pch_cache[path_id].path.empty(); + co_return workspace.pch_cache.count(path_id) && + !workspace.pch_cache[path_id].path.empty() && + llvm::sys::fs::exists(workspace.pch_cache[path_id].path);🤖 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 581 - 596, The PCH reuse logic in compiler.cpp is still trusting cached entries in workspace.pch_cache without verifying the cached file exists on disk. Update the reuse path in the cache lookup block and the preamble-incomplete fallback to check st.path (or the cached path from workspace.pch_cache[path_id]) still exists before reusing it, and only set session.pch_ref or return true when the file is present; otherwise force a rebuild.
567-579: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftHash the preprocessing context into the PCH key.
This path now shares PCHs by preamble text alone, but
fill_compile_args()/ header-context resolution can change-D/-I/-isystem/-stdand even the working directory without changing those bytes. That lets incompatible compile commands reuse the same.pch, which will surface as wrong diagnostics/completions or sporadic build failures.🤖 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 567 - 579, Update the PCH cache key in compiler.cpp so it includes the preprocessing context, not just the preamble text. In the PCH path generation around preamble_hash and pch_path, fold in the compile arguments produced by fill_compile_args() and any header-context inputs that affect preprocessing such as -D, -I, -isystem, -std, and the working directory, so incompatible commands do not reuse the same .pch. Keep the existing location logic in compiler::... around pch_path, but change the hashing inputs used to derive the deterministic cache filename.src/server/service/lsp_client.cpp (3)
397-412: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck the open session before serving definition results.
Line 402 queries the index before Line 410 checks
find_session, so closed documents can still receive definition results instead of the newDocument not openerror.🐛 Proposed fix
auto& uri = params.text_document_position_params.text_document.uri; auto& pos = params.text_document_position_params.position; - auto result = query_at(uri, pos, RelationKind::Definition); - if(!result.empty()) { - co_return to_raw(result); - } - auto& srv = this->server; auto path = uri_to_path(uri); auto path_id = srv.workspace.path_pool.intern(path); - auto* session = srv.find_session(path_id); + auto session = srv.find_session(path_id); if(!session) co_return kota::outcome_error(document_not_open()); + + auto result = query_at(uri, pos, RelationKind::Definition); + if(!result.empty()) { + co_return to_raw(result); + } co_return co_await srv.compiler.forward_query(worker::QueryKind::GoToDefinition, *session, pos);🤖 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/service/lsp_client.cpp` around lines 397 - 412, The Definition request handler in peer.on_request currently serves query_at results before verifying the document session, so closed files can still return definition results. Update the lambda to check srv.find_session(path_id) and return document_not_open() before calling query_at, keeping the existing uri_to_path, path_pool.intern, and query_at flow intact for open sessions only.
216-222: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t silently accept unmappable incremental edits.
If either offset mapping fails, the text replacement is skipped but Line 227 still advances generation and notifies workers with stale text. Fail closed here, log the bad range, and avoid publishing work from a divergent buffer.
🤖 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/service/lsp_client.cpp` around lines 216 - 222, The incremental edit handling in lsp_client::apply_change currently skips replacements when PositionMapper::to_offset fails, but still lets the buffer generation advance and workers get notified with stale text. Change this path to fail closed in the same block by detecting unmappable ranges, logging the bad range details, and preventing the subsequent generation update / worker publish when start or end cannot be mapped or the range is invalid.
185-188: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winKeep
open_sessionaccess aligned with its return type.MasterServer::open_session()returnsstd::shared_ptr<Session>, soauto& session = srv.open_session(path_id);makessession.version/session.textinvalid. Use ashared_ptrand->, or change the API ifSession&is intended.🤖 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/service/lsp_client.cpp` around lines 185 - 188, `MasterServer::open_session()` returns a `std::shared_ptr<Session>`, so the `handle`/document update block in `lsp_client.cpp` is binding the result incorrectly with `auto& session`. Update the code at the `open_session(path_id)` call site to store the returned shared pointer in an `auto` variable and access `Session` members through `->`, or change `open_session()` to return `Session&` if that is the intended API, so `session.version`, `session.text`, and `session.generation` are valid.src/server/worker/worker_pool.cpp (1)
154-162: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard stopped/dead workers during shutdown.
respawn_worker()can moveworkers[index].peerout, then returnfalseon spawn failure.stop()now unconditionally dereferences everyw.peer, so a later shutdown can crash.🐛 Proposed fix
- for(auto& w: stateless_workers) - w.peer->close_output(); - for(auto& w: stateful_workers) - w.peer->close_output(); + for(auto& w: stateless_workers) + if(w.peer) + w.peer->close_output(); + for(auto& w: stateful_workers) + if(w.peer) + w.peer->close_output(); - for(auto& w: stateless_workers) - w.proc.kill(SIGTERM); - for(auto& w: stateful_workers) - w.proc.kill(SIGTERM); + for(auto& w: stateless_workers) + if(w.alive) + w.proc.kill(SIGTERM); + for(auto& w: stateful_workers) + if(w.alive) + w.proc.kill(SIGTERM);Also applies to: 287-318
🤖 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/worker/worker_pool.cpp` around lines 154 - 162, The shutdown path in stop() unconditionally dereferences each worker’s peer, but respawn_worker() can leave workers[index].peer empty after moving it out and then failing to spawn, which can crash during later shutdown. Update stop() to guard the stateless_workers and stateful_workers entries before calling peer->close_output(), and similarly make sure any other shutdown handling in respawn_worker() / related worker cleanup paths safely skips dead or moved-out peers so stopped workers are not dereferenced.
🧹 Nitpick comments (1)
tests/conftest.py (1)
309-313: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReuse
CliceClient.stop_io()here.That helper already centralizes
_stop_event/_async_tasksteardown and waits for the cancellations to settle. Sleeping for 100ms instead can leave client tasks alive past fixture teardown and make async failures flaky.Suggested cleanup
finally: try: - c._stop_event.set() - for task in c._async_tasks: - task.cancel() - await asyncio.sleep(0.1) + await c.stop_io() except Exception: pass🤖 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 `@tests/conftest.py` around lines 309 - 313, The fixture teardown is duplicating `CliceClient.stop_io()` by manually setting `_stop_event`, cancelling `_async_tasks`, and sleeping, which can leave tasks running and make tests flaky. Update the cleanup path to call `CliceClient.stop_io()` directly instead of reimplementing the shutdown sequence here, so the async teardown logic stays centralized and waits for cancellations to settle consistently.
🤖 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/compiler/indexer.cpp`:
- Around line 164-199: Abort the entire snapshot if any shard serialization or
commit fails, instead of silently skipping bad shards or continuing after a
shard commit error. In the shard collection loop in indexer.cpp, make
serialize_blob/shard.serialize failures cause the snapshot to be discarded; then
in the shard commit loop, treat any store.commit failure as fatal, abort any
already-pending shard blobs, and do not leave a published ProjectIndex with
incomplete shards. Update the logic around workspace.merged_indices,
serialize_blob, store.commit, and store.abort so the snapshot is only considered
successful when every shard is saved and committed.
- Around line 387-398: The session-wide scans are dereferencing optional state
without checking it first. Update the affected walks in the cursor/symbol lookup
paths to skip sessions where `session.file_index` or `session.symbols` are not
present, following the existing optional handling used by `resolve_cursor()` and
`find_symbol_info()`. Apply the same guard pattern in the other listed scan
sites so requests like definition/reference/hierarchy/workspace-symbol do not
access uncompiled or failed sessions.
In `@src/server/service/master_server.cpp`:
- Around line 124-136: Keep the session helper API consistent with its callers:
`MasterServer::find_session` and `MasterServer::open_session` now return
`std::shared_ptr<Session>`, but `lsp_client.cpp` still expects
pointer/reference-style access, so the build will break. Either revert these
helpers to the old `Session*`/reference contract or update all call sites to use
`shared_ptr` semantics, and make sure the declaration in `MasterServer`’s header
matches the implementation.
In `@src/server/worker/worker_pool.cpp`:
- Around line 81-87: Worker spawn failures in worker_pool should not be logged
as anomalies because those paths are recoverable and can abort Debug builds
before returning false. Update the failure handling in the spawn logic around
kota::process::spawn in the worker pool code to use LOG_ERROR (or equivalent
non-fatal logging) instead of LOG_ANOMALY, and apply the same change to the
matching failure path later in the same file so both stateful and stateless
worker spawn errors are treated consistently.
---
Outside diff comments:
In `@src/server/compiler/compiler.cpp`:
- Around line 581-596: The PCH reuse logic in compiler.cpp is still trusting
cached entries in workspace.pch_cache without verifying the cached file exists
on disk. Update the reuse path in the cache lookup block and the
preamble-incomplete fallback to check st.path (or the cached path from
workspace.pch_cache[path_id]) still exists before reusing it, and only set
session.pch_ref or return true when the file is present; otherwise force a
rebuild.
- Around line 567-579: Update the PCH cache key in compiler.cpp so it includes
the preprocessing context, not just the preamble text. In the PCH path
generation around preamble_hash and pch_path, fold in the compile arguments
produced by fill_compile_args() and any header-context inputs that affect
preprocessing such as -D, -I, -isystem, -std, and the working directory, so
incompatible commands do not reuse the same .pch. Keep the existing location
logic in compiler::... around pch_path, but change the hashing inputs used to
derive the deterministic cache filename.
In `@src/server/compiler/indexer.cpp`:
- Around line 935-948: Move the CommandSource check in index_one() ahead of the
module prebuild step so synthesized commands are rejected before any PCM work
starts. Specifically, call Compiler::fill_compile_args() and return early on
CommandSource::Fallback before invoking
workspace.compile_graph->compile(server_path_id), since
compile_graph/init_compile_graph can otherwise still cache artifacts from
guessed args. Keep the existing module-interface handling in index_one() and
preserve the current worker::BuildParams setup after the early-exit check.
In `@src/server/service/lsp_client.cpp`:
- Around line 397-412: The Definition request handler in peer.on_request
currently serves query_at results before verifying the document session, so
closed files can still return definition results. Update the lambda to check
srv.find_session(path_id) and return document_not_open() before calling
query_at, keeping the existing uri_to_path, path_pool.intern, and query_at flow
intact for open sessions only.
- Around line 216-222: The incremental edit handling in lsp_client::apply_change
currently skips replacements when PositionMapper::to_offset fails, but still
lets the buffer generation advance and workers get notified with stale text.
Change this path to fail closed in the same block by detecting unmappable
ranges, logging the bad range details, and preventing the subsequent generation
update / worker publish when start or end cannot be mapped or the range is
invalid.
- Around line 185-188: `MasterServer::open_session()` returns a
`std::shared_ptr<Session>`, so the `handle`/document update block in
`lsp_client.cpp` is binding the result incorrectly with `auto& session`. Update
the code at the `open_session(path_id)` call site to store the returned shared
pointer in an `auto` variable and access `Session` members through `->`, or
change `open_session()` to return `Session&` if that is the intended API, so
`session.version`, `session.text`, and `session.generation` are valid.
In `@src/server/worker/worker_pool.cpp`:
- Around line 154-162: The shutdown path in stop() unconditionally dereferences
each worker’s peer, but respawn_worker() can leave workers[index].peer empty
after moving it out and then failing to spawn, which can crash during later
shutdown. Update stop() to guard the stateless_workers and stateful_workers
entries before calling peer->close_output(), and similarly make sure any other
shutdown handling in respawn_worker() / related worker cleanup paths safely
skips dead or moved-out peers so stopped workers are not dereferenced.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 309-313: The fixture teardown is duplicating
`CliceClient.stop_io()` by manually setting `_stop_event`, cancelling
`_async_tasks`, and sleeping, which can leave tasks running and make tests
flaky. Update the cleanup path to call `CliceClient.stop_io()` directly instead
of reimplementing the shutdown sequence here, so the async teardown logic stays
centralized and waits for cancellations to settle consistently.
🪄 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: 0b86ba8b-a9de-473a-b5b5-606cfb6a7316
📒 Files selected for processing (46)
src/compile/diagnostic.cppsrc/feature/code_completion.cppsrc/feature/diagnostics.cppsrc/feature/feature.hsrc/feature/folding_ranges.cppsrc/feature/formatting.cppsrc/feature/inlay_hints.cppsrc/feature/semantic_tokens.cppsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/compiler/indexer.cppsrc/server/protocol/worker.hsrc/server/service/agent_client.cppsrc/server/service/lsp_client.cppsrc/server/service/lsp_client.hsrc/server/service/master_server.cppsrc/server/service/master_server.hsrc/server/worker/stateless_worker.cppsrc/server/worker/worker_pool.cppsrc/server/workspace/config.cppsrc/server/workspace/config.hsrc/support/anomaly.cppsrc/support/anomaly.htests/conftest.pytests/integration/agentic/test_agentic.pytests/integration/agentic/test_cli.pytests/integration/compilation/test_persistent_cache.pytests/integration/compilation/test_staleness.pytests/integration/features/test_guidance_diagnostics.pytests/integration/features/test_index.pytests/integration/features/test_server.pytests/integration/lifecycle/test_anomaly.pytests/integration/lifecycle/test_config.pytests/integration/lifecycle/test_file_operation.pytests/integration/modules/test_modules.pytests/integration/stress/test_rapid_edit.pytests/integration/utils/assertions.pytests/integration/utils/client.pytests/integration/utils/wait.pytests/pytest.initests/replay.pytests/unit/feature/document_link_tests.cpptests/unit/feature/folding_range_tests.cpptests/unit/server/config_tests.cpptests/unit/support/anomaly_tests.cpptests/unit/test/tester.h
💤 Files with no reviewable changes (17)
- tests/pytest.ini
- tests/integration/stress/test_rapid_edit.py
- tests/integration/lifecycle/test_file_operation.py
- tests/unit/feature/folding_range_tests.cpp
- tests/replay.py
- tests/integration/lifecycle/test_config.py
- tests/integration/features/test_index.py
- tests/unit/feature/document_link_tests.cpp
- tests/integration/lifecycle/test_anomaly.py
- tests/integration/modules/test_modules.py
- tests/unit/test/tester.h
- tests/integration/utils/wait.py
- tests/unit/server/config_tests.cpp
- tests/integration/utils/client.py
- tests/integration/features/test_server.py
- tests/unit/support/anomaly_tests.cpp
- tests/integration/utils/assertions.py
✅ Files skipped from review due to trivial changes (2)
- src/server/service/lsp_client.h
- src/server/service/master_server.h
🚧 Files skipped from review as they are similar to previous changes (16)
- src/server/protocol/worker.h
- src/server/worker/stateless_worker.cpp
- src/server/compiler/compiler.h
- src/feature/code_completion.cpp
- src/feature/folding_ranges.cpp
- src/server/service/agent_client.cpp
- src/feature/inlay_hints.cpp
- tests/integration/features/test_guidance_diagnostics.py
- src/feature/formatting.cpp
- src/compile/diagnostic.cpp
- src/server/workspace/config.h
- src/support/anomaly.h
- src/support/anomaly.cpp
- src/server/workspace/config.cpp
- src/feature/feature.h
- tests/integration/compilation/test_staleness.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/server/compiler/indexer.cpp (1)
935-948: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck the command source before the module prebuild step.
index_one()only returns onCommandSource::Fallbackaftercompile_graph->compile(server_path_id). That path callsfill_compile_args()again inCompiler::init_compile_graph()and can still build/cache a PCM from synthesized args, so background indexing still leaves guessed artifacts behind for files this PR intended to skip.Suggested reorder
- // For module interface units, compile their PCM (and transitive deps) - // first so the stateless worker has the artifacts it needs. - if(workspace.compile_graph && workspace.path_to_module.contains(server_path_id)) { - co_await workspace.compile_graph->compile(server_path_id); - } - worker::BuildParams params; params.kind = worker::BuildKind::Index; params.file = file_path; - // Bulk background indexing sticks to real commands; synthesized fallback - // commands would fill the index with guesses. - if(compiler.fill_compile_args(file_path, params.directory, params.arguments, nullptr) == - CommandSource::Fallback) + auto source = + compiler.fill_compile_args(file_path, params.directory, params.arguments, nullptr); + if(source == CommandSource::Fallback) co_return; + + // For module interface units, compile their PCM (and transitive deps) + // first so the stateless worker has the artifacts it needs. + if(workspace.compile_graph && workspace.path_to_module.contains(server_path_id)) { + co_await workspace.compile_graph->compile(server_path_id); + }🤖 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/indexer.cpp` around lines 935 - 948, Move the CommandSource check in index_one() ahead of the module prebuild step so synthesized commands are rejected before any PCM work starts. Specifically, call Compiler::fill_compile_args() and return early on CommandSource::Fallback before invoking workspace.compile_graph->compile(server_path_id), since compile_graph/init_compile_graph can otherwise still cache artifacts from guessed args. Keep the existing module-interface handling in index_one() and preserve the current worker::BuildParams setup after the early-exit check.src/server/compiler/compiler.cpp (2)
581-596: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winVerify the cached PCH file still exists before reusing it.
The new reuse predicate no longer checks
st.pathon disk. If.clice/cache/pchis cleaned or the file is evicted manually, we skip rebuilding and hand the worker a dead path instead.Suggested fix
if(auto it = workspace.pch_cache.find(path_id); it != workspace.pch_cache.end()) { auto& st = it->second; if(st.hash == preamble_hash && !st.path.empty() && + llvm::sys::fs::exists(st.path) && !deps_changed(workspace.path_pool, st.deps)) { st.bound = bound; session.pch_ref = Session::PCHRef{path_id, preamble_hash, bound}; co_return true; } @@ - co_return workspace.pch_cache.count(path_id) && !workspace.pch_cache[path_id].path.empty(); + co_return workspace.pch_cache.count(path_id) && + !workspace.pch_cache[path_id].path.empty() && + llvm::sys::fs::exists(workspace.pch_cache[path_id].path);🤖 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 581 - 596, The PCH reuse logic in compiler.cpp is still trusting cached entries in workspace.pch_cache without verifying the cached file exists on disk. Update the reuse path in the cache lookup block and the preamble-incomplete fallback to check st.path (or the cached path from workspace.pch_cache[path_id]) still exists before reusing it, and only set session.pch_ref or return true when the file is present; otherwise force a rebuild.
567-579: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftHash the preprocessing context into the PCH key.
This path now shares PCHs by preamble text alone, but
fill_compile_args()/ header-context resolution can change-D/-I/-isystem/-stdand even the working directory without changing those bytes. That lets incompatible compile commands reuse the same.pch, which will surface as wrong diagnostics/completions or sporadic build failures.🤖 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 567 - 579, Update the PCH cache key in compiler.cpp so it includes the preprocessing context, not just the preamble text. In the PCH path generation around preamble_hash and pch_path, fold in the compile arguments produced by fill_compile_args() and any header-context inputs that affect preprocessing such as -D, -I, -isystem, -std, and the working directory, so incompatible commands do not reuse the same .pch. Keep the existing location logic in compiler::... around pch_path, but change the hashing inputs used to derive the deterministic cache filename.src/server/service/lsp_client.cpp (3)
397-412: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck the open session before serving definition results.
Line 402 queries the index before Line 410 checks
find_session, so closed documents can still receive definition results instead of the newDocument not openerror.🐛 Proposed fix
auto& uri = params.text_document_position_params.text_document.uri; auto& pos = params.text_document_position_params.position; - auto result = query_at(uri, pos, RelationKind::Definition); - if(!result.empty()) { - co_return to_raw(result); - } - auto& srv = this->server; auto path = uri_to_path(uri); auto path_id = srv.workspace.path_pool.intern(path); - auto* session = srv.find_session(path_id); + auto session = srv.find_session(path_id); if(!session) co_return kota::outcome_error(document_not_open()); + + auto result = query_at(uri, pos, RelationKind::Definition); + if(!result.empty()) { + co_return to_raw(result); + } co_return co_await srv.compiler.forward_query(worker::QueryKind::GoToDefinition, *session, pos);🤖 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/service/lsp_client.cpp` around lines 397 - 412, The Definition request handler in peer.on_request currently serves query_at results before verifying the document session, so closed files can still return definition results. Update the lambda to check srv.find_session(path_id) and return document_not_open() before calling query_at, keeping the existing uri_to_path, path_pool.intern, and query_at flow intact for open sessions only.
216-222: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t silently accept unmappable incremental edits.
If either offset mapping fails, the text replacement is skipped but Line 227 still advances generation and notifies workers with stale text. Fail closed here, log the bad range, and avoid publishing work from a divergent buffer.
🤖 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/service/lsp_client.cpp` around lines 216 - 222, The incremental edit handling in lsp_client::apply_change currently skips replacements when PositionMapper::to_offset fails, but still lets the buffer generation advance and workers get notified with stale text. Change this path to fail closed in the same block by detecting unmappable ranges, logging the bad range details, and preventing the subsequent generation update / worker publish when start or end cannot be mapped or the range is invalid.
185-188: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winKeep
open_sessionaccess aligned with its return type.MasterServer::open_session()returnsstd::shared_ptr<Session>, soauto& session = srv.open_session(path_id);makessession.version/session.textinvalid. Use ashared_ptrand->, or change the API ifSession&is intended.🤖 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/service/lsp_client.cpp` around lines 185 - 188, `MasterServer::open_session()` returns a `std::shared_ptr<Session>`, so the `handle`/document update block in `lsp_client.cpp` is binding the result incorrectly with `auto& session`. Update the code at the `open_session(path_id)` call site to store the returned shared pointer in an `auto` variable and access `Session` members through `->`, or change `open_session()` to return `Session&` if that is the intended API, so `session.version`, `session.text`, and `session.generation` are valid.src/server/worker/worker_pool.cpp (1)
154-162: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard stopped/dead workers during shutdown.
respawn_worker()can moveworkers[index].peerout, then returnfalseon spawn failure.stop()now unconditionally dereferences everyw.peer, so a later shutdown can crash.🐛 Proposed fix
- for(auto& w: stateless_workers) - w.peer->close_output(); - for(auto& w: stateful_workers) - w.peer->close_output(); + for(auto& w: stateless_workers) + if(w.peer) + w.peer->close_output(); + for(auto& w: stateful_workers) + if(w.peer) + w.peer->close_output(); - for(auto& w: stateless_workers) - w.proc.kill(SIGTERM); - for(auto& w: stateful_workers) - w.proc.kill(SIGTERM); + for(auto& w: stateless_workers) + if(w.alive) + w.proc.kill(SIGTERM); + for(auto& w: stateful_workers) + if(w.alive) + w.proc.kill(SIGTERM);Also applies to: 287-318
🤖 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/worker/worker_pool.cpp` around lines 154 - 162, The shutdown path in stop() unconditionally dereferences each worker’s peer, but respawn_worker() can leave workers[index].peer empty after moving it out and then failing to spawn, which can crash during later shutdown. Update stop() to guard the stateless_workers and stateful_workers entries before calling peer->close_output(), and similarly make sure any other shutdown handling in respawn_worker() / related worker cleanup paths safely skips dead or moved-out peers so stopped workers are not dereferenced.
🧹 Nitpick comments (1)
tests/conftest.py (1)
309-313: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReuse
CliceClient.stop_io()here.That helper already centralizes
_stop_event/_async_tasksteardown and waits for the cancellations to settle. Sleeping for 100ms instead can leave client tasks alive past fixture teardown and make async failures flaky.Suggested cleanup
finally: try: - c._stop_event.set() - for task in c._async_tasks: - task.cancel() - await asyncio.sleep(0.1) + await c.stop_io() except Exception: pass🤖 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 `@tests/conftest.py` around lines 309 - 313, The fixture teardown is duplicating `CliceClient.stop_io()` by manually setting `_stop_event`, cancelling `_async_tasks`, and sleeping, which can leave tasks running and make tests flaky. Update the cleanup path to call `CliceClient.stop_io()` directly instead of reimplementing the shutdown sequence here, so the async teardown logic stays centralized and waits for cancellations to settle consistently.
🤖 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/compiler/indexer.cpp`:
- Around line 164-199: Abort the entire snapshot if any shard serialization or
commit fails, instead of silently skipping bad shards or continuing after a
shard commit error. In the shard collection loop in indexer.cpp, make
serialize_blob/shard.serialize failures cause the snapshot to be discarded; then
in the shard commit loop, treat any store.commit failure as fatal, abort any
already-pending shard blobs, and do not leave a published ProjectIndex with
incomplete shards. Update the logic around workspace.merged_indices,
serialize_blob, store.commit, and store.abort so the snapshot is only considered
successful when every shard is saved and committed.
- Around line 387-398: The session-wide scans are dereferencing optional state
without checking it first. Update the affected walks in the cursor/symbol lookup
paths to skip sessions where `session.file_index` or `session.symbols` are not
present, following the existing optional handling used by `resolve_cursor()` and
`find_symbol_info()`. Apply the same guard pattern in the other listed scan
sites so requests like definition/reference/hierarchy/workspace-symbol do not
access uncompiled or failed sessions.
In `@src/server/service/master_server.cpp`:
- Around line 124-136: Keep the session helper API consistent with its callers:
`MasterServer::find_session` and `MasterServer::open_session` now return
`std::shared_ptr<Session>`, but `lsp_client.cpp` still expects
pointer/reference-style access, so the build will break. Either revert these
helpers to the old `Session*`/reference contract or update all call sites to use
`shared_ptr` semantics, and make sure the declaration in `MasterServer`’s header
matches the implementation.
In `@src/server/worker/worker_pool.cpp`:
- Around line 81-87: Worker spawn failures in worker_pool should not be logged
as anomalies because those paths are recoverable and can abort Debug builds
before returning false. Update the failure handling in the spawn logic around
kota::process::spawn in the worker pool code to use LOG_ERROR (or equivalent
non-fatal logging) instead of LOG_ANOMALY, and apply the same change to the
matching failure path later in the same file so both stateful and stateless
worker spawn errors are treated consistently.
---
Outside diff comments:
In `@src/server/compiler/compiler.cpp`:
- Around line 581-596: The PCH reuse logic in compiler.cpp is still trusting
cached entries in workspace.pch_cache without verifying the cached file exists
on disk. Update the reuse path in the cache lookup block and the
preamble-incomplete fallback to check st.path (or the cached path from
workspace.pch_cache[path_id]) still exists before reusing it, and only set
session.pch_ref or return true when the file is present; otherwise force a
rebuild.
- Around line 567-579: Update the PCH cache key in compiler.cpp so it includes
the preprocessing context, not just the preamble text. In the PCH path
generation around preamble_hash and pch_path, fold in the compile arguments
produced by fill_compile_args() and any header-context inputs that affect
preprocessing such as -D, -I, -isystem, -std, and the working directory, so
incompatible commands do not reuse the same .pch. Keep the existing location
logic in compiler::... around pch_path, but change the hashing inputs used to
derive the deterministic cache filename.
In `@src/server/compiler/indexer.cpp`:
- Around line 935-948: Move the CommandSource check in index_one() ahead of the
module prebuild step so synthesized commands are rejected before any PCM work
starts. Specifically, call Compiler::fill_compile_args() and return early on
CommandSource::Fallback before invoking
workspace.compile_graph->compile(server_path_id), since
compile_graph/init_compile_graph can otherwise still cache artifacts from
guessed args. Keep the existing module-interface handling in index_one() and
preserve the current worker::BuildParams setup after the early-exit check.
In `@src/server/service/lsp_client.cpp`:
- Around line 397-412: The Definition request handler in peer.on_request
currently serves query_at results before verifying the document session, so
closed files can still return definition results. Update the lambda to check
srv.find_session(path_id) and return document_not_open() before calling
query_at, keeping the existing uri_to_path, path_pool.intern, and query_at flow
intact for open sessions only.
- Around line 216-222: The incremental edit handling in lsp_client::apply_change
currently skips replacements when PositionMapper::to_offset fails, but still
lets the buffer generation advance and workers get notified with stale text.
Change this path to fail closed in the same block by detecting unmappable
ranges, logging the bad range details, and preventing the subsequent generation
update / worker publish when start or end cannot be mapped or the range is
invalid.
- Around line 185-188: `MasterServer::open_session()` returns a
`std::shared_ptr<Session>`, so the `handle`/document update block in
`lsp_client.cpp` is binding the result incorrectly with `auto& session`. Update
the code at the `open_session(path_id)` call site to store the returned shared
pointer in an `auto` variable and access `Session` members through `->`, or
change `open_session()` to return `Session&` if that is the intended API, so
`session.version`, `session.text`, and `session.generation` are valid.
In `@src/server/worker/worker_pool.cpp`:
- Around line 154-162: The shutdown path in stop() unconditionally dereferences
each worker’s peer, but respawn_worker() can leave workers[index].peer empty
after moving it out and then failing to spawn, which can crash during later
shutdown. Update stop() to guard the stateless_workers and stateful_workers
entries before calling peer->close_output(), and similarly make sure any other
shutdown handling in respawn_worker() / related worker cleanup paths safely
skips dead or moved-out peers so stopped workers are not dereferenced.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 309-313: The fixture teardown is duplicating
`CliceClient.stop_io()` by manually setting `_stop_event`, cancelling
`_async_tasks`, and sleeping, which can leave tasks running and make tests
flaky. Update the cleanup path to call `CliceClient.stop_io()` directly instead
of reimplementing the shutdown sequence here, so the async teardown logic stays
centralized and waits for cancellations to settle consistently.
🪄 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: 0b86ba8b-a9de-473a-b5b5-606cfb6a7316
📒 Files selected for processing (46)
src/compile/diagnostic.cppsrc/feature/code_completion.cppsrc/feature/diagnostics.cppsrc/feature/feature.hsrc/feature/folding_ranges.cppsrc/feature/formatting.cppsrc/feature/inlay_hints.cppsrc/feature/semantic_tokens.cppsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/compiler/indexer.cppsrc/server/protocol/worker.hsrc/server/service/agent_client.cppsrc/server/service/lsp_client.cppsrc/server/service/lsp_client.hsrc/server/service/master_server.cppsrc/server/service/master_server.hsrc/server/worker/stateless_worker.cppsrc/server/worker/worker_pool.cppsrc/server/workspace/config.cppsrc/server/workspace/config.hsrc/support/anomaly.cppsrc/support/anomaly.htests/conftest.pytests/integration/agentic/test_agentic.pytests/integration/agentic/test_cli.pytests/integration/compilation/test_persistent_cache.pytests/integration/compilation/test_staleness.pytests/integration/features/test_guidance_diagnostics.pytests/integration/features/test_index.pytests/integration/features/test_server.pytests/integration/lifecycle/test_anomaly.pytests/integration/lifecycle/test_config.pytests/integration/lifecycle/test_file_operation.pytests/integration/modules/test_modules.pytests/integration/stress/test_rapid_edit.pytests/integration/utils/assertions.pytests/integration/utils/client.pytests/integration/utils/wait.pytests/pytest.initests/replay.pytests/unit/feature/document_link_tests.cpptests/unit/feature/folding_range_tests.cpptests/unit/server/config_tests.cpptests/unit/support/anomaly_tests.cpptests/unit/test/tester.h
💤 Files with no reviewable changes (17)
- tests/pytest.ini
- tests/integration/stress/test_rapid_edit.py
- tests/integration/lifecycle/test_file_operation.py
- tests/unit/feature/folding_range_tests.cpp
- tests/replay.py
- tests/integration/lifecycle/test_config.py
- tests/integration/features/test_index.py
- tests/unit/feature/document_link_tests.cpp
- tests/integration/lifecycle/test_anomaly.py
- tests/integration/modules/test_modules.py
- tests/unit/test/tester.h
- tests/integration/utils/wait.py
- tests/unit/server/config_tests.cpp
- tests/integration/utils/client.py
- tests/integration/features/test_server.py
- tests/unit/support/anomaly_tests.cpp
- tests/integration/utils/assertions.py
✅ Files skipped from review due to trivial changes (2)
- src/server/service/lsp_client.h
- src/server/service/master_server.h
🚧 Files skipped from review as they are similar to previous changes (16)
- src/server/protocol/worker.h
- src/server/worker/stateless_worker.cpp
- src/server/compiler/compiler.h
- src/feature/code_completion.cpp
- src/feature/folding_ranges.cpp
- src/server/service/agent_client.cpp
- src/feature/inlay_hints.cpp
- tests/integration/features/test_guidance_diagnostics.py
- src/feature/formatting.cpp
- src/compile/diagnostic.cpp
- src/server/workspace/config.h
- src/support/anomaly.h
- src/support/anomaly.cpp
- src/server/workspace/config.cpp
- src/feature/feature.h
- tests/integration/compilation/test_staleness.py
🛑 Comments failed to post (4)
src/server/compiler/indexer.cpp (2)
164-199: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Abort the whole snapshot on any shard write or commit failure.
A dirty shard that fails serialization is silently omitted from
shards, and a later shard commit failure only logs afterprojectis already published. After restart,load()can then pair a newProjectIndexwith old shard blobs, which breaks the consistency guarantee this block is trying to provide.🤖 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/indexer.cpp` around lines 164 - 199, Abort the entire snapshot if any shard serialization or commit fails, instead of silently skipping bad shards or continuing after a shard commit error. In the shard collection loop in indexer.cpp, make serialize_blob/shard.serialize failures cause the snapshot to be discarded; then in the shard commit loop, treat any store.commit failure as fatal, abort any already-pending shard blobs, and do not leave a published ProjectIndex with incomplete shards. Update the logic around workspace.merged_indices, serialize_blob, store.commit, and store.abort so the snapshot is only considered successful when every shard is saved and committed.
387-398: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard optional session state before walking it.
resolve_cursor()andfind_symbol_info()already treatsession.file_index/session.symbolsas optional, but these new session-wide scans dereference them unconditionally. Any open document that has not compiled yet, or whose compile failed, can crash definition/reference/hierarchy/workspace-symbol requests here.Suggested guard pattern
foreach_session([&](std::uint32_t, const Session& session) -> bool { + if(!session.file_index) + return true; auto map = session.line_map(); session.file_index->lookup(...); return true; });foreach_session([&](std::uint32_t, const Session& session) -> bool { + if(!session.symbols) + return true; for(auto& [hash, symbol]: *session.symbols) { ... } return true; });Also applies to: 421-434, 515-523, 546-558, 591-610, 689-704, 810-834
🤖 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/indexer.cpp` around lines 387 - 398, The session-wide scans are dereferencing optional state without checking it first. Update the affected walks in the cursor/symbol lookup paths to skip sessions where `session.file_index` or `session.symbols` are not present, following the existing optional handling used by `resolve_cursor()` and `find_symbol_info()`. Apply the same guard pattern in the other listed scan sites so requests like definition/reference/hierarchy/workspace-symbol do not access uncompiled or failed sessions.src/server/service/master_server.cpp (1)
124-136: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Keep the session helper contract consistent with current callers.
find_session()/open_session()now returnstd::shared_ptr<Session>, but the current call sites still treat them asSession*andSession&insrc/server/service/lsp_client.cpp. That turns this change into a compile break. Either keep the old pointer/reference API here or update every caller to useshared_ptrsemantics consistently.Possible fix
-std::shared_ptr<Session> MasterServer::find_session(std::uint32_t path_id) { +Session* MasterServer::find_session(std::uint32_t path_id) { auto it = sessions.find(path_id); - return it != sessions.end() ? it->second : nullptr; + return it != sessions.end() ? it->second.get() : nullptr; } -std::shared_ptr<Session> MasterServer::open_session(std::uint32_t path_id) { +Session& MasterServer::open_session(std::uint32_t path_id) { auto it = sessions.find(path_id); if(it != sessions.end()) { it->second->generation++; } auto session = std::make_shared<Session>(); session->path_id = path_id; - sessions[path_id] = session; - return session; + sessions[path_id] = std::move(session); + return *sessions[path_id]; }Mirror the same signature change in
src/server/service/master_server.h.🤖 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/service/master_server.cpp` around lines 124 - 136, Keep the session helper API consistent with its callers: `MasterServer::find_session` and `MasterServer::open_session` now return `std::shared_ptr<Session>`, but `lsp_client.cpp` still expects pointer/reference-style access, so the build will break. Either revert these helpers to the old `Session*`/reference contract or update all call sites to use `shared_ptr` semantics, and make sure the declaration in `MasterServer`’s header matches the implementation.src/server/worker/worker_pool.cpp (1)
81-87: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t classify worker spawn failures as anomalies.
Spawn failures are recoverable environmental/operational errors;
LOG_ANOMALYaborts Debug builds before these functions can returnfalse. PreferLOG_ERRORor guidance for these paths.🔧 Proposed fix
- LOG_ANOMALY(WorkerSpawnFail, - "Failed to spawn {} worker: {}", - stateful ? "stateful" : "stateless", - result.error().message()); + LOG_ERROR("Failed to spawn {} worker: {}", + stateful ? "stateful" : "stateless", + result.error().message());- LOG_ANOMALY(WorkerSpawnFail, - "Failed to respawn worker {}: {}", - worker_name, - result.error().message()); + LOG_ERROR("Failed to respawn worker {}: {}", worker_name, result.error().message());Also applies to: 312-318
🤖 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/worker/worker_pool.cpp` around lines 81 - 87, Worker spawn failures in worker_pool should not be logged as anomalies because those paths are recoverable and can abort Debug builds before returning false. Update the failure handling in the spawn logic around kota::process::spawn in the worker pool code to use LOG_ERROR (or equivalent non-fatal logging) instead of LOG_ANOMALY, and apply the same change to the matching failure path later in the same file so both stateful and stateless worker spawn errors are treated consistently.
The TOML error location feature (60661c1) is on an unmerged kotatsu branch. Revert to main's 0b38494 which has LineMap, and remove the line/column assertions that depend on the unmerged feature.
16bit-ykiko
force-pushed
the
feat/server-feedback
branch
from
June 29, 2026 14:34
c41956d to
71c692f
Compare
…derflow Mapping failures returned (0,0) which could cause unsigned underflow in the semantic tokens relative encoder and corrupt downstream data. Return std::optional instead and let callers skip the affected artifact.
Callers that reuse a non-empty string kept a stale path when no config file was found, contradicting the documented "stays empty" contract.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/unit/support/anomaly_tests.cpp (1)
20-31: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the previous hook/trap instead of clearing globals.
AnomalyCapturestill overwrites global logging state but only restoreslogging::options.levelon teardown. Clearing the hook and resetting anomaly state here leaves later tests order-dependent and can suppress whatever trap/hook was installed before this fixture. Please capture the previous notify hook/anomaly trap in the constructor and restore those exact values in the destructor.🤖 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 `@tests/unit/support/anomaly_tests.cpp` around lines 20 - 31, AnomalyCapture currently overwrites global logging state and only restores logging::options.level, which leaves the previous notify hook and anomaly trap lost after the fixture. Update AnomalyCapture to save the existing notify hook and anomaly trap in the constructor before calling logging::set_notify_hook and logging::set_anomaly_trap_for_testing, then restore those exact saved values in the destructor instead of clearing globals. Use the existing AnomalyCapture constructor/destructor and the logging::set_notify_hook, logging::set_anomaly_trap_for_testing, and logging::reset_anomaly_for_testing calls to locate the fix.
🧹 Nitpick comments (1)
tests/unit/server/config_tests.cpp (1)
381-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the reported config issue location too.
These tests are named around location coverage, but they only check severity/message. A regression that drops line/column data would still pass, so please assert the emitted location for each case as well.
🤖 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 `@tests/unit/server/config_tests.cpp` around lines 381 - 411, The config issue tests currently verify only severity and message, so they miss regressions where location data is lost. Update the three test cases in SyntaxIssueHasLocation, TypeIssueHasLocation, and UnknownKeyIssueWarns to also assert the reported location fields on ConfigIssue (for example line/column or equivalent location members exposed by Config::load), using the existing issues[0] result after load.
🤖 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/feature/diagnostics.cpp`:
- Around line 154-157: The safe range migration is incomplete in diagnostics
handling: `add_related()` and the main-file path still dereference
`*map.to_range(...)` directly, which can bypass `PositionMapFail` and crash on
bad ranges. Update those branches in `diagnostics.cpp` to use the same safe
range construction as the migrated branch, leveraging `to_position(...)` or
equivalent fallback handling so failures degrade to `(0,0)` instead of
dereferencing an invalid optional.
In `@src/feature/semantic_tokens.cpp`:
- Around line 497-498: The semantic token delta encoder in semantic_tokens.cpp
is currently using the {0,0} fallback from to_position for unmapped endpoints,
which can corrupt last_line/last_start_character state and later deltas. Update
the token emission path around the begin_position/end_position handling to
detect unmapped positions and skip encoding that token entirely when either
endpoint cannot be mapped, rather than passing the fallback into the delta
calculation.
In `@src/server/compiler/compiler.cpp`:
- Around line 1071-1079: The range validation in the compiler request handling
only checks that both endpoints map successfully, but it still forwards reversed
byte ranges when start is greater than end. Update the range handling around the
map.to_offset and wp.range assignment to reject any range where the resolved
start offset exceeds the end offset, and return
kota::ipc::ErrorCode::InvalidParams with the existing invalid-range message
before sending work to the worker. Apply the same validation to the duplicate
range block noted in the other location as well.
In `@tests/conftest.py`:
- Around line 112-117: Move the teardown anomaly check in the fixture teardown
flow so it always runs from a finally block, even if shutdown_client() fails
through assert_server_exited_cleanly(). Update the teardown logic around
check_no_anomaly(request, c) to ensure it is executed after shutdown_client(c,
verbose=test_failed) on all paths, including crashy or timeout shutdowns, and
apply the same pattern in the related teardown section at the additional
referenced location.
---
Duplicate comments:
In `@tests/unit/support/anomaly_tests.cpp`:
- Around line 20-31: AnomalyCapture currently overwrites global logging state
and only restores logging::options.level, which leaves the previous notify hook
and anomaly trap lost after the fixture. Update AnomalyCapture to save the
existing notify hook and anomaly trap in the constructor before calling
logging::set_notify_hook and logging::set_anomaly_trap_for_testing, then restore
those exact saved values in the destructor instead of clearing globals. Use the
existing AnomalyCapture constructor/destructor and the logging::set_notify_hook,
logging::set_anomaly_trap_for_testing, and logging::reset_anomaly_for_testing
calls to locate the fix.
---
Nitpick comments:
In `@tests/unit/server/config_tests.cpp`:
- Around line 381-411: The config issue tests currently verify only severity and
message, so they miss regressions where location data is lost. Update the three
test cases in SyntaxIssueHasLocation, TypeIssueHasLocation, and
UnknownKeyIssueWarns to also assert the reported location fields on ConfigIssue
(for example line/column or equivalent location members exposed by
Config::load), using the existing issues[0] result after load.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34fa9c51-ecf5-4109-bf24-42a62e4a6eb3
📒 Files selected for processing (44)
src/compile/diagnostic.cppsrc/feature/code_completion.cppsrc/feature/diagnostics.cppsrc/feature/feature.hsrc/feature/folding_ranges.cppsrc/feature/formatting.cppsrc/feature/inlay_hints.cppsrc/feature/semantic_tokens.cppsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/compiler/indexer.cppsrc/server/protocol/worker.hsrc/server/service/agent_client.cppsrc/server/service/lsp_client.cppsrc/server/service/lsp_client.hsrc/server/service/master_server.cppsrc/server/service/master_server.hsrc/server/worker/stateless_worker.cppsrc/server/worker/worker_pool.cppsrc/server/workspace/config.cppsrc/server/workspace/config.hsrc/support/anomaly.cppsrc/support/anomaly.htests/conftest.pytests/integration/agentic/test_agentic.pytests/integration/agentic/test_cli.pytests/integration/compilation/test_persistent_cache.pytests/integration/compilation/test_staleness.pytests/integration/features/test_guidance_diagnostics.pytests/integration/features/test_index.pytests/integration/features/test_server.pytests/integration/lifecycle/test_anomaly.pytests/integration/lifecycle/test_config.pytests/integration/lifecycle/test_file_operation.pytests/integration/modules/test_modules.pytests/integration/stress/test_rapid_edit.pytests/integration/utils/assertions.pytests/integration/utils/client.pytests/integration/utils/wait.pytests/pytest.initests/replay.pytests/unit/server/config_tests.cpptests/unit/support/anomaly_tests.cpptests/unit/test/tester.h
✅ Files skipped from review due to trivial changes (3)
- tests/integration/stress/test_rapid_edit.py
- tests/integration/utils/wait.py
- src/server/service/master_server.h
🚧 Files skipped from review as they are similar to previous changes (30)
- tests/pytest.ini
- src/feature/code_completion.cpp
- src/server/worker/stateless_worker.cpp
- src/server/protocol/worker.h
- tests/unit/test/tester.h
- src/compile/diagnostic.cpp
- tests/integration/lifecycle/test_config.py
- src/server/service/agent_client.cpp
- src/feature/feature.h
- src/feature/folding_ranges.cpp
- tests/replay.py
- tests/integration/modules/test_modules.py
- src/server/compiler/indexer.cpp
- tests/integration/features/test_index.py
- src/server/service/lsp_client.h
- tests/integration/lifecycle/test_file_operation.py
- src/server/compiler/compiler.h
- tests/integration/utils/assertions.py
- tests/integration/features/test_guidance_diagnostics.py
- tests/integration/compilation/test_persistent_cache.py
- src/server/workspace/config.h
- tests/integration/utils/client.py
- src/server/service/master_server.cpp
- tests/integration/features/test_server.py
- src/server/workspace/config.cpp
- tests/integration/lifecycle/test_anomaly.py
- src/support/anomaly.h
- src/support/anomaly.cpp
- tests/integration/compilation/test_staleness.py
- src/server/service/lsp_client.cpp
Worker crashes are expected in tests that deliberately kill workers. LOG_ANOMALY aborts in Debug builds, causing CI failures.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3032a1a8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Worker crashes should be reported as anomalies. The unit test fixture now installs a no-op anomaly trap so Debug builds don't abort. Also remove TOML location assertions from config integration tests since the kotatsu main branch does not yet support error locations.
- convert remaining unchecked *map.to_range() derefs to the checked feature-layer helpers; symbols/links/diagnostics skip unmappable entries instead of UB - attach dispatch error codes (cancelled, worker_unavailable) so memory-pressure preemption and crash/restart windows are logged as warnings, not anomalies; real IPC breakage still traps - clamp out-of-range positions/ranges per the LSP spec instead of rejecting them with InvalidParams - apply config rule appends to synthesized fallback commands - anomaly: Count sentinel guards the counter array, rate-limit suppression notice reaches the client, hierarchy error helper - unit tests: position_map_fail and worker crash/spawn-fail triggers, operational-code classification, fallback append coverage
- document the logging taxonomy in logging.h: level usage, anomaly and guidance channels, perf lines, process ownership of log files - LOG_PERF(topic, ...) emits greppable '[perf:<topic>] key=value' lines; instrument index phases (per-file, run, save, load), PCH/PCM cache hit/miss with reason, cache eviction, and startup phases - workers log only to their own file (mirror_stderr=false) and skip the stderr backtrace: crash traces land in the worker log only, so the master log no longer duplicates worker output - startup flow: master banner (pid/mode/workspace), session log dir, config-not-found log restored for the deferred-defaults path - move ScopedTimer to support/timer.h for master-side reuse
- close anomaly-gate bypasses: flag-change, kill9-recovery and config sessions now assert_no_anomaly; gate runs even when shutdown fails - worker crash test uses SIGABRT and asserts the backtrace lands in the worker log file and never leaks into the master log - out-of-range clamp coverage for the range-formatting path - fallback rule-append end-to-end test (no CDB, clice.toml -I rule) - use the public client.server property instead of pygls internals
[perf:request] lines on the master for query/build/format/compile: wait_ms separates time-to-worker-ready (compile or dependency wait) from total_ms end-to-end latency; kinds use readable enum names.
Rewrite the taxonomy as a decision guide: pick the channel by who must act and whether it is clice's fault. Document the two hard rules (anomaly = soft assertion, user-causable failures never trap) and why anomaly.h stays a separate header (policy layer; AnomalyId evolution must not recompile every logging TU).
Delete the hand-written anomaly_name/command_source_name mappings: the blanket reflective enum formatter in support/format.h already renders enums by name, so wire markers and decision logs now use the enumerator spelling directly ([anomaly:PCHBuildFail], source=CDBExact, kind=Hover). Rename PchBuildFail/PcmBuildFail/CDBExact-class enumerators to all-caps acronyms. MarkerNamesStable keeps pinning the wire names.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- reject reversed ranges (start > end) with InvalidParams after clamping, in forward_query and forward_format - synchronize the anomaly notify/trap hooks with a mutex (copy under lock, invoke outside), making reporting safe from any thread - Tester::to_local_range fails loudly on unmappable annotation ranges instead of dereferencing an empty optional
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
16bit-ykiko
changed the title
This was referenced Jul 4, 2026
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.
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
nullresponses), 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_TRAPexists for tests); Release logs[anomaly:<id>], pusheswindow/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 byMarkerNamesStable).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 carrydispatch_errccodes (cancelledfor memory-pressure preemption,worker_unavailablefor crash/restart windows) sois_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 ofnull. 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 aCommandSource(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 (codeinferred-compile-command, linking the quick-start guide). Exact CDB matches never get it. Config rule appends now reach synthesized fallback commands, so-Irules 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
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=valuelines 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-requestwait_ms/total_msfor query/build/format/compile).<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.install_crash_handler, covers SIGABRT soLOG_FATALand 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.Notable fixes uncovered along the way
CompilationDatabase::lookup()synthesizes a command for unknown files, so the oldresults.empty()checks were dead: the automatic include-graph header-context tier was unreachable, and headers without entries silently compiled as bareclang. Tier selection now useshas_entry(); background indexing skips Fallback-sourced files.apply_defaults(): a client-providedcache_dirpreviously didn't propagate into the derivedlogging_dir/index_dir.*map.to_range()dereferences (empty-optional UB) in feature code converted to checked helpers reportingPositionMapFail.Test infrastructure
CliceClientrecordswindow/logMessage;assert_no_anomaly()runs in every integration teardown (all fixtures andmake_clientsessions), scanning notifications and master/worker log files even when shutdown fails.tests/replay.pyfails a smoke trace on any anomaly push.-Irule → clean compile), clice.toml error/unknown-key/clear scenarios, worker SIGABRT →WorkerCrashanomaly + backtrace ownership, closed/unknown documents viapytest.raises, position/range clamping.PositionMapFail,WorkerCrash,WorkerSpawnFail;dispatch_errcclassification; fallback append; lazy-evaluation and rate-limit contracts.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
pixi run formatclean; two rounds of 3 parallel review subagents (correctness / style / tests) — all findings fixed or triagedCloses #436 (superseded).
🤖 Generated with Claude Code