fix(server): header invalidation, index staleness and context ownership - #485
Merged
Merged
Conversation
The merged index shard's staleness check was mtime-only, so touching a file (mtime bumped, bytes unchanged) forced a pointless reindex. Shards now record a content hash per distinct dependency: an unchanged mtime is trusted without reading the file, a changed mtime re-hashes and an unchanged hash is not stale. The mtime baseline is not written back after a hash match — that would rewrite the whole immutable shard blob to save one file read; the tradeoff is recorded where it is made. Re-merging a TU previously accumulated: the old canonical contribution's occurrences and relations stayed live alongside the new ones, so a reindex served pre-edit and post-edit state at once. A re-merge now releases the previous contribution of the same TU (header shards are keyed by the including TU — the call site wrongly passed the header's own id, unlike every test), identical content is resurrected by the ref count, and other TUs' contributions survive. A header shard's stored content also refreshes on re-merge instead of being written only once. The shard format changes, so shards now carry a format version and load() verifies the buffer and silently discards mismatches (it previously wrapped raw bytes with no validation at all) — a cache directory from an older build rebuilds once in the background instead of crashing or being misread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Saving a header updated open sessions but left the index shards of closed TUs that include it untouched, so cross-file references kept serving the pre-save state indefinitely. The BufferSaved case now collects the root TUs transitively including the saved file through the reverse include graph — the union of the sets taken before and after the rescan, a cheap safety net for a reverse map that was stale at save time — and splits them like the module cascade: open dependents are marked for recompilation (their compile inputs changed), closed ones are re-enqueued for background indexing. Enqueueing is O(1) per TU and deliberately uncapped: the index's content-hash staleness check filters TUs whose dependencies did not actually change, and idle/priority scheduling throttles the rest. The integration test anchors the user-visible fix: retargeting a macro in a header updates references served from a TU that was never opened. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The context resolver now owns what it always governed: header self-containment verdicts, user context choices and synthesized-artifact attribution (with their cache.json slices, serialized by the resolver through the same interning order — the disk format is unchanged), and the resolved header contexts. The Session's parallel keys are gone. active_context/active_command duplicated the persisted choice per open buffer and the two had to be kept in sync at every write; the saved-context table is now the single source of truth, validated on didOpen and consulted through one accessor that keeps user choices away from background indexing. Header contexts move out of the Session entirely and outlive it: closing a header no longer discards its synthesized preamble, so reopening reuses it instead of re-synthesizing (a behavior change, anchored by an integration test on artifact mtimes). Every invalidation point moved with the ownership — save-chain hits, orphaned-choice drops, switchContext resets and the trial's context fallback now clear the table entry, and the engine reports verdict resets as a DirtySet effect instead of touching the tables, so invalidator.cpp no longer mutates any context-domain state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
IndexQuery translated ids between the server-wide path pool and the project index's own pool by resolving to a path string and looking it up in the other pool — a StringMap round-trip on every hot-path query (each reference file of every relations/definition lookup). ProjectIndex now keeps a bidirectional proj<->server id cache. It is extended after each TU merge for paths both pools already know, backed by a lazy fallback in the two translators that performs the old string round-trip on a miss and records the result — behavior on misses is unchanged, including files present in only one pool. The mapping is never serialized: server ids are per-session, and a ProjectIndex restored from disk starts empty and relearns links as queries arrive. The pools themselves stay separate; on-disk shards keep their self-contained path tables. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
need_update indexed the path mapping without the bounds check its merge counterpart has; an id the mapping does not cover now conservatively reports the shard stale instead of reading out of bounds. The staleness helper takes an optional baseline hash instead of a value+flag pair, and the merge-time dependency hashing carries a TODO: it re-reads files the indexer worker already read, worth shipping hashes in the TUIndex if cold-start profiles show it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Unit tests for the resolver-owned choice table: a pinned CDB entry steers an open session's command but never background indexing (which passes no session), didOpen validation keeps a valid persisted choice and drops both stale flavors (host without a CDB entry, command hash matching no entry). An integration test anchors the re-synthesis side of preamble reuse: reopening a header after its chain file changed on disk must rebuild the preamble, not reuse the stale one. The header-context table's doc now spells out that an automatic host sticks until invalidation (reuse wins over re-ranking on reopen) and carries a TODO on entry accumulation for never-reopened headers. Co-Authored-By: Claude Fable 5 <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:
📝 WalkthroughWalkthroughThis PR adds content-hash dependency staleness and shard format validation to index persistence, switches indexing and query code to shared path pools, and moves persisted header/context state into ContextResolver with cache-backed validation and invalidation. ChangesIndex Dependency Hashing and Shared PathPool
ContextResolver Refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant BackgroundIndexer
participant PathPool
participant ProjectIndex
participant MergedIndex
participant WorkspaceStorage
BackgroundIndexer->>PathPool: intern and resolve file paths
BackgroundIndexer->>ProjectIndex: merge TU with shared path pool
BackgroundIndexer->>MergedIndex: merge shard using TU path and dep list
BackgroundIndexer->>WorkspaceStorage: save project blob and shard blobs
WorkspaceStorage->>PathPool: serialize compact path tables
WorkspaceStorage->>MergedIndex: load shard, verify format_version, read dep_hashes
sequenceDiagram
participant Compiler
participant ContextResolver
participant Invalidator
participant MasterServer
participant Workspace
Compiler->>ContextResolver: query header_context and record header mode
Invalidator->>ContextResolver: chain_dependents and invalidate_header_deps
MasterServer->>ContextResolver: reset header mode, load/save cache slices
ContextResolver->>Workspace: dump/load persisted context slices
Estimated code review effort: 5 (Critical) | ~120 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 |
![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: 022f102d5a
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/context/context_resolver.cpp (1)
704-717: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrop the cached header context when the saved choice is invalidated.
saved_contextsis erased here, butheader_contexts[path_id]survives. If the file is reopened before a new save,fill_header_context_args()can still reuse that cached context when the dependency snapshot matches, leaving the header pinned to an old host.Proposed fix
if(!valid) { LOG_INFO("didOpen: dropping stale saved context for {}", path); + drop_header_context(path_id); saved_contexts.erase(it); }🤖 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/context/context_resolver.cpp` around lines 704 - 717, The stale-context cleanup in the didOpen path only removes the entry from saved_contexts, but the corresponding cached header state in header_contexts can still be reused. Update the invalidation branch in context_resolver.cpp to also clear the header cache for the same path_id when the saved choice is dropped, so fill_header_context_args() cannot reuse an old host context. Use the existing symbols saved_contexts, header_contexts, and the didOpen stale-context handling block to locate the fix.src/index/merged_index.cpp (1)
764-808: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMain-TU merge can silently wipe stored content on a transient read failure.
Unlike the header-context overload (Line 818, fixed in this same diff to only refresh
content/line_startswhen the new content is non-empty and different), this overload unconditionally doesself.impl->content = content.str()at Line 772.contentoriginates inbackground_indexer.cppfromllvm::MemoryBuffer::getFile(file_path), which is left as an emptyStringRefon read failure (deleted file, permission race, etc.). A failed read on an otherwise-successful reindex would overwrite previously-goodcontent/line_startswith empty data, corrupting position-mapping (hover, go-to-definition text, etc.) for that shard — and nothing here would self-heal it sincecontext.build_atis still stamped fresh.🐛 Proposed fix to match the header-merge guard
self.load_in_memory(); - self.impl->content = content.str(); - self.impl->line_starts = kota::ipc::lsp::build_line_starts(self.impl->content); + if(!content.empty() && self.impl->content != content) { + self.impl->content = content.str(); + self.impl->line_starts = kota::ipc::lsp::build_line_starts(self.impl->content); + }🤖 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/index/merged_index.cpp` around lines 764 - 808, The MergedIndex::merge overload for the main TU unconditionally overwrites stored content and line starts, so a transient empty read can erase previously valid data. Update this merge path to follow the same guard used by the header-context overload: only refresh self.impl->content and self.impl->line_starts when the incoming content is non-empty and actually differs from the existing content. Use the MergedIndex::merge and self.impl members to locate the change, and keep the rest of the merge logic unchanged.
🧹 Nitpick comments (1)
src/index/merged_index.cpp (1)
90-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize
hash_file
src/index/merged_index.cppduplicates the samexxh3_64bitsfile hash already defined insrc/server/workspace/workspace.cppand used bysrc/server/context/context_resolver.cpp. Move it to a shared helper, or reuse the existing one, so the hash scheme stays in one place.🤖 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/index/merged_index.cpp` around lines 90 - 100, The local hash_file helper in merged_index.cpp duplicates the existing workspace file hashing logic, so centralize the xxh3_64bits implementation in a shared helper or reuse workspace::hash_file instead of keeping a second copy. Update the merged index code to call the shared function, and ensure any callers in merged_index.cpp continue to handle the same 0-on-failure behavior consistently.
🤖 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/service/lsp_client.cpp`:
- Around line 203-204: Update the stale inline comment near
validate_saved_context in lsp_client::session handling so it no longer says the
code “restores” a context choice; change it to describe that the saved context
is being validated and discarded if stale, matching the behavior of
srv.contexts.validate_saved_context(*session) and the fact that header contexts
now persist across closes.
In `@tests/integration/features/test_index_staleness.py`:
- Around line 54-65: The baseline snapshot is taken too late in the second
session, so `make_client()` may still be triggering startup indexing when
`before` is recorded. In `test_index_staleness`, move the
`shard_mtimes(tmp_path)` and `project_mtime(tmp_path)` baseline capture to
before calling `make_client(executable, tmp_path)`, then keep using those values
for the `poll` and final `after == before` assertion to avoid a race with
session 2 startup work.
---
Outside diff comments:
In `@src/index/merged_index.cpp`:
- Around line 764-808: The MergedIndex::merge overload for the main TU
unconditionally overwrites stored content and line starts, so a transient empty
read can erase previously valid data. Update this merge path to follow the same
guard used by the header-context overload: only refresh self.impl->content and
self.impl->line_starts when the incoming content is non-empty and actually
differs from the existing content. Use the MergedIndex::merge and self.impl
members to locate the change, and keep the rest of the merge logic unchanged.
In `@src/server/context/context_resolver.cpp`:
- Around line 704-717: The stale-context cleanup in the didOpen path only
removes the entry from saved_contexts, but the corresponding cached header state
in header_contexts can still be reused. Update the invalidation branch in
context_resolver.cpp to also clear the header cache for the same path_id when
the saved choice is dropped, so fill_header_context_args() cannot reuse an old
host context. Use the existing symbols saved_contexts, header_contexts, and the
didOpen stale-context handling block to locate the fix.
---
Nitpick comments:
In `@src/index/merged_index.cpp`:
- Around line 90-100: The local hash_file helper in merged_index.cpp duplicates
the existing workspace file hashing logic, so centralize the xxh3_64bits
implementation in a shared helper or reuse workspace::hash_file instead of
keeping a second copy. Update the merged index code to call the shared function,
and ensure any callers in merged_index.cpp continue to handle the same
0-on-failure behavior consistently.
🪄 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: d9d6e870-f9e6-4997-a72f-880423d4f485
📒 Files selected for processing (25)
src/index/merged_index.cppsrc/index/merged_index.hsrc/index/project_index.hsrc/index/schema.fbssrc/index/serialization.hsrc/server/compiler/compiler.cppsrc/server/context/context_cache.hsrc/server/context/context_resolver.cppsrc/server/context/context_resolver.hsrc/server/index/background_indexer.cppsrc/server/index/query.hsrc/server/service/lsp_client.cppsrc/server/service/master_server.cppsrc/server/session/session.hsrc/server/workspace/invalidator.cppsrc/server/workspace/invalidator.hsrc/server/workspace/workspace.cppsrc/server/workspace/workspace.htests/integration/extensions/test_context_switching.pytests/integration/features/test_header_reindex.pytests/integration/features/test_index_staleness.pytests/unit/index/merged_index_tests.cpptests/unit/index/path_mapping_tests.cpptests/unit/server/context_resolver_tests.cpptests/unit/server/invalidator_tests.cpp
Maintainer feedback on the id-mapping cache: two id spaces plus a bidirectional translation layer is complexity spent preserving an accident. There is now a single runtime path-id space — the server-wide pool — and the mapping layer is gone entirely: symbol reference bitmaps carry pool ids, shard tables are keyed by them, and queries never translate anything. Session ids must not leak to disk, so persistence works by remapping: - The project blob writes a compact path table covering exactly the ids its bitmaps and shard manifest reference — this doubles as garbage collection, since paths the pool accumulated but nothing references never reach disk — and loading interns the table into the running pool. The blob also gains verification and a format version (it was previously wrapped with no validation at all). - Shards are fully self-contained: each carries its own path table and every internal id (dependency locations and hashes, contribution keys) indexes into it, so the staleness check needs no external path mapping any more. - Shard blobs are named by a hash of the path instead of a pool id, and the project blob's manifest says exactly which blobs to fetch; the loader sweeps everything else. The ProjectIndex's dead `indices` field goes away with the old schema. Old blobs fail the version check and rebuild once in the background, covered by the same one-time rebuild this branch already declares. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/index/merged_index_tests.cpp (1)
591-606: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile positional flatbuffer construction in
OldShardDiscarded.
CreateMergedIndex's generated signature binds strictly to schema.fbs field declaration order (confirmed by FlatBuffers docs). Passing 11 anonymous positional args (onlyformat_versionis commented) means a future schema field addition/reorder could silently shift values into the wrong fields — since most fields here are0-compatible types, this wouldn't fail to compile, just silently corrupt the test's intent.Consider using
MergedIndexBuilderwith namedadd_*calls instead, which stays correct across schema changes and self-documents which field is being tested.♻️ Example using named builder methods
- auto root = clice::index::binary::CreateMergedIndex(builder, - 0, - 0, - 0, - 0, - 0, - 0, - 0, - 0, - content, - 0, - 0, - /*format_version=*/0); - builder.Finish(root); + clice::index::binary::MergedIndexBuilder mib(builder); + mib.add_content(content); + mib.add_format_version(0); + auto root = mib.Finish(); + builder.Finish(root);🤖 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/unit/index/merged_index_tests.cpp` around lines 591 - 606, The `OldShardDiscarded` test is using a fragile positional call to `CreateMergedIndex`, which can silently break if the FlatBuffers schema order changes. Replace the anonymous arguments with `MergedIndexBuilder` and explicit `add_*` calls so the test clearly sets the intended fields and remains stable across schema updates.
🤖 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/index/project_index.cpp`:
- Around line 133-148: The symbol loader in project_index.cpp dereferences
entry->symbol() without checking for null, even though SymbolEntry.symbol is
optional. Update the root->symbols() loop in the loading logic to guard
fb_symbol before reading name(), kind(), scope(), or refs(), and skip or safely
handle entries where SymbolEntry::symbol is missing so the loader does not crash
on malformed blobs.
In `@src/server/index/background_indexer.cpp`:
- Around line 58-71: The main-TU read path in background_indexer.cpp can
overwrite existing shard content with an empty string when
llvm::MemoryBuffer::getFile(file_path) fails. Update the logic around
file_content/file_content_storage and shard.merge(...) so a failed read does not
clear previously stored content; either skip the merge for the main TU on read
failure or preserve the prior content, using the same guarded fallback style
already used for header content handling in the nearby merge code.
---
Nitpick comments:
In `@tests/unit/index/merged_index_tests.cpp`:
- Around line 591-606: The `OldShardDiscarded` test is using a fragile
positional call to `CreateMergedIndex`, which can silently break if the
FlatBuffers schema order changes. Replace the anonymous arguments with
`MergedIndexBuilder` and explicit `add_*` calls so the test clearly sets the
intended fields and remains stable across schema updates.
🪄 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: 791546eb-c750-4e94-ad82-2ae67ebbfeac
📒 Files selected for processing (15)
src/index/merged_index.cppsrc/index/merged_index.hsrc/index/path_pool.hsrc/index/project_index.cppsrc/index/project_index.hsrc/index/schema.fbssrc/server/index/background_indexer.cppsrc/server/index/query.cppsrc/server/index/query.hsrc/server/service/agent_client.cppsrc/support/path_pool.htests/unit/index/index_query_tests.cpptests/unit/index/merged_index_tests.cpptests/unit/index/persisted_index_tests.cpptests/unit/index/project_index_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index/merged_index.cpp
Four review findings on the reindex-replacement machinery, each real: - Serialized shards are served through buffer-only lookups that never consult the removed bitmap, so masked rows resurrected after a restart. Serialization now compacts instead: dead rows are dropped, live bitmaps written pre-subtracted, dead canonical-cache entries go with them, and the persisted removed bitmap is always empty — which also stops masked rows accumulating on disk under heavy editing. - A dependency edited between the worker's read and the merge-time hashing got a baseline blessing content the rows were never built from, hiding the edit forever. Deps whose mtime postdates the build now get no baseline: the staleness check stays conservative and the TU reindexes once more instead of never. - A file dropped from a TU (removed transitive include) kept that TU's old contribution alive in its shard, serving references under the removed edge. After each merge the TU's contribution is swept from untouched shards, probed cheaply without deserializing them. - A cached self-contained header context tracks no chain deps, so zeroing its baseline on chain invalidation forced nothing; with contexts now surviving didClose that left reopened headers on a stale automatic host. Invalidation drops such entries outright — the next use re-resolves, cheap on the borrow route. Also: the loader skips a TU merge whose content cannot be read instead of wiping the shard's stored text, guards a nullable field in the project blob loader, un-races the touch integration test's baseline, and refreshes a stale didOpen comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96b23392a3
ℹ️ 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".
The header-save reindex test fails on both Windows targets while every other platform passes, and the shared fixture only surfaces warning and error lines from the server. Own the client in this test and dump the full server log tail on failure so the Windows runs show what the dispatcher and background indexer actually did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/features/test_header_reindex.py (1)
96-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBlind
except Exceptionflagged by Ruff (BLE001).Reasonable as a teardown fallback, but narrowing to the expected exception types (e.g., connection/timeout errors from
shutdown_async/exit) would satisfy the linter without weakening the fallback-to-kill behavior.🤖 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/features/test_header_reindex.py` around lines 96 - 98, The teardown in the client shutdown path uses a blind except Exception, which Ruff flags as BLE001. Update the exception handling around client.exit(None) in the test cleanup to catch only the expected shutdown-related errors from exit/shutdown_async, while keeping the client.kill_server() fallback for those cases. Use the existing client exit/kill_server flow to narrow the handler without changing the fallback behavior.Source: Linters/SAST tools
🤖 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 `@tests/integration/features/test_header_reindex.py`:
- Around line 105-111: The diagnostic block in the integration test can lose the
original failure details if server.stderr.read() times out. Update the
failure-handling path around server.stderr.read() in test_header_reindex.py to
catch asyncio.TimeoutError and still call pytest.fail with the collected
failures plus any available log tail. Keep the existing failures aggregation and
tail-building logic intact, but make the timeout non-fatal so it cannot replace
the real test failure.
---
Nitpick comments:
In `@tests/integration/features/test_header_reindex.py`:
- Around line 96-98: The teardown in the client shutdown path uses a blind
except Exception, which Ruff flags as BLE001. Update the exception handling
around client.exit(None) in the test cleanup to catch only the expected
shutdown-related errors from exit/shutdown_async, while keeping the
client.kill_server() fallback for those cases. Use the existing client
exit/kill_server flow to narrow the handler without changing the fallback
behavior.
🪄 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: 2abff657-5adc-4a90-bae9-e39488e3d729
📒 Files selected for processing (1)
tests/integration/features/test_header_reindex.py
The header-save reindex test failed only on Windows: write_text translated the header to CRLF while didChange carried LF text, so after the save the open buffer and the disk disagreed — a divergence a real editor never produces. Index navigation on an open file resolves positions against the buffer while shards index the disk content, so every lookup missed by the CRLF offset delta and the (correctly) reindexed reference was never found. Writing the file with explicit LF newlines keeps the two in agreement on every platform. The server-log dump instrumentation served its purpose and is removed; the reindex itself was proven to work on Windows all along. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Debug macOS run finishes the suite in just over 20 minutes since the suite gained its invalidation coverage; give it headroom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e74ad93244
ℹ️ 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".
Second round of review findings on the replacement machinery: - The TU's own content joins its shard's dependency hashes: a closed TU edited on disk with unchanged includes never looked stale, and a merge skipped on a transient read failure could otherwise serve the old shard forever. - The post-merge sweep tracks shards actually re-merged instead of every path in the new include graph, so a header whose contribution became empty is swept like a dropped one. - Closed headers whose cached context embeds a saved chain file are re-enqueued for background indexing — their shard rows were built under the old chain, and with contexts surviving didClose there is no session left to recompile them. - A manifested shard blob the loader rejects counts as missing and its file is enqueued for rebuild instead of being served as an empty shard that nothing would ever refresh. - The schema marks unconditionally-dereferenced tables required, so the verifier rejects a syntactically valid blob with missing vectors instead of letting the loader crash on it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
clice/src/index/project_index.cpp
Lines 21 to 22 in 480c72d
Restore closed-shard lookups for local symbols
After filtering ProjectIndex down to External symbols, FileLocal/TULocal symbols in closed files no longer have a reference_files entry, but the location queries still use workspace.project_index.symbols to decide which shards to scan (query_relations(), find_definition_location(), get_definition_text(), etc.). As a result, agentic lookups and hierarchy/definition resolution for closed-file locals such as static functions can find the name in a shard-local symbol table but then fail to locate its definition or references; keep a per-symbol shard map for locals or make these query paths scan the owning shard(s).
ℹ️ 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 6, 2026
This was referenced Aug 13, 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.
Intentional behavior changes
Unlike the preceding refactor PRs, this one exists to change behavior. Four changes, each anchored by a test:
Saving a header reindexes the closed TUs that include it. The invalidation engine walks the reverse include graph (union of the pre/post-rescan snapshots as a safety net for a stale reverse map) and splits the transitively dependent root TUs: open ones recompile, closed ones re-enqueue for background indexing. Cross-file references no longer serve the pre-save state indefinitely — the acceptance test retargets a macro in a header and watches references served from a TU that was never opened follow it. Enqueueing is deliberately uncapped; the point below is the storm filter, and a TODO tracks observing large projects before adding debouncing.
Touching a file (mtime change, same bytes) no longer triggers reindexing. Merged-index shards now record a content hash per distinct dependency: an unchanged mtime is trusted without reading the file, a changed mtime re-hashes, and an unchanged hash is not stale. The baseline mtime is intentionally not written back after a hash match — that would rewrite the whole immutable shard blob to save one file read.
The on-disk index format is versioned; old shards are discarded and rebuilt once. The shard format changes (dependency hashes), so shards now carry a format version and the loader verifies the buffer before trusting it — previously raw bytes were wrapped with no validation at all, so a stale cache directory could be misread. After upgrading, the first start rebuilds the index in the background (minutes-scale on large projects, one time).
In the same area, re-merging a TU now replaces its previous contribution to every shard it touches (canonical reference release; header shards keyed by the including TU, which the call site previously got wrong in a way every test already disagreed with). Without this, a reindex served pre-edit and post-edit occurrences side by side — visible the moment change fix: build with clang-18 toolchain #1 makes live reindexing common.
Reopening a header reuses its compilation context. Resolved header contexts are now owned by the context resolver and survive didClose, so reopening reuses the synthesized preamble instead of re-synthesizing it. Entries re-validate at use by content hash and are invalidated by saves along their include chain; an integration test pins both directions (reuse when nothing changed, re-synthesis when a chain file changed on disk). Consequence: an automatically chosen host sticks until an invalidation instead of being re-ranked on every reopen.
Behavior-preserving refactors riding along
Known costs surfaced for follow-up (TODOs in code)
Tests
Locally green: 809 unit (17 new), 3/3 smoke, 242 integration (4 new). Snapshot changes: none beyond the listed behavior changes.