feat(store): index-format rebuild boundary for the File-QN change - #1104

Thanks for the focused diagnosis and for linking the public #769 repro. I have triaged this as a high-priority 0.9.1-rc parsing/search correctness fix. The current diff has no dependency, workflow, network, credential, prompt-injection, or supply-chain concern.

The present unit coverage is not sufficient for this change:

1. Angular component sibling files (ts/html/scss) merged into one File node; search_code silently incomplete #769's existing maintainer acceptance criterion requires an end-to-end red regression that asserts one File node per .ts/.html/.scss sibling and complete search_code marker coverage. Please add that repro test; simple FQN inequality does not prove the graph and scoped file list.
2. Assert the exact new File QNs, not only pairwise inequality, and cover multi-extension names, extensionless files, and dotfiles.
3. This changes the persisted qualified_name of every File node. An unchanged existing index can retain old identities, while an incremental update can introduce new identities only for touched files. That can create a mixed graph with duplicates, stale edges, or missing lookups. Please do not implement a migration or CI expansion yet; maintainers need to choose between an explicit full-reindex compatibility boundary and a migration/versioning strategy.
4. Remove the trailing whitespace in fqn.c and the leading "# " from the PR title.

CI is still running. The identity compatibility decision remains a blocker even if it turns green.

Maintainer decision: the File-QN direction is accepted, and we will use an automatic one-time full-reindex compatibility boundary rather than an in-place migration.

Please update this PR with the following scope:

1. Add the end-to-end RED regression requested above for the Angular component sibling files (ts/html/scss) merged into one File node; search_code silently incomplete #769 sibling-file corpus. It must prove distinct File nodes and complete search_code marker coverage, then turn green with the fix.
2. Add a narrow persisted index-format compatibility marker for this File-QN format change. A pre-change index must be routed through the existing full-reindex path before incremental or watcher refresh can produce mixed old/new File identities. A freshly rebuilt/current-format index must not rebuild again on the next unchanged run.
3. Add a regression that starts from a pre-change index, proves the one-time full rebuild, verifies the distinct .ts, .html, and .scss File nodes afterward, and verifies that a subsequent run does not force another rebuild. Preserve the existing ADR/project metadata behavior of the full-reindex path.
4. Keep this as a rebuild boundary; do not add an in-place node-renaming migration. Old collided databases no longer contain enough information to reconstruct every missing sibling safely.
5. Retain the exact-QN and filename-edge-case coverage requested above and fix the current lint failure from trailing whitespace.

This resolves the maintainer-owned compatibility decision. Merge review will resume against the new exact head after these correctness and upgrade-path tests are present and CI is green.

Rebased onto main. #1108 landed the same file guard while this was in review, so I’ve dropped my fqn.c change — this PR no longer touches that file.
What’s left is the compatibility boundary from your scope, which isn’t in #1108. So main currently has the gap you flagged: a pre-change index keeps collided File QNs, and the next incremental run mints new-format QNs only for touched files. This adds the PRAGMA user_version marker stale format routes through the existing full reindex path once, rebuilt index doesn’t rebuild again.
Worth flagging: the stamp has to be on both dump paths. cbm_writer_open truncates the DB file on every dump, so without re-stamping on the incremental path, every run resets the marker and forces a rebuild on the next. test_incremental.c’s noop/rapid tests caught it.
Also keeping repro_issue769.c for the search_code sibling coverage #1108’s tests don’t have.

Thanks for rebasing this on top of #1108 and narrowing the PR to the compatibility boundary. That remaining boundary is useful, but this head is not ready to merge yet:

• Please move or port the new end-to-end regressions into a gating CI suite. They are currently registered only under test-repro, so the green required checks do not execute them.
• Please make the upgrade test represent an unmistakably legacy/collided graph. Building a current-format graph and only setting PRAGMA user_version = 0 proves the rebuild message, but not that the rebuild repairs old File identities. The test should prove the stale identity is removed, exact new QNs appear, ADR/project metadata survives, and a second unchanged run stays incremental.
• A failed format-stamp write should fail the indexing operation. Logging and returning success leaves the database stale and can force a full rebuild on every run.
• Please update the PR description to match the post-fix(fqn): preserve full filename in File-node QNs so sibling files don't collide #1108 scope and clean the trailing whitespace/final newline findings.

Once those are in place, we can re-review the exact updated head.

Reviewed in depth. This closes a real gap that still exists on main today, the mechanism is the right one, and it needs a focused rebase rather than a rewrite.

