Skip to content

refactor: pull-based compilation for document lifecycle - #385

Merged
16bit-ykiko merged 6 commits into
mainfrom
feature/document-lifecycle
Apr 3, 2026
Merged

16bit-ykiko merged 6 commits into
mainfrom
feature/document-lifecycle

Conversation

@16bit-ykiko

@16bit-ykiko 16bit-ykiko commented Apr 3, 2026 •

Copy link
Copy Markdown
Member

Summary

Replace the push-based compilation model with a pull-based (lazy) model where compilation is driven entirely by feature requests.

Server core (master_server.cpp/h)

  • Remove schedule_build(), run_build_drain(), debounce timers, and DocumentState flags (build_running, build_requested, drain_scheduled)
  • Remove debounce_ms config field
  • didOpen/didChange only update DocumentState and mark ast_dirty — no compilation triggered
  • didSave marks dependent docs dirty via CompileGraph::update(), invalidates PCH hashes, marks all open documents ast_dirty (header saves), and queues background indexing
  • Implement ensure_compiled(path_id) — the pull-based entry point called by forward_stateful()/forward_stateless() before every feature request:
    1. Fast-path if !ast_dirty
    2. Compile C++20 module deps via compile_graph->compile_deps()
    3. Build/reuse PCH via ensure_pch() (only attach on success)
    4. Send CompileParams to stateful worker
    5. Publish diagnostics, clear dirty, schedule indexing
    6. Generation mismatch → return false, keep dirty for retry
  • forward_stateless() now also calls compile_graph->compile_deps() before stateless requests (completion/signatureHelp)
  • Move module-implementation-unit implicit dependency handling into resolve_fn (was duplicated in run_build_drain and ensure_compiled)

CompileGraph (compile_graph.cpp/h)

  • Add compile_deps(path_id) — compiles all transitive module dependencies but NOT the file itself (used for plain .cpp files that import modules)
  • Unify compile/compile_deps via compile_impl(path_id, ancestors, dispatch_self) parameter
  • compile_deps compiles dependencies concurrently via when_all
  • Extract finish() lambda to deduplicate compiling=false; completion->set() cleanup across all exit paths
  • Use std::ranges::remove instead of legacy std::remove

Test infrastructure (conftest.py)

  • open_and_wait() now sends a hover request to trigger ensure_compiled() (pull-based model requires a feature request to compile)
  • Fix URI handling: send percent-encoded URI on the wire, normalize for internal lookups, store diagnostics under both raw and normalized URI keys
  • Add _normalize_uri() helper using urllib.parse.unquote

Integration tests

  • Update all tests for pull-based model: no more waiting on didOpen diagnostics
  • _wait_for_index() sends hover to trigger compilation before polling workspace/symbol
  • test_hover_save_close simplified — hover directly triggers compilation
  • test_save_recompile and test_pch_* wait for fresh diagnostics after hover-triggered recompilation

Unit tests (compile_graph_tests.cpp)

  • Extract compiled/graph as TEST_SUITE members with std::optional<CompileGraph>
  • Extract execute(callback) helper to deduplicate event_loop boilerplate
  • Add 8 new compile_deps tests: no-deps, single dep, chain, diamond, failure, plain-cpp, concurrent dedup, resolve-once
  • Remove redundant inline on file-scope helpers

Test plan

  • Unit tests: 426 passed, 5 skipped
  • Smoke tests: 1/1 passed
  • Integration tests: 69 passed, 0 failed, no hangs

🤖 Generated with Claude Code

Replace the push-based compilation model (schedule_build/run_build_drain
with debounce timers) with a pull-based model where compilation is
triggered lazily by feature requests via ensure_compiled().

- didOpen/didChange only update DocumentState and mark ast_dirty
- didSave marks dependent documents dirty and queues indexing
- ensure_compiled() handles PCM deps, PCH, and AST compilation on demand
- Add CompileGraph::compile_deps() to compile module dependencies
  without compiling the file itself
- Update integration tests for pull-based model
- Fix URI encoding mismatch in test infrastructure

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Apr 3, 2026 •

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Replaces debounce-driven push builds with a pull-based on-demand compilation flow. Adds CompileGraph::compile_deps(path_id) and a dispatch_self flag to control deps-only recursion, removes per-document debounce timers, updates MasterServer to call ensure_compiled(path_id), and adjusts tests to trigger compilation via LSP requests.

Changes

Cohort / File(s) Summary
Compile graph
src/server/compile_graph.h, src/server/compile_graph.cpp
Added compile_deps(std::uint32_t). Extended compile_impl(..., bool dispatch_self = true) to support deps-only recursion, refactored cleanup into a local finish() lambda, and switched update() edge removal to use std::ranges::remove.
Master server & config
src/server/master_server.h, src/server/master_server.cpp, src/server/config.h
Removed debounce/queued-build machinery and debounce_ms config. Simplified ensure_compiled to take only path_id, introduced pull-based compile flow using compile_deps and ensure_pch, and changed LSP handlers to mark ast_dirty without scheduling timers.
Tests — integration & harness
tests/conftest.py, tests/integration/..., tests/unit/server/compile_graph_tests.cpp
Added URI percent-decoding normalization and updated test harness. Integration tests now trigger compilation/indexing via LSP requests (hover/completion) instead of waiting on diagnostics; unit tests refactored, migrated to std::ranges, and extended to cover compile_deps semantics and concurrency.
Integration tests adjustments
tests/integration/test_index.py, tests/integration/test_file_operation.py, tests/integration/test_modules.py, tests/integration/test_pch.py
Updated helper signatures to accept URIs and replaced diagnostic-wait sync with explicit LSP requests (hover/completion) to provoke compilation; removed sleeps and explicit save-driven build sequences.
Unit tests refactor
tests/unit/server/compile_graph_tests.cpp
Consolidated shared test state and event-loop scaffolding, renamed tests, added many compile_deps scenarios (no-deps, chains, diamond, failure propagation, concurrency/deduplication), and switched queries to std::ranges/std::views.

Sequence Diagram

sequenceDiagram
    participant Client as LSP Client
    participant MS as MasterServer
    participant CG as CompileGraph
    participant Pool as CompilationPool

    Client->>MS: hover/completion request
    MS->>MS: ensure_compiled(path_id)
    Note over MS: check DocumentState.ast_dirty

    alt doc dirty
        MS->>CG: compile_deps(path_id)
        CG->>CG: compute ancestors & deps list
        loop per dependency
            CG->>Pool: dispatch compilation for dep (compile_impl dep, dispatch_self=false)
            Pool-->>CG: compile result
        end
        CG-->>MS: deps succeeded/failed
        MS->>MS: ensure_pch(path_id)
        MS->>Pool: dispatch compilation for path_id
        Pool-->>MS: compilation result
        MS->>MS: publish diagnostics / set ast_dirty=false (on success)
    else not dirty
        Note over MS: fast-path (no compile)
    end

    MS-->>Client: respond to original LSP request
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐇 I hop through graphs and trace each thread,

Dependencies first — no timers to dread.
A hover nudges builds to wake and play,
Modules stitch, then diagnostics say “hooray.”
🥕 Small hops, tidy trees — I clear the way.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.91% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'refactor: pull-based compilation for document lifecycle' accurately describes the main architectural change: replacing push-based (debounce timer) compilation with pull-based (lazy request-driven) compilation triggered by feature requests.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/document-lifecycle

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

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…iled

- Extract finish() lambda to deduplicate compiling/completion cleanup
- Use early return to reduce nesting in compile_deps and ensure_compiled
- Flatten if/else into early-return style for result handling

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 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/master_server.cpp`:
- Around line 768-769: Stateless, module-aware requests are skipping the
preflight compilation, so update forward_stateless() to call
ensure_compiled(path_id) (and co_await its result) before handling requests like
completion/signatureHelp when the request is module-aware; if
ensure_compiled(...) returns false, return serde_raw{"null"} just like the
stateful path does. Also apply the same guard where similar stateless branches
exist (the other occurrence referenced around the second diff) so that pcm_paths
cleared by didSave() are rebuilt before servicing stateless module requests.
- Around line 376-379: The fast-path in ensure_compiled() incorrectly returns
early on !doc.ast_dirty even though didSave() can clear pch_hashes and thereby
invalidate stateful AST for headers; update the fast-path to conservatively
bypass the early return when pch_hashes were cleared/changed by a save (i.e.,
treat the TU as dirty if pch_hashes is empty/has been modified) so
hover/definition/semantic tokens rebuild using a fresh AST; locate references to
didSave(), pch_hashes and ensure_compiled()/doc.ast_dirty to implement this
change, and apply the same conservative check at the other occurrence around the
1379-1384 region.
- Around line 438-442: ensure_pch(...) may fail but the code always re-attaches
the previous PCH from pch_paths[path_id]; capture the boolean result of co_await
ensure_pch(path_id, params.path, params.text, params.directory,
params.arguments) and only set params.pch from pch_paths and pch_bounds when
that result is true; if ensure_pch returns false, clear or avoid using the
existing pch_paths[path_id] (e.g. do not assign params.pch or explicitly reset
it) so a failed rebuild cannot leave a stale PCH attached.
- Around line 469-489: The code currently returns true for a generation-mismatch
or failed compile which lets forward_stateful() run against a stale AST and
clears/marks the document clean incorrectly; change the logic so that only a
successful compile for the current generation returns true: when doc2.generation
!= gen, return false (do not clear diagnostics or set doc2.ast_dirty=false);
when result.has_value() is false, do not clear diagnostics or set
ast_dirty=false and return false so the compile loop can retry; only after
publish_diagnostics(...) for the matching generation set doc2.ast_dirty = false,
schedule_indexing(), and return true; reference ensure_compiled(),
forward_stateful(), publish_diagnostics(), clear_diagnostics(), and
doc2.ast_dirty to locate and update the branches.

In `@tests/conftest.py`:
- Around line 75-78: The test client currently unquotes URIs before sending and
only stores diagnostics under a single key; change sends the percent-encoded URI
produced by Path.as_uri() directly when calling didOpen/other wire requests (do
not unquote before sending) and update the diagnostics handling in
_normalize_uri / diagnostics storage so that when receiving diagnostics you
store them under both the raw server-provided URI and the normalized form (i.e.,
keep diagnostics[uri] and also diagnostics[self._normalize_uri(uri)]) so lookups
succeed for either representation; ensure diagnostics_events uses the same
dual-key logic (references: _normalize_uri, diagnostics, diagnostics_events,
didOpen).

In `@tests/integration/test_modules.py`:
- Around line 192-201: The test reads client.diagnostics for leaf_uri but may
see stale diagnostics from the first compile; before calling
client.text_document_hover_async()/assert on client.diagnostics,
subscribe/capture the next diagnostics publish for leaf_uri and await it so you
only read the fresh diagnostics produced after reopening leaf.cppm—e.g.,
register a diagnostics listener or use the test harness's "next diagnostics"
helper for leaf_uri, await that event, then call
client.text_document_hover_async(...) and assert
len(client.diagnostics.get(leaf_uri, [])) == 0.

In `@tests/integration/test_pch.py`:
- Around line 44-49: The test erroneously asserts uri in client.diagnostics
which was already set by open_and_wait(), so change the test to record the prior
diagnostics (e.g., prev = client.diagnostics.get(uri)) before sending the
hover/didChange and then wait for the next diagnostics notification for that uri
(use the client's diagnostics-wait helper or loop/await until
client.diagnostics[uri] != prev) before asserting; update the code around
client.text_document_hover_async / client.diagnostics to wait for the new
diagnostics publication rather than relying on the existing entry and then close
the document as before.
🪄 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: 7dfbc840-4d34-4137-b818-3921d739be39

📥 Commits

Reviewing files that changed from the base of the PR and between c697ffc and 8b97c9f.

📒 Files selected for processing (11)
  • src/server/compile_graph.cpp
  • src/server/compile_graph.h
  • src/server/config.h
  • src/server/master_server.cpp
  • src/server/master_server.h
  • tests/conftest.py
  • tests/integration/test_file_operation.py
  • tests/integration/test_index.py
  • tests/integration/test_modules.py
  • tests/integration/test_pch.py
  • tests/unit/server/compile_graph_tests.cpp
💤 Files with no reviewable changes (1)
  • src/server/config.h

Comment thread src/server/master_server.cpp
Comment thread src/server/master_server.cpp Outdated
Comment thread src/server/master_server.cpp
Comment thread src/server/master_server.cpp
Comment thread tests/conftest.py Outdated
Comment thread tests/integration/test_modules.py
Comment thread tests/integration/test_pch.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/master_server.cpp`:
- Around line 1365-1373: The loop that marks ast_dirty for documents currently
skips the saved file by checking if(dirty_id == path_id) which prevents the
saved/open file from being marked; to fix, treat the saved path like other
dirtied entries by removing or changing the conditional so that when iterating
over dirtied (variable dirtied) you still lookup in documents and set
doc_it->second.ast_dirty = true for dirty_id equal to path_id as well; update
the loop around documents.find(dirty_id) and the check involving path_id so the
saved file's ast_dirty is set when it is present in documents.
🪄 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: f526dff5-1823-4df4-bf47-a0eeb467fbf9

📥 Commits

Reviewing files that changed from the base of the PR and between 8b97c9f and ce00694.

📒 Files selected for processing (2)
  • src/server/compile_graph.cpp
  • src/server/master_server.cpp

Comment thread src/server/master_server.cpp
16bit-ykiko and others added 2 commits April 4, 2026 01:21
Extract event_loop/schedule/run pattern into execute() helper.
Use std::optional<CompileGraph> and setup()/teardown() for shared state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove redundant setup() since TEST_SUITE creates fresh instances per
case. Remove unnecessary inline on file-scope helper functions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/unit/server/compile_graph_tests.cpp (1)

416-418: Minor: Inconsistent use of llvm::find vs ranges::find.

The file uses ranges::find elsewhere (lines 101-102, 123-125, etc.), but this test uses llvm::find. Consider using ranges::find for consistency.

♻️ Suggested fix
-        EXPECT_TRUE(llvm::find(dirtied, 1u) != dirtied.end());
-        EXPECT_TRUE(llvm::find(dirtied, 2u) != dirtied.end());
-        EXPECT_TRUE(llvm::find(dirtied, 3u) != dirtied.end());
+        EXPECT_TRUE(ranges::find(dirtied, 1u) != dirtied.end());
+        EXPECT_TRUE(ranges::find(dirtied, 2u) != dirtied.end());
+        EXPECT_TRUE(ranges::find(dirtied, 3u) != dirtied.end());
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/server/compile_graph_tests.cpp` around lines 416 - 418, The three
EXPECT_TRUE assertions use llvm::find but the test suite otherwise uses
ranges::find; update the assertions in tests/unit/server/compile_graph_tests.cpp
to use ranges::find(dirtied, Xu) instead of llvm::find so the checks remain
identical but consistent with other tests (update the three lines that call
EXPECT_TRUE(llvm::find(dirtied, 1u) != dirtied.end()) / for 2u and 3u to use
ranges::find and compare against dirtied.end()); keep the existing EXPECT_TRUE
and the dirtied variable unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/unit/server/compile_graph_tests.cpp`:
- Around line 59-73: The tests share the global vector compiled and never call
setup(), causing state leakage; fix by either (A) invoking setup() at the top of
every TEST_CASE that uses compiled/graph (e.g., add a call to setup(); as the
first statement in those tests that also use execute()/event loop), or (B)
convert to a doctest fixture: create a struct (e.g., CompileGraphFixture) with
setup() as its constructor that clears compiled and resets graph, then change
TEST_CASE to TEST_CASE_FIXTURE(CompileGraphFixture, "name") so setup runs before
each test. Ensure references to compiled, graph, setup(), and execute() remain
valid after the change.

---

Nitpick comments:
In `@tests/unit/server/compile_graph_tests.cpp`:
- Around line 416-418: The three EXPECT_TRUE assertions use llvm::find but the
test suite otherwise uses ranges::find; update the assertions in
tests/unit/server/compile_graph_tests.cpp to use ranges::find(dirtied, Xu)
instead of llvm::find so the checks remain identical but consistent with other
tests (update the three lines that call EXPECT_TRUE(llvm::find(dirtied, 1u) !=
dirtied.end()) / for 2u and 3u to use ranges::find and compare against
dirtied.end()); keep the existing EXPECT_TRUE and the dirtied variable
unchanged.
🪄 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: 72f449d3-7d7b-4cb5-8a42-42dff6ae2eb1

📥 Commits

Reviewing files that changed from the base of the PR and between ce00694 and 558668f.

📒 Files selected for processing (1)
  • tests/unit/server/compile_graph_tests.cpp

Comment thread tests/unit/server/compile_graph_tests.cpp

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

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

⚠️ Outside diff range comments (1)
tests/unit/server/compile_graph_tests.cpp (1)

494-516: ⚠️ Potential issue | 🟡 Minor

Synchronize update(1) with dispatch actually starting.

Line 513 currently relies on compiler running far enough that unit 1 is already in-flight before updater executes. If scheduling or compile() changes, graph->update(1) can run first and this test stops exercising cancellation at all. Gate updater on a dispatch_started event from gated_dispatch().

Possible hardening
 TEST_CASE(UpdateDuringCompile) {
     et::event_loop loop;
     et::event gate;
+    et::event dispatch_started;

-    auto gated_dispatch = [&gate](std::uint32_t) -> et::task<bool> {
+    auto gated_dispatch = [&gate, &dispatch_started](std::uint32_t) -> et::task<bool> {
+        dispatch_started.set();
         co_await gate.wait();
         co_return true;
     };
@@
     // Coroutine 2: update(1) while dispatch is in flight, then unblock gate.
     auto updater = [&]() -> et::task<> {
+        co_await dispatch_started.wait();
         graph->update(1);
         gate.set();
         co_return;
     };

Also applies to: 518-522

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/server/compile_graph_tests.cpp` around lines 494 - 516, The test
races because updater calls graph->update(1) before the dispatched coroutine is
actually started; modify gated_dispatch (the lambda used as the dispatch) to
signal a "dispatch_started" synchronization primitive (e.g., a second et::gate
or promise/flag) immediately when it begins, then co_await the existing gate;
change updater to wait on that dispatch_started signal before calling
graph->update(1) (then unblock the original gate as before). Reference:
gated_dispatch, updater, compiler, graph->update(1), gate.set().
🧹 Nitpick comments (1)
tests/unit/server/compile_graph_tests.cpp (1)

680-708: Force the shared dependency to stay in-flight before asserting concurrent dedup.

Lines 693-695 launch both requests, but tracking_dispatch() returns immediately. That means this still passes in a schedule where request A fully compiles dep 3 before request B reaches it, so the test only proves clean-skip reuse, not the new in-progress dedup path. Hold path 3 inside dispatch and queue the second request while it is blocked.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/server/compile_graph_tests.cpp` around lines 680 - 708, The test
currently races both compile_deps calls but tracking_dispatch(compiled)
completes immediately, so it doesn't exercise the in-flight dedup path; modify
the test (CompileDepsConcurrentDedup) so tracking_dispatch blocks when invoked
for dependency 3 (e.g., use a latch/promise/future or condition_variable inside
tracking_dispatch to keep dep 3 "in-flight"), start both graph->compile_deps(1)
and graph->compile_deps(2) while dep 3 is blocked, then release the blocker for
dep 3 only after both calls have been launched and waiting; ensure you still
record compiled entries into the compiled vector and then assert the three
unique deps were compiled exactly once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@tests/unit/server/compile_graph_tests.cpp`:
- Around line 494-516: The test races because updater calls graph->update(1)
before the dispatched coroutine is actually started; modify gated_dispatch (the
lambda used as the dispatch) to signal a "dispatch_started" synchronization
primitive (e.g., a second et::gate or promise/flag) immediately when it begins,
then co_await the existing gate; change updater to wait on that dispatch_started
signal before calling graph->update(1) (then unblock the original gate as
before). Reference: gated_dispatch, updater, compiler, graph->update(1),
gate.set().

---

Nitpick comments:
In `@tests/unit/server/compile_graph_tests.cpp`:
- Around line 680-708: The test currently races both compile_deps calls but
tracking_dispatch(compiled) completes immediately, so it doesn't exercise the
in-flight dedup path; modify the test (CompileDepsConcurrentDedup) so
tracking_dispatch blocks when invoked for dependency 3 (e.g., use a
latch/promise/future or condition_variable inside tracking_dispatch to keep dep
3 "in-flight"), start both graph->compile_deps(1) and graph->compile_deps(2)
while dep 3 is blocked, then release the blocker for dep 3 only after both calls
have been launched and waiting; ensure you still record compiled entries into
the compiled vector and then assert the three unique deps were compiled exactly
once.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 726ed29f-344c-49d5-b7fe-716051b3dc54

📥 Commits

Reviewing files that changed from the base of the PR and between 558668f and 0287921.

📒 Files selected for processing (1)
  • tests/unit/server/compile_graph_tests.cpp

- didSave: mark saved file itself as dirty (not just dependents)
- didSave: mark ALL open docs dirty after pch_hashes.clear() for
  header saves not tracked by CompileGraph
- ensure_compiled: return false on generation mismatch (stale AST)
- ensure_pch: only attach PCH when build succeeds
- forward_stateless: compile module deps before stateless requests
- Fix URI encoding: send encoded URI on wire, normalize for lookups
- Tests: wait for fresh diagnostics after reopen/hover

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

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

⚠️ Outside diff range comments (1)
src/server/master_server.cpp (1)

830-834: ⚠️ Potential issue | 🟠 Major

Guard stateless PCH attachment on rebuild success.

Line 831 awaits ensure_pch(...) but ignores its boolean result, and Lines 832-834 still attach any cached PCH. If rebuild fails, completion/signatureHelp can run with stale preamble state.

Proposed fix
-    // Ensure PCH is available for stateless compilation (completion/signatureHelp).
-    co_await ensure_pch(path_id, path, wp.text, wp.directory, wp.arguments);
-    if(auto pch_it = pch_paths.find(path_id); pch_it != pch_paths.end()) {
-        wp.pch = {pch_it->second, pch_bounds[path_id]};
-    }
+    // Ensure PCH is available for stateless compilation (completion/signatureHelp).
+    auto pch_ok = co_await ensure_pch(path_id, path, wp.text, wp.directory, wp.arguments);
+    if(pch_ok) {
+        if(auto pch_it = pch_paths.find(path_id); pch_it != pch_paths.end()) {
+            wp.pch = {pch_it->second, pch_bounds[path_id]};
+        }
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/master_server.cpp` around lines 830 - 834, ensure_pch(...) can
fail but its boolean result is ignored; only attach cached PCH to wp.pch when
ensure_pch(path_id, path, wp.text, wp.directory, wp.arguments) returns true and
the entry exists. Modify the code around the call to ensure_pch (the ensure_pch
invocation and the subsequent use of pch_paths, pch_bounds, and wp.pch) so you
capture the returned bool and conditionally set wp.pch = {pch_it->second,
pch_bounds[path_id]} only when the rebuild succeeded and pch_it !=
pch_paths.end().
♻️ Duplicate comments (1)
tests/conftest.py (1)

167-171: ⚠️ Potential issue | 🟠 Major

Use encoded URI for hover wire request as well.

open() correctly sends encoded Path.as_uri() on the wire, but open_and_wait() sends the normalized URI in HoverParams (Line 169). For files with reserved characters, this can break URI round-tripping and request targeting.

🔧 Suggested fix
-        uri, content = self.open(filepath)
+        uri, content = self.open(filepath)
+        wire_uri = filepath.as_uri()
         event = self.wait_for_diagnostics(uri)
@@
         await self.text_document_hover_async(
             HoverParams(
-                text_document=TextDocumentIdentifier(uri=uri),
+                text_document=TextDocumentIdentifier(uri=wire_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/conftest.py` around lines 167 - 171, The hover request is using the
unencoded/native `uri` so URIs with reserved characters don't round-trip; update
the `HoverParams` call in `text_document_hover_async` usage to pass an encoded
URI (the same form used by `open()`), e.g. replace
`TextDocumentIdentifier(uri=uri)` with an encoded form such as
`TextDocumentIdentifier(uri=Path(uri).as_uri())` or reuse the existing
`encoded_uri` variable so `text_document_hover_async(... HoverParams(...))`
sends the same encoded `Path.as_uri()` value as `open()`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/server/master_server.cpp`:
- Around line 830-834: ensure_pch(...) can fail but its boolean result is
ignored; only attach cached PCH to wp.pch when ensure_pch(path_id, path,
wp.text, wp.directory, wp.arguments) returns true and the entry exists. Modify
the code around the call to ensure_pch (the ensure_pch invocation and the
subsequent use of pch_paths, pch_bounds, and wp.pch) so you capture the returned
bool and conditionally set wp.pch = {pch_it->second, pch_bounds[path_id]} only
when the rebuild succeeded and pch_it != pch_paths.end().

---

Duplicate comments:
In `@tests/conftest.py`:
- Around line 167-171: The hover request is using the unencoded/native `uri` so
URIs with reserved characters don't round-trip; update the `HoverParams` call in
`text_document_hover_async` usage to pass an encoded URI (the same form used by
`open()`), e.g. replace `TextDocumentIdentifier(uri=uri)` with an encoded form
such as `TextDocumentIdentifier(uri=Path(uri).as_uri())` or reuse the existing
`encoded_uri` variable so `text_document_hover_async(... HoverParams(...))`
sends the same encoded `Path.as_uri()` value as `open()`.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f42a1c5d-1256-47f6-ac65-2680d79b7a37

📥 Commits

Reviewing files that changed from the base of the PR and between 0287921 and 4635c14.

📒 Files selected for processing (4)
  • src/server/master_server.cpp
  • tests/conftest.py
  • tests/integration/test_modules.py
  • tests/integration/test_pch.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/integration/test_pch.py
  • tests/integration/test_modules.py

- Extract ensure_deps() to share module deps / PCH / PCM filling
  between ensure_compiled() and forward_stateless()
- Remove setup() since TEST_SUITE members are fresh per test case
- Remove redundant `inline` on file-scope test helpers

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant