Repository navigation
refactor(server, tests): improve error feedback, logging, and test infrastructure - #436
Closed
16bit-ykiko wants to merge 1 commit into
Closed
16bit-ykiko wants to merge 1 commit into
16bit-ykiko wants to merge 1 commit into
Conversation
…frastructure Replace silent null returns with proper LSP errors (kota::fail) for feature requests on closed documents, failed compilations, invalid positions, and unresolvable hierarchy items. Add client notifications (window/logMessage) for key failures so integration tests can observe errors without reading server logs. Expand logging coverage in compilation pipeline (compile args, compilation phases, worker results). Improve test infrastructure: conditional log dump on failure, yield-based workspace fixture with post-test cleanup, named timing constants replacing hardcoded sleeps, and log message capture/assertion helpers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds comprehensive warning and error logging throughout compilation and server layers, replaces silent failures (returning Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/config.cpp (1)
177-184:⚠️ Potential issue | 🟡 MinorDelay the warning until defaults are actually used.
Line 182 sets
warningimmediately after one config file fails. If a later standard location loads successfully, the caller still emits a stale “falling back to defaults” warning even though defaults were not used.Suggested fix
Config Config::load_from_workspace(llvm::StringRef workspace_root, std::string* warning) { + std::string invalid_config_path; if(!workspace_root.empty()) { for(auto* name: {"clice.toml", ".clice/config.toml"}) { auto config_path = path::join(workspace_root, name); if(!llvm::sys::fs::exists(config_path)) continue; if(auto config = load(config_path, workspace_root)) return std::move(*config); // Present but malformed: fall through to defaults, but surface // the situation clearly so users know their config wasn't applied. LOG_WARN("Falling back to default configuration because {} is invalid", config_path); - if(warning) - *warning = std::format("Configuration file {} is invalid, falling back to defaults", - config_path); + if(invalid_config_path.empty()) + invalid_config_path = std::move(config_path); } } Config config; config.apply_defaults(workspace_root); + if(warning && !invalid_config_path.empty()) + *warning = std::format("Configuration file {} is invalid, falling back to defaults", + invalid_config_path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/config.cpp` around lines 177 - 184, The code logs a warning and sets *warning immediately when a single config_path load fails, which can produce a stale "falling back to defaults" message even if a later standard location succeeds; change the logic so the LOG_WARN and assignment to *warning are NOT done here but instead only performed at the point where the function actually decides to return the defaults (i.e., after all load attempts fail). Concretely, remove the LOG_WARN(...) and the *warning = ... lines from the failing-try block around load(config_path, workspace_root) and set that warning (or a boolean that triggers it) in the final branch where the default configuration is returned; reference symbols: load(), config_path, workspace_root, warning, LOG_WARN.
🧹 Nitpick comments (5)
tests/integration/utils/wait.py (1)
13-13:MTIME_GRANULARITY = 1.1is fine for ext4/APFS/NTFS but not FAT32/exFAT.Most Linux/macOS/Windows dev filesystems have ≤1s mtime resolution, so 1.1s covers them with margin. FAT32/exFAT have 2s granularity; if any CI runner or tmp dir ever lands on such a filesystem, staleness-detection tests could flake. If that's plausible in this project's CI matrix, consider bumping to ~2.1s or detecting the FS; otherwise the current value is a reasonable default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/utils/wait.py` at line 13, MTIME_GRANULARITY is set to 1.1 which can still flake on FAT32/exFAT (2s) filesystems; update the constant MTIME_GRANULARITY in tests/integration/utils/wait.py to a safer default such as 2.1 to cover 2s-granularity filesystems, or implement a small runtime probe in the module that detects the filesystem mtime resolution (e.g., create a temp file, sleep short intervals and check mtime changes) and set MTIME_GRANULARITY dynamically based on that probe so tests are robust across CI environments.src/server/stateful_worker.cpp (1)
95-118: Potential log noise on every query to an in-progress/broken document.
with_astnow logs a warning for both "document not found" and "AST not available". Since feature handlers (Hover,SemanticTokens,InlayHints,FoldingRange,DocumentSymbol,DocumentLink) all funnel through here, a client firing multiple feature requests against a path whose compilation hasn't completed (or failed non-fatally) will produce one warn per request per feature. Consider downgrading toLOG_DEBUG/LOG_TRACE, or rate-limiting by caching the last-warned (path, state) — the error has already been surfaced to the client viakota::fail/window/logMessageat the compile layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/stateful_worker.cpp` around lines 95 - 118, with_ast currently emits LOG_WARN in two benign cases ("document not found" and "AST not available"), causing noisy warnings for every feature request (Hover, SemanticTokens, InlayHints, FoldingRange, DocumentSymbol, DocumentLink) while a document is compiling or has non-fatal errors; change those LOG_WARN calls in with_ast to a lower-severity level (e.g., LOG_DEBUG or LOG_TRACE) or implement simple rate-limiting by tracking the last warned state per path (keyed by path + state like "not_found" or "ast_unavailable") and only log again after a cooldown; update the messages where LOG_WARN("with_ast: document not found: {}", path.str()) and LOG_WARN("with_ast: AST not available for {}", path.str()) appear, and ensure the doc lifecycle helpers (touch_lru, doc->ast_ready, doc->unit.completed(), doc->unit.fatal_error()) remain unchanged.tests/integration/lifecycle/test_file_operation.py (1)
60-63: Tighten thepytest.raisesexception type.Matching on the base
Exceptionclass will also swallow unrelated errors (e.g.TimeoutError, connection drops) as long as their message happens to contain"Document not open". If the LSP client raises a specific exception type (e.g. anlsprotocol/JSON-RPC response error), prefer matching that class so a regression that surfaces a different failure still fails the test.Illustrative refactor
- with pytest.raises(Exception, match="Document not open"): + with pytest.raises(JsonRpcError, match="Document not open"): # or whichever type client raises await client.text_document_hover_async( HoverParams(text_document=doc(uri), position=Position(line=0, character=0)) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/lifecycle/test_file_operation.py` around lines 60 - 63, The test currently uses pytest.raises(Exception, ...) which is too broad; change the first argument to the specific exception class raised by the LSP client when the document is not open (the concrete JSON-RPC/LS client error type your client emits — e.g., JSONRPCError, ResponseError, RpcError, or the lsprotocol-specific exception) so the assertion is precise; keep the same call to client.text_document_hover_async (and the match="Document not open") but replace Exception with that concrete class in the pytest.raises invocation to avoid swallowing unrelated errors.tests/conftest.py (2)
96-103: Pre- and post-test.clicecleanup is duplicated.The fixture removes
clice_dirboth beforeyield(lines 97-99) and after (lines 102-103). The post-test rmtree makes the pre-test one redundant in the common case, though the pre-test cleanup does still guard against stale state from a crashed prior run. Not a bug — flagging for awareness; feel free to keep both as belt-and-suspenders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 96 - 103, Duplicate cleanup of the .clice directory occurs around the fixture using clice_dir and yield; either remove the redundant pre-test or post-test rmtree or extract a small helper (e.g., def _remove_clice(path): ...) and call it both before yield and after to avoid duplicated code while preserving the crash-guard behavior; update the fixture to call the helper or remove one of the rmtree blocks and add a short comment explaining the remaining cleanup choice.
77-103: Updateworkspacefixture return type hint.Now that
workspaceis a generator fixture, the-> Path | Noneannotation is inaccurate. ConsiderIterator[Path | None](orGenerator[Path | None, None, None]) for clarity.Proposed change
-from pathlib import Path +from collections.abc import Iterator +from pathlib import Path @@ `@pytest.fixture` -def workspace(request: pytest.FixtureRequest, test_data_dir: Path) -> Path | None: +def workspace( + request: pytest.FixtureRequest, test_data_dir: Path +) -> Iterator[Path | None]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 77 - 103, The workspace fixture is a generator yielding Path|None but is annotated as returning Path|None; update its type hint to an iterator/generator type (e.g., Iterator[Path | None] or Generator[Path | None, None, None]) so the signature matches its generator behavior; modify the annotation on the workspace function (which contains calls to generate_cdb and uses the clice_dir variable) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/compiler.cpp`:
- Around line 836-849: The current forward_query treats any false from
ensure_compiled(session) as a compilation failure and returns an LSP error,
which misclassifies generation-mismatch/concurrent edits; change the logic so
that when ensure_compiled(session) returns false you re-check the live session
state (find sessions via sessions.find(path_id) and inspect
sit->second.ast_dirty) before emitting an error: if the session was closed, keep
the fail("Document was closed during compilation") path; if the session is still
present but ast_dirty is true (concurrent edit), return the benign
serde_raw{"null"} result instead of a compilation failure; alternatively, have
ensure_compiled return a richer status (e.g., enum {Ok, Dirty, Closed, Error})
and switch on that in forward_query to map Dirty->serde_raw{"null"},
Closed->fail(...), and Error->fail("Compilation failed").
In `@tests/conftest.py`:
- Around line 206-209: The teardown prints client log messages but directly
accesses c.log_messages which can raise AttributeError for clients missing that
attribute (e.g., older fixtures or partially-initialized CliceClient after
start_io failure); change the access to use getattr(c, "log_messages", None) and
only iterate/print when the result is not None to mirror the existing getattr(c,
"_server", None) defensive pattern used by _shutdown_client / shutdown_client.
In `@tests/integration/compilation/test_staleness.py`:
- Line 24: The test references SETTLE_TIME but the top import only brings in
MTIME_GRANULARITY and wait_for_recompile; add SETTLE_TIME to the import from
tests.integration.utils.wait so SETTLE_TIME, MTIME_GRANULARITY, and
wait_for_recompile are all imported, then run the test(s) that use SETTLE_TIME
(around the staleness test that uses SETTLE_TIME at lines ~127-133) to verify
the NameError is resolved.
---
Outside diff comments:
In `@src/server/config.cpp`:
- Around line 177-184: The code logs a warning and sets *warning immediately
when a single config_path load fails, which can produce a stale "falling back to
defaults" message even if a later standard location succeeds; change the logic
so the LOG_WARN and assignment to *warning are NOT done here but instead only
performed at the point where the function actually decides to return the
defaults (i.e., after all load attempts fail). Concretely, remove the
LOG_WARN(...) and the *warning = ... lines from the failing-try block around
load(config_path, workspace_root) and set that warning (or a boolean that
triggers it) in the final branch where the default configuration is returned;
reference symbols: load(), config_path, workspace_root, warning, LOG_WARN.
---
Nitpick comments:
In `@src/server/stateful_worker.cpp`:
- Around line 95-118: with_ast currently emits LOG_WARN in two benign cases
("document not found" and "AST not available"), causing noisy warnings for every
feature request (Hover, SemanticTokens, InlayHints, FoldingRange,
DocumentSymbol, DocumentLink) while a document is compiling or has non-fatal
errors; change those LOG_WARN calls in with_ast to a lower-severity level (e.g.,
LOG_DEBUG or LOG_TRACE) or implement simple rate-limiting by tracking the last
warned state per path (keyed by path + state like "not_found" or
"ast_unavailable") and only log again after a cooldown; update the messages
where LOG_WARN("with_ast: document not found: {}", path.str()) and
LOG_WARN("with_ast: AST not available for {}", path.str()) appear, and ensure
the doc lifecycle helpers (touch_lru, doc->ast_ready, doc->unit.completed(),
doc->unit.fatal_error()) remain unchanged.
In `@tests/conftest.py`:
- Around line 96-103: Duplicate cleanup of the .clice directory occurs around
the fixture using clice_dir and yield; either remove the redundant pre-test or
post-test rmtree or extract a small helper (e.g., def _remove_clice(path): ...)
and call it both before yield and after to avoid duplicated code while
preserving the crash-guard behavior; update the fixture to call the helper or
remove one of the rmtree blocks and add a short comment explaining the remaining
cleanup choice.
- Around line 77-103: The workspace fixture is a generator yielding Path|None
but is annotated as returning Path|None; update its type hint to an
iterator/generator type (e.g., Iterator[Path | None] or Generator[Path | None,
None, None]) so the signature matches its generator behavior; modify the
annotation on the workspace function (which contains calls to generate_cdb and
uses the clice_dir variable) accordingly.
In `@tests/integration/lifecycle/test_file_operation.py`:
- Around line 60-63: The test currently uses pytest.raises(Exception, ...) which
is too broad; change the first argument to the specific exception class raised
by the LSP client when the document is not open (the concrete JSON-RPC/LS client
error type your client emits — e.g., JSONRPCError, ResponseError, RpcError, or
the lsprotocol-specific exception) so the assertion is precise; keep the same
call to client.text_document_hover_async (and the match="Document not open") but
replace Exception with that concrete class in the pytest.raises invocation to
avoid swallowing unrelated errors.
In `@tests/integration/utils/wait.py`:
- Line 13: MTIME_GRANULARITY is set to 1.1 which can still flake on FAT32/exFAT
(2s) filesystems; update the constant MTIME_GRANULARITY in
tests/integration/utils/wait.py to a safer default such as 2.1 to cover
2s-granularity filesystems, or implement a small runtime probe in the module
that detects the filesystem mtime resolution (e.g., create a temp file, sleep
short intervals and check mtime changes) and set MTIME_GRANULARITY dynamically
based on that probe so tests are robust across CI environments.
🪄 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: 804a332c-b653-4f96-8672-5d298748ab15
📒 Files selected for processing (20)
src/compile/compilation.cppsrc/server/compiler.cppsrc/server/config.cppsrc/server/config.hsrc/server/master_server.cppsrc/server/stateful_worker.cppsrc/server/stateless_worker.cpptests/conftest.pytests/integration/compilation/test_persistent_cache.pytests/integration/compilation/test_staleness.pytests/integration/features/test_server.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/unit/feature/document_link_tests.cpptests/unit/feature/folding_range_tests.cpptests/unit/test/tester.h
💤 Files with no reviewable changes (1)
- tests/unit/feature/document_link_tests.cpp
Comment on lines
836
to
849
| if(!co_await ensure_compiled(session)) { | ||
| co_return serde_raw{"null"}; | ||
| LOG_WARN("forward_query: compilation failed for {}", path); | ||
| co_await kota::fail("Compilation failed"); | ||
| } | ||
|
|
||
| auto sit = sessions.find(path_id); | ||
| if(sit == sessions.end() || sit->second.ast_dirty) { | ||
| if(sit == sessions.end()) { | ||
| LOG_WARN("forward_query: session lost after compile for {}", path); | ||
| co_await kota::fail("Document was closed during compilation"); | ||
| } | ||
| if(sit->second.ast_dirty) { | ||
| LOG_DEBUG("forward_query: still dirty after compile for {} (concurrent edit)", path); | ||
| co_return serde_raw{"null"}; | ||
| } |
There was a problem hiding this comment.
Don’t report concurrent edits as compilation failures.
ensure_compiled() returns false when the session remains dirty, including the generation-mismatch path for edits during compilation. Because Line 838 fails before the Line 846 dirty check, that branch is effectively unreachable for this case and normal rapid edits can surface as "Compilation failed".
Return a richer compile status, or re-check the current session state before converting the result into an LSP error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/compiler.cpp` around lines 836 - 849, The current forward_query
treats any false from ensure_compiled(session) as a compilation failure and
returns an LSP error, which misclassifies generation-mismatch/concurrent edits;
change the logic so that when ensure_compiled(session) returns false you
re-check the live session state (find sessions via sessions.find(path_id) and
inspect sit->second.ast_dirty) before emitting an error: if the session was
closed, keep the fail("Document was closed during compilation") path; if the
session is still present but ast_dirty is true (concurrent edit), return the
benign serde_raw{"null"} result instead of a compilation failure; alternatively,
have ensure_compiled return a richer status (e.g., enum {Ok, Dirty, Closed,
Error}) and switch on that in forward_query to map Dirty->serde_raw{"null"},
Closed->fail(...), and Error->fail("Compilation failed").
Comment on lines
+206
to
+209
| if verbose and c.log_messages: | ||
| for msg in c.log_messages: | ||
| level = {1: "ERROR", 2: "WARN", 3: "INFO", 4: "LOG"}.get(msg.type, "?") | ||
| print(f"[logMessage/{level}] {msg.message}", flush=True) |
There was a problem hiding this comment.
Guard against missing log_messages attribute.
_shutdown_client is also exposed as the public shutdown_client alias (line 220) for multi-session tests that may construct CliceClient instances differently. If a client object ever lacks log_messages (e.g., an older fixture, a partially-initialized client after start_io failure), this line will raise AttributeError during teardown and mask the original failure. Consider getattr(c, "log_messages", None) for defensive parity with the existing getattr(c, "_server", None) pattern above.
Proposed change
- if verbose and c.log_messages:
- for msg in c.log_messages:
+ log_messages = getattr(c, "log_messages", None)
+ if verbose and log_messages:
+ for msg in log_messages:
level = {1: "ERROR", 2: "WARN", 3: "INFO", 4: "LOG"}.get(msg.type, "?")
print(f"[logMessage/{level}] {msg.message}", flush=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/conftest.py` around lines 206 - 209, The teardown prints client log
messages but directly accesses c.log_messages which can raise AttributeError for
clients missing that attribute (e.g., older fixtures or partially-initialized
CliceClient after start_io failure); change the access to use getattr(c,
"log_messages", None) and only iterate/print when the result is not None to
mirror the existing getattr(c, "_server", None) defensive pattern used by
_shutdown_client / shutdown_client.
|
|
||
| from tests.integration.utils import write_cdb, doc | ||
| from tests.integration.utils.wait import wait_for_recompile | ||
| from tests.integration.utils.wait import MTIME_GRANULARITY, wait_for_recompile |
There was a problem hiding this comment.
Import SETTLE_TIME before using it.
Line 133 references SETTLE_TIME, but Line 24 only imports MTIME_GRANULARITY and wait_for_recompile, so this test currently fails with NameError.
Proposed fix
-from tests.integration.utils.wait import MTIME_GRANULARITY, wait_for_recompile
+from tests.integration.utils.wait import MTIME_GRANULARITY, SETTLE_TIME, wait_for_recompileAlso applies to: 127-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/compilation/test_staleness.py` at line 24, The test
references SETTLE_TIME but the top import only brings in MTIME_GRANULARITY and
wait_for_recompile; add SETTLE_TIME to the import from
tests.integration.utils.wait so SETTLE_TIME, MTIME_GRANULARITY, and
wait_for_recompile are all imported, then run the test(s) that use SETTLE_TIME
(around the staleness test that uses SETTLE_TIME at lines ~127-133) to verify
the NameError is resolved.
4 tasks done
16bit-ykiko
added a commit
that referenced
this pull request
Jun 29, 2026
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).
16bit-ykiko
added a commit
that referenced
this pull request
Jun 29, 2026
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).
16bit-ykiko
added a commit
that referenced
this pull request
Jul 3, 2026
## Background Server-side error feedback is the operability foundation for v1.0 (roadmap: server task 3 "error handling & logging"). Before this PR, internal failures died silently in logs (or as silent `null` responses), users got no signal when their setup was broken, and there was no structured way to profile the server on real codebases. This PR supersedes #436 (reimplemented on current main rather than rebased — it predates the #437/#447/#449 restructures). ## Feedback channels **Anomaly (soft assertions, `src/support/anomaly.h`)** — `LOG_ANOMALY(id, fmt, ...)` for internal states that must be unreachable when clice works correctly. Debug builds abort after logging (the CI Debug matrix and local dev catch bugs earliest; `CLICE_ANOMALY_NO_TRAP` exists for tests); Release logs `[anomaly:<id>]`, pushes `window/logMessage` (master) and continues. Per-ID rate limit with a final suppression notice; the gate runs **before** format arguments are evaluated (lazy contract locked by unit tests with side-effecting args). IDs: `PCHBuildFail`, `PCMBuildFail`, `CompileFail`, `WorkerRequestFail`, `WorkerCrash`, `WorkerSpawnFail`, `PositionMapFail` (markers are the enumerator names, rendered by the reflective enum formatter and pinned by `MarkerNamesStable`). Situations reachable by user input or normal operation are deliberately **not** anomalies. That split is structural: worker build failures carry `BuildResult.has_user_errors`, and master-side dispatch failures carry `dispatch_errc` codes (`cancelled` for memory-pressure preemption, `worker_unavailable` for crash/restart windows) so `is_operational_error()` keeps them out of the anomaly channel. **Guidance** — `LOG_GUIDANCE(...)` (`[guidance]` + Warning logMessage) for user-actionable situations: no compile_commands.json, invalid initializationOptions, config problems. **LSP errors instead of silent null** — feature requests on closed documents → `Document not open`; unresolvable call/type hierarchy items → error; reversed ranges → `InvalidParams`; empty hierarchy/workspace-symbol results return `[]` instead of `null`. Out-of-range positions **clamp** per the LSP spec (character → line end, line → end of content) instead of erroring. **Guidance diagnostics** — `fill_compile_args()` returns a `CommandSource` (`CDBExact`/`IncludeGraph`/`Inferred` (reserved)/`Fallback`) and emits a per-file decision log (tiers tried, tier hit, args hash). When a *guessed* command produces file-not-found errors, the publish merges a file-top Warning explaining it (code `inferred-compile-command`, linking the quick-start guide). Exact CDB matches never get it. Config rule appends now reach synthesized fallback commands, so `-I` rules work without a CDB. **clice.toml diagnostics** — parse/type errors (Error, defaults apply) and unknown keys (Warning via a strict second decode pass, config still applies) are published on the config file's URI. Line/column ranges wait for the kotatsu TOML location feature (FIXMEs mark the re-enable points); until then diagnostics anchor at the file top. ## Logging system - **Design doc in `logging.h`**: channel selection is decision-oriented — pick by *who must act and whether it is clice's fault*. Levels, perf topics and process ownership are specified there. - **`LOG_PERF(topic, ...)`** emits greppable `[perf:<topic>] key=value` lines for profiling on real codebases: `startup` (CDB load, dep scan, index load), `index` (per-file index/merge timings, run summary, save), `cache` (PCH/PCM hit/miss with reason, evictions), `request` (per-request `wait_ms`/`total_ms` for query/build/format/compile). - **Per-process log files**: workers log only to their own `<session>/<worker>.log` (no stderr mirror), so the master log no longer duplicates worker output. Worker stderr is reserved for unexpected third-party output (sanitizers, libc asserts), relayed line-by-line into the master log by the pool. - **Crash backtraces** land in the owning process's log file (`install_crash_handler`, covers SIGABRT so `LOG_FATAL` and Debug anomaly traps too); a worker's backtrace goes *only* to its own log, verified by an integration test that SIGABRTs a worker and asserts the trace stays out of the master log. - **Startup flow** is fully logged: master banner (pid/mode/workspace), config source, session log dir, worker pool summary, CDB/dep-scan/index-load timings. ## Notable fixes uncovered along the way - `CompilationDatabase::lookup()` synthesizes a command for unknown files, so the old `results.empty()` checks were dead: the automatic include-graph header-context tier was unreachable, and headers without entries silently compiled as bare `clang`. Tier selection now uses `has_entry()`; background indexing skips Fallback-sourced files. - initializationOptions now overlay **before** `apply_defaults()`: a client-provided `cache_dir` previously didn't propagate into the derived `logging_dir`/`index_dir`. - Six unchecked `*map.to_range()` dereferences (empty-optional UB) in feature code converted to checked helpers reporting `PositionMapFail`. - Anomaly notify/trap hooks are mutex-synchronized (copy under lock, invoke outside), making reporting safe from any thread. ## Test infrastructure - `CliceClient` records `window/logMessage`; **`assert_no_anomaly()` runs in every integration teardown** (all fixtures and `make_client` sessions), scanning notifications and master/worker log files even when shutdown fails. `tests/replay.py` fails a smoke trace on any anomaly push. - E2E: guidance-diagnostic lifecycle (no CDB → appears; add CDB + restart → gone while the include error stays), fallback rule-append (no CDB + clice.toml `-I` rule → clean compile), clice.toml error/unknown-key/clear scenarios, worker SIGABRT → `WorkerCrash` anomaly + backtrace ownership, closed/unknown documents via `pytest.raises`, position/range clamping. - Unit triggers for `PositionMapFail`, `WorkerCrash`, `WorkerSpawnFail`; `dispatch_errc` classification; fallback append; lazy-evaluation and rate-limit contracts. - Named timing constants (`MTIME_GRANULARITY`/`SETTLE_TIME`/`IDLE_TIMEOUT`) replace hardcoded sleeps; yield-based workspace cleanup; log dump on test failure. ## Out of scope Module-cycle guidance diagnostics wait for the module refactor; kotatsu TOML line/column re-enable after the kotatsu-side PR merges; worker logMessage forwarding and request-correlation IDs belong to the worker-pool follow-up. ## Test plan - [x] RelWithDebInfo: 732 unit / 187 integration / 3 smoke — all green locally - [x] Debug: 732 unit / 187 integration / 3 smoke — all green locally (anomalies abort in Debug, so this run is the "zero anomalies on regular paths" acceptance) - [x] `pixi run format` clean; two rounds of 3 parallel review subagents (correctness / style / tests) — all findings fixed or triaged - [x] Full CI matrix green (Linux/macOS/Windows native, aarch64/x64/arm64 cross, editor test) Closes #436 (superseded).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nullreturns with proper LSP errors (kota::fail) for feature requests on closed documents, failed compilations, invalid positions, and unresolvable hierarchy itemswindow/logMessagenotifications for key failures (PCM/PCH build, no compile_commands.json, config warnings, worker pool errors) so integration tests can observe errors without reading server logsTester::to_local_range()helper, remove unused codeTest plan
pixi run formatclean🤖 Generated with Claude Code
Codesmith can help with this PR — just tag
@codesmithor enable autofix.Summary by CodeRabbit
New Features
Bug Fixes
Tests