The gap is live. #1108 (File QNs keep the extension) merged on 16 July without any format boundary, and main has no format-version mechanism of any kind — I checked, there are zero user_version references in src/. So on current main, a pre-#1108 index that gets refreshed incrementally mints new-format QNs only for the files that changed, leaving the rest on old-format QNs: duplicate File nodes and stale edges in the same graph. That is exactly the corruption this PR prevents, and nothing else on main prevents it.

The design choices are right, and two in particular are worth calling out.

PRAGMA user_version is the correct carrier rather than sniffing the data. It lives in the SQLite header, so it survives a file copy and VACUUM INTO — which is what artifact export uses. That means the artifact path falls out coherently for free: a legacy artifact imports as version 0 and rebuilds once (correct, since it holds collided QNs), while a new artifact imports already stamped and does not. No extra code needed for it.

And you caught the trap that would have made this useless: stamping both dump paths. If only the full path stamped, every incremental run would reset the marker and force a perpetual rebuild loop. Getting that right is the difference between a boundary and a footgun.

On correctness in both directions, which is what I most wanted to check:

• False negatives (the worse failure): every legacy DB has user_version = 0, since nothing in this codebase has ever set it. 0 != 1 → rebuild. No legacy DB can slip through.
• False positives: the healthy loop converges — full stamps, incremental re-stamps. If the post-dump reopen fails the stamp is skipped and you get one spurious rebuild, then convergence. Fail-safe direction.

Your tests are binding. Run 1 fabricates a genuinely legacy graph — a collided QN node with user_version = 0 — drives the production index_repository flow, and asserts the route fires, the collided QN is gone, all three new QNs exist, the ADR survives, and the DB ends up stamped. Run 2 pins the false-positive guard: a current-format run does not rebuild again. That second one is the test most people forget.

Four things before it lands:

1. The incremental-path stamp is not actually bound by the new tests. Run 2 reads the format before the incremental dump, so deleting cbm_store_set_format_version from pipeline_incremental.c alone would leave both tests passing. Your description says test_incremental.c catches it, but that file asserts no pipeline.route tokens — I grepped. A third run, or a direct user_version == 1 assertion after an incremental dump, closes it cheaply.
2. Reopen failure should be treated as stamp failure. Right now if the post-dump cbm_store_open_path returns NULL the stamp is silently skipped and the run reports success — which is inconsistent with your own stance that a failed stamp fails the operation.
3. sqlite3_prepare_v2(..., "PRAGMA user_version;", CBM_NOT_FOUND, ...) passes an error-code constant as the nByte argument. Any negative value happens to work, but this should be -1; as written it reads as a copy-paste slip and will trip a future reader.
4. The migration needs to be visible. It is currently log-only. Someone watching an interactive index_repository sit for minutes has no indication it is a one-time format migration rather than a hang. Worth surfacing in the response or index_status — and it must be in the release notes, because on upgrade every existing project rebuilds once, including ones already correct on post-fix(fqn): preserve full filename in File-node QNs so sibling files don't collide #1108 binaries. At real scale that is minutes and tens of GB of RSS per project. That is inherent to stamping late and the alternative (sniffing) would be fragile, so I think you made the right call — it just needs announcing.

On the rebase: store.c/store.h are additive and clean, and the pipeline.c router ports nearly as-is. The real work is pipeline_incremental.c — dump_and_persist was independently reworked on main (already int-returning, repo_path removed, and the dump now goes through a staged delete plus atomic publication). The stamp needs re-siting onto the staging dump before publication. Your premise still holds: the dump rebuilds the DB file and wipes the pragma.

Your tests should port unchanged. Ping me when it is rebased and I will pick it straight back up — I would like this on main.

Checking back in, and I want to be clear that this is still wanted rather than quietly dying.

The gap is still live on main. I re-checked today: there is still no format-version mechanism anywhere in src/, so a pre-#1108 index refreshed incrementally still mints new-format QNs only for the files that changed and leaves the rest on old-format QNs — duplicate File nodes and stale edges in the same graph. Nothing on main prevents that, and this PR is the only thing that does. That is the reason I keep coming back to it rather than closing it.

One correction I owe you first. In my last review I flagged sqlite3_prepare_v2(..., "PRAGMA user_version;", CBM_NOT_FOUND, ...) as a copy-paste slip. That was wrong, and my apologies — CBM_NOT_FOUND is -1, and it is already used as the nByte argument in 56 places in store.c. You were following the local convention exactly. If anything the convention is the flaw, and it is not yours to fix in this PR. (SQLITE_AUTO_LEN is the better-named constant that also exists in that file, but that is a cleanup for another day.)

The other three points from that review still stand: bind the incremental-path stamp with a test that actually fails if cbm_store_set_format_version is deleted from pipeline_incremental.c, treat a failed post-dump reopen as a stamp failure, and make the one-time migration visible rather than log-only.

