refactor(server, tests): improve error feedback, logging, and test infrastructure#436
Open
16bit-ykiko wants to merge 1 commit intomainfrom
Open
refactor(server, tests): improve error feedback, logging, and test infrastructure#43616bit-ykiko wants to merge 1 commit intomainfrom
16bit-ykiko wants to merge 1 commit intomainfrom
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 <[email protected]>
📝 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 |
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.
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