Repository navigation
feat(server): implement compilation contexts end to end - #479
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughThis PR adds compilation-context selection for headers and sources, synthesized prefix fallback, content-keyed PCH reuse, editor commands/UI for switching contexts, and integration/unit coverage. The English and Chinese design docs are updated to match the new flow. ChangesHeader compilation context feature
Estimated code review effort: 5 (Critical) | ~120 minutes Compilation context design documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
- Gate CDB lookup on has_entry() so automatic header context resolution is reachable: lookup() always synthesizes a fallback command, which made the dependency-graph path dead code for auto resolution. The synthesized command remains as the final fallback for standalone files. - Rewrite preamble synthesis on scan() + real include resolution instead of textual basename matching. Quoted includes are absolutized so the preamble compiles correctly from the cache directory; matching prefers unconditional directives, and a cut inside #if blocks (e.g. include guards on intermediate headers) emits balancing #endifs. - Track the include chain in HeaderContext with a DepsSnapshot hashed from the buffers embedded in the preamble; is_stale and fill_header_context_args re-synthesize when any chain file changes. - Fix on_file_saved shadowing bug that made host-save invalidation a no-op. The save now zeroes the snapshot's build_at to force content re-validation instead of resetting the context, which would race with in-flight compiles and leave sessions permanently stale. - Normalize resolver output through the path pool so exact matching works with native separators on Windows; escape #line marker paths. - queryContext/switchContext use has_entry too: no more bogus contexts for headers with no hosts, and switch validation actually validates. - Drop the thread_local context hack, delete unused semantic/context.h, rename HeaderFileContext to HeaderContext.
- A header context session builds its PCH even when the header itself has no preamble directives (bound == 0): the -include'd synthesized prefix is processed through the predefines buffer and baked into the PCH, so X-macro style headers stop re-parsing the prefix per edit. - Re-key Workspace.pch_cache by the content key instead of path_id: files with identical preambles now share one build, concurrent builds of the same key coalesce, and didClose no longer drops shared state. - cache.json v2: PCH entries drop source_file; bump cache format.
- Include-occurrence contexts: a guard-less header included N times by one host exposes N contexts; synthesis, queryContext enumeration, switchContext pinning and currentContext reporting all carry the 0-based occurrence index. - Source files with multiple CDB entries switch via canonical command hash (stable across CDB reordering); fill_compile_args honors the pinned entry. - queryContext dedupes contexts by canonical frontend flags hash and orders hosts by relevance (stem match > same directory > path proximity, deterministic tie-break). - switchContext validates the chosen host actually includes the header and the occurrence is in range, instead of accepting anything.
- Headers without a CDB entry compile self-contained first (borrowed host command, no prefix). If the trial diagnostics hit a strict set of missing-context signals (unknown type name, undeclared identifier, unterminated conditional), the header is recompiled with a synthesized prefix; trial diagnostics are never published. - .def/.inc files skip the trial and go straight to prefix synthesis. - Verdicts and user switchContext choices persist in cache.json and are restored on didOpen; saving a header resets its verdict.
- New clice.selectContext command: QuickPick over clice/queryContext with pagination, switching via clice/switchContext (occurrence and command hash aware); status bar shows the active context. - Remove the dead header-contexts TreeView skeleton. - Add header_context E2E scenario for vscode and nvim harnesses. - Update compilation-context design docs (zh/en) to match the implementation: resolution priority, self-containment detection, dedup semantics, occurrence contexts, staleness, PCH unification.
- switchContext bumps session generation: an in-flight compile could otherwise clobber ast_dirty on completion and publish results for the old context with nothing left for is_stale to recover from. - Self-containment verdicts self-heal: only NeedsContext persists; the session-local trial re-runs whenever compile inputs change for reasons other than buffer edits (didSave cascades, chain invalidation, mtime staleness), so a stale self-contained impression cannot survive a dependency change. header_mode moves to Workspace. - queryContext no longer merges hosts of a NeedsContext header by flags hash: each host synthesizes a different prefix, so each is distinct. - An explicit occurrence choice (> 0) forces prefix synthesis. - ensure_pch drops content-keyed metadata whose blob the store evicted, bounding the in-memory map. - Promote get_field/write_entries into tests/integration/utils; add rank_hosts unit tests, trial-with-ordinary-error, header-save verdict reset, dependency-change re-trial, occurrence out-of-range rejection and queryContext pagination tests.
The header_context scenario now drives clice/queryContext, clice/switchContext and clice/currentContext through the real extension client (exported from activate() for tests).
Cross-file go-to-definition into headers returns no locations — a pre-existing index gap confirmed on main — so the header_context scenario skips the definition step instead of asserting a known failure.
16bit-ykiko
force-pushed
the
header-context-synthesis
branch
from
July 3, 2026 07:03
a427457 to
5991caf
Compare
16bit-ykiko
force-pushed
the
header-context-synthesis
branch
from
July 3, 2026 07:18
b0a3c85 to
fffff04
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
editors/vscode/src/feature/context.ts (1)
60-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd error handling around
select()'s LSP requests.
refresh()wraps its request in try/catch (Lines 36-45), butselect()'sclice/queryContextandclice/switchContextcalls (Lines 60-63, 105-108) are unguarded. If the server throws (disconnected, method unsupported, etc.), the command handler's promise rejects without a user-facing message, unlike the gracefulstatus.hide()fallback elsewhere.♻️ Suggested fix
async function select() { const editor = vscode.window.activeTextEditor; if (!isCppEditor(editor)) { return; } const uri = editor.document.uri.toString(); const loaded: ContextItem[] = []; let total = Number.POSITIVE_INFINITY; + try { while (true) { ... } + } catch (err) { + vscode.window.showErrorMessage(`clice: failed to load compilation contexts: ${err}`); + } }Also applies to: 105-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editors/vscode/src/feature/context.ts` around lines 60 - 63, The `select()` flow in `context.ts` sends unguarded LSP requests via `client.sendRequest` for both `clice/queryContext` and `clice/switchContext`, so add try/catch around those calls similar to `refresh()`. In the `select()` method, catch request failures, surface a user-facing message or fallback, and ensure the command does not reject silently when the server is disconnected or the method is unsupported.tests/integration/extensions/test_context_switching.py (1)
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename ambiguous variable
l.Ruff flags
las an ambiguous single-letter name (E741) at Lines 33-34.🔧 Proposed fix
- plain_hash = next(h for l, h in hashes.items() if "-DEXPECTED" not in l) - defined_hash = next(h for l, h in hashes.items() if "-DEXPECTED" in l) + plain_hash = next(h for label, h in hashes.items() if "-DEXPECTED" not in label) + defined_hash = next(h for label, h in hashes.items() if "-DEXPECTED" in label)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/extensions/test_context_switching.py` around lines 33 - 34, The generator expression in the test_context_switching logic uses the ambiguous single-letter variable l, which Ruff flags as E741. Update the comprehensions that derive plain_hash and defined_hash to use a clearer name like line or label so the intent is obvious, while keeping the same filtering behavior based on the "-DEXPECTED" substring.Source: Linters/SAST tools
tests/integration/compilation/test_staleness.py (1)
474-474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared
MTIME_GRANULARITYconstant instead of hardcoded1.1.This file already defines/uses
MTIME_GRANULARITYfor the same purpose elsewhere (e.g.test_didsave_triggers_recompile_for_dependents). Reusing it here keeps the sleep duration tunable in one place and avoids drift if the granularity needs to change for a different CI filesystem.♻️ Proposed fix
- await asyncio.sleep(1.1) + await asyncio.sleep(MTIME_GRANULARITY)Also applies to: 504-504
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/compilation/test_staleness.py` at line 474, Replace the hardcoded asyncio.sleep duration in the staleness tests with the shared MTIME_GRANULARITY constant. Update the affected assertions in test_staleness.py, including the one in the same flow as test_didsave_triggers_recompile_for_dependents, so the wait time is derived from MTIME_GRANULARITY everywhere instead of 1.1. This keeps the timing consistent and makes the test behavior easier to adjust in one place.src/server/workspace/workspace.h (2)
49-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing default initializer on
host_path_id.Unlike sibling structs in this same diff (
SavedContext::host_path_id = 0,Session::ActiveContext::host_path_id = 0),HeaderContext::host_path_idhas no default value. IfHeaderContextis ever default/value-constructed (e.g. viastd::optional<HeaderContext>::emplace()with no args, container resize, or future refactors), this field would be left uninitialized. Current call sites use full aggregate initialization, so this isn't exploitable today, but it's an inconsistent contract vs. the rest of this file.🛡️ Proposed fix
struct HeaderContext { - std::uint32_t host_path_id; ///< Source file acting as host. + std::uint32_t host_path_id = 0; ///< Source file acting as host. std::string preamble_path; ///< Path to generated preamble file on disk. std::uint64_t preamble_hash; ///< Hash of preamble content for staleness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/workspace/workspace.h` around lines 49 - 65, HeaderContext::host_path_id is missing a default initializer, unlike the matching host_path_id fields in SavedContext and Session::ActiveContext. Update HeaderContext so host_path_id has an explicit default value, keeping its initialization contract consistent and safe for default/value construction across future call sites.
184-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
rank_hostscould beconst.Per the linked implementation (
workspace.cpp:32-61),rank_hostsonly readspath_pooland produces a sorted copy — it doesn't mutateWorkspacestate. Marking itconstwould better document the contract and let it be called from const contexts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/workspace/workspace.h` around lines 184 - 190, `rank_hosts` is read-only and should be callable on const `Workspace` instances. Update the `Workspace::rank_hosts` declaration (and matching definition in `workspace.cpp`) to be `const`, since it only consults `path_pool` and returns a sorted copy without mutating state. Ensure any callers or overrides, if present, match the new const-qualified signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/compiler/compiler.cpp`:
- Around line 738-751: The fast path in compiler.cpp’s PCH decision logic is too
broad because it treats every header_context as if an injected -include exists,
which makes self-contained header contexts with bound == 0 still build a useless
PCH. Update the guard around compute_preamble_bound(text) so it only skips the
PCH when there is no bound and no actual injected preamble content to
precompile, using the resolved header_context / preamble_path state from
resolve_header_context rather than header_context.has_value() alone. Keep the
existing PCH build path for real injected preambles, but let self-contained
headers and .def-like files with empty own preambles hit the reset/co_return
fast path.
---
Nitpick comments:
In `@editors/vscode/src/feature/context.ts`:
- Around line 60-63: The `select()` flow in `context.ts` sends unguarded LSP
requests via `client.sendRequest` for both `clice/queryContext` and
`clice/switchContext`, so add try/catch around those calls similar to
`refresh()`. In the `select()` method, catch request failures, surface a
user-facing message or fallback, and ensure the command does not reject silently
when the server is disconnected or the method is unsupported.
In `@src/server/workspace/workspace.h`:
- Around line 49-65: HeaderContext::host_path_id is missing a default
initializer, unlike the matching host_path_id fields in SavedContext and
Session::ActiveContext. Update HeaderContext so host_path_id has an explicit
default value, keeping its initialization contract consistent and safe for
default/value construction across future call sites.
- Around line 184-190: `rank_hosts` is read-only and should be callable on const
`Workspace` instances. Update the `Workspace::rank_hosts` declaration (and
matching definition in `workspace.cpp`) to be `const`, since it only consults
`path_pool` and returns a sorted copy without mutating state. Ensure any callers
or overrides, if present, match the new const-qualified signature.
In `@tests/integration/compilation/test_staleness.py`:
- Line 474: Replace the hardcoded asyncio.sleep duration in the staleness tests
with the shared MTIME_GRANULARITY constant. Update the affected assertions in
test_staleness.py, including the one in the same flow as
test_didsave_triggers_recompile_for_dependents, so the wait time is derived from
MTIME_GRANULARITY everywhere instead of 1.1. This keeps the timing consistent
and makes the test behavior easier to adjust in one place.
In `@tests/integration/extensions/test_context_switching.py`:
- Around line 33-34: The generator expression in the test_context_switching
logic uses the ambiguous single-letter variable l, which Ruff flags as E741.
Update the comprehensions that derive plain_hash and defined_hash to use a
clearer name like line or label so the intent is obvious, while keeping the same
filtering behavior based on the "-DEXPECTED" substring.
🪄 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: d912b673-b29c-42b5-9730-b1d32d90814d
📒 Files selected for processing (38)
docs/en/design/compilation-context.mddocs/zh/design/compilation-context.mdeditors/nvim/tests/e2e.luaeditors/vscode/.vscode-test.mjseditors/vscode/package.jsoneditors/vscode/src/extension.tseditors/vscode/src/feature/context.tseditors/vscode/src/feature/header.tseditors/vscode/src/test/e2e.test.tspixi.tomlsrc/command/argument_parser.cppsrc/command/argument_parser.hsrc/semantic/context.hsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/protocol/extension.hsrc/server/service/lsp_client.cppsrc/server/service/master_server.cppsrc/server/service/session.hsrc/server/workspace/workspace.cppsrc/server/workspace/workspace.hsrc/syntax/preamble_synthesis.cppsrc/syntax/preamble_synthesis.hsrc/syntax/scan.cppsrc/syntax/scan.htests/integration/compilation/test_header_pch.pytests/integration/compilation/test_persistent_cache.pytests/integration/compilation/test_self_containment.pytests/integration/compilation/test_staleness.pytests/integration/extensions/test_context_switching.pytests/integration/extensions/test_header_context.pytests/integration/utils/__init__.pytests/integration/utils/cache.pytests/integration/utils/client.pytests/integration/utils/workspace.pytests/unit/server/rank_hosts_tests.cpptests/unit/syntax/preamble_synthesis_tests.cpptests/unit/syntax/scan_tests.cpp
💤 Files with no reviewable changes (2)
- src/semantic/context.h
- editors/vscode/src/feature/header.ts
A self-contained header context borrows a command but injects no -include, so a bound == 0 header under it has nothing to precompile — gate the PCH build on an actual synthesized prefix instead of the mere presence of a header context.
pnpm-workspace.yaml carried literal 'set this to true or false' placeholders from an unfinished pnpm approve-builds run, failing every pnpm install with ERR_PNPM_IGNORED_BUILDS. vsce-sign and keytar build scripts are not needed for local dev, so decline them explicitly.
The extension-host launch configs used the watch task whose $ts-webpack-watch matcher comes from an optional extension; without it the background task never reports ready and F5 stalls with matcher errors. Pre-launch now runs a one-shot compile with no matcher.
16bit-ykiko
force-pushed
the
header-context-synthesis
branch
from
July 3, 2026 13:15
7e17e4c to
33db73f
Compare
When the editor tears the whole process group down (hard restart), all workers die from SIGTERM before the master has entered its own stop path, and each one was reported through the WorkerCrash anomaly channel straight to the user. SIGTERM/SIGINT/SIGHUP are termination requests, not crashes — log a warning and restart quietly.
Explorer view listing the active file's contexts (active one checked, click to switch, Load more for pagination, refresh button), plus an editor context-menu entry for Select Compilation Context. The QuickPick command stays.
Splicing the PCH's pre-serialized links into the worker's array left a trailing comma when the PCH side was an empty [] (a preamble of only defines), producing invalid JSON that killed the client connection.
queryContext stamps its results with a workspace epoch (bumped on didSave); switchContext rejects choices made against an older epoch with stale = true, so a client can never silently apply a context picked from an outdated listing.
Tree view and QuickPick pass the listing epoch through switchContext and re-query on a stale answer. Files like .def/.inc/.inl that open as plain text are flipped to C++ when clice reports compilation contexts for them, so the language server attaches to X-macro fragments.
Workers compute the untaken #if branch bodies from the tracked condition directives; the master pushes them as a clice/inactiveRegions notification after every compile and the vscode extension renders them dimmed. Switching the compilation context recompiles and flips the dimmed regions — the most direct visual feedback of a switch.
The content after the include position is synthesized (mirrored along the chain) into a suffix file, injected by appending one #include line to the header's buffer: X-macro fragments embedded in enums or function bodies now see their braces close instead of piling errors at EOF. A cut inside #if blocks is reopened with matching '#if 1's so both sides stay balanced. Includes of the header itself along the chain are redirected to a disk snapshot — its real path is remapped to the buffer with the trailing include, which would otherwise recurse forever. The snapshot joins the staleness snapshot; docs updated, suffix removed from known limitations.
A user can open the synthesized prefix/suffix/snapshot files for debugging. They are fragments of the TU they were synthesized for, so resolve_header_context records an artifact -> host mapping (persisted in cache.json) and fill_header_context_args compiles them with the host's command, treated as self-contained — never deriving context from other synthesized state. Stale artifacts without a recorded host fall back to the default command.
Windows' <csignal> defines only a POSIX subset; compare against POSIX SIGHUP's value directly — a worker can only receive it on POSIX anyway.
Member
Author
|
@codex review |
Three stacked causes kept clice/inactiveRegions empty: - DirectiveCollector mapped CVK_False to ConditionValue::None, so a failed #if was never recorded as False (latent — no reader used the value until now). - #else carries no condition value; inactivity is now derived from whether an earlier branch of the level was taken. - Conditions inside the preamble bound live in the PCH and never replay in the AST compile. The PCH build now scans its share and reports the conditional stack still open at the bound; the AST compile resumes from that stack and the master publishes both halves.
A user resetting state with rm -rf .clice under a running server left the store's tmp and namespace directories gone forever — every PCH build after that failed with 'unable to open output file'. Re-create them on demand in begin_store/commit.
The palette exposed one blended command; now it mirrors the protocol: Switch (query + pick + switch, formerly Select), Show Current (the active choice as a message), and Query (focus and refresh the contexts view). Switch and Show Current also sit in the editor context menu.
Member
Author
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4a77ade35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Hash CDB entries after applying config rules everywhere, so command switching works in workspaces with clice.toml rules. - A multi-configuration host provides one context per CDB entry; switchContext pins the entry by hash (ActiveContext.command_hash, threaded through resolution, persistence and currentContext). - Path pool ids start at 0: use ~0u as the saved-host sentinel instead of a truthiness test that dropped choices for the first indexed file. - Rescan a saved file's include edges (Workspace::rescan_includes + DependencyGraph::reset_includes), so context queries see includes the save added or removed instead of the startup snapshot. - A chain-file save also drops a persisted NeedsContext verdict: the dependency may now provide the missing declarations. - Align the forced-fragment extensions (.def/.inc/.inl/.tpp/.ipp) with the client's C++ fragment detection. - ActiveContext.occurrence is optional: an explicitly chosen occurrence — including #0 — forces prefix synthesis. - Persist PCH inactive-region metadata in cache.json; encode the open conditional stack with both the inactive and branch-taken bits so an #else after the bound resolves correctly. - The vscode extension fires a documentSymbol request after switching, refreshing diagnostics and inactive regions without user interaction. - Nits: default-init HeaderContext::host_path_id, const rank_hosts, rename an ambiguous test variable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e1ef041b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Include the working directory in canonical command hashes so identical argv under different build dirs stay distinct contexts. - Match pinned host hashes through a host-rules lookup: header-path rules change the argv the published hashes were computed from. - Copy PCH inactive regions before awaiting the stateful worker; the pch_cache StringMap can rehash across the suspension point. - Reset trial_done on switchContext so the new context re-earns its self-containment verdict. - Persist the pinned command hash for header contexts and validate restored choices in didOpen, dropping stale saved contexts. - Resolve incremental didSave rescans under every CDB entry instead of collapsing all configs to the first command's search paths. - Fingerprint persisted NeedsContext verdicts with the header's content hash; a mismatch on cache load drops the verdict. - Activate the VSCode extension on startup so first-opened fragment files (.def/.inc/...) are detected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e668a225db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Escape backslashes/quotes in the appended suffix #include so Windows cache paths survive string-literal parsing. - Drop diagnostics remapped onto the phantom suffix line (past the user's EOF) before publishing. - Offer a file's own CDB contexts even when hosts exist, so a host override can be switched back. - Drop orphaned context choices when a save removes the include edge they depend on, instead of stranding the header on the fallback command; bump the generation to invalidate in-flight compiles. - Record an earned SelfContained verdict in memory (never persisted) and dedup queryContext hosts only on that confirmed verdict, not on un-trialed Unknown; clear it wherever the trial is re-armed. - VSCode: align the language client's documentSelector with the context UI (c/cuda-cpp); pin the QuickPick's target document so switching editors mid-pick cannot retarget the request.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c733c59cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Record branch truth (taken or not) for #ifndef/#elifndef conditions instead of macro definedness — inactive-region dimming was painting include-guard bodies as inactive. - Escape backslashes/quotes in rewritten include paths emitted into synthesized preambles/suffixes (Windows paths). - Revalidate pinned occurrences after saves: a vanished occurrence drops the context choice even when other inclusions keep the chain alive; occurrence counting hoisted to Workspace::count_occurrences. - VSCode: discard stale context-tree refresh/loadMore responses that finish after the active editor changed; scan already-open documents for C++ fragments on activation (onDidOpenTextDocument does not fire retroactively). - Make the dedup integration test deterministic: dedup now requires a confirmed self-contained verdict, so wait for the header's trial compile before querying.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a00953c95d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This was referenced Jul 4, 2026
This was referenced Jul 12, 2026
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.
Implements the full compilation-context design (docs/*/design/compilation-context.md), closing the gap between the documented model and the implementation, and fixing several latent bugs found along the way.
What changed
Synthesis & invalidation
cdb.lookup()never returns empty (it synthesizes a default command), which made automatic header-context resolution dead code — headers silently compiled with a bare default command. Gated onhas_entry(); the default command remains as the final fallback for isolated files.scan()+ real search-path resolution: quoted includes are absolutized (the prefix file lives in the cache dir), matching prefers unconditional directives, and a cut inside#ifblocks (include guards on intermediate headers) emits balancing#endifs.HeaderContexttracks its include chain with a content snapshot; changing any chain file re-synthesizes the prefix. Fixed theon_file_savedshadowing bug that made host-save invalidation a no-op.#includes update host lookups without a full workspace rescan.Suffix injection
.defincluded mid-enum, mid-function, or mid-class now sees the rest of its includer) is restored by appending a single#includeline past the buffer's EOF, pointing at a synthesized suffix file.#ifnesting cut at the include point is closed by the prefix and reopened at equal depth in the suffix; braces need no balancing since prefix/main/suffix form one continuous token stream.<hash>.self.h), which participates in staleness tracking.PCH
bound == 0): the-include'd prefix is baked in via the predefines buffer, and clang's PPOpts subsumption prevents double processing. X-macro.deffiles stop re-parsing the prefix per edit.pch_cachere-keyed by content key: identical preambles share one build; cache.json v2. Metadata is dropped when the store evicts the blob, and the cache store self-heals externally wiped directories.Protocol
queryContextdedupes by canonical flags hash (self-contained files only; NeedsContext headers stay per-host), ranks hosts by relevance, and paginates.switchContextvalidates the host actually includes the header, the command hash names a real entry, and the listing epoch is current (stale listings force a re-query); it bumps the session generation so an in-flight compile cannot clobber the switch.clice/inactiveRegionspush notification: inactive preprocessor regions are computed by the worker (PCH-covered preamble regions merged with the resumed main-file scan, threading the open conditional stack across the boundary) and rendered dimmed in VSCode.Self-containment detection
.def/.inc/.inl/.tpp/.ippskip the trial and always get context.Editors & docs
Clice: Switch/Show Current/Query Compilation Contextscommands in the palette and editor context menu, driving the three custom requests..def/.inc/.inl/.tpp/.ipp) are flipped to C++ when the server knows a context for them; the extension activates on startup so first-opened fragments are detected.header_contextE2E scenario for vscode/nvim harnesses, including direct custom-request roundtrips through the real extension client.Fixed along the way
CVK_Falseconditions were recorded asCondition::Nonein the directive table.SIGHUPdoesn't exist on Windows).Review fixes — 3-agent adversarial review, then two Codex review rounds (all threads addressed and resolved; latest:
e668a22)StringMaprehash across a suspension point).switchContextresets the self-containment trial for the new context.Testing
compilation context requeststest passes.Known follow-ups (out of scope)
CommandSource::Inferred) but not implemented.