On the rebase — you have three options and I am happy with any of them.

1. You rebase it. The work is concentrated in pipeline_incremental.c: dump_and_persist was independently reworked on main and the dump now goes through a staged delete plus atomic publication, so the stamp needs re-siting onto the staging dump before publication. Your store.c/store.h additions and the pipeline.c router should port nearly as-is, and your tests should port unchanged.
2. We rebase your branch for you. "Allow edits by maintainers" is enabled on this PR, so I can do the rebase on your branch and merge it as your commit, with your authorship intact. This costs you nothing and you keep the credit properly rather than as a trailer. Say the word and I will do it.
3. We distill it — reimplement here with Co-authored-by credit to you and close this with a pointer to the commit.

My preference is 1 or 2, in that order, because the design is yours and the reasoning behind it is yours. If I do not hear back I will fall back to option 2 rather than let a live data-corruption gap sit unfixed — but I would much rather you told me which you want.

For the record, the two design calls I most wanted to credit still hold up: PRAGMA user_version is the right carrier because it survives VACUUM INTO, so the artifact-export path falls out coherently for free; and stamping both dump paths is the trap that would have turned this from a boundary into a perpetual-rebuild footgun. Getting that second one right is the whole difference.

I’ll take the rebase should have it up in a few days

Rebased onto current main

The main structural change: dump_and_persist now funnels every route through cbm_pipeline_publish_staged, so the stamp lives in that one shared site. Full, incremental, and delta all inherit it. The "both paths" trap from before became a single choke point, which is cleaner than stamping two dump sites.

On your three points:

Stamp binding. The stamp is now in publish_staged, not pipeline_incremental.c. index_format_legacy_index_rebuilds_and_repairs asserts user_version == 1 after the rebuild. I verified the binding by deleting the stamp and watching it fail (fmt == 0, expected 1), then restored it.

Reopen failure as stamp failure. publish_staged returns PERSIST_FAILED if the staging store will not open, and the stamp joins the existing ok chain, so a failed stamp discards the generation rather than reporting success.

Migration visibility. Surfaced as "format_migration": true on the index_repository response, via cbm_pipeline_had_format_migration. A test binds it: the field appears on a stale-format rebuild and is absent on an unchanged run, so dropping the emit fails a test.

Upgrade note for the release: the first index after upgrading rebuilds each project's graph once, since a pre-#1108 index holds collided File identities. Subsequent unchanged runs stay incremental.

Tests: index_format and incremental green, lint clean. One pre-existing mcp failure (test_mcp.c:10420, an IDXPAR supervisor hang) reproduces on a clean 010569fa build with none of my changes, so it is unrelated to this PR.

Merged in feb7208. Thank you, @LA-10 — this closes a real data-corruption window, and it closed it the careful way.

Two things in here deserve explicit credit:

The PRAGMA user_version choice. It survives VACUUM INTO, which means the artifact path gets format coherence for free rather than needing its own parallel stamp. That is the kind of property you either notice at design time or spend a release discovering the hard way.

Collapsing to a single stamp site. My 07-31 review worried about a second, unbound stamp location drifting out of step. You made that concern structurally impossible instead of merely fixing it — every published generation now inherits one stamp at the publication choke point, so full, incremental and delta cannot disagree. That is a better answer than the one I asked for.

I verified the important parts rather than taking them on trust:

• The gap was live: on main at 095acc5c there was no format marker anywhere in src/, so a pre-fix(fqn): preserve full filename in File-node QNs so sibling files don't collide #1108 index refreshed incrementally really did mint mixed QNs.
• The check lands before the incremental route — a stale-format DB is diverted to a full reindex before anything can misread it, so this is detect-and-rebuild, never silent misinterpretation.
• Both cbm_pipeline_run callers go through it, so there is no refresh route that bypasses the boundary.
• index_format_legacy_index_rebuilds_and_repairs fabricates a genuinely legacy graph — extension-stripped __file__ QN, format_version = 0, ADR present — then drives the real index_repository flow, and a second run asserts no repeat rebuild. A false-positive guard matters as much as the positive case here; thank you for writing it.

Carried forward, not forgotten: every existing project rebuilds once on first index after upgrade. That was the accepted trade-off when we chose a boundary over an in-place migration, and your format_migration response field makes it visible rather than mysterious — but it needs to be stated plainly in the release notes so nobody is surprised by a long first index. I have recorded that against the next release and will use your suggested wording from 08-25.

Sorry this took as long as it did to get over the line after your 08-25 rebase — the delay was mine, not yours.