feat: initial CompileGraph integration into MasterServer

[16bit-ykiko]

Summary

Initial integration of CompileGraph (#375) into MasterServer, enabling basic end-to-end C++20 module support: on-demand PCM building, dependency-ordered compilation, cascade invalidation on save, and diagnostic integration.

This is a first-pass implementation — the core pipeline works, but there are known areas for follow-up:

• PCM files go to system temp dir instead of .clice/cache/; no disk cleanup on invalidation
• run_build_drain scans imports itself rather than delegating fully to CompileGraph
• No incremental/partial rebuild (full PCM rebuild on any change)
• Cycle detection is tested at unit level but integration-level coverage is minimal

Changes

Module dependency compilation (master_server.cpp)

Before sending a file to the stateful worker, run_build_drain now:

1. Scans imports via scan_precise() to discover module dependencies
2. Compiles each dep through compile_graph->compile(), which recursively builds transitive PCMs
3. Handles implementation units — module M; implicitly needs the interface PCM
4. Passes all built PCMs to the stateful worker, excluding the file's own PCM
5. Skips compile on dep failure and resets build_running / drain_scheduled
6. Re-lookups iterators after co_await to avoid use-after-invalidation

Cascade invalidation (didSave / didClose)

• didSave: calls compile_graph->update() to mark transitive dependents dirty, removes stale PCM paths, schedules rebuilds for open dirtied files
• didClose: cancels in-flight compilations for the closed file

Other fixes in this PR

• Debounce timers switched to shared_ptr to prevent use-after-free when didClose destroys the timer mid-wait
• fill_compile_args returns bool; callers handle empty CDB gracefully
• Adapt all PositionMapper call sites to the new optional return API from eventide

Test plan

• 25 C++ unit tests for CompileGraph (cycles, partial failure, cancel, update, empty graph)
• 24 C++ integration tests with real clang PCM compilation
• 3 worker-level module tests (BuildPCM, PCM-dependent compile, multi-module)
• 26 Python LSP integration tests (single module through circular deps, hover, error diagnostics)
• 371 unit tests + 54 integration tests pass

🤖 Generated with Claude Code

[coderabbitai]

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

Adds C++20 module-aware compilation to the server: a lazy CompileGraph, module provenance and PCM tracking, precompilation of imported modules during build/drain, safer LSP position-to-offset mapping, shared debounce timers, protocol/result payload updates, and extensive tests/fixtures for module scenarios.

Changes

Cohort / File(s)	Summary
Master server core src/server/master_server.cpp, src/server/master_server.h	Introduce CompileGraph, path_to_module, pcm_paths; switch per-document timers to std::shared_ptr<et::timer>; add SafePositionMapper use; make fill_compile_args() return bool; update run_build_drain() to precompile module imports and populate transitive PCM paths; invalidate/clear PCM entries on save/close.
Protocol / Stateless worker src/server/protocol.h, src/server/stateless_worker.cpp	Extend clice::worker::BuildPCMResult with std::string pcm_path; stateless worker now returns PCM output path on BuildPCM success (empty on failure).
Compile argument & drain robustness src/server/master_server.cpp (debounce/timer capture, fill_compile_args callers)	Capture shared timer before awaiting to prevent destruction mid-wait; make compile-argument preparation fail-fast (abort when CDB/toolchain lookup fails); skip file’s own PCM when populating pcm lists.
Tests — Python integration & CDB tests/conftest.py, tests/integration/test_modules.py	Fix compile_commands.json generation (use Path.as_posix(), split arguments) and add comprehensive async integration tests covering many module layouts, imports, invalidation/recompile scenarios, hovers, and negative cases.
Tests — C++ unit/integration tests/unit/server/compile_graph_integration_tests.cpp, tests/unit/server/module_worker_tests.cpp, tests/unit/server/compile_graph_tests.cpp	Add CompileGraph in-process integration tests, worker-based PCM/compile tests, and new unit tests for selective dispatch, update/cancel behavior, and partial-failure scenarios.
Test fixtures — module sources tests/data/modules/... (many new files)	Add numerous module interface/partition/consumer fixtures (chained, diamond, dotted names, partitions, GMF/private fragments, re-exports, templates, implementation units, circular deps, error cases) used by the new tests.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant Server as MasterServer
    participant CG as CompileGraph
    participant CDB as CompilationDatabase
    participant Worker as Worker
    participant FS as FileSystem

    Client->>Server: textDocument/didOpen / didSave / didClose
    activate Server

    Server->>CG: resolve/update(path_id) (lazy)
    activate CG
    CG->>CDB: scan_precise / lookup file & imports
    CDB-->>CG: file path & imports
    CG-->>Server: required module nodes / dirtied units
    deactivate CG

    Server->>Server: erase stale pcm_paths for dirtied units

    loop for each required module node
        Server->>CG: compile(module_node)
        activate CG
        CG->>Server: request BuildPCM (with transitive pcms)
        Server->>Worker: BuildPCMParams (pcms)
        Worker->>FS: invoke compiler -> produce PCM
        Worker-->>Server: BuildPCMResult (pcm_path)
        Server->>CG: provide pcm_path
        deactivate CG
    end

    Server->>Worker: CompileParams for consumer (with all pcms)
    Worker->>FS: compile consumer source
    Worker-->>Server: compile result / diagnostics

    Server-->>Client: publish diagnostics / responses
    deactivate Server

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• feat: add CompileGraph for pull-based module dependency compilation #375: Implements the CompileGraph core (compile/update APIs) that this change integrates and invokes.
• refactor: simplify CompilationDatabase, extract ArgumentParser, remove pimpl #371: Refactors CompilationDatabase/toolchain lookup APIs used by MasterServer; related to fill_compile_args and CDB lookup changes.
• feat: add include resolver, dependency graph, BFS scanner #368: Adds dependency-graph/PathPool scanning used to build the path_to_module mapping and resolve imports.

Poem

🐇 I hopped through headers, traced each name,
I stitched the graph and sparked the compile flame,
PCMs appeared, neat piles in a row,
Consumers smiled when imports stole the show,
A crunchy carrot for each successful go 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.58% 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'feat: initial CompileGraph integration into MasterServer' directly describes the main objective of the changeset: integrating CompileGraph functionality into the MasterServer for C++20 module support.

_{✏️ 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 feat/compile-graph-integration

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

tests/integration/test_modules.py (1)

71-83: Consider making the initialization sleep configurable.

The 1-second sleep is hardcoded. For slower CI environments or complex workspaces, this might not be sufficient.

💡 Optional: Make sleep duration configurable

-async def _init(client, workspace: Path):
+async def _init(client, workspace: Path, init_delay: float = 1.0):
     """Initialize the LSP server with a workspace."""
     result = await client.initialize_async(
         InitializeParams(
             capabilities=ClientCapabilities(),
             root_uri=workspace.as_uri(),
             workspace_folders=[WorkspaceFolder(uri=workspace.as_uri(), name="test")],
         )
     )
     client.initialized(InitializedParams())
     # Give the server time to load CDB and scan dependency graph.
-    await asyncio.sleep(1.0)
+    await asyncio.sleep(init_delay)
     return result

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/test_modules.py` around lines 71 - 83, The hardcoded 1.0s
delay in _init can fail on slow CI; make it configurable by adding an optional
parameter (e.g., init_delay: float = 1.0) to _init and use that value instead of
the literal in the asyncio.sleep call, or read a fallback from an environment
variable (e.g., os.getenv("TEST_INIT_SLEEP")). Update any callers of _init in
tests to use the new parameter when needed and preserve the current default so
existing tests keep the same behavior; reference _init, client.initialize_async,
and asyncio.sleep when locating the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/integration/test_modules.py`:
- Around line 71-83: The hardcoded 1.0s delay in _init can fail on slow CI; make
it configurable by adding an optional parameter (e.g., init_delay: float = 1.0)
to _init and use that value instead of the literal in the asyncio.sleep call, or
read a fallback from an environment variable (e.g.,
os.getenv("TEST_INIT_SLEEP")). Update any callers of _init in tests to use the
new parameter when needed and preserve the current default so existing tests
keep the same behavior; reference _init, client.initialize_async, and
asyncio.sleep when locating the change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3f03ec60-ec62-4aea-acbf-6026f0d54d44

📥 Commits

Reviewing files that changed from the base of the PR and between 7ed558c and 7caa188.

📒 Files selected for processing (8)

• src/server/master_server.cpp
• src/server/master_server.h
• src/server/protocol.h
• src/server/stateless_worker.cpp
• tests/conftest.py
• tests/integration/test_modules.py
• tests/unit/server/compile_graph_integration_tests.cpp
• tests/unit/server/module_worker_tests.cpp

[coderabbitai]

🧹 Nitpick comments (2)

src/server/master_server.cpp (2)

118-127: Consider logging when multiple files provide the same module.

When mod_ids contains multiple entries (multiple files exporting the same module name), only mod_ids[0] is compiled. This is likely intentional, but if it occurs, it might indicate a configuration issue worth logging.

💡 Optional: Add debug logging for ambiguous module providers

                 for(auto& mod_name: scan_result.modules) {
                     auto mod_ids = dependency_graph.lookup_module(mod_name);
+                    if(mod_ids.size() > 1) {
+                        LOG_DEBUG("Module {} has {} providers, using first", mod_name, mod_ids.size());
+                    }
                     if(!mod_ids.empty()) {
                         auto r = co_await compile_graph->compile(mod_ids[0]).catch_cancel();

🤖 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 118 - 127, When
dependency_graph.lookup_module(scan_result.modules) returns multiple providers,
the code currently just compiles mod_ids[0] which can hide ambiguous exports;
update the loop in which scan_result.modules is iterated to detect when
mod_ids.size() > 1 and emit a debug/warn log including the module name and the
list of provider IDs (or file names) before proceeding to compile via
compile_graph->compile(mod_ids[0]) so operators can investigate configuration
issues (look for symbols: scan_result.modules, dependency_graph.lookup_module,
mod_ids, compile_graph->compile, mod_ids[0]).

---

690-700: Minor comment clarification.

The comment on line 692 says "rebuilt by its own didChange", but didSave doesn't necessarily follow a didChange. The saved file would be rebuilt via the normal didOpen/didChange flow if it's open. The skip is still correct (avoid double-scheduling), but the comment could be slightly misleading.

📝 Optional: Clarify the comment

             for(auto dirty_id: dirtied) {
                 if(dirty_id == path_id)
-                    continue;  // The saved file itself is rebuilt by its own didChange.
+                    continue;  // The saved file itself; already scheduled via didOpen/didChange.
                 if(documents.count(dirty_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 690 - 700, The comment on the skip
in the loop (around the dirtied/path_id check) is misleading: change the comment
for the if(dirty_id == path_id) continue; to say that we skip scheduling the
saved file to avoid double-scheduling because it may already be rebuilt via
open/change notifications (didOpen/didChange) when the document is open, rather
than asserting it is rebuilt by its own didChange; keep the existing skip
behavior and refer to symbols dirtied, path_id, didSave, didOpen, didChange, and
schedule_build when updating the comment.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/server/master_server.cpp`:
- Around line 118-127: When dependency_graph.lookup_module(scan_result.modules)
returns multiple providers, the code currently just compiles mod_ids[0] which
can hide ambiguous exports; update the loop in which scan_result.modules is
iterated to detect when mod_ids.size() > 1 and emit a debug/warn log including
the module name and the list of provider IDs (or file names) before proceeding
to compile via compile_graph->compile(mod_ids[0]) so operators can investigate
configuration issues (look for symbols: scan_result.modules,
dependency_graph.lookup_module, mod_ids, compile_graph->compile, mod_ids[0]).
- Around line 690-700: The comment on the skip in the loop (around the
dirtied/path_id check) is misleading: change the comment for the if(dirty_id ==
path_id) continue; to say that we skip scheduling the saved file to avoid
double-scheduling because it may already be rebuilt via open/change
notifications (didOpen/didChange) when the document is open, rather than
asserting it is rebuilt by its own didChange; keep the existing skip behavior
and refer to symbols dirtied, path_id, didSave, didOpen, didChange, and
schedule_build when updating the comment.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fcad4a81-9f2d-4bbf-9eb2-d9f5c6f83951

📥 Commits

Reviewing files that changed from the base of the PR and between 7caa188 and f4e853a.

📒 Files selected for processing (4)

• src/server/master_server.cpp
• src/server/master_server.h
• tests/integration/test_modules.py
• tests/unit/server/compile_graph_tests.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/integration/test_modules.py

[coderabbitai]

🧹 Nitpick comments (1)

tests/unit/server/compile_graph_integration_tests.cpp (1)

180-188: Unnecessary this capture in test lambdas.

The this capture in the test lambda appears unused. While harmless, removing it would improve clarity.

✨ Suggested cleanup

-    auto test = [this, &cg, &env, pid_a]() -> et::task<> {
+    auto test = [&cg, &env, pid_a]() -> et::task<> {

This pattern repeats throughout the test file.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/server/compile_graph_integration_tests.cpp` around lines 180 -
188, The lambda named test is capturing this unnecessarily; remove this from the
capture list (i.e., change [this, &cg, &env, pid_a] to [&cg, &env, pid_a]) and
update any other similar test lambdas in
tests/unit/server/compile_graph_integration_tests.cpp to drop the unused this
capture so captures only include cg, env, pid_a (and any other actually used
variables) before scheduling with loop.schedule(...) and running with
loop.run().

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/unit/server/compile_graph_integration_tests.cpp`:
- Around line 180-188: The lambda named test is capturing this unnecessarily;
remove this from the capture list (i.e., change [this, &cg, &env, pid_a] to
[&cg, &env, pid_a]) and update any other similar test lambdas in
tests/unit/server/compile_graph_integration_tests.cpp to drop the unused this
capture so captures only include cg, env, pid_a (and any other actually used
variables) before scheduling with loop.schedule(...) and running with
loop.run().

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d756e3d2-ccc7-468b-94eb-cc2f22e35a2d

📥 Commits

Reviewing files that changed from the base of the PR and between f4e853a and 479e3b5.

📒 Files selected for processing (66)

• src/server/master_server.cpp
• tests/data/modules/chained_modules/mod_a.cppm
• tests/data/modules/chained_modules/mod_b.cppm
• tests/data/modules/circular_module_dependency/cycle_a.cppm
• tests/data/modules/circular_module_dependency/cycle_b.cppm
• tests/data/modules/circular_module_dependency/ok.cppm
• tests/data/modules/class_export_and_inheritance/circle.cppm
• tests/data/modules/class_export_and_inheritance/shape.cppm
• tests/data/modules/consumer_imports_module/main.cpp
• tests/data/modules/consumer_imports_module/math.cppm
• tests/data/modules/deep_chain/m1.cppm
• tests/data/modules/deep_chain/m2.cppm
• tests/data/modules/deep_chain/m3.cppm
• tests/data/modules/deep_chain/m4.cppm
• tests/data/modules/deep_chain/m5.cppm
• tests/data/modules/diamond_modules/base.cppm
• tests/data/modules/diamond_modules/left.cppm
• tests/data/modules/diamond_modules/right.cppm
• tests/data/modules/diamond_modules/top.cppm
• tests/data/modules/dotted_module_name/app.cppm
• tests/data/modules/dotted_module_name/io.cppm
• tests/data/modules/export_block/block.cppm
• tests/data/modules/export_block/consumer.cppm
• tests/data/modules/export_namespace/calc.cppm
• tests/data/modules/export_namespace/ns.cppm
• tests/data/modules/global_module_fragment/gmf.cppm
• tests/data/modules/global_module_fragment/legacy.h
• tests/data/modules/gmf_with_import/base.cppm
• tests/data/modules/gmf_with_import/combined.cppm
• tests/data/modules/gmf_with_import/util.h
• tests/data/modules/hover_on_imported_symbol/defs.cppm
• tests/data/modules/hover_on_imported_symbol/use.cpp
• tests/data/modules/independent_modules/x.cppm
• tests/data/modules/independent_modules/y.cppm
• tests/data/modules/module_compile_error/bad.cppm
• tests/data/modules/module_compile_error/good.cppm
• tests/data/modules/module_implementation_unit/greeter.cppm
• tests/data/modules/module_implementation_unit/greeter_impl.cpp
• tests/data/modules/module_partitions/lib.cppm
• tests/data/modules/module_partitions/part_a.cppm
• tests/data/modules/module_partitions/part_b.cppm
• tests/data/modules/no_modules_plain_cpp/plain.cpp
• tests/data/modules/partition_chain/core.cppm
• tests/data/modules/partition_chain/sys.cppm
• tests/data/modules/partition_chain/types.cppm
• tests/data/modules/partition_interface/part.cppm
• tests/data/modules/partition_interface/primary.cppm
• tests/data/modules/partition_with_external_import/app.cppm
• tests/data/modules/partition_with_external_import/ext.cppm
• tests/data/modules/partition_with_external_import/part.cppm
• tests/data/modules/partition_with_gmf/cfg.cppm
• tests/data/modules/partition_with_gmf/config.h
• tests/data/modules/partition_with_gmf/part_cfg.cppm
• tests/data/modules/private_module_fragment/priv.cppm
• tests/data/modules/re_export/core.cppm
• tests/data/modules/re_export/user.cppm
• tests/data/modules/re_export/wrapper.cppm
• tests/data/modules/save_recompile/leaf.cppm
• tests/data/modules/save_recompile/mid.cppm
• tests/data/modules/single_module_no_deps/mod_a.cppm
• tests/data/modules/template_export/tmpl.cppm
• tests/data/modules/template_export/use_tmpl.cppm
• tests/integration/test_modules.py
• tests/unit/server/compile_graph_integration_tests.cpp
• tests/unit/server/compile_graph_tests.cpp
• tests/unit/server/module_worker_tests.cpp

✅ Files skipped from review due to trivial changes (47)

• tests/data/modules/partition_with_gmf/config.h
• tests/data/modules/gmf_with_import/util.h
• tests/data/modules/save_recompile/mid.cppm
• tests/data/modules/global_module_fragment/legacy.h
• tests/data/modules/re_export/core.cppm
• tests/data/modules/partition_with_external_import/app.cppm
• tests/data/modules/save_recompile/leaf.cppm
• tests/data/modules/module_compile_error/bad.cppm
• tests/data/modules/circular_module_dependency/cycle_b.cppm
• tests/data/modules/module_compile_error/good.cppm
• tests/data/modules/hover_on_imported_symbol/use.cpp
• tests/data/modules/partition_with_gmf/cfg.cppm
• tests/data/modules/partition_interface/part.cppm
• tests/data/modules/module_partitions/part_a.cppm
• tests/data/modules/deep_chain/m4.cppm
• tests/data/modules/dotted_module_name/io.cppm
• tests/data/modules/diamond_modules/left.cppm
• tests/data/modules/circular_module_dependency/ok.cppm
• tests/data/modules/chained_modules/mod_a.cppm
• tests/data/modules/deep_chain/m1.cppm
• tests/data/modules/independent_modules/y.cppm
• tests/data/modules/single_module_no_deps/mod_a.cppm
• tests/data/modules/class_export_and_inheritance/shape.cppm
• tests/data/modules/partition_chain/types.cppm
• tests/data/modules/deep_chain/m5.cppm
• tests/data/modules/no_modules_plain_cpp/plain.cpp
• tests/data/modules/template_export/use_tmpl.cppm
• tests/data/modules/consumer_imports_module/math.cppm
• tests/data/modules/export_block/block.cppm
• tests/data/modules/partition_with_gmf/part_cfg.cppm
• tests/data/modules/independent_modules/x.cppm
• tests/data/modules/deep_chain/m2.cppm
• tests/data/modules/partition_interface/primary.cppm
• tests/data/modules/chained_modules/mod_b.cppm
• tests/data/modules/partition_with_external_import/ext.cppm
• tests/data/modules/gmf_with_import/base.cppm
• tests/data/modules/partition_with_external_import/part.cppm
• tests/data/modules/class_export_and_inheritance/circle.cppm
• tests/data/modules/deep_chain/m3.cppm
• tests/data/modules/module_partitions/part_b.cppm
• tests/data/modules/dotted_module_name/app.cppm
• tests/data/modules/diamond_modules/base.cppm
• tests/data/modules/hover_on_imported_symbol/defs.cppm
• tests/data/modules/export_namespace/ns.cppm
• tests/data/modules/template_export/tmpl.cppm
• tests/data/modules/partition_chain/core.cppm
• tests/integration/test_modules.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/unit/server/compile_graph_tests.cpp

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/server/master_server.cpp (1)

143-154: Consider logging when scan_precise fails or returns no CDB results.

If cdb.lookup() returns empty results (line 143), the code silently skips module dependency compilation. While this might be intentional for files without CDB entries, a debug log could help diagnose unexpected build failures.

💡 Suggested enhancement

         bool deps_ok = true;
-        if(!cdb_results.empty()) {
+        if(cdb_results.empty()) {
+            LOG_DEBUG("No CDB entry for {}, skipping module scan", file_path);
+        } else {
             auto scan_result = scan_precise(cdb_results[0].arguments, cdb_results[0].directory);

🤖 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 143 - 154, When cdb_results is
empty or scan_precise produces no modules the code silently skips dependency
compilation; update the block around cdb_results, scan_precise, and the
dependency loop to emit a debug/info log when cdb_results.empty() or when
scan_precise returns an empty/failed result so unexpected misses are visible;
specifically add a log entry referencing the file/path or the inputs used (use
existing logger in this module) before skipping, and also log any error/empty
condition returned by scan_precise(mods) (symbols: cdb_results, scan_precise,
dependency_graph.lookup_module, compile_graph->compile, deps_ok) so maintainers
can see why compilation was skipped.

🤖 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 348-381: Add deterministic teardown to avoid use-after-free:
implement an explicit MasterServer destructor (~MasterServer()) that calls
compile_graph->cancel_all() (or equivalent cancel method) before the
compile_graph unique_ptr is destroyed, ensuring no queued lambdas
(dispatch/resolve) will run after MasterServer begins destruction; and implement
an explicit CompileGraph destructor (~CompileGraph()) that cancels remaining
work and synchronously waits for all pending et::task instances to finish or be
cancelled (e.g. signal cancellation and join/wait any worker futures/promises)
before returning, so stored lambdas that capture this are never invoked after
CompileGraph/MasterServer are destroyed. Ensure the destructors reference the
existing methods/fields (compile_graph, cancel_all, dispatch/resolve task
queues) to locate the implementation points.

---

Nitpick comments:
In `@src/server/master_server.cpp`:
- Around line 143-154: When cdb_results is empty or scan_precise produces no
modules the code silently skips dependency compilation; update the block around
cdb_results, scan_precise, and the dependency loop to emit a debug/info log when
cdb_results.empty() or when scan_precise returns an empty/failed result so
unexpected misses are visible; specifically add a log entry referencing the
file/path or the inputs used (use existing logger in this module) before
skipping, and also log any error/empty condition returned by scan_precise(mods)
(symbols: cdb_results, scan_precise, dependency_graph.lookup_module,
compile_graph->compile, deps_ok) so maintainers can see why compilation was
skipped.

🪄 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: 0fb798ff-e85d-4456-95c9-b720b6ec0015

📥 Commits

Reviewing files that changed from the base of the PR and between 479e3b5 and d81ce78.

📒 Files selected for processing (2)

• src/server/master_server.cpp
• tests/integration/test_modules.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/integration/test_modules.py

[coderabbitai]

🧹 Nitpick comments (1)

src/server/master_server.cpp (1)

390-392: Consider adding explicit co_return; to silence static analysis warning.

The static analysis tool flags a missing return statement. While falling off the end of a coroutine returning et::task<> is valid C++20 behavior (equivalent to co_return;), an explicit statement would silence the warning and make the intent clearer.

     compile_graph = std::make_unique<CompileGraph>(std::move(dispatch), std::move(resolve));
     LOG_INFO("CompileGraph initialized with {} module(s)", path_to_module.size());
+    co_return;
 }

🤖 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 390 - 392, The coroutine that
constructs compile_graph (where you call
std::make_unique<CompileGraph>(std::move(dispatch), std::move(resolve)) and then
LOG_INFO("CompileGraph initialized with {} module(s)", path_to_module.size()))
returns et::task<>; add an explicit co_return; as the final statement in that
function to silence the static-analysis warning and make the intent clear (i.e.,
append a single co_return; after the LOG_INFO line in the function that sets
compile_graph).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/server/master_server.cpp`:
- Around line 390-392: The coroutine that constructs compile_graph (where you
call std::make_unique<CompileGraph>(std::move(dispatch), std::move(resolve)) and
then LOG_INFO("CompileGraph initialized with {} module(s)",
path_to_module.size())) returns et::task<>; add an explicit co_return; as the
final statement in that function to silence the static-analysis warning and make
the intent clear (i.e., append a single co_return; after the LOG_INFO line in
the function that sets compile_graph).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: edef0aa4-92bc-435e-839a-d404c852b065

📥 Commits

Reviewing files that changed from the base of the PR and between d81ce78 and 6b7b37a.

📒 Files selected for processing (1)

• src/server/master_server.cpp