Skip to content

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

Open
LA-10 wants to merge 1 commit into
DeusData:mainfrom
LA-10:fix/769-file-qn-keeps-extension
Open

feat(store): index-format rebuild boundary for the File-QN change#1104
LA-10 wants to merge 1 commit into
DeusData:mainfrom
LA-10:fix/769-file-qn-keeps-extension

Conversation

@LA-10

@LA-10 LA-10 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

What this does

#1108 made File-node QNs keep the extension, but shipped without a compatibility boundary. So on main, an index built before #1108 keeps its old collided QNs, and the next incremental run only updates changed files, mixing old and new identities (duplicate nodes, stale edges).

This adds a persisted format marker (PRAGMA user_version). A stale-format index goes through the existing full-reindex path once (keeping ADR/metadata, per #516) and is stamped so the next unchanged run stays incremental. No in-place migration an old collided DB can't reconstruct the missing siblings safely.

Notes

Tests (tests/test_index_format.c, in the gating test suite)

  • distinct File node per sibling + full search_code coverage
  • a fabricated legacy graph rebuilds once, removes the stale QN, produces the exact new QNs, preserves ADR, and stays incremental on the next run

@LA-10
LA-10 requested a review from DeusData as a code owner July 15, 2026 20:50
@LA-10
LA-10 force-pushed the fix/769-file-qn-keeps-extension branch from 4cb3014 to b79f397 Compare July 15, 2026 20:54
@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges ux/behavior Display bugs, docs, adoption UX priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jul 15, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 15, 2026
@DeusData

Copy link
Copy Markdown
Owner

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.

@LA-10 LA-10 changed the title # fix(fqn): keep the extension in __file__ QNs so sibling files stay distinct Refs #769 fix(fqn): keep the extension in __file__ QNs so sibling files stay distinct Refs 769 Jul 15, 2026
@DeusData

Copy link
Copy Markdown
Owner

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.

@LA-10
LA-10 force-pushed the fix/769-file-qn-keeps-extension branch 2 times, most recently from 9408889 to 074edcc Compare July 16, 2026 14:12
@LA-10 LA-10 changed the title fix(fqn): keep the extension in __file__ QNs so sibling files stay distinct Refs 769 feat(store): index-format rebuild boundary for the File-QN change Jul 16, 2026
@LA-10

LA-10 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

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.

@LA-10
LA-10 marked this pull request as draft July 16, 2026 15:45
@DeusData

Copy link
Copy Markdown
Owner

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.

@LA-10
LA-10 marked this pull request as ready for review July 19, 2026 14:40
@LA-10
LA-10 force-pushed the fix/769-file-qn-keeps-extension branch 2 times, most recently from 55c7e2f to 09f7936 Compare July 19, 2026 15:54
…ary Refs DeusData#769

Signed-off-by: LA-10 <leenshuhail1@gmail.com>
@LA-10
LA-10 force-pushed the fix/769-file-qn-keeps-extension branch from 09f7936 to 9925970 Compare July 19, 2026 20:18
@DeusData

Copy link
Copy Markdown
Owner

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.cdump_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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants