feat: header context protocol — queryContext, currentContext, switchContext#398
Merged
16bit-ykiko merged 11 commits intomainfrom Apr 6, 2026
Merged
feat: header context protocol — queryContext, currentContext, switchContext#39816bit-ykiko merged 11 commits intomainfrom
16bit-ykiko merged 11 commits intomainfrom
Conversation
Header files without compile_commands.json entries can now get hover, completion, diagnostics, and other IDE features by inferring compilation context from a host source file that includes them. Implementation: - Add reverse include map to DependencyGraph (header → includer mapping) - Add find_host_sources() BFS to find source files that include a header - Add find_include_chain() to trace the include path from host to target - Add resolve_header_context() to generate preamble files with #line directives preserving correct source locations - Modify fill_compile_args() to fall back to header context when CDB lookup fails: uses host's compile args + -include <preamble> - Cache preamble files in <cache_dir>/header_context/<hash>.h - Invalidate header contexts when host file is saved Tests: - test_hover_in_header_file: hover on function in header returns result - test_diagnostics_in_header_file: valid header gets zero diagnostics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds header-file context resolution: reverse-include graph traversal, host-source discovery, preamble construction and disk caching, compile-argument injection for headers without CDB entries, save-driven cache invalidation, and three LSP extension requests to query/switch/inspect header contexts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as LSP Client
participant Server as MasterServer
participant CDB as CompilationDatabase
participant DepGraph as DependencyGraph
participant Cache as Preamble Cache
Client->>Server: request fill_compile_args(header_path)
Server->>CDB: has_entry(header_path)
alt CDB has entry
CDB-->>Server: compile_args
Server-->>Client: compile_args
else no CDB entry
Server->>DepGraph: find_host_sources(header_path)
DepGraph-->>Server: [host_paths]
Server->>DepGraph: find_include_chain(host_path, header_path)
DepGraph-->>Server: include_chain
Server->>Server: build_preamble(include_chain)
Server->>Server: hash_preamble
Server->>Cache: write/check preamble/<hash>.h
Cache-->>Server: preamble_path
Server->>CDB: get_toolchain(host_path)
CDB-->>Server: host_compile_args
Server->>Server: inject -include preamble_path into args
Server-->>Client: compile_args (with preamble)
end
sequenceDiagram
participant Client as LSP Client
participant Server as MasterServer
participant DepGraph as DependencyGraph
Client->>Server: clice/queryContext(header_uri)
Server->>DepGraph: find_host_sources(header_path_id)
DepGraph-->>Server: [host_1, host_2, ...]
Server-->>Client: QueryContextResult{contexts, total}
Client->>Server: clice/switchContext(header_uri, context_uri)
Server->>Server: validate context (CDB entry)
Server->>Server: update active_contexts, erase header cache, invalidate pch/ast_deps
Server-->>Client: SwitchContextResult{success:true}
Client->>Server: textDocument/didSave(host_uri)
Server->>Server: erase header_file_contexts with host_path_id == saved_path_id
Server-->>Client: (log + mark ast_dirty for affected docs)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🤖 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 2195-2209: The current invalidation only checks
header_file_contexts[*].host_path_id and misses cases where the saved file is an
intermediate contributor to a preamble; update the preamble tracking so each
header_file_context records all contributing path IDs (e.g., add a contributors
set/list to the header context structure used by header_file_contexts) and
change the invalidation loop to mark any hdr_ctx whose contributors contain the
saved path_id as stale (and set ast_dirty on documents as before); additionally,
after saves that can affect includes, invoke the workspace include-graph rebuild
(the same logic performed in load_workspace() or a new rebuild_include_graph()
helper) to refresh the reverse-include map so future resolution uses the current
include edges.
- Around line 624-656: The current scan uses line.contains(next_filename) which
can falsely match substrings; instead parse the include directive on each
candidate line (handle both `#include` and "# include" with arbitrary spacing,
angle brackets <> and quoted "..." forms) by extracting the exact include target
token, normalize it, and resolve that token to the intended include ID (next_id)
using the same include-resolution logic/lookup you use elsewhere (do not rely on
substring matching of next_filename); if the resolved ID equals next_id then set
include_line_start and break, otherwise continue scanning—update references to
next_filename/ include_line_start/content_ref/preamble/cur_path/LOG_DEBUG
accordingly so the fallback full-file emission only happens when no real include
resolved to next_id.
- Around line 568-590: The resolve_header_context function currently selects a
host by iterating hosts and only stores host_path_id and chain, then later
re-looks up a compile command with cdb.lookup(host_path).front(), which can
produce a non-deterministic or mismatched compile command; modify the selection
loop in MasterServer::resolve_header_context to also capture and store the
specific compile-command/config returned by cdb.lookup (e.g., the chosen
CdbEntry or CompileCommand object) when you pick host_path_id and chain, and
return that stored compile command inside the resulting HeaderFileContext
instead of performing a fresh cdb.lookup by path later; apply the same change to
the second similar block referenced (lines ~705-727) so both code paths pair the
exact include chain with the exact compile command chosen during selection.
- Around line 730-742: The current logic drops the last positional argument by
position (copy_count) which can remove the wrong token (e.g. the output file)
and leave dangling flags; instead iterate host_ctx.arguments and skip entries
that are actual host source file paths by identity: for each token in
host_ctx.arguments, if it is not a flag (doesn't start with '-') and its file
extension matches known source extensions (e.g. .c, .cpp, .cc, .cxx, .m, .mm,
.C, .c++), do not copy it into arguments; otherwise copy it via
arguments.emplace_back; also ensure you don't accidentally drop flag values
(handle flags like "-o" by treating them as flags whose following token should
be preserved unless that following token itself is a recognized source filename
to remove). This replaces the copy_count/back-based approach and uses
host_ctx.arguments, arguments.emplace_back, and source-extension checks to
identify/removes host source args by identity.
In `@tests/integration/test_header_context.py`:
- Line 22: The assignment to main_uri from client.open_and_wait is unused and
flagged by RUF059; update the binding to a throwaway name (e.g., _main_uri) or
drop the value (e.g., await client.open_and_wait(...)) where main_uri is
assigned (search for client.open_and_wait and the other similar assignment
around line 47) so both unused variables are prefixed with _ or removed to
satisfy the linter.
🪄 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: e84cd845-b387-4379-aa62-21883156585d
📒 Files selected for processing (8)
src/server/master_server.cppsrc/server/master_server.hsrc/syntax/dependency_graph.cppsrc/syntax/dependency_graph.htests/data/header_context/compile_commands.jsontests/data/header_context/main.cpptests/data/header_context/utils.htests/integration/test_header_context.py
- clice/queryContext: returns all possible compilation contexts for a file (header files → host source files, source files → CDB entries), paginated - clice/currentContext: returns the currently active context for a file - clice/switchContext: switches the active context, invalidates caches, triggers recompile - Non self-contained header test data (utils.h depends on host's <vector>) - 5 integration tests covering the full query→switch→verify flow - CDB lookup helper (command.h/cpp) for querying multiple compilation entries Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
16bit-ykiko
changed the title
There was a problem hiding this comment.
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 2811-2846: The handler for the "clice/switchContext" request
(inside the peer.on_request lambda) currently only checks
cdb.lookup(context_path) but never verifies that the chosen context actually
transitively includes the target header (the file identified by path), so it may
accept unrelated context files; before updating active_contexts, call or
implement a validation that the context source file transitively includes the
header (e.g., reuse existing include-resolution logic or a helper like
resolve_header_context/includes_graph lookup to confirm context_path -> ... ->
path), and if that check fails set result.success = false and return; only
proceed to modify active_contexts, header_file_contexts, pch_states, and mark
documents[].ast_dirty when the include-chain validation succeeds.
🪄 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: 9e960280-73da-4631-94d7-97b879d469d1
📒 Files selected for processing (10)
src/command/command.cppsrc/command/command.hsrc/server/master_server.cppsrc/server/master_server.hsrc/server/protocol.htests/conftest.pytests/data/header_context/compile_commands.jsontests/data/header_context/main.cpptests/data/header_context/utils.htests/integration/test_header_context.py
✅ Files skipped from review due to trivial changes (3)
- tests/data/header_context/compile_commands.json
- tests/conftest.py
- src/server/protocol.h
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/data/header_context/main.cpp
- src/server/master_server.h
…ted + multi-context tests - queryContext now returns compilation contexts (CDB entries) for source files, not just header contexts. Labels show distinguishing flags (-D, -O, -std=). - Handle split args (-D CONFIG_A → -DCONFIG_A) in label generation. - Add inner.h for deep nested chain: main.cpp → utils.h → inner.h - Test: deep nested header finds main.cpp as host through 2-level chain - Test: source file with 2 CDB entries (-DCONFIG_A, -DCONFIG_B) returns both - Fix hover line number after adding #include "inner.h" to utils.h Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bels - Add ast_deps.erase(path_id) in switchContext to prevent stale dependency snapshots - Remove -I from split-arg handling in CDB label generation (noisy long paths) - Untrack generated compile_commands.json (conftest generates it dynamically) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
tests/integration/test_header_context.py (1)
35-35:⚠️ Potential issue | 🟡 MinorDrop the unused
main_uribindings.These assignments are still flagged by Ruff (
RUF059). Use_main_urior just await the helper without binding the return value.Suggested fix
- main_uri, _ = await client.open_and_wait(workspace / "main.cpp") + await client.open_and_wait(workspace / "main.cpp") @@ - main_uri, _ = await client.open_and_wait(workspace / "main.cpp") + await client.open_and_wait(workspace / "main.cpp") @@ - main_uri, _ = await client.open_and_wait(workspace / "main.cpp") + await client.open_and_wait(workspace / "main.cpp")Also applies to: 74-74, 193-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_header_context.py` at line 35, The test assigns an unused return value from client.open_and_wait (e.g., main_uri = await client.open_and_wait(workspace / "main.cpp")), which triggers RUF059; fix by removing the binding and either await the call without assignment or name it _main_uri (use the same approach for the other occurrences of client.open_and_wait in this file), updating the calls where main_uri is unused so no unused variable is created.src/server/master_server.cpp (5)
759-772:⚠️ Potential issue | 🔴 CriticalDon’t replace the input by dropping the last positional argument.
Common compile commands put output paths after the source file. With something like
clang++ -c main.cpp -o main.o, this removesmain.o, leaves a dangling-o, and still keepsmain.cpp.🤖 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 759 - 772, The current logic decrements copy_count to drop the last non-flag argument which can mistakenly remove an output filename (e.g., "-o main.o") and leave dangling flags; instead locate the index of the last non-flag argument in host_ctx.arguments (scan from host_ctx.arguments.size()-1 backwards until you find an entry that does not start with "-"), copy all arguments up to but not including that index into arguments, then append the header path (path) in place of that single positional argument (or if no positional arg found, simply append path). Update the code that uses copy_count and arguments.emplace_back(path) so it replaces the identified positional arg rather than removing the last element unconditionally.
641-656:⚠️ Potential issue | 🟠 MajorParse the include directive instead of substring-matching the basename.
line.contains(next_filename)can match comments, string literals, or a different header with the same basename. That truncates the generated preamble at the wrong line and changes the compilation context.🤖 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 641 - 656, The current loop uses line.contains(next_filename) which can match comments or unrelated tokens; instead parse the `#include` token properly: after detecting line.starts_with("#include") (or "# include"), extract the include operand (the token between <...> or "..." ), trimming whitespace and stripping any trailing comments, then compare the extracted filename (or its basename via llvm::sys::path::filename / equivalent StringRef splitting) to next_filename for an exact match; update the include detection in the block around content_ref, line_start, include_line_start and next_filename so only a true include of the target file sets include_line_start.
577-607:⚠️ Potential issue | 🟠 MajorStore the chosen CDB entry with the resolved host.
This still picks the first viable host and later does
cdb.lookup(host_path).front(). If the same source has multiple CDB entries, the include chain can be paired with the wrong config, and the advertised open-file/shortest-chain preference is not enforced here.Also applies to: 748-756
🤖 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 577 - 607, The current logic selects host_path_id and chain but re-looks-up cdb entries later, which can pair the include chain with the wrong compilation entry when a source has multiple CDB entries; modify the code in the active-context branch and the hosts fallback loop to capture and store the selected CDB lookup result alongside host_path_id and chain (e.g., introduce a chosen_cdb_entry variable and assign it from the local results when you set host_path_id and chain), use that saved chosen_cdb_entry later instead of calling cdb.lookup(host_path) again, and apply the same change to the duplicate block around the other occurrence (the later 748-756 block) so the chosen compile command is the one corresponding to the resolved host and chain.
2233-2247:⚠️ Potential issue | 🟠 MajorInvalidate header contexts for every preamble contributor.
The cached preamble is built from the whole include chain, but this only evicts entries whose
host_path_idmatches the saved file. Saving an intermediate header can therefore keep a stale preamble alive; if include edges changed, the reverse map also stays stale until reload.🤖 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 2233 - 2247, The current eviction only removes header contexts whose hdr_ctx.host_path_id == path_id but must also evict any header whose cached preamble was contributed to by the saved file; update the first loop over header_file_contexts to treat a header as stale if hdr_ctx.host_path_id == path_id OR hdr_ctx.preamble_contributors (or the equivalent include-contributor set on hdr_ctx) contains path_id, then proceed to erase from header_file_contexts and mark documents[hdr_id].ast_dirty exactly as done now; use the existing symbols header_file_contexts, hdr_ctx, host_path_id, path_id, and preamble_contributors (or the corresponding reverse-include structure) to locate and change the condition.
2857-2879:⚠️ Potential issue | 🟠 MajorReject unrelated context files up front.
This only checks whether
context_urihas a CDB entry. If it does not actually include the target header,resolve_header_context()later falls back to some other host, butswitchContextstill returnssuccess=trueandcurrentContextkeeps reporting the requested source.Suggested fix
auto context_cdb = cdb.lookup(context_path, {.suppress_logging = true}); if(context_cdb.empty()) { result.success = false; co_return to_raw(result); } + + auto chain = dependency_graph.find_include_chain(context_path_id, path_id); + if(chain.empty()) { + result.success = false; + co_return to_raw(result); + } // Set active context and invalidate cached header context so // resolve_header_context will pick the new host on next compile.🤖 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 2857 - 2879, The code only verifies the requested context exists but not that it actually provides the target header; before setting active_contexts and returning success, perform a second check that the selected context (context_path / context_path_id) contains the header identified by path_id (i.e. that a lookup for the header in that CDB is non-empty) — if that header lookup fails, set result.success = false and return; keep references to context_path, context_path_id, path_id and the later resolve_header_context() behavior so the guard runs here in switchContext (or the function wrapping this logic) to prevent reporting a currentContext that does not actually serve the requested header.
🤖 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 707-716: The current fast-path returns when cdb.has_entry(path)
causing clice/switchContext changes in active_contexts to be ignored; modify the
logic in master_server.cpp so that before using cdb.lookup(path) and returning
early (the block around cdb.has_entry and cdb.lookup), you check active_contexts
for an active override for this path and if present use that context (directory
and arguments) instead of the direct CDB result; only fall back to cdb.lookup
when no active_contexts entry applies, and remove the unconditional early return
so switchContext can take effect.
- Around line 2755-2818: The code reads a client-controlled params.offset into
offset_val and then uses it as the start of the loop (for(int i =
offset_val...)) which can be negative and cause out-of-bounds access; fix by
validating/clamping offset_val from params.offset (e.g., set offset_val =
std::max(0, params.offset.value_or(0)) or return an error for negative offsets)
before computing end and iterating, ensuring the loop start is non-negative and
consistent with result.total and page_size when filling result.contexts (refer
to symbols params.offset, offset_val, page_size, all_items, result and the
for(int i = offset_val...) loop).
In `@tests/integration/test_header_context.py`:
- Around line 227-230: The assertion generator uses a single-letter variable `l`
which triggers Ruff E741; update both assertions to use a clearer name like
`label` (e.g., change `any("CONFIG_A" in l for l in labels)` and `any("CONFIG_B"
in l for l in labels)` to `any("CONFIG_A" in label for label in labels)` and
`any("CONFIG_B" in label for label in labels)`), keeping the surrounding
variables `labels`, `_get`, and `contexts` unchanged.
---
Duplicate comments:
In `@src/server/master_server.cpp`:
- Around line 759-772: The current logic decrements copy_count to drop the last
non-flag argument which can mistakenly remove an output filename (e.g., "-o
main.o") and leave dangling flags; instead locate the index of the last non-flag
argument in host_ctx.arguments (scan from host_ctx.arguments.size()-1 backwards
until you find an entry that does not start with "-"), copy all arguments up to
but not including that index into arguments, then append the header path (path)
in place of that single positional argument (or if no positional arg found,
simply append path). Update the code that uses copy_count and
arguments.emplace_back(path) so it replaces the identified positional arg rather
than removing the last element unconditionally.
- Around line 641-656: The current loop uses line.contains(next_filename) which
can match comments or unrelated tokens; instead parse the `#include` token
properly: after detecting line.starts_with("#include") (or "# include"), extract
the include operand (the token between <...> or "..." ), trimming whitespace and
stripping any trailing comments, then compare the extracted filename (or its
basename via llvm::sys::path::filename / equivalent StringRef splitting) to
next_filename for an exact match; update the include detection in the block
around content_ref, line_start, include_line_start and next_filename so only a
true include of the target file sets include_line_start.
- Around line 577-607: The current logic selects host_path_id and chain but
re-looks-up cdb entries later, which can pair the include chain with the wrong
compilation entry when a source has multiple CDB entries; modify the code in the
active-context branch and the hosts fallback loop to capture and store the
selected CDB lookup result alongside host_path_id and chain (e.g., introduce a
chosen_cdb_entry variable and assign it from the local results when you set
host_path_id and chain), use that saved chosen_cdb_entry later instead of
calling cdb.lookup(host_path) again, and apply the same change to the duplicate
block around the other occurrence (the later 748-756 block) so the chosen
compile command is the one corresponding to the resolved host and chain.
- Around line 2233-2247: The current eviction only removes header contexts whose
hdr_ctx.host_path_id == path_id but must also evict any header whose cached
preamble was contributed to by the saved file; update the first loop over
header_file_contexts to treat a header as stale if hdr_ctx.host_path_id ==
path_id OR hdr_ctx.preamble_contributors (or the equivalent include-contributor
set on hdr_ctx) contains path_id, then proceed to erase from
header_file_contexts and mark documents[hdr_id].ast_dirty exactly as done now;
use the existing symbols header_file_contexts, hdr_ctx, host_path_id, path_id,
and preamble_contributors (or the corresponding reverse-include structure) to
locate and change the condition.
- Around line 2857-2879: The code only verifies the requested context exists but
not that it actually provides the target header; before setting active_contexts
and returning success, perform a second check that the selected context
(context_path / context_path_id) contains the header identified by path_id (i.e.
that a lookup for the header in that CDB is non-empty) — if that header lookup
fails, set result.success = false and return; keep references to context_path,
context_path_id, path_id and the later resolve_header_context() behavior so the
guard runs here in switchContext (or the function wrapping this logic) to
prevent reporting a currentContext that does not actually serve the requested
header.
In `@tests/integration/test_header_context.py`:
- Line 35: The test assigns an unused return value from client.open_and_wait
(e.g., main_uri = await client.open_and_wait(workspace / "main.cpp")), which
triggers RUF059; fix by removing the binding and either await the call without
assignment or name it _main_uri (use the same approach for the other occurrences
of client.open_and_wait in this file), updating the calls where main_uri is
unused so no unused variable is created.
🪄 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: e12920da-6d60-40a4-87b5-3a877e8447c5
📒 Files selected for processing (6)
src/server/master_server.cpptests/conftest.pytests/data/header_context/inner.htests/data/header_context/utils.htests/data/multi_context/main.cpptests/integration/test_header_context.py
✅ Files skipped from review due to trivial changes (2)
- tests/data/header_context/inner.h
- tests/data/header_context/utils.h
- Replace host source file in args by matching against CDB file entry instead of removing last positional arg. Fixes "clang++ -c main.cpp -o main.o" case where the old logic would strip -o's argument and leave main.cpp in argv. - Remove unused main_uri bindings in 3 tests (Ruff RUF059) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (7)
src/server/master_server.cpp (7)
2233-2247:⚠️ Potential issue | 🟠 MajorHost-only invalidation leaves header preamble contexts stale.
At Line 2237, invalidation checks only
hdr_ctx.host_path_id; saves to intermediate include-chain contributors can keep stale preambles alive. Also, include-graph-derived host resolution can go stale without reverse-map refresh after include-affecting saves.🤖 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 2233 - 2247, The current loop only invalidates header contexts whose hdr_ctx.host_path_id equals the saved path_id, leaving preamble contexts contributed via includes or include-graph mapping stale; update the invalidation to also find headers that list the saved file as a contributor or whose resolved host via the include graph now maps to the saved path_id (use your include_graph / reverse include map helper), then erase those hdr_id entries from header_file_contexts, mark their corresponding documents[hdr_id].ast_dirty = true, and refresh or rebuild the include-graph reverse map so subsequent saves see correct host resolution (apply changes around header_file_contexts iteration and anywhere include-graph resolution is cached).
707-718:⚠️ Potential issue | 🟠 Major
active_contextsis still bypassed for files that already have direct CDB entries.The early return at Line 707-Line 717 makes
clice/switchContextineffective on those files because override resolution never runs.🤖 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 707 - 718, The early return after using cdb.lookup bypasses active_contexts/override resolution and prevents clice/switchContext from taking effect; instead of returning immediately in the block that handles cdb.has_entry and results (the code that fills directory and arguments from ctx and uses cdb.lookup/.has_entry), populate directory and arguments from ctx but do not return—allow the function to continue so override resolution (active_contexts or the existing override-resolution logic) can run and potentially alter the chosen context; ensure any merge logic preserves arguments populated from ctx and that subsequent override checks reference the same variables (results/ctx/directory/arguments) so overrides can override or replace the CDB-provided values.
2755-2816:⚠️ Potential issue | 🔴 CriticalNegative
offsetcan cause out-of-bounds pagination access.At Line 2755 and Line 2815, a client-supplied negative offset leads to invalid indexing into
all_items.Suggested fix
- int offset_val = params.offset.value_or(0); + int offset_val = std::max(0, params.offset.value_or(0)); constexpr int page_size = 10; @@ - int end = std::min(offset_val + page_size, static_cast<int>(all_items.size())); - for(int i = offset_val; i < end; ++i) { + int start = std::min(offset_val, static_cast<int>(all_items.size())); + int end = std::min(start + page_size, static_cast<int>(all_items.size())); + for(int i = start; i < end; ++i) { result.contexts.push_back(std::move(all_items[i])); }🤖 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 2755 - 2816, The code reads a client-supplied offset into offset_val and then uses it to index into all_items causing out-of-bounds when offset is negative; update the logic that sets/uses offset_val (the variable offset_val and the pagination block that computes end and loops over i) to sanitize the offset: clamp negative offsets to 0 (e.g. offset_val = std::max(0, params.offset.value_or(0))) and also ensure offset_val is no larger than all_items.size() (or return an empty result when offset_val >= total) before computing end/page bounds so the for-loop over all_items[i] cannot index out-of-range.
641-654:⚠️ Potential issue | 🟠 Major
contains(next_filename)can match the wrong include edge.At Line 653, basename substring matching is ambiguous and can pick the wrong line, which changes generated preamble content.
🤖 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 641 - 654, The include detection is using line.contains(next_filename) which can match substrings and pick the wrong include; instead parse the include token from the directive (handle both "#include" and "# include" forms), extract the quoted or angle-bracket filename (strip surrounding '"' or '<' '>'), then compare that extracted filename exactly to next_filename (or compare basenames explicitly if next_filename is a basename) before setting include_line_start; update the logic around content_ref, line_start and the line.starts_with checks to perform this exact filename comparison.
594-607:⚠️ Potential issue | 🟠 MajorHost/context selection is still non-deterministic and can pair the chain with the wrong compile command.
At Line 594-Line 607, selection stops at the first host with a CDB hit; at Line 748-Line 756, you re-lookup and take
front()again. This can mismatch when a source has multiple CDB entries and does not enforce the advertised host preference strategy.Also applies to: 748-756
🤖 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 594 - 607, The current host/compile-command selection is non-deterministic because you pick the first host with a CDB hit (using path_pool.resolve, cdb.lookup and dependency_graph.find_include_chain) and later re-query and take cdb.front(), which can return a different compile entry; instead, when you discover a valid chain for a candidate host (in the loop that sets host_path_id and chain), also capture and persist the exact CDB entry identifier or index returned by cdb.lookup (e.g., the specific entry object or its unique key) and use that stored entry later rather than calling cdb.lookup(...).front(); ensure both the initial selection code and the later code (the block that currently rereads front()) consult the same stored CDB entry for that host so the include chain and compile command remain paired deterministically.
2856-2861:⚠️ Potential issue | 🟠 Major
switchContextshould validate that the selected context actually reaches the target header.Right now success is granted on CDB presence alone; unrelated files can be accepted, then silently ignored later by fallback resolution.
Suggested fix
auto context_cdb = cdb.lookup(context_path, {.suppress_logging = true}); if(context_cdb.empty()) { result.success = false; co_return to_raw(result); } + + // Ensure the selected context can actually compile the target file. + auto chain = dependency_graph.find_include_chain(context_path_id, path_id); + if(chain.empty()) { + result.success = false; + co_return to_raw(result); + }🤖 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 2856 - 2861, The code in switchContext only checks cdb.lookup(context_path) exists but doesn't verify that this context can actually reach the requested target header; update switchContext to validate reachability by confirming the returned context_cdb includes/points to the target header (e.g. check that context_cdb contains an entry or mapping for the target header identifier or that cdb.resolve(context_path, target_header) succeeds) before setting result.success; use the existing symbols context_path, context_cdb, and the target header variable/name and fail (result.success = false; co_return) if the context does not reach the target.
759-773:⚠️ Potential issue | 🔴 CriticalDropping the “last positional arg” can remove the wrong token and leave broken argv.
At Line 759-Line 767, removing by position can strip output paths (e.g.,
main.o) instead of the actual host source, leaving invalid flags and duplicate inputs.Suggested fix
- // Copy host arguments, replacing the source file path (last non-flag arg) - // with the header file path, so the compiler processes the header in context. - auto num_args = host_ctx.arguments.size(); - std::size_t copy_count = num_args; - if(copy_count > 0) { - llvm::StringRef last(host_ctx.arguments[copy_count - 1]); - if(!last.starts_with("-")) - copy_count -= 1; - } - for(std::size_t i = 0; i < copy_count; ++i) { - arguments.emplace_back(host_ctx.arguments[i]); - } - // Append the header file path as the source file. - arguments.emplace_back(path); + // Copy host arguments and remove only source-file inputs, not arbitrary + // trailing positionals (e.g. -o <file> values). + auto is_source_input = [](llvm::StringRef tok) { + if(tok.empty() || tok.starts_with("-")) return false; + auto ext = llvm::sys::path::extension(tok); + return ext == ".c" || ext == ".cc" || ext == ".cpp" || ext == ".cxx" || + ext == ".c++" || ext == ".C" || ext == ".m" || ext == ".mm"; + }; + + for(auto* arg : host_ctx.arguments) { + llvm::StringRef tok(arg); + if(is_source_input(tok)) { + continue; + } + arguments.emplace_back(tok); + } + arguments.emplace_back(path);🤖 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 759 - 773, The current logic in master_server.cpp that computes copy_count by simply removing the last non-flag token (using host_ctx.arguments, copy_count, last) can drop the wrong positional (e.g., output file). Replace this with a backward scan to locate the actual source positional token: iterate host_ctx.arguments from the end to find the last argument that does not start with "-" and is not the value of a flag that expects a parameter (e.g., the token after "-o" or other known flags), treat that index as the source to be excluded, set copy_count to that index, copy host_ctx.arguments[0..copy_count-1] into arguments, then append path; use the existing variables host_ctx.arguments, copy_count, arguments, and path to implement this fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/master_server.cpp`:
- Around line 2233-2247: The current loop only invalidates header contexts whose
hdr_ctx.host_path_id equals the saved path_id, leaving preamble contexts
contributed via includes or include-graph mapping stale; update the invalidation
to also find headers that list the saved file as a contributor or whose resolved
host via the include graph now maps to the saved path_id (use your include_graph
/ reverse include map helper), then erase those hdr_id entries from
header_file_contexts, mark their corresponding documents[hdr_id].ast_dirty =
true, and refresh or rebuild the include-graph reverse map so subsequent saves
see correct host resolution (apply changes around header_file_contexts iteration
and anywhere include-graph resolution is cached).
- Around line 707-718: The early return after using cdb.lookup bypasses
active_contexts/override resolution and prevents clice/switchContext from taking
effect; instead of returning immediately in the block that handles cdb.has_entry
and results (the code that fills directory and arguments from ctx and uses
cdb.lookup/.has_entry), populate directory and arguments from ctx but do not
return—allow the function to continue so override resolution (active_contexts or
the existing override-resolution logic) can run and potentially alter the chosen
context; ensure any merge logic preserves arguments populated from ctx and that
subsequent override checks reference the same variables
(results/ctx/directory/arguments) so overrides can override or replace the
CDB-provided values.
- Around line 2755-2816: The code reads a client-supplied offset into offset_val
and then uses it to index into all_items causing out-of-bounds when offset is
negative; update the logic that sets/uses offset_val (the variable offset_val
and the pagination block that computes end and loops over i) to sanitize the
offset: clamp negative offsets to 0 (e.g. offset_val = std::max(0,
params.offset.value_or(0))) and also ensure offset_val is no larger than
all_items.size() (or return an empty result when offset_val >= total) before
computing end/page bounds so the for-loop over all_items[i] cannot index
out-of-range.
- Around line 641-654: The include detection is using
line.contains(next_filename) which can match substrings and pick the wrong
include; instead parse the include token from the directive (handle both
"#include" and "# include" forms), extract the quoted or angle-bracket filename
(strip surrounding '"' or '<' '>'), then compare that extracted filename exactly
to next_filename (or compare basenames explicitly if next_filename is a
basename) before setting include_line_start; update the logic around
content_ref, line_start and the line.starts_with checks to perform this exact
filename comparison.
- Around line 594-607: The current host/compile-command selection is
non-deterministic because you pick the first host with a CDB hit (using
path_pool.resolve, cdb.lookup and dependency_graph.find_include_chain) and later
re-query and take cdb.front(), which can return a different compile entry;
instead, when you discover a valid chain for a candidate host (in the loop that
sets host_path_id and chain), also capture and persist the exact CDB entry
identifier or index returned by cdb.lookup (e.g., the specific entry object or
its unique key) and use that stored entry later rather than calling
cdb.lookup(...).front(); ensure both the initial selection code and the later
code (the block that currently rereads front()) consult the same stored CDB
entry for that host so the include chain and compile command remain paired
deterministically.
- Around line 2856-2861: The code in switchContext only checks
cdb.lookup(context_path) exists but doesn't verify that this context can
actually reach the requested target header; update switchContext to validate
reachability by confirming the returned context_cdb includes/points to the
target header (e.g. check that context_cdb contains an entry or mapping for the
target header identifier or that cdb.resolve(context_path, target_header)
succeeds) before setting result.success; use the existing symbols context_path,
context_cdb, and the target header variable/name and fail (result.success =
false; co_return) if the context does not reach the target.
- Around line 759-773: The current logic in master_server.cpp that computes
copy_count by simply removing the last non-flag token (using host_ctx.arguments,
copy_count, last) can drop the wrong positional (e.g., output file). Replace
this with a backward scan to locate the actual source positional token: iterate
host_ctx.arguments from the end to find the last argument that does not start
with "-" and is not the value of a flag that expects a parameter (e.g., the
token after "-o" or other known flags), treat that index as the source to be
excluded, set copy_count to that index, copy host_ctx.arguments[0..copy_count-1]
into arguments, then append path; use the existing variables host_ctx.arguments,
copy_count, arguments, and path to implement this fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00512512-efab-4686-8d28-a3a619087f90
📒 Files selected for processing (1)
src/server/master_server.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/server/master_server.cpp (1)
763-769: Path comparison may fail due to normalization differences.The comparison
llvm::StringRef(arg) == host_pathassumes both paths use the same format. If the CDB entry stores a relative path whilehost_pathis absolute (or uses different path separators/normalization), the match will fail and the header path won't be substituted.Consider normalizing both paths before comparison, or matching against the CDB entry's
filefield directly.🤖 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 763 - 769, The loop that substitutes header paths compares llvm::StringRef(arg) to host_path which can fail due to differing path formats; update the comparison to normalize both sides (e.g., convert arg and host_path to std::filesystem::path and compare their canonical/weakly_canonical or lexically_normal forms) before deciding to push path, or alternatively compare arg against the CDB entry's file field directly (use host_ctx.file or the CDB file identifier) to ensure correct matching; keep the rest of the logic (arguments.emplace_back(path) vs arguments.emplace_back(arg)) unchanged.tests/integration/test_header_context.py (1)
167-186: Hardcoded hover position is fragile.Line 173 uses a fixed position
(line=14, character=12)for thesumfunction. Ifutils.his modified (even formatting changes), this test will break silently or hover on the wrong symbol. Consider adding a comment referencing the expected line content, or use a marker-based approach if the test framework supports it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_header_context.py` around lines 167 - 186, The test uses a hardcoded Position(line=14, character=12) when calling client.text_document_hover_async with HoverParams for utils_uri to hover the 'sum' symbol, which is fragile; change the test to locate the 'sum' token dynamically (e.g., search the utils.h text for the 'sum' substring or use a marker in the file) and compute the Position from that index before constructing HoverParams, or add a stable marker comment in utils.h and find its position; update the call sites around client.text_document_hover_async / HoverParams to use the computed Position so the hover assertion remains resilient to formatting changes.
🤖 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 763-769: The loop that substitutes header paths compares
llvm::StringRef(arg) to host_path which can fail due to differing path formats;
update the comparison to normalize both sides (e.g., convert arg and host_path
to std::filesystem::path and compare their canonical/weakly_canonical or
lexically_normal forms) before deciding to push path, or alternatively compare
arg against the CDB entry's file field directly (use host_ctx.file or the CDB
file identifier) to ensure correct matching; keep the rest of the logic
(arguments.emplace_back(path) vs arguments.emplace_back(arg)) unchanged.
In `@tests/integration/test_header_context.py`:
- Around line 167-186: The test uses a hardcoded Position(line=14, character=12)
when calling client.text_document_hover_async with HoverParams for utils_uri to
hover the 'sum' symbol, which is fragile; change the test to locate the 'sum'
token dynamically (e.g., search the utils.h text for the 'sum' substring or use
a marker in the file) and compute the Position from that index before
constructing HoverParams, or add a stable marker comment in utils.h and find its
position; update the call sites around client.text_document_hover_async /
HoverParams to use the computed Position so the hover assertion remains
resilient to formatting changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 466b5407-1740-4854-9628-22d8ce471e7a
📒 Files selected for processing (2)
src/server/master_server.cpptests/integration/test_header_context.py
…urce arg replacement - Add pch_test compile_commands.json generation in conftest.py (was missing, causing all test_pch tests to hang on CI due to hardcoded local paths) - Remove cdb.has_entry() check in fill_compile_args, use cdb.lookup() directly - Replace host source arg by identity match instead of removing last positional arg - Remove unused preamble_hash field from HeaderFileContext - Remove -I from label generation (too noisy) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When switchContext has been called for a header file, fill_compile_args must skip the normal cdb.lookup (which may return a toolchain fallback entry without proper includes) and go directly to header context resolution with preamble injection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- fill_compile_args: checks active_contexts first (switchContext), then normal CDB lookup, then auto header context as fallback - fill_header_context_args: uses host source's CDB entry with file path replaced and preamble injected - Fixes: switchContext now correctly uses host's compile args instead of falling through to toolchain fallback Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace #include <vector> with custom types.h (Point struct) to avoid slow PCH builds on Windows Debug CI. The test still validates non self-contained header compilation via preamble injection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Parse #include directive to extract actual filename instead of substring match (fixes false positive when util.h matches util_base.h) - Warn and fallback when host source path not found in CDB arguments - Clamp negative queryContext offset to 0 - Add test: deep nested switchContext + hover (main.cpp -> utils.h -> inner.h) - Add path_to_uri helper to CliceClient Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
16bit-ykiko
changed the title
This was referenced Apr 7, 2026
Merged
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
Add three LSP protocol extensions that allow users to manage compilation contexts for header files and source files with multiple CDB entries.
Protocol extensions (
protocol.h)clice/queryContextoffsetparam).clice/currentContextclice/switchContextHeader context resolution (
master_server.cpp,dependency_graph.cpp)find_host_sources(): BFS the reverse include graph to find source files that transitively include a headerfind_include_chain(): BFS the forward include graph to find the shortest include chain from host to headerresolve_header_context(): walks the include chain, extracts content before each#includedirective, concatenates with#linemarkers into a preamble file (hash-addressed under.clice/header_context/)fill_header_context_args(): uses the host source's CDB entry, replaces source path with header path, injects-include preamble.hCompilation flow
switchContext:fill_compile_argschecksactive_contextsfirst → uses host's CDB entry + preamble injectionresolve_header_context#includedirective matching uses precise filename extraction from"..."/<...>, not substring matchingSource file multiple contexts (
multi_contextworkspace)queryContexton a source file returns all CDB entries with distinguishing labels (extracted from-D,-O,-std=flags)Test data
header_context/: non-self-contained 3-level chain (main.cpp→utils.h→inner.h),types.hprovidesPointstructmulti_context/: single source with two CDB entries (-DCONFIG_A,-DCONFIG_B)Tests (9 integration tests)
inner.h(3 levels deep)Test plan
🤖 Generated with Claude Code