mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-29 06:32:28 +00:00
58 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Gergő Magyar
|
62f86ea068
|
feat(mcp): reduce response size and query overhead (#3621)
* docs(mcp): document focused response filters * feat(mcp): add opt-in response section and relation filters * test(mcp): verify response filters and compatibility * test(mcp): add deterministic response-size benchmark * docs(skills): teach focused MCP calls and safe filter use * docs(skills): update debugging response-filter guidance * docs(skills): update exploration response-filter guidance * docs(skills): explain focused MCP response options * docs(plugin): mirror focused debugging guidance * docs(plugin): mirror focused exploration guidance * docs(plugin): mirror focused MCP tool guide * docs(cursor): mirror focused debugging guidance * docs(cursor): mirror focused exploration guidance * test(mcp): give filter cases unique identities Use named cases to keep array inputs intact and let the CI execution audit distinguish every malformed-filter test. * perf(mcp): batch BM25 symbol hydration * test(mcp): serialize native hydration coverage * test(eval): publish child PID atomically * test(mcp): cover hydration on native platforms --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Abhigyan Patwari <126312502+abhigyanpatwari@users.noreply.github.com> |
||
|
Gergő Magyar
|
17b056fc79
|
test(lbug): guard max DB size accounting across repeated checkpoints (#3529) (#3624)
On LadybugDB 0.18.3 (1.6.12, 1.6.13-rc.57) every CHECKPOINT left its shadow file handle's frame groups charged against max_db_size until the database closed (LadybugDB #924, fixed by #925 before 0.21.1): about 8-9 MiB per checkpoint, empty ones included. Analyze checkpoints every 5 s and per embedding window, so a long run can report "Maximum database size ... has been reached" while the file is small, and a retry in a fresh process succeeds. The new native test opens the database through initLbug with a 256 MiB ceiling and issues 128 driver-path checkpoints (tryFlushWAL). It fails on 0.18.3 with the reporter's message and passes on 0.21.1, so a future engine pin that reintroduces the leak is caught. Registered in the lbug-db project and the cross-platform native list. Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Gergő Magyar
|
22a7608b7c
|
feat(cache): derive parser and PDG cache identities automatically (#3606) | ||
|
Nguyễn Đăng Minh Lực
|
5c3961756b
|
fix(ingestion): preserve Go method ownership across files (#3559)
* fix(ingestion): re-point Go struct member edges when struct is declared in a sibling file (#3558) findEnclosingClassInfo mints a Go method's owner id from the file containing the method (Struct:<methodFile>:<Type>), while the Struct node is keyed by the file declaring the struct (parse-worker uses declFile). Same package, different file => the HAS_METHOD edge's sourceId points at a node that does not exist and the edge is silently dropped at COPY into LadybugDB. Add reconcileGoStructOwnerEdges, run at the end of the parse phase after ALL worker chunks are merged so every package's structs are visible. It rewrites only edges that would otherwise dangle, and only when exactly one Struct node shares the dangling id's package directory and simple name; 0 or >=2 candidates keep the old drop behavior (a wrong owner is worse than a dropped edge). Index build is lazy: dangling Struct sources are rare (struct fields never dangle — they live in the struct's own file), so the common non-Go or fully-same-file case pays two per-type edge scans and no whole-node pass. Regression test: test/integration/go-cross-file-struct-owner.test.ts (fails-before: 1 HAS_METHOD edge; passes-after: 3). * fix(ingestion): scope Go struct-owner reconcile by language and package Address PR #3559 review findings: - Scope the pass to Go: dangling HAS_METHOD/HAS_PROPERTY owner ids whose Struct:<file>:<name> file is not Go source (Rust, Swift, ...) are never re-pointed, and only Struct nodes stamped language 'go' (the marker the worker sets on every declared symbol) are candidates. Negative test pins a Rust-shaped dangling edge staying untouched beside a same-name Go struct. - Key candidate structs by (dir, declared package clause, name) instead of (dir, name): one directory can carry 'package pkg' and external-test 'package pkg_test' files declaring the same type name, which the old key saw as ambiguous and dropped fail-closed. Clauses are read via the shared inferGoPackageName derivation with a call-scoped cache. Integration test covers the 3-file layout (pkg decl, pkg_test decl, pkg_test methods file) and asserts the edge lands on the pkg_test.Widget node. - Keep the direct call in parse-impl with a TODO: the pass needs a graph-level post-merge hook, which no LanguageProvider/ScopeResolver hook provides today. * refactor(ingestion): remove Go graph repair from shared parse phase * fix(ingestion): resolve Go ownership during ingestion (language-provider.ts) * Add resolveDetachedOwnerName function to Go config * Refactor method owner population logic in Go * Refactor receiver binding to handle unnamed receivers * Enhance route handler method resolution logic Refactor route handler to improve ownership resolution and method indexing. * Add emitPostResolutionEdges to scope resolver * fix(ingestion): resolve Go ownership during ingestion (ownership.ts) * fix(ingestion): resolve Go ownership during ingestion (ast-helpers.ts) * Add resolveDetachedOwnerName to provider methods * Update SCHEMA_BUMP to 141 Increment schema version to 141 for new features and fixes. * fix(ingestion): resolve Go ownership during ingestion (decl.go) * fix(ingestion): resolve Go ownership during ingestion (external_decl_test.go) * fix(ingestion): resolve Go ownership during ingestion (external_methods_test.go) * fix(ingestion): resolve Go ownership during ingestion (foreign.rs) * fix(ingestion): resolve Go ownership during ingestion (go.mod) * fix(ingestion): resolve Go ownership during ingestion (local.go) * fix(ingestion): resolve Go ownership during ingestion (methods.go) * fix(ingestion): resolve Go ownership during ingestion (go-cross-file-struct-owner.test.ts) * Refactor comments and method registration in tests Updated comments for clarity and modified method registration to include qualified names and ownership. * fix(ingestion): resolve Go ownership during ingestion (has-method.test.ts) * Update SCHEMA_BUMP to 141 in tests * refactor(ingestion): remove obsolete Go struct owner reconciliation * fix(go): model build constraints for method ownership * test(go): cover build constraint grammar and platform variants * fix(go): resolve method owners across compatible build variants * test(go): verify variant ownership through ingestion pipeline This test verifies that build-variant owners are consistent across different workers, the semantic registry, and the graph. It sets up various scenarios to ensure that ownership is correctly resolved and that there are no dangling edges in the ownership graph. * fix(go): reject impossible known constraints when peer is unknown Address PR #3559 review feedback. Regression coverage follows in this update. * feat(ingestion): allow providers to group incremental resolution files Add the optional language-neutral hook used by the Go ownership writeback fix for PR #3559. * fix(go): group sibling files for incremental ownership resolution Use the existing Go package-directory key for the provider hook. * fix(incremental): collect sibling resolution groups for changed paths Keep group keys local to each provider and include current siblings of deleted files. * fix(analyze): persist ownership changes in unchanged Go siblings Expand changed and deleted resolution groups before importer traversal. Use the same effective file set for deletion and writeback so package-wide ownership changes reach the database. * test(incremental): cover provider-local resolution groups Cover changed, added, deleted and unrelated paths for the new sibling expansion helper. * test(go): compare incremental ownership writes with cold rebuilds Reproduce ownership changes after deleting and restoring a Windows declaration while the shared method file stays unchanged. Verify incremental writes and persisted ownership parity with forced rebuilds. * test(go): exclude impossible owners with unknown method constraints Use real parsing to cover impossible owner edges and valid owners hidden by impossible competitors. * test(go): pin filename rules and unknown constraint overlap Document Go first-dot filename behavior and verify impossible known constraints in both operand orders. * test(go): register incremental ownership coverage across platforms Include the new database-backed Go ownership suite in the cross-platform test list. * test(go): schedule incremental ownership suite with native database tests Complete PR #3559 review fixes. Local validation: 47 focused tests across five suites passed; build and source/test type checks passed. The full suite was interrupted without a result. * refactor(ingestion): centralize worker owner hook forwarding Pass the language provider into the private cached owner lookup instead of repeating four owner hooks at five call sites. Keep qualified-name selection explicit and preserve the exported helper signature. Validation: CLI/shared build and test typecheck pass; repository lint has no errors; 591 tests pass with one skipped across 13 ownership and worker suites. Web typecheck is blocked by uninstalled web dependencies. Graph checks are unavailable because this checkout has no registered index. * test(rename): add Go cross-file fixture module Part of the cross-file method ownership regression for PR #3559. The completed fixture uses real indexing, public rename preview/apply, warm cache replay and post-rename reindexing. All 14 Go rename integration cases pass locally. * test(rename): add cross-file fixture before/types.go * test(rename): add cross-file fixture before/methods.go * test(rename): add cross-file fixture before/callers.go * test(rename): add cross-file fixture after/go.mod * test(rename): add cross-file fixture after/types.go * test(rename): add cross-file fixture after/methods.go * test(rename): add cross-file fixture after/callers.go * Add test case for renaming pointer receiver method Complete the real persisted-index regression fixture for PR #3559. The type, pointer receiver method and callers are in separate Go files. Public rename preview/apply must make exactly four edits across the structural interface family while preserving an unrelated same-name method, comments and strings. The fixture checks six exact ownership, interface and call edges before rename, after warm parse-cache replay and after reindexing the edited sources. All 14 Go rename integration cases pass. Formatting and the CLI typecheck pass. Graph impact/detect-changes could not run because this checkout has no registered index. --------- Co-authored-by: System administrator <root@nixos-infra.home> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> |
||
|
Gergő Magyar
|
a665031c96
|
fix(lbug): diagnose staged-copy embedding failures, round max DB size (#3570)
* fix(lbug): round GITNEXUS_LBUG_MAX_DB_SIZE up to a power of two The native buffer manager rejects any max DB size that is not a power of two, but the env parser accepted any positive integer, so a value such as 300000000000 failed at open and was then reported with a misleading "set a larger limit" hint. Round a positive safe integer up to the next power of two and warn once naming the raw and effective values. Values that are not safe integers now fall back to the 16 GiB default. Refs #3529 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * feat(lbug): add remedies for the address-space ceiling and past-EOF read Add two pure classifiers beside bufferPoolExhaustionRemedy. They parse only what the engine message states, label the byte count as the limit the engine reported (not the configured value), and name GITNEXUS_ATOMIC_INCREMENTAL=0 only for runs that knob changes. Neither asserts why the failure occurred. Refs #3529, #3530 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * feat(lbug): add size-evidence formatting for failure diagnostics formatFileEvidence is a pure formatter over stat-shaped input (apparent size, allocated blocks, raw statfs f_type), so sparse and regular cases are testable without a real sparse file. describeFileEvidence wraps it, never throws, and reports a missing file as unavailable. Refs #3530 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(analyze): honor GITNEXUS_ATOMIC_INCREMENTAL=0 and classify staged failures GITNEXUS_ATOMIC_INCREMENTAL was only read as === '1', so the documented =0 mitigation did nothing for watch and auto-sync, which pass atomicIncremental as an option. An explicit =0 now beats the caller's request. Full rebuilds and the escalation to staging still stage and are unaffected. In the analyze catch block, snapshot the live and staging files before the checkpoint driver stops and the database closes (closing can checkpoint the staging file), classify the error once, then log the remedy and the held evidence for a size-ceiling or past-EOF failure. The off-switch is named only when the failed run was an atomic-incremental staged write. A max-size message the classifiers do not recognise keeps the generic hint, and the original error is rethrown unchanged. The atomic-incremental staging log line now records the staged file size. Refs #3529, #3530 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * docs: document max DB size rounding and GITNEXUS_ATOMIC_INCREMENTAL Note that GITNEXUS_LBUG_MAX_DB_SIZE must be a power of two and is rounded up, add GITNEXUS_ATOMIC_INCREMENTAL (1 and 0, and which builds 0 does not affect) to both README env tables, and add a Troubleshooting entry for the two intermittent embedding write-back failures and the evidence lines GitNexus now logs for them. Refs #3529, #3530 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(review): apply review findings Review fixes: - Label the GITNEXUS_LBUG_BUFFER_POOL_SIZE=0 sentinel in the failure evidence line instead of printing "buffer pool 0 B". - Replace FailureRemedyContext {staged, offSwitchApplies} with a single StagedBuildMode ('in-place' | 'staged-rebuild' | 'staged-incremental'), removing the { staged: false, offSwitchApplies: true } illegal state. - Re-attach the LBUG_MAX_DB_SIZE doc comment to the exported constant. - Cover the in-place classified failure through runFullAnalysis, and the buffer-pool sentinel label, with new tests. Also carries the simplification pass: failure evidence is collected only for a classified failure, the staged-build flag is reused, the env var is read once, and the evidence tests share one temp-dir helper. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(lbug): bound failure-evidence stats and record checkpoint activity Review findings #1 and #3 on #3570. describeFileEvidence raced nothing: a stat on a stalled 9p/NFS/virtiofs mount hung the analyze failure handler before the driver stopped, the database closed and the original error was rethrown. Each stat/statfs now has a 2000 ms deadline and fails open; a missing file reads "absent" instead of "unavailable", a stalled one reads "unavailable (timed out ...)". collectFailureEvidence (new) is the single evidence collector: it reads the checkpoint activity snapshot and dispatches every stat in one Promise.all before its first await, covering the live index, the staging copy and its .wal/.shadow/.wal.checkpoint sidecars. That is what lets a #3530 report tell a short main file from pages pending in the sidecars or from a CHECKPOINT GitNexus itself ran just before the failing DELETE. runCheckpointWithRetry now records completed count, last completion time and in-flight count. Both the periodic driver tick and checkpointOnce (the staged window-start CHECKPOINT) go through it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(lbug): state the off-switch cost, pin the rounded max size, isolate ambient env Review findings #5 (note), #6 (wiring) and #11 on #3570. offSwitchNote now says that GITNEXUS_ATOMIC_INCREMENTAL=0 can leave the live index partly written after a failed in-place write, and that the next run then rebuilds it in full. A new test stubs GITNEXUS_LBUG_MAX_DB_SIZE=300000000000, reloads lbug-config and asserts the native Database constructor receives 2 ** 39 and that exactly one rounding warning is logged. The existing tests all called the parsers directly and matched the constructor argument with expect.any(Number), so reverting the wiring left the suite green. vitest.config.ts sets GITNEXUS_ATOMIC_INCREMENTAL to the empty string (neither '0' nor '1', i.e. unset) like GITNEXUS_MEMORY, so an exported =0 no longer switches off every staging test. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(analyze): carry the failure diagnosis in the error message and label escalated runs Review findings #2, #4, #5, #7, #8 and the run-analyze half of #6 on #3570. #4: the remedy and `Failure evidence` lines were only sent to onLog, which `analyze --watch` wires only with GITNEXUS_VERBOSE=1 and the auto-sync launcher drops - the only two callers that request atomic-incremental staging. They are now appended to the rethrown error's message in place (identity and prototype preserved, native text first so classifiers anchored on it still match), which every caller prints. A thrown value that is not an Error falls back to a log. Auto-sync truncated that message to 240 characters before logging it; the log now allows 2000 while the persisted summary stays short. #8: remedy delivery is best effort, so a throwing onLog sink or an unwritable message can no longer replace the native error. #2: escalatedFullWrite is declared above the try, and an escalated atomic-incremental run is classified as a rebuild. Its remedy no longer points at GITNEXUS_ATOMIC_INCREMENTAL=0, which does not stop the escalation from staging. #5: when GITNEXUS_ATOMIC_INCREMENTAL=0 overrides a caller that requested staging, the run logs one line saying so and what a failed write costs. #7: the stage-time size line uses the same "staging copy" label as the failure evidence. #6: tests pin the evidence-before-driver-stop-before-close order, the exact configured max DB size in the evidence, and the appended text against the substring classifiers in the CLI and auto-sync. The READMEs describe the new evidence lines, ask reporters to include the stage-time line, and name GITNEXUS_WAL_MANUAL_CHECKPOINT=0 as the bisect knob for the checkpoint hypothesis. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Address PR review feedback (#3570) - wal-checkpoint-driver: count a checkpoint as completed only when tryFlushWAL actually ran one. It returns false when no connection is open, and the failure evidence renders the count as "N completed". - auto-sync runner: an oversized native error now logs its head and tail (1000 characters each) so the appended failure diagnosis, which is always the tail, survives. The persisted 240-character summary is unchanged. - lbug-config: the max-size remedy said GITNEXUS_LBUG_MAX_DB_SIZE "must be a power of 2", but non-powers are rounded up. It now says so, as do the matching lines in gitnexus/README.md. - failure-evidence and file-evidence tests use the default registered temp-dir prefix, so the stale-sidecar sweep recognises their fixtures. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Address PR review feedback (#3570) - run-analyze: stringify the thrown value through a helper that cannot throw, so an exotic throw (null-prototype object, throwing toString) cannot abort the failure handler before it stops the driver, closes the database and reclaims the staging copy. - run-analyze: a classified non-Error throw is wrapped in an Error (original kept as `cause`) so the diagnosis travels in the message like every other case, instead of in a log that the auto-sync launcher drops. This removes the separate log fallback. - auto-sync runner: the oversized-error log keeps head and tail within the 2000-character cap; the separator no longer pushes it to 2003. - lbug-config: the max-size rounding warning prints the operator's value as supplied (300000000000.9), not the integer it floored to. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Address PR review feedback (#3570) - lbug-config: the max-size remedy's rounding example was itself a power of two. It now shows a value being rounded: 300000000000 becomes 549755813888. - auto-sync-runner test: the comment said the whole log line is capped at 2000 characters; the assertion covers the error payload after the prefix. - extension-loader test: pin that the installer child is handed the rounded max DB size for a non-power-of-two override. The installer prefers that argument over the env var and has its own non-rounding parser, so this is what keeps a valid override working there. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Address PR review feedback (#3570) - Keep the whole failure diagnosis in auto-sync's capped log line: the tail now reserves the remedy and evidence lines (with a 240-char floor for the native head) instead of an even head/tail split. - Count only this run's checkpoints in the failure evidence: snapshot the process-global activity when the run begins and report the difference, so an earlier `analyze --watch` refresh no longer leaks in. - Diagnose classified failures thrown while opening the staging copy, via helpers shared with the write-path catch. - Halt checkpoint ticks before measuring failure evidence: start `walCheckpointDriver.stop()` (which clears its timer synchronously) before the stats, then await the drain before close. Note: pre-existing timeouts in run-analyze-fts-repair ("fails repair mode when no base meta exists", "abandons a pending set that fails EMBEDDING_RESUME_MAX_ATTEMPTS times in a row") reproduce on the unchanged PR head on this host and are not addressed by this commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> |
||
|
Gergő Magyar
|
cae60c2bd4
|
fix(mcp): reject corrupt query identities and refresh index metadata (#3571)
Some checks are pending
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Scorecard / Scorecard analysis (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
* fix(mcp): reject corrupt query identities and refresh index metadata * fix(mcp): preserve reader and search integrity Keep replaced readers unknown until matching metadata is published. Defer shared-handle refresh until other consumers release the old graph to avoid rebuilding connections against the same stale generation. Propagate identity corruption after both hybrid search lanes settle, so the sibling cannot query after its database session is released. Select the regressions for the OS matrix and retain Python coverage without optional Swift grammars. Validation: 393 focused tests, nine native generation tests and ten race executions across five fresh processes pass. Build and source/test typechecks pass. The early full run passed 26,096 tests and failed eight: seven reproduce on baseline; the mixed-source publication case passes in clean reruns. Windows/macOS native execution remains for CI. Graph analysis maps all edited production symbols; coverage remains partial for the anonymous FTS test callback, manually reviewed. Refs #3571 * fix(lbug): bump @ladybugdb/core to 0.21.2 to stop doubled rows after delete and re-copy LadybugDB 0.21.0 and 0.21.1 return every row of a table that spans more than one native vector twice once the incremental writeback has DETACH DELETEd it and a COPY into an earlier table has run. NodeGroup::scan let a morsel that ended before the group's first live row wrap an unsigned subtraction and rescan to the end of the group (LadybugDB #1085, fixed in 0.21.2). No primary key error fires. The identity guard from #3442 stops the run with "unexpected or duplicate ID", so a Swift edit that pulls every importer into the write set failed on repos with more than 2048 structs. Only unreleased main was affected: 1.6.12 ships 0.18.3. Pin 0.21.2 in package.json, update the six @ladybugdb/core entries in the lockfile, and move coreVersion in the vendored FTS manifest. The FTS extension stays at 0.21.0 because no v0.21.1 or v0.21.2 artifact is published; the pairing assertion and its unit tests pass. Add a regression to the delete-nodes-for-files suite: 2049 structs with CONTAINS edges are loaded, every file is deleted, the graph is loaded again, and each id and edge must appear exactly once. Verification (Linux, Node 22.18): - lbug-delete-nodes-for-files: 11/11 on 0.21.2. With the 0.21.1 native packages swapped in, the new test fails with 4098 rows instead of 2049. - Reporter recipe end to end on the built CLI: incremental analyze exits 0 with no duplicate ids on a Swift-heavy fixture and on a 70-file Python fixture, with and without FTS. On 0.21.1 the Swift fixture exits 1. - A 0.21.1-written database opens and reads under 0.21.2. - prettier, eslint and typecheck:tests are clean on the changed files. - The full lbug-db project had not finished when this was committed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(fts): say what the Windows warm-session branch covers On Windows the repair step calls poolClose, which removes the pool entry before the in-place writer opens. The step therefore checks the backend's cached capability state, not a live pool connection. The comment now says so instead of implying the pool stays warm. No behavior change. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * revert: take the @ladybugdb/core 0.21.2 bump out of this PR Reverts |
||
|
Gergő Magyar
|
8fa8e64388
|
fix(rename): extend safe refactoring and flow coverage across languages (#3557) | ||
|
Gergő Magyar
|
dd096e690c
|
chore(deps): consolidate open dependabot updates (#3496) | ||
|
Gergő Magyar
|
df49f90889
|
fix(ci): require every test to execute across the CI matrix (#3479) | ||
|
Gergő Magyar
|
e8a09d067b
|
fix(search): reject incomplete FTS repairs and verify worktree lookup (#3475)
Some checks are pending
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Scorecard / Scorecard analysis (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
|
||
|
Gergő Magyar
|
526b6f249b
|
test(search): guard native lookup and Unicode FTS after COPY (#3477) | ||
|
Gergő Magyar
|
1a5d88391c
|
fix(mcp): reject corrupt impact and context identities (#3466) | ||
|
Gergő Magyar
|
412446408d
|
fix(index): guard graph integrity and fail closed on incomplete risk (#3442) | ||
|
Gergő Magyar
|
ad5c7364e1
|
feat(storage): share one index store across linked worktrees and sibling clones (#3374)
* feat(storage): resolve the shared sibling-store identity and layout (#3352) Linked worktrees of one repository resolve to one store under GITNEXUS_HOME/stores/<key>, keyed by the canonical git common dir. The resolver reads the .git entry directly, so hot paths spawn no git. Slot naming moves to a leaf module so storage-resolver and shared-store do not import each other. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(storage): resolve a shared checkout's graph and existing store slot (#3352) getStoragePaths reads a flat slot's recorded graphPath only for checkout slots under the stores directory; other paths keep <storagePath>/lbug with no I/O. A recorded path outside the store's commit graphs is ignored. resolveStoragePath falls back to an existing store slot for an unregistered checkout, so reads never move to an empty slot. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(analyze): share one immutable commit graph across clean worktrees (#3352) A linked worktree writes its own slot in the shared store. A new slot is seeded with a pointer to the nearest commit graph, so a clean checkout at an indexed commit takes the up-to-date path and writes no graph. A checkout with local changes gets a copy-on-write private graph before its first write. After a successful run, a clean checkout at HEAD publishes its graph into commits/ under a store lock, or drops it when that commit graph already exists. Commit graphs are never written after publish. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(analyze): seed a worktree's graph from the nearest index (#3352) A new shared slot is seeded from the store's commit graph nearest to HEAD, else from this checkout's or the main checkout's repository-local index (copied under that index's lock, source left in place). The run that follows is up to date or incremental instead of a full build. If a pointed-at shared graph has been removed, analyze falls back to a full build instead of an incremental update over a missing baseline. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(analyze): keep one parse cache per shared store (#3352) Linked worktrees read and write the parse cache and durable ParsedFile store under the store's caches/ directory. Before pruning, a run folds in the chunk keys recorded by every member slot and commit graph, and the fold, prune and save run under a store-wide cache lock so one member never evicts another's live chunks. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(clean): remove only what no shared-store member references (#3352) Deleting a shared checkout slot (clean, clean --all, remove, and the server delete route) recounts references under the store's publish lock and deletes commit graphs no member points at, then the store itself once empty. A graph that cannot be deleted (open on Windows) is reported and kept for the next pass. clean --gc also drops member slots whose worktree is gone or no longer resolves to the store. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(analyze): let a clone opt in to a shared store with --share-with (#3352) An independent clone joins a linked worktree's shared store only with analyze --share-with <repo>, and only when its normalized origin URL matches that member's (credentials stripped, as #2054 compares). The registry remembers the choice. --no-share moves an opted-in clone back to its own .gitnexus and reclaims its old slot; linked worktrees always share and are pointed at GITNEXUS_SHARED_STORE=off instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(storage): count a shared checkout's commit graph as its code index (#3352) A clean shared checkout reads a commit graph and owns no graph file, so the code-index presence check made status report it unindexed and registry validation skip it. The check now follows the slot's validated graphPath. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(storage): adopt existing worktree indexes and report shared-store state (#3352) A shared checkout gets <repo>/.gitnexus/store.json pointing at its store slot; resolution follows it only when it names that checkout's own slot. An existing local index seeds the slot and is left in place. status (text and --json) and doctor report the store, whether the graph is shared or private, and any leftover local index, which clean --local-index removes while keeping the pointer. With GITNEXUS_SHARED_STORE=off a previously shared checkout indexes into its own .gitnexus again and never writes a commit graph. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(mcp): read the graph a shared checkout points at (#3352) MCP, the HTTP API, group sync, augmentation, and the Claude hook (all three byte-identical copies) resolve a flat slot's graph through resolveGraphPath instead of joining 'lbug' onto the storage path, so checkouts at one commit share one open database. The embeddings writers (embeddings sync and the server embed job) take a private copy first and never write an immutable commit graph. The post-analyze settle probe accepts fresh metadata that points at an existing commit graph. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * docs: describe the shared worktree index store (#3352) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * refactor(storage): reuse helpers across the shared-store code (#3352) One graph-clone helper replaces two copy-then-rename blocks; clean and status reuse formatSlotSize; leaving a store reuses removeSharedStorePointer; withStoreLock is imported from its own module instead of a re-export. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(storage): address review findings in the shared store (#3352) - Never publish a graph whose build saw dirty files, and never trust a local-index seed on the up-to-date path; it may hold reverted edits. - Record slot pointers and reclaim under the publish lock, and reclaim right after each publish, so a commit graph is never deleted between publish and pointer save and superseded graphs don't pile up. - clean --gc decides membership from the registry, so opted-in clones and a main checkout without worktrees are not dropped. - Leaving a store re-registers first, so an up-to-date run cannot leave the registry pointing at a deleted slot. - Drop pinned branch summaries when an entry moves into a store slot. - Keep run.cjs and the AGENTS.md runner path inside the checkout. - MCP handles follow the slot's current graph, and branch scoping reads the slot's own metadata. - Cache resolveGraphPath by metadata file identity; look up opted-in entries with canonical registry paths. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(cli): localize help for the shared-store flags (#3352) --help renders option text from the i18n catalog, so --share-with, --no-share, clean --gc and clean --local-index need keys in both locales, not just index.ts literals. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(storage): lock the embed-job graph copy and skip it on forced rebuilds (#3352) The server embed job copies a shared checkout's graph under the slot's index lock, so a CLI analyze in another process cannot interleave. A forced rebuild with no embeddings to carry over drops the pointer without copying a graph it would discard unread. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * docs(agents): bump AGENTS.md version for the shared store notes (#3352) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(storage): keep shared-store file reads inside checked paths (#3374) Address CodeQL js/path-injection and js/file-system-race on shared-store.ts: every filesystem read rebuilds its path under a fixed parent with an inline path.relative barrier, the .git probe is one read (EISDIR marks a directory) instead of stat-then-read, and the graph pointer cache stats and reads through one file descriptor. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(storage): address review feedback on the shared store (#3374) - Hold the slot index lock for the whole server embedding job, not just the graph copy. - --no-share re-registers and deletes the old slot under that slot's index lock, so a running shared analyze cannot re-register it. - clean --gc previews without --force, like every other destructive arm. - status reports a pinned branch index as private. - Slot names prefix Windows device names that carry an extension. - A slot or store named ..<name> is a legal direct child. - Shared-store suites run in the serialized lbug-db vitest project and clear an inherited GITNEXUS_SHARED_STORE. - Doc and test accuracy fixes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * test(storage): pin shared-store behavior for worktrees of a bare repository (#3374) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(storage): address second review round on the shared store (#3374) - Reject --no-share in a linked worktree before taking any lock or indexing anything. - clean --all and remove also delete each shared checkout's pointer. - Delete an empty store only while also holding its cache lock, and re-check emptiness under it. - A store pointer is trusted only when the slot's own metadata names this checkout; the editable pointer file just says where to look. - Device names with an extension are prefixed on Windows only, so POSIX slot names stay stable. - Test fixtures use the gitnexus-test- prefix the stale-sidecar sweep recognizes; help text and the private-graph label are accurate. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(storage): address third review round on the shared store (#3374) - clean --gc never collects a slot whose index lock is held: an analyze holds it until it registers the checkout, so a seeded but not yet registered slot is busy, not orphaned. - Reclaim compares absolute paths, so a relative GITNEXUS_HOME does not make live commit graphs look unreferenced. - graphPath is followed only into a published <commit>-<featureKey> dir, never .publish-* staging (TS and all three hook copies). - clean --gc fails on an unreadable stores root instead of reporting nothing to collect. - --no-share help states it is for opted-in clones. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(cli): land the English --no-share help and private-graph label (#3374) These two strings were meant for |
||
|
Nguyễn Đăng Minh Lực
|
5be80b1fc3
|
fix(lbug): self-heal read-only opens refused by an interrupted checkpoint (#3340)
Some checks are pending
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Scorecard / Scorecard analysis (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
* fix(lbug): self-heal read-only opens refused by an interrupted checkpoint Homelab repro 2026-09-19 (image 1.6.10-20260917, @ladybugdb/core 0.19.x): a wiki pod killed mid-CHECKPOINT left the engine's checkpoint artifacts on disk (lbug.wal.checkpoint / lbug.shadow / checkpoint intent+apply locks), and every later READ-ONLY open refused with 'Cannot open database in read-only mode while checkpoint is in progress' — permanently, until a writable open (any gitnexus analyze) happened to run. Two defects, both fixed: 1. The refusal was unclassified. ensureReadOnlyConnectionUsable (direct adapter) and openReadOnlyDatabase (pool) recovered missing-shadow and shadow-replay errors but rethrew this one raw, so 'LadybugDB unavailable for __wiki__' repeated forever. Add isReadOnlyCheckpointInProgressError (LADYBUGDB-CONTRACT, live-verified against 0.19.1) and route it through the same writable-open recovery — including at OPEN time, where the refusal fires before any probe can run (pool: init() moved inside the try; direct: doInitLbug catch). 2. The existing shadow-replay recovery was not durable. Reproduction matrix on 0.19.1: the writable probe replays the WAL in MEMORY only — without an explicit CHECKPOINT the engine drops the pages at close and the follow-up read-only open silently serves the pre-checkpoint state. Both recovery paths now CHECKPOINT after the probe, which applies the replay, consumes the sidecars, and clears the checkpoint locks. Verified end-to-end: real engine 0.19.1, killed-mid-CHECKPOINT state → exact refusal → pool adapter self-heals → 80,800 rows intact, sidecars consumed. The planted-signature integration test runs on any engine version (refusal asserted only where the engine emits it, 0.19+). * docs(architecture): list checkpoint-in-flight artifacts and the read-path self-heal * fix(lbug): address review findings — shared cursor closer, tighten structural guard Both findings from the gitnexus-check review on this PR: 1. pool-adapter.ts: the recovery CHECKPOINT closed its cursor with a bare unawaited result.close?.(). Use the shared best-effort closer (closeQueryResults) the repo already funnels both adapters through, so a cursor-close failure stays cleanup and cannot escape as an unhandled rejection. 2. sidecar-recovery.test.ts: the structural regex omitted the leading negation, so as a substring match it also accepted the inverted predicate (recover ONLY shadow-replay, exclude checkpoint) — the exact regression the guard exists to prevent. Pin the full '!isReadOnlyShadowReplayError(err) && !isReadOnlyCheckpointInProgressError(err)' throw-through shape. * test(lbug): wire the recovery plant into lbug-db/LBUG_NATIVE; force the refusal on any engine pin Address the tri-review findings (all four): P1 — the planted-signature integration test was collected by the parallel 'default' project and never by the serialized 'lbug-db' project, and Windows/macOS CI never ran it: register it next to its sibling in vitest.config.ts (lbug-db include + default exclude) and in cross-platform-tests.ts LBUG_NATIVE, per TESTING.md's rule for native @ladybugdb/core suites. Verified via 'vitest list --project lbug-db'. P2 — on the committed 0.18.3 pin the plant passes as 'pool opens and count(n)=300' without ever exercising the new classifier or recovery CHECKPOINT. Add forced-refusal behavioral tests for BOTH adapters: a mocked native layer whose first read-only Database refuses with the canonical 0.19 message, asserting the exact self-heal shape (ro-refused -> writable open -> CHECKPOINT -> ro retry) and that a healthy db never triggers a writable open. The direct adapter is lazy, so its refusal is scripted at the first probe query rather than init(). P2 — the LADYBUGDB-CONTRACT header on isReadOnlyCheckpointInProgressError claimed '^0.18.0' like its siblings; only 0.19.x emits this string (0.18.3 tolerates the state). State the first-observed version so a bump reviewer validates the right binary. P2 — the pool's writable replay recovery quarantined on missing-shadow even when the error came from the post-probe CHECKPOINT, where the main file has already changed: mirror the direct adapter's probeSucceeded guard (replaySucceeded) and fail closed instead of parking a live sidecar. Also assert walBuffer.byteLength > 0 in the plant so the fixture cannot silently degrade into an empty shell on tolerant engines. * test(lbug): fix hosted-CI failures — Windows handle release, version-gated plant, prettier Address the CHANGES_REQUESTED review of the hosted run: Windows blocker (Win32 Error 33, locked file region at the pooled reopen): the fixture now makes handle release explicit — waitForFixtureRelease probe-reads the db and its residual WAL with bounded retries after every native close (plant, raw refusal probe, pool close), mirroring the adapter's own Windows handle-release probing. The WAL is re-planted from the captured bytes instead of renamed: a close-time auto-checkpoint can consume the live .wal out from under the rename (ENOENT, second flake). Version-gate the plant itself: on < 0.19 engines that tolerate the planted signature, its synthetic sidecars are not a consistent staging state for the old engine (double-apply replays surfaced as 'Person already exists in catalog' — the third flake), and no refusal can be forced there anyway. The plant now skips below 0.19 with that rationale in-file; the behavioral contract on every pin stays with the forced-refusal units, and this native suite remains registered in lbug-db + LBUG_NATIVE so Win/macOS exercise it as soon as the pin moves off 0.18.3. Also: prettier on the two flagged files (format gate). * fix(lbug): keep failed checkpoint heals from re-entering CHECKPOINT Wrap recovery failures without repeating native refusal text, skip already-wrapped errors in doInitLbug, and pin constructor-time heal plus the Windows reopen skip. Co-authored-by: Cursor <cursoragent@cursor.com> * Address PR review feedback (#3340) - Clean each forced-heal tmpDir in afterEach so earlier cases do not leak - Compare major.minor when version-gating the interrupted-checkpoint plant Co-authored-by: Cursor <cursoragent@cursor.com> * Address PR review feedback (#3340) Reset the pool forced-refusal mock Database sequence in native.reset() so later tests can still script the first construction as the checkpoint victim. * Address PR review feedback (#3340) Count every MATCH on the pool forced-refusal mock and assert the writable replay probe so CHECKPOINT-without-probe cannot stay green. --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> |
||
|
Gergő Magyar
|
a2e1710ac0
|
fix(embeddings): stop Caching embeddings OOM on large incremental analyze (#3310)
* fix(embeddings): spill cached vectors to a Float32 temp file Keep restore metadata in RAM and write embeddings once the in-memory row limit is exceeded so incremental analyze can survive large caches without a full-table number[] heap (#3306). Co-authored-by: Cursor <cursoragent@cursor.com> * fix(lbug): stream CodeEmbedding cache under the connection lock Spill vectors once the in-memory limit is crossed and fail the load instead of adopting an empty snapshot, so incremental analyze cannot OOM or quietly drop the restore cache (#3306). Co-authored-by: Cursor <cursoragent@cursor.com> * fix(analyze): restore cached embeddings from a streamed spill snapshot Hold row metadata across wipe, materialize 200-row batches, and treat cache-load failures as warn-and-continue so incremental analyze can preserve vectors without a full-table heap (#3306). Co-authored-by: Cursor <cursoragent@cursor.com> * Address PR review feedback (#3310) Loop spill writes until the full vector lands, keep materialize failures out of the insert catch and the Phase 4 hash skip-set, and assert spilled restore subsets by node id instead of scan order. Co-authored-by: Cursor <cursoragent@cursor.com> * Address PR review feedback (#3310) Discard only this analyze run's embedding spills so a concurrent analyze on another index keeps its restore file, and isolate the default in-memory limit test from inherited env. Co-authored-by: Cursor <cursoragent@cursor.com> * Address PR review feedback (#3310) Mark a node stale when any restore batch fails so leftover chunks are deleted and rembedded, and exercise a full-length bad-magic spill header. --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> |
||
|
mengkaka
|
79543c8f83
|
feat(storage): add configurable index storage and content retention tiers (#3060)
* feat(storage): add configurable index storage and content retention tiers Rebase #3060 onto current origin/main. Keep GITNEXUS_STORAGE_PATH, GITNEXUS_STORAGE_ROOT, and GITNEXUS_CONTENT_RETENTION, and fold in main's FTS skip, embed-session, and help-text updates. Co-authored-by: Cursor <cursoragent@cursor.com> * Address PR review feedback (#3060) Keep legacy registry rows on the local storage fallback, resolve symlinks before the destructive-path guard, and align hook lookup with CLI branch slugs, branch-slot metadata, and longest-path match. Co-authored-by: Cursor <cursoragent@cursor.com> * Address PR review feedback (#3060) Only list swept upload directories after a successful removal so callers cannot treat a permission or transient rm failure as gone. Co-authored-by: Cursor <cursoragent@cursor.com> * Address PR review feedback (#3060) Document that getStoragePath may consult registered storage while this module still does not mutate the global registry. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(storage): close review findings for external indexes and retention Re-inspect ownership under the analyze lock, fail-closed when the registry file is missing, and keep skip-git hook discovery plus retention fields on HTTP/MCP list surfaces. /api/file stays 410 unless contentRetention is full. Co-authored-by: Cursor <cursoragent@cursor.com> * chore(autofix): apply prettier + eslint fixes via /autofix command * Address PR review feedback (#3060) Treat lock-only index dirs as empty, honor HTTP --force storage policy, and prefer registered plus branch-aware slots in hooks and augment. Co-authored-by: Cursor <cursoragent@cursor.com> * Address PR review feedback (#3060) Keep hook fallbacks inside the current worktree, compare foreign-local slots canonically, and make storage fixtures survive ownership validation. Co-authored-by: Cursor <cursoragent@cursor.com> * Fix macOS hook test expecting realpath'd registry paths. resolveHookRepo returns the written registry path, not a filesystem realpath, so the assertion must match that. * Address gitnexus-check warnings on hook install docs and slot tests. The Cursor troubleshooting list omitted registry-query.cjs, and the writable-slot test only checked that isDirectory exists instead of that the path is a directory. * Align the HTTP catalog source-scan with skippable resolveRepo validation. resolveRepo lists fresh repos with validate: options.validateStorage !== false so DELETE can skip prune; the test still required a literal validate: true. * Harden storage path sinks so CodeQL path-injection and ReDoS alerts clear. Contain every filesystem probe inside the resolved storage slot with the inline path.relative idiom, reject filesystem-root slots, and trim slot basenames in linear time. * Settle bridge stamps before writing so CI size/mtime matches stay stable. LadybugDB can still flush into bridge.lbug after close+rename; persist whole-millisecond mtimes and wait for consecutive stats to agree so a freshly written pair matches. * Type the settled bridge stat as fs.Stats so tsc does not see bigint. Awaited<ReturnType<typeof fsp.stat>> collapsed the bigint overload and broke prepare/typecheck on CI. * Keep the bridge mtime stamp exact so same-size swaps still fail the pair check. Co-authored-by: Cursor <cursoragent@cursor.com> * Wrap the bridge stamp predicate so prettier --check stays green. Co-authored-by: Cursor <cursoragent@cursor.com> * Require a quiet interval before stamping a settled bridge file. Co-authored-by: Cursor <cursoragent@cursor.com> * Reuse shared storage and settle helpers instead of local copies. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
azizur100389
|
f8036ac349
|
feat(analyze): add explicit FTS opt-out (#3205)
* feat(analyze): add explicit FTS opt-out * chore(autofix): apply prettier + eslint fixes via /autofix command * fix(analyze): treat flag and env FTS opt-out as one mode Avoid a same-commit rebuild when only the skipReason discriminator changes, and advertise disablement on dirty meta before leftover indexes are wiped. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> |
||
|
Navid EMAD
|
506432017f
|
fix(zig): model callable-value references, and stop reporting their absence as exact (#3219)
Some checks are pending
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
Scorecard / Scorecard analysis (push) Waiting to run
* fix(impact): stop reporting 'exact' over unmodelled callable-value references
A function named in VALUE position — `bridge.accessor(Element.getNamespaceUri,
null, .{})`, `{ onClick: handler }`, a comparator handed to a sort — is
registered somewhere rather than called. The registration is modelled (a
`value-ref` site becomes a USES edge; Kythe `ref` vs `ref/call`, Joern
METHOD_REF), but the invocation THROUGH the stored value is not: it happens
later via a struct field, a registry lookup or comptime reflection.
impact()/context() nonetheless reported such a target as `epistemic: 'exact'`.
For lightpanda-io/browser that meant the DOM `Element.namespaceURI` accessor
came back with two internal callers, LOW risk and a claim of completeness —
worse than no answer, because 'exact' tells the reader not to look further.
tools.ts defines 'lower-bound' as "the walk provably missed callers", which is
precisely this case.
computeEpistemicBoundary now probes for inbound USES edges stamped with the
value-ref reason and hedges when it finds any, contributing a boundary note and
a new `causes.callableValueReferences` (unit: distinct referrer symbols).
Read from the graph, not from index metadata: unlike a dropped receiver — which
leaves no edge to find and therefore needs a persisted summary — a value
reference IS in the graph. So the signal needs no re-index and no analyzer
change, and it works on indexes written before this commit.
The cause gets its own slot rather than joining `dispatchBoundary`: a value
referrer is neither an implementation nor an interface-level consumer, and the
two differ in what the reader should do about them — a dispatch boundary is
irreducible, a callable value usually becomes traceable once the provider
models the store/load that carries it.
Language-neutral: every provider that emits a `value-ref` capture participates.
The writer and the reader now share VALUE_REF_EDGE_REASON, because drift
between them would fail silently — the probe would match nothing and every
answer would go back to claiming certainty.
* fix(zig): emit `value-ref` for a callable named in value position
`mapReferenceKindToEdgeType` has handled the registration-vs-invocation case
since #2437, and TypeScript, JavaScript and C++ all emit `value-ref`. Zig
emitted zero — so a Zig function handed somewhere as a VALUE was absent from
the graph entirely.
That is not a corner: Zig's JS bridge is built out of this one shape,
pub const namespaceURI = bridge.accessor(Element.getNamespaceUri, null, .{});
and `bridge.{accessor,function,indexed,…}` appears 2,047 times across 257 files
in lightpanda-io/browser — the project's whole JS<->Zig surface, none of it
reaching the graph. `impact` on `Element.getNamespaceUri`, which IS the DOM
`Element.namespaceURI` accessor, answered with its two in-file callers.
Three query rules, tagging `@reference.value-ref` on: a bare identifier in
argument position (`bridge.accessor(_tagName, …)`), a qualified one
(`bridge.accessor(Element.getNamespaceUri, …)`), and a const binding initialiser
(`pub const defaultHandler = onReset;`). Everything downstream already existed —
no new edge type, no schema change, no capture-machinery change, no baseline
edited.
One grammar detail is load-bearing: in tree-sitter-zig, call arguments are
DIRECT children of `call_expression` — there is no `arguments` node, only
builtins have one — so the callee has to be consumed explicitly by `function:`.
Without that binding the same rule also matches the callee of `foo(bar)` and
mints a USES edge shadowing the call's own CALLS edge. There is a test for it.
The rules are deliberately broad — `js.Bridge(Element)` and `register(count)`
match too — because the callable gate in the property-dispatch pass
(Function/Method/Constructor only) is the filter, the same design that keeps
TypeScript's `{ port: DEFAULT_PORT }` from registering anything. Measured on the
real corpus: all 3,169 emitted edges land on a Method (3,146) or Function (23).
Deliberately NOT modelled: the terminal invoke. The value reaches
`Accessor.init` -> a struct field -> `Factory.zig` reflection (`inline for`,
`@typeInfo`) -> `Caller.zig`'s `@call(.auto, func, args)` over `func: anytype`.
Resolving that needs comptime evaluation. The point is to stop dropping the
reference; the shortfall is now reported as `epistemic: "lower-bound"` by the
preceding commit instead of being papered over.
Measured on lightpanda-io/browser (698 .zig files), wiped index both times:
30,222 nodes / 71,070 edges -> 30,222 nodes / 74,229 edges. The delta is
entirely `USES` (3,169 after vs 10 before, and every one is a value-ref); CALLS,
ACCESSES, IMPORTS, HAS_METHOD, DEFINES and the rest are unchanged — purely
additive, no node invented.
The fixture joins the existing `zig-idioms` corpus rather than adding a new
one, because `bench/receiver-resolution` uses `test/fixtures/lang-resolution` as
its `--check` corpus. That gate, `scope-capture` (15 languages),
`zig-cross-file-resolution` and every other bench `--check` pass unchanged.
* fix(review): resolve qualified value references through their owner, and stop
hedging on registrations the analyzer already followed
Five findings from the review bot on #3219; four valid, all addressed.
1. QUALIFIED VALUE REFERENCES RESOLVED BY TAIL NAME (the serious one).
`bridge.accessor(Element.getNamespaceUri, …)` was resolved with
`findCallableBindingInScope(site.inScope, site.name, …)`, which never sees
the receiver and gives LOCAL bindings precedence. Reproduced:
const Element = @This();
pub fn getThing(...) // main.getThing
pub const JsApi = struct {
fn getThing(...) // JsApi.getThing
pub const thing = bridge.accessor(Element.getThing, null, .{});
};
emitted `USES JsApi → JsApi.getThing` — a WRONG edge, which is worse than
the missing edge this PR set out to fix.
`resolveValueRefTarget` now resolves a site carrying an explicit receiver
through `findClassBindingInScope` → `findOwnedMember` (the machinery
`receiver-bound-calls` already uses), gated on `CALL_TARGET_TYPES` because
`findOwnedMember` also answers with fields. A bare site keeps the lexical
walk, which is what an unqualified name means. When the owner cannot be
resolved the site is DECLINED rather than falling back: declining costs a
reference, falling back mints a confident edge to the wrong function, and
the missing reference is now reported as `lower-bound` anyway.
Cost on lightpanda-io/browser: value-ref edges 3,169 → 2,799 (−12%). Those
370 were tail-name coincidences, not registrations — all 94 `Element.zig`
`JsApi` entries survive, the cross-container case resolves and is correctly
attributed (`IntersectionObserverEntry.JsApi` → `IntersectionObserverEntry.
getTarget#0`, not the enclosing observer), and both acceptance probes are
unchanged: `getNamespaceUri` 7/3/LOW/lower-bound, `getTagNameLower`
31/10/HIGH/exact.
2. A FAILED PROBE READ AS "NO BOUNDARY". `.catch(() => [])` turned an
unanswerable query into count 0 and no note, so `exact` could be published
on the strength of a question that was never asked. It now returns `null`
and emits a boundary note. This file's own `loadMeta` comment states the
rule: a probe failing must never read as certainty.
3. NOT EVERY VALUE REFERENCE IS AN UNMODELLED INVOCATION. Where
`emitPropertyDispatchCalls` sweep 2 synthesized the dispatch (reason
`property-dispatch`), the walk did NOT provably miss the caller, so hedging
was noise over an answer that was computed — and a signal that fires on
every JS/TS hook table stops carrying information. A second probe excludes
those targets. Zig never sets a property key, so the motivating case is
untouched. The exclusion is symbol-level, not per-edge, because the graph
does not record which registration produced which synthesized call; that
residual is documented at the call site.
4. `LIMIT 50` SILENTLY UNDERSTATED THE COUNT. `rows.length` over a capped row
set published a ceiling as the documented symbol count. Replaced with
`COUNT(DISTINCT other.id)` — bounded work without a bounded answer, the
shape `countByType` in the same file already uses.
5. THE TEST DID NOT PIN THE CALLER COUNT its own comment promised. Added
`expect(result.impactedCount).toBe(2)`.
New regression tests: qualified references bind the written owner and not the
nearer lexical match; an unresolvable receiver emits nothing rather than a
wrong edge; a dispatch-modelled registration stays `exact`; a probe that cannot
run hedges instead of claiming certainty.
Known limitation, pinned by a test rather than left implicit: when a `@This()`
alias's NAME differs from its container's (`const Element = @This();` inside
`main.zig`), the receiver does not resolve and the reference is declined. The
provider has `rewriteZigThisAlias` for this, but it is applied to type nodes
and extending it to reference receivers would change existing CALL-site
behaviour. Lightpanda's `const Foo = @This();` in `Foo.zig` convention makes
the names coincide, which is why the corpus is unaffected.
* fix(review): bump the parse-cache schema and resolve module-owned value references
Review round 2 on #3219. Four inline items, all reproduced against the
worktree before deciding.
P1 — the new captures could stay INERT on a warm parse cache. Adding
`@reference.value-ref` rules to `ZIG_SCOPE_QUERY` changes
`ParsedFile.referenceSites`, which is a PARSE-TIME fact, but `SCHEMA_BUMP`
stayed at 93. A repo indexed before this branch and re-analyzed after it
replays the old, empty site list for every unchanged `.zig` file — `--force`
included, since shards are content-addressed — so no USES edge is emitted, the
boundary probe measures a real zero, and `impact` on a registered accessor goes
back to `epistemic: "exact"`. #3399 un-fixed on the incremental path most users
are on, with every cold-run test still green. DECISIONS D1-1's "zero changes to
… the schema" conflated the graph schema with the cache schema.
Bumped 93 -> 98, not 94: #3190 claims 94 and #3179 claims 94 through 97 in one
PR. `incremental-parse-cache.test.ts` re-pinned, with 93-97 added to the taken
list. Re-check against origin/main and open PRs immediately before merging.
P2 — a qualified value reference through a MODULE was declined with no hedge.
R1-2 resolves a written receiver with `findClassBindingInScope`, which requires
`isClassLike`; a namespace-only `@import` handle is not class-like, so
const dom_utils = @import("dom_utils.zig"); // no `@This()` in that file
pub const comparator = bridge.accessor(dom_utils.compare, null, .{});
resolved to nothing. That is not the conservative half of R1-2's trade-off: a
declined site emits NO edge, so there is nothing for the probe to read and
`impact` on `compare` reports `exact`. Silence, not a hedge — and the pass
comment claiming otherwise was wrong on this path.
`resolveValueRefTarget` now tries the second kind of owner a qualified name can
have. `findNamespaceValueRefTarget` reads the file's `namespace` import edges
for the handle and the target module's own `origin: 'local'` module-scope
bindings for the member — the same channel `receiver-bound-calls` Case 1
already trusts for `dom_utils.compare()`, with Case 1's three guards for Case
1's reasons: `isNamespaceNameShadowed`, local-origin bindings only, and two
distinct defs under one name resolve nothing. `CALL_TARGET_TYPES` gates module
owners exactly as it gates container owners. Language-neutral: it reads generic
namespace import edges, names no language.
Still declined, deliberately: a receiver this index knows under no name at all
(the `@This()`-alias case, owners outside the workspace). There the alternative
is a confident edge to a lexically-nearer function the source did not name.
P2 — `impact-callable-value-references.test.ts` was in neither vitest list.
It opens a real engine via `withTestLbugDB(poolAdapter: true)`, so TESTING.md
puts it in the `lbug-db` include list and the `default` exclude list; it was in
neither, so `default` also collected it into the parallel pool. Added next to
its `impact-epistemic-lower-bound` sibling in both arrays.
P3 — DECISIONS.md was stale against HEAD and embedded host paths. D2-3 still
described the `LIMIT 50` that R1-5 removed and D1-3 still described the
receiver-blind resolution that R1-2 replaced; both now carry explicit
"superseded by" pointers. The `~/code/...` and mise-node paths are replaced
with placeholders.
Two smaller corrections the new cause made necessary:
- `formatImpactResult`'s `lower-bound` header hard-coded "callers binding via
DI / dynamic dispatch", which now contradicts the value-reference bullet
printed directly under it. The bullets carry the cause; the header only
states that the count is a floor.
- `tools.ts` said a `causes.callableValueReferences` of 0 means "nothing was
missed". The dispatch exclusion is symbol-level, not edge-level, so a target
with both a followed registration and an unfollowed escape also reads 0. The
docs now say a 0 means "no unfollowed registration was proven".
Fixture: `src/webapi/dom_utils.zig` (namespace-only) plus three cases in
`Element.zig` — the module-qualified registration, a non-callable module member,
and a `u8` parameter shadowing the handle. The shadow case was verified to FAIL
with the guard disabled, so it is not passing for an unrelated reason.
Gates: `tsc --noEmit` clean, `npm run build` clean, prettier clean.
`resolvers/` 3,630 passed / 3 skipped; `unit/scope-resolution` 2,010 passed;
`impact-callable-value-references` 7 passed under `lbug-db`;
`incremental-parse-cache` 40 passed; eval formatters 104 passed. Bench --check
all PASS with no baseline edited: receiver-resolution, zig-cross-file-resolution,
scope-emission, callable-value-flow, scope-capture (15 languages), python-scope.
* fix(review): guard the class receiver against a shadowing binding, and pin the dispatchability partition
Review round 3 (`gitnexus-check` bot on `cf53bbaa`). Three findings, each
reproduced against the code before deciding.
R3-1 (Error, valid, REPRODUCED) — a CLASS receiver could resolve through a
shadowing value binding. `findClassBindingInScope` is a class-only walk: it
filters the scope chain by `isClassLike`, so it steps over a nearer binding that
is a value and keeps climbing — and past the chain entirely, into a
qualified-name fallback that answers with the unique workspace definition of the
name. A `u8` parameter named `Ticker`, in a file that neither declares nor
imports the `Ticker` container another file defines, therefore emitted
`register(Ticker.fire)` as a confident USES edge to that container's method.
That is exactly the wrong-edge failure R1-2 exists to prevent, arriving through
the class channel instead of the lexical one.
Fixed with `isOwnerNameShadowedBySomethingElse` — a sibling of
`isNamespaceNameShadowed` with one extra clause. The plain namespace guard could
NOT be reused: a container is often its own local declaration
(`fn make() { const Local = struct {…}; register(Local.go); }`), and reading that
binding as its own shadow suppresses precisely the resolutions this path exists
to make — the #2723 mistake, one channel over. So a scope that binds the name
answers immediately, and the answer is "not shadowed" only when one of that
scope's own bindings IS the def just resolved.
Both halves are pinned and both were verified to fail when the guard is
weakened: the parameter case fails with no guard at all, the local-container
case fails with the plain `isNamespaceNameShadowed`.
R3-2 (Warning; mechanism correct, unreachable today; fragility fixed instead) —
the dispatch exclusion could suppress an unfollowed registration. The bot is
right about the code: sweep 2 synthesizes CALLS only for a registration whose
site carried a `propertyKey`, while the exclusion zeroes the note on ANY inbound
`property-dispatch` CALLS edge. It is not reachable in the current rule set, and
the reason is measured rather than assumed: `@reference.value-ref` is emitted by
exactly three languages — JavaScript (2 rules), TypeScript (2), Zig (3) — every
JS/TS rule also captures `@reference.property-key` (both are object-literal
shapes) and no Zig rule does. A dispatchable registration is therefore always a
JS/TS one, an undispatchable one always a Zig one, and they cannot meet on one
symbol.
Rejected: splitting the edge `reason` into dispatchable / undispatchable. It is
the precise fix, but it is a graph-content change that churns whichever side
keeps the old literal — the Zig, TypeScript and probe suites all pin
'scope-resolution: value-ref' by hand as a drift canary — and it buys nothing
against a case no rule can produce.
What was actually wrong is that the exclusion's soundness rested on a
coincidence recorded nowhere, in files nobody reading `local-backend.ts` would
open. Fixed at both ends: the exclusion site now states the invariant, the three
facts it rests on and the two options for when it breaks; and
`value-ref-dispatchability.test.ts` fails the day it does — a JS/TS rule for a
bare callback argument, a Zig rule that grows a key, or a fourth language
emitting `value-ref` at all. Verified to fire (adding a property key to a Zig
value-ref rule fails the Zig case), and its rule splitter has its own guard test
so the suite cannot pass vacuously. `ZIG_SCOPE_QUERY` is exported for that test
only.
R3-3 (Nit, valid) — a test comment claimed the wrong epistemic result. The
`declines a qualified reference whose receiver cannot be resolved` case said the
shortfall shows up as `lower-bound`. It does not: with no edge there is no
evidence and the target stays `exact`. The pass docstring was corrected in round
2 and this comment was missed. It now says the decline costs the reference AND
the hedge, and why that is still the right trade.
Gates: tsc --noEmit clean, npm run build clean, prettier clean.
`test/integration/resolvers` 3,632 passed / 3 skipped (70 files);
`test/unit/scope-resolution` 2,015 passed (120 files);
`impact-callable-value-references` 7 passed under `lbug-db`. Bench --check:
receiver-resolution, zig-cross-file-resolution, scope-capture (15 languages),
scope-emission PASS with no baseline edited; callable-value-flow failed once on
its TIMING budget (2.006 > 1.9) with a byte-identical fingerprint, then passed
twice at 1.788 / 1.813 — machine load, not a regression.
* fix(review): let a file's own `@import` outrank the workspace-wide class fallback
Found by running the local preflight harness before pushing rather than after.
One of its five findings is a real hole in R2-2; the rest are documentation that
overclaimed.
R4-1 (valid, REPRODUCED) — the container channel preempted the file's own
`@import`. R2-2 added the module channel as a FALLBACK after
`findClassBindingInScope`, and that order is wrong: `findClassBindingInScope`
does not stop at the scope chain. When its `isClassLike` walk misses — and a
namespace handle binds a Module, so it always misses — it falls back to
`scopes.qualifiedNames`, a workspace-wide index, and answers with the unique def
of that name anywhere in the repo. A container named `dom_utils` in a file
`Element.zig` never imports therefore captured
`bridge.accessor(dom_utils.compare, …)`, binding
`Method:src/webapi/decoy.zig:dom_utils.compare#2` while the module channel that
would have answered correctly was never reached. R3-1's shadow guard cannot
catch it: the import binds at MODULE scope, which that guard treats as the floor.
Fixed by trying the module channel FIRST. An `@import` written in this file is
the strongest available statement about what the name means here and outranks a
global uniqueness guess; when the handle is not an import of this file the
channel answers nothing and the container path runs exactly as before. Pinned by
`decoy.zig` and a strengthened assertion on the existing module-owner test,
which fails on the old order.
R4-2 — the cause documentation named shapes nothing captures. `tools.ts`
illustrated `callableValueReferences` with "a callback argument", "a stored
function pointer" and `qsort(xs, n, sz, compareItems)`. Only Zig captures a call
argument or a const initialiser; JS/TS capture only object-literal property
values, and C has no value-ref rule, so the `qsort` example is counted in no
language. Both cause blocks now name the captured shapes and say that a bare
JS/TS callback argument is not among them, so a 0 does not rule it out.
R4-3 — the same block exempted itself from the re-index caveat this PR proves it
needs. "Read from the graph, so it needs no index-time metadata" is true of the
probe and false of the edges: an index built before a language emitted these
captures has none and reports 0 — the warm-cache failure R2-1 bumped
SCHEMA_BUMP for. It now says to re-analyze before reading a 0 as measured.
R4-4 — the dispatchability canary was narrower than its own promise. Its header
claimed it fails on "a fourth language emitting value-ref at all"; it reads
`languages/<dir>/query.ts`, so Vue — which owns no query and borrows
`emitTsScopeCaptures` / `emitJsScopeCaptures` — emits value-refs while the
assertion lists three languages, and a capture synthesized in code is invisible
to it. The case now asserts on query OWNERS, which is what it checks and is
sound because a delegating language inherits the rules it borrows; the header
states the synthesized-capture gap instead of letting a green tick imply it away.
R4-5 — the BARE docstring described a lexical walk that is not one.
`findCallableBindingInScope` applies the callable predicate WHILE walking, so a
nearer parameter or local is stepped over: the defect R3-1 fixed on the container
channel, unguarded here, pre-existing since #2437 and reachable in JS/TS. Out of
scope for #3399, so behaviour is unchanged and the sentence now says what the
walk does rather than implying a guarantee it does not give.
Gates: tsc --noEmit clean, npm run build clean, prettier clean.
`test/integration/resolvers` 3,632 passed / 3 skipped (70 files);
`test/unit/scope-resolution` 2,015 passed (120 files);
`impact-callable-value-references` 7 passed under `lbug-db`. Bench --check:
receiver-resolution, zig-cross-file-resolution, scope-capture (15 languages),
scope-emission, callable-value-flow all PASS with no baseline edited.
* fix(review): honour the hub opt-in for value references, so `hub.fn` means one thing
Review round 5 (`gitnexus-check` bot on `1c7c05ff`). One finding, valid and
reproduced before fixing.
`findNamespaceValueRefTarget` accepted only `ref.origin === 'local'`. R2-2
recorded that as deliberate — "the `namespaceExportsIncludeImportedNames` hub
opt-in is a provider decision this language-neutral pass does not make" — and
that reasoning was wrong twice over. Zig sets the flag
(`languages/zig/scope-resolver.ts:41`, measured on ghostty and tigerbeetle before
it landed), and the pass does have the provider in scope: `runScopeResolution`
takes one and already forwards several of its hooks to other passes.
The result was the exact asymmetry R2-2 argued against in its own first
paragraph. A Zig hub declares nothing — every name it publishes it imported — so
requiring a local declaration declines every member reached through one:
// hub.zig
pub const scale = @import("dom_utils.zig").scale;
// Element.zig
const hub = @import("hub.zig");
pub fn callsThroughTheHub(v: u8) u8 { return hub.scale(v); } // resolved
pub const scaled = bridge.accessor(hub.scale, null, .{}); // declined
One name meaning two different things depending on whether a `(` follows it.
Fixed by forwarding `provider.namespaceExportsIncludeImportedNames` into the pass
and consulting the published channel when it is set — the same question
`receiver-bound-calls` Case 1 asks, through the same `lookupBindingsAt` read
`findExportedDefIncludingImportedNames` performs for the CALL form. The pass
still names no language; it asks the provider, which is the sanctioned hook.
Precedence is unchanged where it mattered: a locally declared member still wins
over a republished one, two distinct defs under one name still resolve nothing,
and `CALL_TARGET_TYPES` still gates the answer — pinned by `hub.DEFAULT_NS`, a
re-exported CONSTANT, which stays unregistered. Languages that do not opt in are
unaffected: the parameter defaults to `false`, and passing `false` was verified
to fail the new hub test, so it is load-bearing. The fixture republishes a member
(`scale`) that nothing else uses, so the hub assertion cannot be satisfied by an
edge another case emitted.
Gates: tsc --noEmit clean, npm run build clean, prettier clean.
`test/integration/resolvers` 3,634 passed / 3 skipped (70 files);
`test/unit/scope-resolution` 2,015 passed (120 files);
`impact-callable-value-references` 7 passed under `lbug-db`. Bench --check:
receiver-resolution, zig-cross-file-resolution, scope-capture (15 languages),
scope-emission, callable-value-flow all PASS with no baseline edited.
Also recorded in DECISIONS.md: `/autofix` returned "No successful autofix run"
because the `PR Autofix` run on `1c7c05ff` failed at `actions/upload-artifact`
with a 403 from GitHub's artifact storage, not because of anything in this
branch. This push triggers a fresh run.
* fix(review): inspect the module scope in the owner-shadow guard, and check the TSX suffix
The `gitnexus-check` pass on `bd6e577e` carried three findings of its own, and I
answered the later `1c7c05ff` pass without noticing them. Recording that as a
process failure too: bot reviews are per-head, and a later pass does not
necessarily repeat an earlier one's findings.
R6-1 (Error, valid, REPRODUCED) — the shadow guard stopped one rung short.
`isOwnerNameShadowedBySomethingElse` returned `false` on reaching the module
scope, justified as "a container declared there IS the binding, and the caller
already resolved it". That holds when the owner came from the scope chain and
fails when it came from the workspace-wide qualified-name fallback:
// Gauge.zig — never imported by Element.zig
const Gauge = @This();
pub fn read(self: *Gauge) u8 { … }
// Element.zig
const Gauge = @import("dom_utils.zig").DEFAULT_NS; // NOT a container
pub const level = bridge.accessor(Gauge.read, null, .{}); // → Gauge.zig's read
`findClassBindingInScope` steps over the module-scope binding because it is not
class-like, answers from `scopes.qualifiedNames`, and the guard waved it through.
Worth recording: the first fixture attempt did NOT reproduce. A local
`const Gauge: u8 = 3;` also claims the workspace qualified name `Gauge`, leaving
two candidates, and the fallback refuses to guess between two — so the shape
defeated itself. Binding the name by IMPORT claims no qualified name, the
fallback stays unique, and it fires. A negative result on the first shape was not
evidence the finding was wrong.
Fixed by inspecting the module scope as the last rung instead of skipping it.
The identity exemption is what makes that safe where `isNamespaceNameShadowed`
cannot do it (#2723: a namespace import writes its own name into the module scope
and would read as its own shadow) — the binding that IS the owner exempts itself,
and only a binding to something else answers `true`. `lookupBindingsAt` is
consulted at that scope and only there, because an imported alias lives in the
finalized channel rather than in `scope.bindings`.
R6-2 — the hub re-export finding, already fixed in `5d8fe9d8`; the same defect
restated on the later head.
R6-3 (valid, fixed) — the dispatchability canary omitted the TSX suffix.
`getTsScopeQuery` analyzes a `.tsx` file with `TYPESCRIPT_SCOPE_QUERY +
TSX_JSX_QUERY_SUFFIX`, and the test read only the base, so a `value-ref` rule
added to the suffix would be emitted in TSX analysis with the canary green. The
suffix is now exported and concatenated into the check; verified load-bearing by
adding an unkeyed `jsx_expression` value-ref rule to it, which fails the
TypeScript case.
Gates: tsc --noEmit clean, npm run build clean, prettier clean.
`test/integration/resolvers` 3,635 passed / 3 skipped (70 files);
`test/unit/scope-resolution` 2,015 passed (120 files);
`impact-callable-value-references` 7 passed under `lbug-db`. Bench --check:
receiver-resolution, zig-cross-file-resolution, scope-capture (15 languages),
scope-emission, callable-value-flow all PASS with no baseline edited.
* perf(bench): gate callable-value reference resolution on linear scaling
`resolveValueRefTarget` (#3399) replaced one lexical walk with four
channels, and the last of them — a qualified receiver resolved through
`findClassBindingInScope` — falls back to `scopes.qualifiedNames`, a
WORKSPACE-WIDE index consulted once per site. Keyed that is O(1); scanned
it is O(files) per site, and a registration table that costs O(sites)
today costs O(sites x files) tomorrow. Nothing in the suite can see that:
the fixtures are single-file, and the corpus it actually matters on is
lightpanda-io/browser, where `bridge.{accessor,function,…}` appears 2,047
times across 257 files.
`bench/value-ref-resolution/measure.mjs` builds two synthetic Zig corpora
of identical shape 4x apart in file count, and times ONLY the per-site
resolution loop — extraction, `reconcileOwnership` and finalize are setup.
`linear_factor` is `(t_large/t_small)/(N_large/N_small)`: measured
1.00-1.09 across runs, with `us_per_site` flat at 1.4-1.5 between the arms.
Correctness comes first, because a timing gate alone is satisfied by a fast
wrong answer: exact site/resolved/declined counts per arm plus an
order-independent sha256 over every (site -> resolved target) pair. The
corpus exercises all four channels — CONTAINER, NAMESPACE, HUB and BARE —
and carries two DECLINE controls per module (a non-callable namespace
member, a non-callable bare argument), so a widened callable gate moves
`declined` instead of hiding inside the timing.
Verified load-bearing rather than assumed: making the qualified lookup scan
a workspace-sized collection leaves the fingerprint IDENTICAL and takes
`linear_factor` to 3.752 against a slack of 1.375 — the regression class
this exists for is exactly the one no correctness gate can see.
Zig is the corpus because it is the only language whose provider sets
`namespaceExportsIncludeImportedNames`, so it is the only one that can
exercise the hub channel at all; the pass itself names no language.
`resolveValueRefTarget` is exported for the bench. Timing
`emitPropertyDispatchCalls` instead would fold the signal into edge
emission, and re-implementing the channel order in the bench would pin the
bench's idea of the function rather than the function.
* feat(zig): index every build package in the repo, not only the root one
Zig already had workspace setup — `loadZigBuildConfig` has parsed
`build.zig.zon` `.path` deps, the root `build.zig`'s named modules and each
build module's own `addImport` table since #1432, threaded through
`ScopeResolver.loadResolutionConfig` exactly as tsconfig is for TypeScript.
What it did not have is the part `tsconfigFor` supplies: per-package scope.
`loadZigBuildConfig` reads `<repoRoot>/build.zig{,.zon}` and nothing else, so
a repo laying its packages out as `packages/<name>/build.zig` — no root build
files at all — got `null`, and EVERY bare `@import("<module>")` in it went
unresolved. Cross-file resolution silently degraded to relative imports.
Measured on a two-package probe before this change: `config = null`,
`@import("core")` from `packages/app/src/main.zig` → `null`.
`loadZigWorkspaceIndex` discovers packages the way `findTsconfigFiles`
discovers configs — one bounded breadth-first walk that skips the hardcoded
ignore set — and `zigPackageFor` is the `tsconfigFor` analogue: the nearest
enclosing package governs a file, deepest-first, with no fall-through to an
outer package. Fall-through is what makes a vendored dependency's
`@import("config")` resolve to the outer repo's `config` module, the same
failure `loadTsconfigIndex` documents for a package declaring no `baseUrl`.
`resolveZigImportInternal` is UNCHANGED — impact analysis puts it at HIGH risk
with 6 dependents, and it does not need to move: it is handed one package's
config, and which config it receives is the only difference. Its 48 existing
tests pass untouched. A nested package's paths are rebased to repo-relative at
load time (`.path = "../core"` → `packages/core`, which the root-relative
reading rejects outright as an escape); the ROOT package keeps its raw
spelling, which is what `parseZigBuildZon` promises and its tests pin, so a
single-package repo is byte-identical.
`loadImportConfigs` — which runs unconditionally for every repo, Zig or not —
keeps calling the root-only loader, so no non-Zig repo pays for the walk. That
is the split TypeScript already has between the cheap `loadTsconfigPaths` and
the repo-walking `loadTsconfigIndex`, which `loadResolutionConfig` reaches only
during a language pass.
The `zig-monorepo` fixture carries the discriminating case rather than only the
happy path: `tool` binds the alias `core` to its OWN `src/core.zig`, so a
repo-wide flattened module map — the shape a workspace index invites — would
point `measure` at `packages/core`, a confident edge into a package `tool` does
not depend on. That is worse than the unresolved import this fixes, and only
per-package scoping keeps them apart.
Tests: 7 unit cases pinning the index (including `loadZigBuildConfig` answering
`null` on the same fixture, side by side) and 3 integration cases pinning the
EDGES — cross-package CALLS from the module root AND from a non-root file, and
`tool` not crossing over. Verified load-bearing: restricting the index to the
root package fails all three.
Second consumer caught by the integration test and fixed with it:
`populateZigWorkspaceStaticGating` reads the same `resolutionConfig`, per file
because two files of that pass can belong to different packages.
Gates: build clean, `tsc --noEmit` clean, prettier clean, eslint 0 errors.
`test/integration/resolvers` 3,768 passed / 4 skipped, `test/unit/scope-resolution`
2,015 passed. Bench `--check` with NO baseline edited: `receiver-resolution`
(whose corpus is `test/fixtures/lang-resolution`, where the fixture lands),
`zig-cross-file-resolution`, `value-ref-resolution`, `scope-capture` (15
languages), `import-target`, `callable-value-flow`, `scope-emission`.
* perf(zig): dequeue the package walk by head index, not `shift()`
`findZigPackageDirs` pushes children while it drains the queue, which keeps
the array in a mode where `Array.prototype.shift()` memmoves the whole
remainder rather than taking V8's left-trimming fast path — so the walk is
quadratic in the frontier, bounded only by `ZIG_SCAN_MAX_DIRS` (20,000).
Measured at that bound rather than estimated from the move count: 53 ms at
fan-out 4 and 81 ms at fan-out 20, against 0.8 ms with a head index — 66-106x,
and paid before a single config is read.
FIFO order is unchanged, and the package ordering never depended on the walk
anyway: `loadZigWorkspaceIndex` sorts by (dir length desc, localeCompare).
Memory is unchanged too — the entries were already retained by the pushes;
`shift()` only dropped the head.
`findTsconfigFiles` and `loadNodeWorkspacePackages` carry the same walk with
the same bound and are equally affected. Deliberately NOT fixed here: they are
pre-existing, outside this PR's diff, and reach TypeScript/Node import
resolution, which nothing in this change set covers. Filed separately.
Reported by gitnexus-check on
|
||
|
glier
|
dea396a13c
|
feat(ingestion): resolve Spring messaging destinations into Destination nodes (#3132) | ||
|
azizur100389
|
f64cc8b7a8
|
feat(group): add GraphQL cross-repo contracts (#3070)
* feat(group): add GraphQL contract extraction * fix(group): tighten GraphQL contract guards * fix(group): complete GraphQL review hardening * fix(group): isolate bounded GraphQL reads * fix(group): harden GraphQL contract extraction |
||
|
azizur100389
|
4f16bd8023
|
fix(impact): report scope extraction omissions (#3071)
* fix(impact): surface scope extraction omissions * fix(impact): preserve complete index fixtures * fix(impact): preserve scope completeness evidence * test(analyze): model successful scope extraction in harnesses --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> |
||
|
Gergő Magyar
|
2be508e796
|
fix(mcp): stop scaling the detect_changes query with the diff's hunk count (#2915) (#2930)
* fix(mcp): map diff hunks to symbols without per-hunk OR conditions (#2915) `detect_changes` folded one `(n.startLine <= $hunkEndI AND n.endLine >= $hunkStartI)` pair per diff hunk into a single WHERE clause, one query per changed file. A machine-generated file (cache JSON, lockfile, golden fixture) diffs at thousands of hunks with `-U0`, and the expression tree that produces overflows LadybugDB's recursive evaluator copy on a TaskScheduler worker thread: a bare SIGBUS with no error output where secondary threads get 512 KB of stack (macOS), a swallowed 30s query timeout where they get more (Linux), which the CLI then printed as "No changes detected." with exit 0. Coalesce each file's hunks into sorted, disjoint ranges and run the overlap test in JS instead. Only ranges that overlap or abut are merged, so the union covers exactly the lines the raw hunks covered. Query text and parameters are now identical whether a file changed in 1 place or 100,000, and files are queried in batches of 100 rather than one full node scan each. Reproduced on Linux by running the engine with macOS-sized (512 KB) thread stacks: 2,500 hunks passed, 3,333 and 4,000 segfaulted — matching the reporter's macOS threshold table. After the change the same repo maps a 100,001-hunk diff in 2.1s with no crash. Also fixes a line-base mismatch the rewrite exposed: graph rows are 0-based (#2377) while git hunk lines are 1-based, so the raw comparison shifted every symbol one line up. An edit to a symbol's LAST line reported nothing changed — a one-line function whose body was edited was invisible to the pre-commit gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d85r7TrMYjWbTUT3pXccS * fix(cli): say when a detect_changes result is partial (#2915) When a graph query fails, `detect_changes` swallows the error, sets `partial: true` and leaves the counts at zero (#2283). The CLI formatter never read that flag, so a degraded run printed "No changes detected." and exited 0 — the pre-commit safety gate reporting a clean bill of health for a check that did not complete. Print the partial note in both the empty and non-empty branches. Also restore the `Symbol` placeholder for rows whose label came back as an empty string: the changed-symbol mapping now keeps `''` instead of dropping it to undefined, so the formatter needs `||`, not `??`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d85r7TrMYjWbTUT3pXccS * refactor(mcp): bound the hunk→symbol query and simplify the overlap helpers (#2915) Cleanup pass over the #2915 fix. No change to which symbols detect_changes reports, except that a node matched by two changed paths is now reported once. * Push a per-file [lo, hi] span into the query. Coalesced ranges are sorted and disjoint, so a file's whole touched span is free, and the engine can drop the symbols outside it instead of shipping every row in the file across the native boundary. Measured on a 400-file batch against a 25k-node index: 546ms/13,870 rows before, 84ms/1,555 rows after, identical kept set. Depth stays constant (two comparisons per file, not per hunk), so #2915 cannot come back — the JS test still rejects symbols landing in the gaps between hunks. The struct-list parameter was verified against @ladybugdb/core 0.18.3 and 0.19.1. * Convert hunks into the graph's 0-based space once, at the point they are grouped, with the existing `toZeroBasedLine`. Every comparison downstream is then base-neutral, and `toDisplayLine` goes back to being what its doc says it is: an MCP response-boundary converter, not a filter input. * Deduplicate matched nodes by id. `ENDS WITH` is a plain string suffix, so a diff touching both `README.md` and `pkg/README.md` counted the same node twice (169 duplicates in 13,870 rows on a real 400-file diff). Pre-existing, free to fix now that the rows are shaped in one place. * Drop the positional `?? sym[N]` row fallbacks in this block. `executeParameterized` returns `getAll()` rows, which are alias-keyed objects, so the fallbacks were dead — and they coupled the mapping to RETURN column order, which is what made adding a column a renumbering exercise. * Build the path→hunks map in one pass, so "every value is coalesced" holds at every point rather than being repaired by a second loop. Simplify `coalesceHunks` (the length<2 branch and the sort tiebreaker changed nothing) and state `hunksOverlapRange` as a standard half-open lower bound. * Document `partial` in the detect_changes tool description. The CLI now prints it, but the MCP client — the main consumer of the pre-commit gate — was getting the flag as an undocumented raw key. * Tests: pin the query text as identical for a 1-hunk and a 3,000-hunk diff (replacing a magic length bound), pin the 0-based bounds parameter, pin the dedup, and fold two near-identical row mocks into one helper. Temp dirs now come from the shared pool helper, whose cleanup is per-directory and Windows-lock aware. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d85r7TrMYjWbTUT3pXccS * feat(mcp): bound and batch the hunk→symbol query, and anchor its path match (#2915) Follow-up review pass on the #2915 fix, implementing every remaining finding. * Push a per-file `[lo, hi]` span into the query. Coalesced hunks are sorted and disjoint, so a file's touched span is free, and the engine drops the symbols outside it instead of shipping every row in the file across the native boundary. Measured on a 25k-node index, 400-file batch: 546ms/13,870 rows before, 84ms/1,555 after, identical kept set. Depth stays constant (two comparisons per file, not per hunk), so #2915 cannot return. The struct-list parameter was probed against @ladybugdb/core 0.18.3 and 0.19.1 first; the index-subscript form `$paths[i]` does not parse. * Anchor the path match: `n.filePath = b.path OR n.filePath ENDS WITH b.suffix` where suffix is the path with a leading separator. A bare `ENDS WITH` is a plain string suffix, so a diff touching `lib/a.py` also reported a symbol from an indexed `src/mylib/a.py` — a file the diff never touched. This is the form `explain` already uses. Pinned by an integration test against a real engine (it fails 3/3 with the un-anchored predicate). * Run batches a few at a time. `executeParameterized` checks a connection out of the 8-connection per-repo pool for the duration of a query, so parallel calls never share one — the same reason ~15 other queries in this file already run under `Promise.all`. `allSettled`, so one failed batch degrades the result to `partial` instead of discarding the batches that succeeded beside it. * Deduplicate matched nodes by id, and count `changed_files` as distinct paths: a path can appear twice in one diff (a rename reported alongside an edit). * Cap the listed symbols at 1,000 with `symbols_truncated: {listed, total}`. A repo-wide diff otherwise puts an unbounded array in one MCP payload — the CLI has `--limit`, an MCP client has nothing. Counts are never capped, so the risk level and the CLI's "... and N more" still see the true total. * Extract `chunk` / `mapBatches` / `LBUG_QUERY_BATCH_SIZE` into `core/lbug/query-batch.ts`. Every query built from a caller-sized array has this ceiling; the shape now has one name and the measured batch size is recorded where it is defined rather than in three constants under three names. * Move hunk grouping and the 0-based conversion into `coalesceHunksByPath`, at the parse boundary. `parseDiffHunks` stays faithful to git (1-based, like the `@@` headers it reads), consumers compare graph-native values, and the conversion is unit-testable instead of living in the backend. * Document `partial` and `symbols_truncated` in the detect_changes tool description — the MCP client is the main consumer of the pre-commit gate and was getting both as undocumented raw keys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d85r7TrMYjWbTUT3pXccS * refactor(core): batch every remaining repo-sized query list (#2915) `detect_changes` was not the only place building query text from a caller-sized array. `core/wiki/graph-queries.ts` interpolated the whole file list of a module into four `IN [...]` literals, growing the query with the repo — flat breadth rather than the nested depth that crashed #2915, but the same unbounded shape, and the one the repo's own `DELETE_FILES_CHUNK_SIZE` precedent already chunks elsewhere. All four now run one query per batch and merge in JS. The membership arms need care, and each is documented where it happens: * `getIntraModuleCallEdges` batches the caller arm only. A per-batch callee arm would drop a call from batch 0 to batch 2, both inside the module, so that predicate moves to JS against the whole set. Results are now sorted: the single-query form had no ORDER BY, and batch order would hand the entire 30-edge window `formatCallEdges` keeps to the first 100 files (#2787). * `getInterModuleCallEdges` keeps the SAME batch list in its `NOT` arm. That is sound — a file outside the module is outside every batch — and it preserves the null handling: `NOT null IN [...]` is null, so the original dropped edges to a node with no filePath, where a JS-only `!has(undefined)` would admit them. ORDER BY and LIMIT move to JS because a per-batch limit would cut rows before the cross-batch membership filter ran. * `getProcessesForFiles` keeps `LIMIT` inside the batch: `stepCount DESC, id` is a total order, so a process in the global top-N is in its own batch's top-N. Also adopt the shared `chunk()` at the hand-rolled slice loops in `lbug-adapter.ts`, `embeddings/http-client.ts` and `run-analyze.ts`. The loops whose index fed a progress callback or an error message use `chunk(...).entries()`, which removes the `i / SIZE` and `Math.floor(i / SIZE)` arithmetic rather than reproducing it. No batch size changed. One trap that survived tsc and is worth naming: after renaming a loop variable away from `chunk`, a leftover `chunk.length` silently resolved to the imported FUNCTION's arity, reporting `chunkSize: 1` for a 200-path batch. Only `lbug-query-importers-batch`'s exact-value assertion caught it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d85r7TrMYjWbTUT3pXccS * refactor: name the line-base conversions and share the symbol line (#2915) The 0-based-graph vs 1-based-elsewhere rule was open-coded in five places with the reasoning living only in comments — the same rule that, applied by hand and skipped once, hid every last-line edit from `detect_changes`. * Add `toOneBasedLine` beside `toZeroBasedLine` in `ingestion/utils/line-base.ts` so the module owns both directions, and adopt it at the four CFG/PDG join sites in `pdg-impact.ts` and the two in `local-backend.ts`. This is NOT `line-display.ts`'s `toDisplayLine`, which is documented as a response boundary converter with an `undefined` passthrough; the joins need arithmetic, and the guards that produce `Number.NaN` for an absent line are kept verbatim. * `http-route-extractor.ts` probed graph spans with a bare `line - 1` and a 20-line comment. It calls `toZeroBasedLine` now; the `?? pick(line)` fallback arm is untouched, so which node is picked cannot change (the clamp differs only for a negative line, which no emitter can produce). * Extract `formatSymbolLine`: `detect-changes-format.ts` and `eval-server.ts` rendered the same `type name → filePath` line. One behavior note — the two were not byte-identical, and eval-server had no placeholder on `name`, so a definition with an empty name rendered the literal `undefined` and now renders `?`. Both `definitions[]` shapes set name from a graph row, so this is unreachable in practice, and printing `undefined` into LLM-facing output is the bug, not the intent. `||` (not `??`) in the placeholders is deliberate and documented: a node label can come back as an empty string and still needs the placeholder. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d85r7TrMYjWbTUT3pXccS * perf(wiki): bind the module file list instead of splicing it into the query (#2915) The wiki's four `IN [...]` sites interpolated every file of a module into the query text, so the text grew with the repo — the shape that overflowed LadybugDB's recursive evaluator copy in `detect_changes`. The previous commit chunked them, which worked but cost real complexity: the callee arm had to leave Cypher and be re-implemented in JS, DISTINCT had to be re-established across batches, and ORDER BY/LIMIT had to move to JS so a per-batch window could not cut rows the cross-batch filter still needed. Binding the list as a parameter removes the reason for all of it. The text is constant at any list length, and measured against a real index a bound list is ~3x faster than the equivalent literal (5,000 items: 139ms vs 459ms; 20,000: 598ms vs 1,686ms). Every predicate goes back into Cypher, including the `NOT ... IN` arms whose null handling is load-bearing — `NOT null IN [...]` is null, so a callee with no filePath is dropped by the engine, where a JS membership test would have admitted it. Verified on this repo's own index: a 2,000-path bound list returns 14,856 rows in 877ms. Also collapses the per-process step query into one grouped `p.id IN $ids` fetch — 105ms to 13ms for 20 processes — and drops `fileListLiteral`, `callEdgeKey`, `compareProcessHeaders` and the batching loops with it. `compareStrings` was a byte-identical re-roll of `compareCodeUnits` (src/lib/utils.ts), including its #2787 rationale; it now calls the shared one. Intra-module edges are sorted where the original had no ORDER BY: `formatCallEdges` keeps only the first 30, and an unordered cut keeps a different subset per machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d85r7TrMYjWbTUT3pXccS * refactor(core): one home for batching, and a backstop for the shape that crashed (#2915) `chunk` moves to `src/lib/utils.ts`, the repo's generic-utility home: it is an array helper, and leaving it in `core/lbug/query-batch.ts` made an HTTP embedding client import batching from the graph-DB namespace. `query-batch.ts` keeps what is actually about queries — the measured `LBUG_QUERY_BATCH_SIZE`, the concurrency helper, and the ceiling — and now documents the preference the wiki change proved: bind the list as a parameter first, chunk only when you cannot. `mapBatches` becomes `mapConcurrent`: nothing about it is batch-specific, and it now has non-query callers. Its body is a per-item try/catch plus `Promise.all`, so ordering comes from the primitive rather than from unwrapping a settled union. The wave barrier stays — measured against a rolling window it is 538ms vs 532ms on a 1,000-file diff, whose per-batch times spread only 1.35x. Adopted at the loops that were still hand-rolled: `file-hash.ts`, `cluster-enricher.ts` (its progress callback now accumulates `batch.length` instead of clamping an index), `filesystem-walker.ts` and `language-config.ts` (wave scheduling with `allSettled`, which is exactly `mapConcurrent`). Deliberately not adopted, each for a stated reason: the analyzer-identity probe runs as a standalone `node -e` script with no module resolution; the embedding sub-batch loop slices two parallel arrays and breaks early; `walkRepositoryPaths` reports progress from inside each wave, which `mapConcurrent` cannot express. `warnIfQueryTextUnbounded` is the backstop: #2915 died in native code with no message, and a query built by concatenating a caller-sized list is the shape that gets there. Wired at both execution chokepoints (`pool-adapter`'s `executeParameterized`, `lbug-adapter`'s `executePrepared`/`streamQuery`; their `executeQuery` siblings delegate and are covered once). It never throws — a long query the engine can actually run must not start failing on a heuristic — and it is deliberately absent from the raw write path, where a node's `content` is inlined and a large source file would warn legitimately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d85r7TrMYjWbTUT3pXccS * refactor(mcp): name the path-match rule, and key detect_changes by node id (#2915) * `path-predicate.ts` names the three ways a caller's path can match a stored `filePath` — `exact`, `pathSuffix`, `fragment` — instead of each call site copying whichever idiom its neighbour used. A bare `ENDS WITH` is a plain string suffix, which is how a diff touching `lib/a.ts` came to report a symbol from `src/mylib/a.ts`; the loose `CONTAINS` sites are loose ON PURPOSE (a user hint of `src/mcp` should match a directory fragment), and naming the modes is what lets a call site choose rather than inherit. * `detectChanges` kept four structures over one row set — an array, a dedup Set, an id list and an id→name Map — that had to stay in sync by hand. One id-keyed Map is all of them; insertion order is preserved, so every output is byte-identical. * `symbols_truncated: {listed, total}` becomes `truncated: true`, the key `explain`/`pdg_query`/`trace` already use. The true total was always in `summary.changed_count`, so the nested object said nothing the existing vocabulary could not. * `GraphLineRange` is now a distinct type from `DiffHunk`: they carry the same two fields in different bases, and mixing them IS #2377. The name means a 1-based hunk cannot reach `hunksOverlapRange` without a conversion between. * `coalesceHunksByPath` accumulates raw ranges and coalesces once per path rather than re-sorting on every occurrence. * `chunk` adopted at this file's own five loops — the point of extracting it — including two locals named `chunk` that shadowed the import. That shadowing is not cosmetic: it is how a leftover `chunk.length` silently became the function's arity earlier in this branch. One bug caught by the real-engine integration test and worth naming: Cypher comments are `//`, not `--`. A `--` comment inside the query string made LadybugDB reject the whole query at PREPARE, which `detect_changes` swallows into `partial` and renders as "No changes detected." Every mocked unit test passed. Prose stays out of query strings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d85r7TrMYjWbTUT3pXccS * refactor(test): share the git-repo bootstrap, and move the shared formatter out (#2915) `formatSymbolLine` lived in `detect-changes-format.ts` but is rendered by `eval-server`'s query formatter too, so a `query` formatter imported from a `detect_changes` module. It moves to `src/cli/format-symbol.ts`; both callers import it from there. The `||`-not-`??` fallbacks stay documented — a node label can come back as an empty string and still needs its placeholder. `test/helpers/temp-git-repo.ts` gives `initGitRepo(dir, identity?)` and `commitAll(dir, message)` to the ~10 test files that hand-rolled the same `git init -q` + two `git config` + `add -A` + `commit` sequence. It takes a directory and never owns one, matching `temp-dir-pool.ts`'s split of lifecycle from seeding; the identity is a parameter because the existing consumers genuinely disagree about it, and each keeps exactly what it configured. Four files stay hand-rolled for stated reasons — pinned author dates for a deterministic digest, remote handling, `--allow-empty`, and the `-c key=value` form that never persists to the repo. Test trims: the `formatSymbolLine` fallback cases collapse into one `it.each` table (the case pinning that BOTH consumers emit the helper's exact line stays — no table row can express it); two `line-base` cases that were compositions of their neighbours go; and `detect-changes-path-anchoring` runs its `detect_changes` call once in `beforeAll` instead of three times, keeping the three named failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d85r7TrMYjWbTUT3pXccS * perf(mcp): filter the batched hunk query before the engine materialises (#2915) `UNWIND $bounds AS b MATCH (n) WHERE …b…` compiles to a CROSS_PRODUCT whose build side is a RESULT_COLLECTOR over the whole filtered node table: only the `n`-only predicates get pushed below the accumulate, so neither the anchored path match nor the [lo, hi] span could reduce the scan. Measured at 1M nodes: +242 MB for one batch and +922 MB for the four concurrent ones, paid even for a one-file diff — and at a 268 MB buffer pool the query died with `Buffer manager exception` where the old per-file query completed, landing in `partial:true` + `changed_count:0`, the #2915 false clean by another route. Adding the batch-wide, `b`-free disjunction as a redundant leading conjunct lets the planner push it below the accumulate: EXPLAIN now shows it as FILTER[2] directly under SCAN_NODE_TABLE[0]. It is a provable superset of the correlated predicate, so it cannot drop a row the correlated filter keeps. 10x less memory, ~20% faster, identical result sets. Also in detect_changes: - Sort rows on (filePath, startLine, id) before the 1000-symbol cut. The cut was slicing engine row order — measured 5 distinct orders across 8 runs on one connection, the #2787 class this branch fixes 200 lines away in the wiki. - Chunk `symIds`, the one caller-sized list left unbatched: 500k ids measured 4.0 GB RSS. Binding keeps the query TEXT constant, which is all the unbounded guard measures, while the bound VALUE stayed repo-sized. - Prefer exact path equality and widen to the anchored suffix only for paths that matched nothing, so a root README.md stops reporting pkg/*/README.md. - Report `risk_level:'unknown'` rather than 'low' when a query was swallowed. A degraded pre-commit gate must not read as an all-clear. - Pass --no-ext-diff --src-prefix=a/ --dst-prefix=b/. `diff.noprefix` in a user's gitconfig makes git emit `+++ f.py`, which parseDiffHunks cannot match, so every run printed "No changes detected." and exited 0 before any query ran. A diff that parses to zero files now raises `partial` instead of the clean branch. - `labels(n)`, not `labels(n)[0]`: labels() returns a scalar string here, so the subscript was always '' and `type` never carried a label. - Validate IMPACT_MAX_CHUNKS. The chunk() adoption turned an entry condition into an exit condition, so a non-numeric value ran every chunk instead of none. - Record why four-way concurrency is safe here, and scope the arm64 sequential comment to the query it was written for (#496). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv * fix(cli): fail the detect_changes gate instead of exiting 0 when it degrades (#2915) The secondary half of #2915 was that a swallowed query failure printed "No changes detected." and exited 0, so a shell pre-commit gate passed on a broken analysis. This branch added the PARTIAL text. It did not change the exit status, so `gitnexus detect-changes && git commit` still proceeded. `detectChangesCommand` passed a STRING to `output()`, and `output()` sets a failing code only for an OBJECT carrying `error` — under a comment calling itself "the one place that keeps scripted callers honest". A string never matches, so this command opted itself out of the only mechanism the file provides. It was broader than `partial`: the formatter also renders a backend `{error}` payload as text, so hard failures exited 0 too. Fixed narrowly in `detectChangesCommand`, following the object-first shape `checkCommand` already uses, rather than widening `output()`'s shared contract — every one of its other seven callers already passes an object and is unaffected. One code for both `error` and `partial`: `&&` only distinguishes zero from non-zero, and a softer code for `partial` would invite `|| [ $? -eq 2 ]` exemptions that reopen exactly this hole. `truncated` deliberately stays exit 0 — only the listing is capped, while the counts and risk are computed over the full set, so the verdict is sound and failing on it would fire on every large-but-healthy diff. Also wires `truncated` through the formatter, which this branch had left as a producer-only flag while `partial` went end to end, with the note in both locales and no count of its own so the existing "... and N more" line stays the sole numeric report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv * fix(wiki): restore step order and symbol labels, and cut the edge list in Cypher (#2915) Found by running the queries against a real engine, which nothing did before: this branch's regrouped `withSteps` returned step traces OUT OF ORDER. `ORDER BY pid, r.step` combined with `WHERE p.id IN $ids` silently drops the second sort key — `proc_1_incrementalupdate` came back 2,7,1,3,4,5,6. `ORDER BY step` alone is correct, and so was the pre-branch per-process query, so this was introduced by the batching. `formatProcesses` prints "${s.step}. ${s.name}", so every module and overview page was getting scrambled execution traces. The mocked suite passed 112/112 before and after. `labels(x)[0]` is always the empty string: labels() returns a scalar string and the subscript is 1-based over its characters ([1] is "F"). `prompts.ts` renders "${s.name} (${s.type})", so all 5,027 exported symbols reached the LLM as "name ()". `getIntraModuleCallEdges` shipped every edge to use 30 — measured 18,299 rows and 851 ms with all 2,079 paths bound, against 30 rows and 94 ms with ORDER BY + LIMIT in Cypher, which the sibling `getInterModuleCallEdges` twenty lines below already did. The determinism fix (#2787) was right; the placement was not. `compareCallEdges` goes with it — it was intransitive when a name was null or empty, so `Array.sort` was input-permutation dependent, i.e. the nondeterminism it was added to remove. Deletes the positional row ABI this branch newly documented. The vendor declaration is `getAll(): Promise<Record<string, LbugValue>[]>` — string keys only — and `row[0]` probes back `undefined`; the same PR deleted ~30 identical fallbacks from local-backend.ts. They were already stale here: `withSteps` prepends `p.id AS pid`, so `toProcessStep` was reading the pre-branch layout. Rows are now typed by alias, so renaming an `AS` is a compile error. `??` for `||` so a step of 0 or an empty label keeps its own value. Tests: a real-engine integration suite covering all seven exported queries (PREPARE included — the trap that shipped a `--` comment on this branch), and the four holes that let the ordering bug through — a vacuous order assertion, a LIMIT never reached by a 2-edge fixture, a fake that returned rows pre-ordered and ignored ORDER BY, and a hardcoded `type: 'Function'` that hid labels(). The step-ordering fixture is empirically sized: 2 processes never reproduced the bug, ~400 step edges was intermittent, 710 (20 processes x 26-45 steps) hit 11 of 11 runs. Seeded descending and interleaved so no grouping looks sorted by accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv * refactor: put the shared helpers where their callers are, and make their contracts true (#2915) `mapConcurrent` moves to lib/utils.ts beside chunk(). Nothing about it is query-specific and it already had filesystem callers, while its docstring justified concurrency safety through the per-repo connection pool — an argument that does not apply to fs.readFile. This is the precondition the branch's own commit message stated ("it now has non-query callers") and then did not apply. LBUG_QUERY_BATCH_SIZE and warnIfQueryTextUnbounded genuinely are query-specific and stay. `pathMatch`/`PathMatchMode` deleted: zero callers, and none of the three sites its docstring cited were migrated, so the tree carried the abstraction and the copies it was written to replace. `pathSuffixOf` stays and the module now documents the anchoring rule it actually implements. Contracts that were not true: - QUERY_TEXT_CEILING_BYTES was compared against `cypher.length` — UTF-16 code units, not bytes — so non-ASCII query text was undercounted and the reported KB was wrong. Buffer.byteLength now, behind a `length * 3 <= ceiling` early return so only text over ~21 KB pays for the count. - chunk(items, NaN) returned [[]], against a docstring promising never to return an empty slice, and mapConcurrent's Math.max(1, NaN) propagated it — which would have resolved [] for non-empty input with no error, read as "no results" by every call site. - GraphLineRange claimed a 1-based hunk could not reach hunksOverlapRange without a conversion, but it was structurally identical to DiffHunk so tsc accepted one with no diagnostic, and coalesceHunks<T extends GraphLineRange> actively laundered the base while its accumulator was still DiffHunk[]. The useless generic is gone and a one-line phantom on each interface makes the claim real; a bare {startLine, endLine} literal still satisfies both, so no construction site needs a cast. Pure deletions no longer vanish. A -U0 deletion emits `+N,0`, which parseDiffHunks dropped, so the file survived with no hunks, no query ran, and detect_changes reported `changed_files:1, changed_count:0, risk_level:'low'` — "No changes detected." for a commit that deleted a function. A unified diff spells an empty range as the line before it, so the anchor is line N alone: a symbol containing the deleted text also contains N, while extending to N+1 would claim a symbol that merely starts after the gap — the widening coalesceHunks guarantees it never does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv * docs: say that a partial or truncated detect_changes is not a clean gate (#2915) The gate itself now fails loudly, but the instructions every agent reads still described a zero as a result. Fixed at the source: AGENTS.md's gitnexus block is generated from a template in cli/ai-context.ts and injected into every user's repo, so the sentence goes there and AGENTS.md/CLAUDE.md are regenerated through the real code path (which also picks up a pre-existing `analyze --index-only` drift the committed docs were behind). That block is under a test-enforced size cap with 30 characters of headroom, so the 144-character clause was paid for in the same currency: the header exhortation, which the Always Do list restates as MUSTs with commands, and a verbatim repeat of the detect-changes command in the regression-compare example. 3549 of 3552. Worth noting for whoever adds the next line — #2899 replaced an absolute cap with a 0.65 ratio to let "a legitimate clause fit without ceremony", but set the ratio flush against the block's then-current size, so it is a ratchet with no ratchet. The canonical block does not make the skills redundant: three of the four install channels ship skills without touching AGENTS.md, --skip-agents-md does the same in-repo, and a user-trimmed gitnexus:keep block legitimately has no Always Do section — in those repos the skill file is the only carrier. Precedent agrees: the risk:UNKNOWN rule is deliberately carried in both places. So one sentence each in gitnexus-work (the commit gate), gitnexus-impact-analysis (beside the UNKNOWN paragraph) and gitnexus-refactoring, whose post-hoc "verify only expected files changed" is the worst of the three because a degraded result makes it vacuously pass. gitnexus-taint-analysis is left alone: its audience is always inside this repo, where the canonical block loads. All copies mirrored to npm, plugin and cursor. The cursor copies are condensed checklists rather than byte-mirrors, so they carry the equivalent note placed where it governs every detect_changes line in the file — and nothing tests that, since standard skills are fragment-checked rather than byte-compared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv * refactor: break the seven small import cycles gitnexus check reports (#2915) `check` reported 11 cycles. Five are paths inside a single 257-file strongly connected component in core/ingestion (call-extractors / cfg visitors / utils/ast-helpers), with a second 26-file component behind it — fixing those paths would only make check print different ones, so both are left for their own PR. This closes the seven that are genuinely separable, taking the graph from 9 strongly connected components to 2. Six of the seven were one value import plus one `import type` edge. tsconfig sets neither verbatimModuleSyntax nor isolatedModules, so those edges erase entirely — the cut is a graph and readability change with no emitted-JS difference. Each moved type went to a leaf module, with a re-export left behind only where an importer outside the change actually needed it: - cli/ai-context <-> cli/skill-gen: GeneratedSkillInfo -> cli/generated-skill.ts. One importer, no package export surface, so a clean move with no re-export. - cli/analyze-config <-> cli/analyze (+core/run-analyze): AnalyzeOptions -> cli/analyze-options.ts. Re-export kept because a test imports it from analyze.js. run-analyze needed no edit — cutting the one type edge collapses the 3-file component into a DAG. Its own same-named AnalyzeOptions is a different interface and was deliberately not merged. - ingestion/import-resolvers/types <-> ingestion/language-config: type-only in BOTH directions, so it had no runtime existence at all. ImportConfigs has no importers outside the pair and is the return type of loadImportConfigs, so it moved into language-config. Side effect worth having: the shared resolver types module no longer names a single language, which is an AGENTS.md rule for core/ingestion shared pipeline code. - ingestion/di-extractors barrel <-> spring: DiResolver and the two match types -> di-extractors/types.ts, following the import-resolvers/types.ts precedent. - scope-resolution/walkers <-> workspace-index: WorkspaceResolutionIndex -> workspace-index-types.ts. Re-export is load-bearing — 9 src importers, 4 test files, and a dynamic import() at contract/scope-resolver.ts. Moving the value isClassLike instead was rejected: ~15 value importers, and it is documented as a pair with isShapeLike. - server/analyze-worker <-> analyze-worker-core: the WorkerMessage protocol -> analyze-worker-protocol.ts, a declarations-only leaf. storage/branch-index <-> storage/repo-manager was the one genuine two-way runtime cycle: branch-index called getStoragePaths/loadMeta, repo-manager used branchSlug/BRANCHES_DIR. branch-index's header conceded the cycle and argued it was ESM-safe because neither side calls across at module-evaluation time — a guarantee resting on call ordering rather than structure. Folding resolveBranchPlacement back the other way does not help, because BranchSummary.stats is typed RepoMeta['stats'], so RepoMeta had to move either way. Extracted storage/repo-meta.ts, a leaf importing only fs and path, holding the metadata read primitives; repo-manager re-exports the public names so all 54 RepoMeta and 50 loadMeta importers are untouched. The moved block diffs byte-identical against HEAD. Verified beyond typecheck, because the worker entrypoint is the risky part and nothing in the suite forks it: emitted analyze-worker.js still contains exactly one runtime import, and forking the real worker over IPC boots it through entry -> core -> protocol -> terminal-claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv * refactor: apply the reuse, simplification, efficiency and altitude cleanups (#2915) The one that mattered: the degradation exit code was fixed at the wrong depth. `output()` has never inspected `partial` — it tests `error` only — so putting the check in `detectChangesCommand` left every other tool exiting 0 on a degraded run. `partial` is cross-tool vocabulary: query (enrichmentDegraded || ftsPartial), impact (!traversalComplete, perSymbolEnrichmentCapped) and the mode:'pdg' envelope all emit it. A truncated impact traversal returns a short caller set and an under-ranked risk, then exits 0 — so `gitnexus impact … && <edit>` proceeds, in the tool AGENTS.md makes a MUST gate before every edit. The justification also cited checkCommand as precedent, but checkCommand passes STRINGS too — it was the second command already hand-rolling around this gap, while output()'s docstring called itself "the one place that keeps scripted callers honest". output() now takes an optional renderer and fails on error OR partial; two hand-rolled sites go away and three tools are covered instead of one. truncated stays exit 0 (only the listing is capped) and checkCommand's cycleCount policy stays put. Efficiency, all re-measured on the 25k-node index: - The process lookup was chunked with LBUG_QUERY_BATCH_SIZE, calibrated for the opposite query shape — that constant is for a whole-node-table scan where more items amortise the scan, while this is an `id IN $ids` probe where round trips dominate. 20k ids: 617ms at 100, 261ms at 1000. New LBUG_ID_PROBE_BATCH_SIZE, documented against its sibling so they cannot be re-merged. This also settles the older "chunking this query is a regression" measurement — that was chunk=100. - The sort comparator re-coerced fields ChangedSymbolRow already types, O(n log n) redundant conversions (+31-38%). Row shape probed directly: alias-keyed, no positional keys, numeric columns are JS numbers. - exactlyMatchedPaths built two throwaway arrays; one loop instead (40k rows 11.4ms -> 4.5ms). - The integration fixture seeded 710 step edges one round trip at a time; one UNWIND instead. File wall time 6.91s -> 3.63s. Fixture size unchanged — its docstring records the threshold below which the bug stops reproducing, and the mutation check still fails 3/3 when ORDER BY step is reverted. Reuse and simplification: - CALL_EDGE_LIMIT existed in four places; its own docstring predicted the drift it then caused. prompts.ts owns it now — it is a zero-import leaf so the direction cannot cycle, and had graph-queries.ts owned it the four suites that vi.mock that module would have left slice(0, undefined), silently returning every edge in exactly the tests meant to police the cap. - Six dead positional row fallbacks survived the rewrite in the loop this branch re-indented, in the same PR that deleted the identical ABI from graph-queries.ts. - Two test files independently modelled the same labels() scalar-string quirk. Deleted the wiki one — the file's own new header says semantics belong in the real-engine test — and kept projectTypeColumn, the only instrument that can see the bug for the detect_changes query. - makeRepo onto the shared git bootstrap (the eleventh copy of the sequence the helper was extracted to own), the duplicate diff-args unwrapper merged into test/helpers, hand-rolled comparators onto compareCodeUnits, real-timer sleeps replaced by wave-released promises with a strengthened per-wave assertion. - Re-exports trimmed to what is actually imported, a cross-reference this branch invalidated by moving mapConcurrent, and a "~20% faster" claim that does not survive at real index sizes (1-9%; the 10x memory win does). Also adds the drift guard the new doc text lacked: fragment coverage for the partial/truncated paragraph in every skill copy and in the managed AGENTS.md / CLAUDE.md block. Falsifiability checked — none of those fragments exist at the merge base. Not done here, deliberately: 27 live labels(x)[0] projections remain across impact/context/query/trace and MCP resources, with four load-bearing workarounds that have begun depending on each other and one that fabricates rather than degrades. That is a semantic change to five agent-facing tools and wants its own PR, scoped to delete the workarounds too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv * fix(cli): restore the detect-changes subcommand in the regression example (#2915) Caught by the gitnexus-check bot on the PR. The regression-review fallback in the injected mandate rendered as `--scope compare --base-ref "main" --repo .` with no command, so anyone copying it invokes the runner with an option as its first argument. Self-inflicted, and by exactly the mechanism flagged when it landed: the block is under a test-enforced size cap (#856) that had 30 characters of headroom, so adding the partial/truncated clause required paying for it, and the 38-character "repeat" that was dropped turned out to be the subcommand rather than a repeat. Paid for the restoration out of the clause instead — both parentheticals are gone, since `partial` and `truncated` are already defined in the tool description this text points at. Block is back under the cap at 3548/3552. Notably the cap has now been raised four times (2700 -> 2900 -> 2950, then 0.55 -> 0.65) each with the argument that the new line is load-bearing, and it has now also caused a user-facing defect. It is not functioning as a budget. Left at 0.65 here rather than making it five: moving the threshold to fit one's own text is how it got here. Worth restructuring separately. The fragment guard added a commit ago caught the rewording immediately, which is what it is for; its fragments now pin the two policy claims rather than the prose around them, since that prose is what gets re-trimmed under the cap. Also verified and NOT changed: the bot's other error, that detect_changes compares 1-based hunks against 0-based graph lines. `bounds` is built from `coalesceHunksByPath`, which applies `toZeroBasedLine` to both ends at the grouping boundary, and both a mocked and a real-engine test pin an edit landing on a symbol's last line. The bot read `parseDiffHunks` in isolation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv * fix(core): reject a fractional chunk size, and stop the truncation note overclaiming (#2915) All five from the gitnexus-check bot's pass on the previous push; two were introduced by the cleanup round that preceded it. `chunk` guarded with `Number.isFinite`, which admits a fractional size — and that one does not fail, it DUPLICATES. `slice` truncates its indices while `i` does not, so size 1.5 yields slice(0, 1.5) = items 0-1 then slice(1.5, 3) = items 1-2, putting item 1 in two batches; a caller batching a query would send it twice. A size is a count, so `Number.isInteger`. Unreachable today (every caller passes a constant) but the guard existed precisely for the unreachable case, and the NaN half of it was already there. `mapConcurrent`'s per-item degradation contract had a hole: `onError` is caller-supplied and was invoked outside a try, so a throwing reporter rejected `settle`, rejected the whole `Promise.all` wave, and discarded the neighbouring successes the function exists to preserve. Reporting a failure must not become one. The CLI truncation note asserted "the counts and risk level still cover all of them", which is true only when `truncated` fires alone — with `partial` the counts are summed from the batches that succeeded. It now varies: a distinct string when both flags are set, saying the counts are a lower bound. This is the same claim already corrected in the tool description; the CLI text still had the old one. The di-extractors contract docstring claimed the barrel re-exports everything from it. That stopped being true when the re-export was trimmed to what is actually imported, one commit earlier. The real-engine wiki test claimed to prepare "every exported query" and omitted `getInterModuleEdgesForOverview`, which `generateOverview` calls. Added — it aggregates in JS over `getInterFileCallEdges` rather than issuing its own Cypher, so the note says why it is in a prepare test. Verified and NOT changed: the bot's other error, that detect_changes compares 1-based hunks against 0-based graph lines. `bounds` is built from `coalesceHunksByPath`, which converts both ends at the grouping boundary (storage/git.ts), and two tests pin an edit landing on a symbol's last line. The remaining seven findings are changed-symbol heads-ups with no signature change; their callers' suites are green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv * fix(mcp): make the IMPACT_MAX_CHUNKS fallback actually fire (#2915) The validation added earlier this branch used `Number.parseInt`, which takes the numeric PREFIX: '1.5' parses to 1, satisfies `Number.isInteger`, and silently caps enrichment after a single 100-item batch — the opposite of the fallback the comment beside it promised. `Number` instead, so a fractional value is rejected and falls back to 10. The emptiness check is load-bearing rather than defensive: `Number('')` is 0 and 0 is a legitimate value here (enrich nothing), so an UNSET variable would otherwise mean "enrich nothing" rather than "use the default". Behaviour table, old vs new: '1.5' 1 -> 10 (the bug), and undefined/''/' '/ '10junk'/'-2'/'all' -> 10, '0' -> 0, '3' -> 3, ' 5 ' -> 5 all unchanged. So the only case that moves is the reported one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Gergő Magyar
|
e69d3c49c4
|
fix(analyze): gate FTS-indexed DML before the incremental writeback (#2841) (#2854)
* fix(lbug): never report a drop that could not happen, and gate FTS-indexed DML `CALL DROP_FTS_INDEX` is itself an FTS-extension function, so with the extension unloaded it fails with `Catalog exception: function DROP_FTS_INDEX is not defined`. `isBenignDropFtsIndexError` classifies that as "nothing to drop" — correct when the index does not exist, wrong when it does: the drop silently no-ops and the next write to that table dies at bind time with an engine message that never mentions FTS (#2841). The classifier stays pure (a message cannot tell you whether an index is live). Instead `dropFTSIndex` settles liveness with a catalog read on the ERROR path only and raises an FTS-named, remedy-bearing error when the index is present but undroppable. Adds `ensureFtsRowDmlSafe`, the FTS twin of `ensureEmbeddingRowDmlSafe` (#2623): catalog first, load FTS with the analyze policy only when an index actually gates DML. LadybugDB refuses that DML at BIND time — a DETACH DELETE matching zero rows fails exactly as hard as one matching thousands — and the indexes cannot be cleared in place, so a verdict is the only useful answer. Both gates now share one `SHOW_INDEXES` read via `readIndexCatalogRows`, so adding the FTS check costs no extra catalog round-trip. Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * fix(analyze): escalate instead of crashing when FTS blocks incremental DML The incremental writeback decided its write plan without ever asking whether row-level DML was legal. On a DB carrying FTS indexes with an unloadable FTS extension, `deleteNodesForFiles` then died mid-writeback: Binder exception: Trying to delete from an index on table File but its extension is not loaded. with no mention of FTS anywhere in the run — the only install-capable load happened in Phase 3, long after the writes (#2841). The incremental branch now reads the index catalog once and derives both extension verdicts before any DML. When FTS (or VECTOR) blocks in-place writes, the run falls through to the existing wipe-and-bulk-COPY escalation — the same answer #2623 gave for VECTOR, and the only one available, since the indexes cannot be dropped without the extension. Every blocked extension is named in the reason log, not just the first one checked: a DB can carry both a vector index and FTS indexes, and reporting half the cause is how this failure stayed mis-diagnosed. Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * test(analyze): cover the FTS DML gate, both-blocked escalation, and the drop guard New `incremental-index-extension-dml-gate.test.ts` drives the real `runFullAnalysis` against a real mini-repo and a real LadybugDB: - a DB carrying FTS indexes with FTS made unloadable escalates to a full DB write, names FTS in the log, ends with zero FTS indexes, and still has the newly committed content in the graph (pre-fix: Binder exception, exit 1); - FTS available keeps the surgical plan and the indexes; - a DB that never carried FTS indexes is not escalated (the catalog-first check must not tax FTS-less machines); - FTS and VECTOR both blocked produce ONE escalation naming both. `drop-fts-index-error-classification.test.ts` gains the two `dropFTSIndex` cases the #2841 guard turns on: live index + unloaded extension rejects with an FTS-named error, absent index still resolves. The existing classifier assertions are unchanged — it stays pure. The CLI e2e reproduces the reporter's exact journey (analyze with the extension, remove it, touch a file, analyze again) and asserts exit 0 plus an FTS-named reason. It skips visibly when the seeded extension cannot load on the host, so it can never report a false red about the fix. Mutation-verified: reverting the run-analyze gate fails the first scenario; reverting the dropFTSIndex guard fails the live-index case. Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * fix(lbug): make every catalog-gated path fail closed, and classify the drop remedy Review findings on #2854 (two-engine, 17 lanes). H3 — `ftsIndexExistsInCatalog` returned `false` when the catalog could not be read, i.e. "index absent", so `dropFTSIndex` swallowed the error and the caller proceeded as if the index were gone. That is the #2841 symptom the guard exists to make loud, and it contradicted the contract `readIndexCatalogRows` states two functions above. It now fails closed. §6.A — `ensureFtsRowDmlSafe` keyed on `index_type === 'FTS'`, which answers `undefined === 'FTS'` → false → *no gate* for a row whose shape cannot be read: fail-open, in the gate whose only job is preventing an unsafe write, while the VECTOR twin fails closed on the same input. Now only a positively-identified non-FTS index is waved through. Deliberately NOT the twin's `!== 'HASH'`: that is safe there only because it is scoped to the embedding table first, and this gate is table-agnostic — `!== 'HASH'` would let the HNSW index gate FTS DML. §5.A — `undefined` was overloaded: "caller passed nothing" and "caller tried and could not prove anything" shared one value, so a failed shared read silently became three reads and the two gates could decide from different snapshots. The failed snapshot is now representable (`INDEX_CATALOG_UNREADABLE`), leaving one unambiguous `??` in `resolveGateRows`. §5.B — both gates regained the unconditional null-connection precondition the refactor moved into the reader. §5.G — the throw's remedy now routes through `diagnoseExtensionLoad`, like `--repair-fts` and `ftsDegradedWarning`, so a missing runtime dependency is not told to reinstall. The message stays path-free (#2374/#2375). The dead positional row fallbacks are kept and marked `LADYBUGDB-CONTRACT`: removing them would turn a proven-inert hedge into a fail-open gate if a future engine returns unnamed tuples. Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * fix(analyze): never undo an explicit wipe, stage extension-forced rebuilds, report honestly Review findings on #2854 (two-engine, 17 lanes). H1 (P1, both engines) — `analyze --drop-embeddings` was silently reverted. The `--drop-embeddings` → `force` conversion sits inside the `embeddingCheckpoint` branch, so without a checkpoint the run stays incremental and reaches the gate; the flag then *deliberately* leaves `cachedEmbeddings` empty, which is exactly the rescue's trigger, so every row the operator asked to destroy was read back and restored, exit 0. Widening the rescue from `!embeddingRowDmlSafe` to `extensionForcedRebuild` moved that latent bug onto the dominant path, because every analyzed DB carries FTS indexes. Guarded on the flag itself — NOT on `shouldLoadCache`, which is false in the meta-under-reports case the rescue exists for and would have deleted the safeguard while fixing the wipe. The `--drop-embeddings --embeddings` variant is covered by the same guard. H2 — an extension-forced escalation wiped the LIVE index in place: `buildPath` was frozen ~440 lines earlier while the run was still classified incremental, so an interrupt or ENOSPC left no complete index, where main failed at bind time with it intact. Extension-forced rebuilds now build into a staging file and publish via the existing atomic swap; size-forced ones stay in place, since that trigger is the repo's own churn rather than a machine condition. H5 — the escalation log asserted a vector index "exists" and that the store "carries FTS indexes" in exactly the case the catalog read proved nothing, while the only truthful signal went to stderr rather than the IPC log. It now emits a distinct unreadable-catalog cause, and "this index carries" (which pointed at the vector index just named) reads "the graph store carries". §5.D — the write-set cause was dropped whenever an extension cause co-occurred; causes are appended now, not selected between. §5.C — after an FTS-forced rebuild stamped lastCommit, a plain rerun on the same commit hit the alreadyUpToDate fast path before Phase 3, so the CLI's "install … then rerun" advice could never restore FTS. The fast path is now bypassed when meta records FTS unavailable and the extension can load again, keyed on the persisted capabilities stamp rather than new state. §5.F (skip the escalation for a zero-change commit) is deliberately NOT implemented: `deleteSpringAutoConfigurationSyntheticClasses` and `deleteSpringAopEvidenceNodes` run unconditionally on the surgical branch and bind against FTS-indexed `Class`/`CodeElement`, and a zero-row DETACH DELETE fails at bind time exactly as hard as a large one — so the skip would restore the original crash. Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * perf(search): read the index catalog once per drop sweep, and state the real contract Review findings on #2854. H4 — on a machine where FTS cannot load and the DB carries no FTS index, the gate correctly returned early without loading the extension, but the surgical path still ran the full 20-entry drop sweep: every `CALL DROP_FTS_INDEX` raised "function DROP_FTS_INDEX is not defined", and the new liveness guard then fired a fresh catalog read per table — 20 reads every run, forever, for exactly the offline/load-only population, contradicting the "healthy path costs nothing" claim shipped with the guard. The sweep now reads the catalog once and skips entirely when no FTS-typed index exists. An unreadable catalog runs the sweep, so an unprovable catalog never skips real work. H8 — the docstring still promised `dropFTSIndex` "tolerates" an unloadable extension. Post-#2854 a live index plus an unloadable extension throws, and safety rests on caller ordering discipline rather than the type system — which is what would have talked the next caller out of that ordering. GUARDRAILS — the "switching to a full DB write" sign described exactly one trigger (write set >~50%). Since #2623 and #2841 an unloadable extension escalates regardless of write-set size; documented with its recovery steps. Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * test(analyze): cover the wipe guard, the staged rebuild, and the fail-closed branches Review findings on #2854. H1/H2 mutation-verified: removing `!options.dropEmbeddings` fails the new drop-embeddings case ("expected true to be false"); disabling the staging upgrade fails the staging case ("expected 0 to be greater than 0"), so both assert behaviour rather than describe it. Gate suite (7 cases): `--drop-embeddings` under an FTS-forced escalation ends at zero embedding rows and logs no "Preserving"; the escalation is one-shot — a third run on a healthy host returns to surgery and rebuilds every FTS index; an extension-forced rebuild is observed building into `lbug.staging.*` and leaves none behind; the rescue complement still preserves un-stamped rows when no wipe was requested; the never-built case now asserts the commit reached the graph. H6 — the both-blocked case hard-asserted `createVectorIndex()` while the suite probed FTS only, so it went red on any FTS-yes/VECTOR-no host. VECTOR is probed now and gates only that case, with a GITNEXUS_REQUIRE_VECTOR hard-fail. H7 — the fail-closed branches had no coverage although the VECTOR twin's test and interception technique were ready to copy: `ensureFtsRowDmlSafe` under an unreadable catalog now proves it routes to the load, and `dropFTSIndex` proves it rejects rather than silently tolerating. Plus a redaction case that forces a real path-bearing load failure — under policy `never` the assertion would have been vacuous, since that reason carries no path. §5.E/§6.B — the suite is registered in the cross-platform matrix (its sibling was; it wasn't, and GITNEXUS_REQUIRE_VECTOR is set only on that job) and moved into the sequential lbug-db project per TESTING.md:68, verified not to drop it from the sharded ubuntu job. A Windows shard weight is added as a labelled estimate — the 8s floor would skew the split it exists to protect. Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * refactor(analyze): make the FTS gate's fast path cheap, its claims provable, and its remedies classified Cleanup review of the #2841 work (four parallel angles: reuse, simplification, efficiency, altitude). Behaviour-preserving except where the previous behaviour was wrong. Correctness the review caught: - The fast-path probe keyed on `capabilities.fts.status === 'unavailable'`, which collapses "extension unavailable" and "index build failed". A deterministic build failure (an un-tokenizable row, #2544) therefore bypassed `alreadyUpToDate` on EVERY subsequent run, re-analyzed the whole repo, failed the same way, and restamped — a permanent loop where the run used to be one `stat`. Phase 3 already computes the discriminator; it is now persisted as `fts.skipReason` and the probe only runs for `extension-unavailable`. Metas written before this carry no field and keep today's behaviour. - `dropSearchFTSIndexes` skipped its sweep when no row read `index_type === 'FTS'`, while `ensureFtsRowDmlSafe` treats an unreadable type as "might be FTS". Opposite polarity, under a comment claiming they matched: a row-shape change would let the gate wave the surgical plan through while the sweep dropped nothing, putting DELETEs back on tables carrying live FTS indexes — #2589 again. The sweep now decides per configured index on identity, which is also strictly more precise. Its old justification (leftover indexes under other names) was unreachable — the loop only ever drops configured entries. - `dropFTSIndex` threw "FTS index X on table Y exists" on the one path where the catalog could not be read — a fabricated claim, on a DB the same run had just shown carries no FTS index. Presence is now `present | absent | unverifiable` and the message says which. - The remedy was hand-written for three of the four load-failure classes, discarding `missingFileRemedy`/`corruptFileRemedy`, so a corrupt extension file was told to retry an install — the misdirection #2383 fixed. Both the drop error and the escalation log now use the classified remedy. Cost, measured on a 391 MB index (cold open ~1 s, SHOW_INDEXES ~4 ms): - The probe opened the live index WRITABLE on the millisecond fast path, dragging in schema DDL, the cross-process write lock, sidecar reclaim and a CHECKPOINT on close. It is read-only now. That also closes an install trap: `doInitLbug`'s pre-load resolves the env policy on the writable branch, so an operator following our own `GITNEXUS_LBUG_EXTENSION_INSTALL=auto` advice paid a forked 15 s installer on every up-to-date run (memoized per process; the CLI is a fresh process each time). The read-only branch pins `load-only`. - A failed staged rebuild orphaned a full index-sized copy until the next lock sweep; the failure path now reclaims it. - The sweep re-read a catalog the run already held, defeating the invariant the snapshot type exists to enforce. Structure: row-shape accessors have one home, so the LADYBUGDB-CONTRACT grep claim is true by construction; staging now applies to both escalation causes, since recoverability is a property of the wipe-then-COPY plan, not of the trigger; `getExtensionCapability`/`getFtsCapability` replace hand-spelled lookups where the seam allows. Two lookups in run-analyze.ts deliberately keep the exported `getExtensionCapabilities()` form: the #2383 tests stub that export, and an ESM module mock does not intercept a helper's internal call — routing through it silently degraded the classified remedy to generic text. Recorded in-comment. Not taken, deliberately: extracting the escalation message and replacing the snapshot protocol with a connection-scoped catalog memo (both sound, both restructure code this PR just stabilised — they belong in their own change); an extension registry (premature at two instances, and the FTS/VECTOR polarity difference is exactly what it would have to parameterize back out). Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * test(analyze): pin both sides of the degraded-FTS fast-path bypass `healDegradedFts` (§5.C) had zero coverage — three separate review angles flagged it, and the cleanup pass then found it sat one conjunct away from a permanent full-re-analyze loop. Both sides are pinned now: - it re-analyzes past `alreadyUpToDate` when the stored meta says FTS is degraded and the extension loads again: run 1 analyzes with loads blocked (asserting the precondition — `status: 'unavailable'`, `skipReason: 'extension-unavailable'` — rather than assuming it), then a same-commit clean-tree rerun rebuilds every FTS index without a file changing; - it stands down when the degradation was a BUILD failure: the stored `skipReason` is rewritten to 'build-failed' and the rerun must take the fast path, because that rebuild would fail identically on every run forever. The build-failed state is reached by rewriting the stamped discriminator, not by provoking a real tokenizer failure: a genuine one needs a stored row the native tokenizer rejects (#2544/#2546), which is neither portable across the CI matrix nor deterministic, and §5.C reads only that field. Also folds the first escalation case into the one-shot case. The claim that it was fully subsumed did not hold on audit: `logs` containing 'FTS' was unique as expected, but so was the duplicate-File-node row count — every other reader goes through a Map keyed by path, which collapses a stale twin an appending rebuild would leave. Both assertions moved rather than one being dropped. Net suite runtime goes UP (two cycles removed, four added), against the cross-platform-matrix argument that motivated the dedup — recorded here because the shard weight is an estimate pending a real Windows measurement. Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * test(search): keep the whole-module adapter mock in step with the row accessors The cleanup pass moved the LadybugDB row-shape reads behind named accessors so the column contract has one home. `fts-indexes.test.ts` mocks the entire adapter module with a hand-written factory, which still exposed only the three exports the file imported before — so `verifySearchFTSIndexes` failed with "No `indexRowName` export is defined on the mock" while production was fine. The added accessors mirror the real implementations rather than returning stubs. A stub would have read `undefined` out of every catalog row and let the suite pass for the wrong reason — the failure mode a whole-module mock invites whenever the module under test grows an import. Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * revert(analyze): drop the degraded-FTS auto-heal, fix the advice it existed to justify §5.C's complaint was that the CLI tells users to "install the extension … then rerun" when a rerun lands on the up-to-date fast path and rebuilds nothing. The answer shipped for it was a probe that bypasses that fast path. Four independent problems later, the sentence is cheaper to fix than to make true: - it could not tell "extension was missing" from "index build failed" without a stamped discriminator, so a deterministic build failure (#2544/#2546) re-analyzed the entire repo on every invocation, forever, where the run used to be one `stat`; - it opened the live index on the millisecond fast path — writable at first, dragging in DDL, the cross-process lock and a CHECKPOINT (~1 s on a 391 MB index), and even read-only it is a full open; - `doInitLbug`'s pre-load resolves the env policy, so an operator following our own `GITNEXUS_LBUG_EXTENSION_INSTALL=auto` advice paid a forked 15 s installer per up-to-date run; - and it turns the fast path into a full re-analysis whenever an index authored where FTS was unavailable is later read where it loads — a legitimate, common state, and the invariant `analyzer-identity-cli.test.ts` pins. So: no probe. The degraded-search warning now points at `gitnexus analyze --repair-fts`, which rebuilds the search indexes without re-parsing the repo, instead of "then rerun". One line, no new failure modes, and it is what the issue actually asked for. `capabilities.fts.skipReason` stays in the meta stamp: it costs three lines, makes the two degradation causes distinguishable for support, and is what any future correct answer here would key on. Also gates the H2 staging assertion on the production predicate. It asserted staging unconditionally while the upgrade requires `posixSwap || windowsSwapOk`, and `windowsSwapOk` is opt-in (#2614) — so it failed on the Windows matrix for a reason unrelated to #2841. Registering this suite cross-platform is what exposed it; the assertion now mirrors the condition it is testing. Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB * fix(analyze): never stage around a damaged index — escalate in place when the catalog is unreadable CI caught this on ubuntu and macOS: `analyze-wal-checkpoint-failure` stopped failing, which is worse than it sounds. That test plants a directory at `.gitnexus/lbug.wal.checkpoint` so the auto-checkpoint's rename target is blocked, and asserts analyze exits non-zero with the `--wal-checkpoint-threshold` hint. But LadybugDB cannot open that path at all, so `CALL SHOW_INDEXES()` now fails with `IO exception: … Is a directory`. The catalog read returns UNREADABLE, both DML gates correctly fail closed, both extension loads fail with the same IO error, and the run escalates — and since the escalation stages, it built a fresh index at `lbug.staging.<uuid>`, swapped it in, and exited 0. The blocked path was never touched. The run "succeeded" while the damage sat untouched on disk, waiting to break the next in-place writeback. So the staging upgrade is now conditional on the catalog having been READ. Staging exists to protect a healthy live index from a machine-level cause (an extension that will not load); it must not be used to route around a damaged one. When we are escalating out of ignorance, build in place so the underlying IO fault lands on the failure path where the operator gets a diagnosis. Verified against the real CLI, not just the suite: with a directory planted at the checkpoint path, analyze now exits 1 and prints `gitnexus analyze --wal-checkpoint-threshold 67108864`. The healthy extension-forced case still stages (gate suite 6/6). Refs #2841 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxokkpfssUCxvBwRZMtRCB --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Gergő Magyar
|
990d79ba8c
|
fix(mcp): make impact/context reproducible — deterministic ordering on every capped query (#2787) (#2796) | ||
|
azizur100389
|
d268f351d3
|
fix(group): preserve manifest-only impact crossings (#2784)
* fix(group): preserve manifest-only impact crossings Keep proven manifest cross-repo hits when the far endpoint has no concrete graph symbol, avoiding a guaranteed failed UID fan-out. * fix(group): verify manifest-only neighbor repos Keep manifest-only crossings from bypassing neighbor repository resolution so unavailable repos still surface as truncated fan-out. * fix(group): distinguish boundary-only impact crossings Keep manifest-only boundaries visible without treating unattempted fan-out as completed impact or escalating risk, and cover service scope, deduplication, and real bridge persistence. --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> |
||
|
MyShining
|
1147646518
|
feat(spring): model AOP transactions, caching, and security (#2783)
* feat(spring): model AOP advice and proxy behavior * fix(spring): address AOP review findings --------- Co-authored-by: Shining <xuenning@qiyi.com> |
||
|
Gergő Magyar
|
ad1b9227c4
|
fix: large-repo analyze OOM and false worker-timeout cascade (#2649) (#2679) | ||
|
Gergő Magyar
|
df1fc36094
|
fix: make large incremental writebacks commit reliably (#2409) (#2425) | ||
|
Gergő Magyar
|
8402963198
|
fix(ci): shard platform-sensitive matrix + spawn built CLI to fix Windows cross-platform timeout (#2394)
* fix(ci): shard platform-sensitive matrix + spawn built CLI to fix Windows cross-platform timeout The `windows-latest (platform-sensitive)` job was hitting its 15-min internal vitest watchdog in run-cross-platform.ts. It's cumulative slowness, not a hang: the fixed 72-file suite is dominated by ~50 CLI/worker process spawns, and Windows is ~5x slower than macOS at process startup (macOS ran the same set in ~3min of tests). Two complementary changes bring it back under the watchdog with headroom, without touching any test assertion: - Shard the platform-sensitive matrix (windows/macos × shard [1,2]) and forward `--shard=i/2` through run-cross-platform.ts to vitest, which partitions the fixed file list deterministically (sha1, equal file-count) — halving each runner. macOS/Ubuntu were already under budget. - New test/helpers/cli-entry.ts (`CLI_SPAWN_PREFIX`): spawn the built `dist/cli/index.js` when `GITNEXUS_E2E_CLI=dist` (set on the cross-platform job, which already builds) instead of `node --import tsx src/cli/index.ts`, which re-transpiles the whole CLI on every spawn. Defaults to tsx-on-source so local runs always reflect current source; `GITNEXUS_E2E_CLI=dist` on an unbuilt tree throws an actionable "run npm run build" error. dist is opt-in only — never inferred from a generic `CI` env — so an ambient `CI=1` can't silently run a stale build. Converted 8 spawn-based e2e suites; added test/unit/cli-entry.test.ts. The Ubuntu coverage job leaves `GITNEXUS_E2E_CLI` unset, so the tsx-on-source path stays exercised in CI too (both entry points covered). Measured on Linux: cli-limit-e2e 121.5s→91s, cli-e2e 289s→217s (~25%); larger on Windows where the transpile is a bigger share of each spawn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(ci): derive platform-sensitive shard count from one source (#2394) The shard total was hardcoded in three coupled, unenforced places (matrix length, job-name suffix, --shard denominator); editing one without the others silently dropped a shard's tests with green CI. Add a checkout-free shard-plan job whose single TOTAL generates both the shard index list (consumed via fromJSON) and the /N denominator (job name + --shard arg), so they cannot drift. Asserts TOTAL>=1 to rule out an empty-matrix silent skip. No behavior change — still 2 shards per OS. Addresses PR #2394 tri-review finding F2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ci): 3 shards for real Windows headroom + honest sharding comments (#2394) vitest shards by file COUNT, not runtime, so the heaviest spawn suites cluster into one shard: live CI showed Windows shard 1/2 at 12m12s (~81% of the 15-min watchdog) vs shard 2/2 at 3m0s. The old comments claimed "comfortable/generous headroom", which the count-based split doesn't deliver at 2 shards. Bump TOTAL to 3 (one line, single source) so even the busiest Windows shard clears the watchdog, and reword the comments to describe count-based (not time-based) sharding. Addresses PR #2394 tri-review finding F1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ci): extract testable parseShardArg from run-cross-platform (#2394) The --shard parse/forward glue had no unit test. Extract it into a pure scripts/shard-arg.ts (mirroring the computeSpawnPrefix extraction precedent) so the branch logic is lockable without the script's top-level execFileSync, and add test/unit/shard-arg.test.ts (absent -> undefined, valid token -> passed through, found amid other args). Behavior unchanged; U4 adds the malformed fail-loud on top. Addresses PR #2394 tri-review finding F3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ci): fail loud on a malformed --shard arg (#2394) A shard-shaped-but-malformed arg (--shard=1, --shard, --shard=abc) was silently ignored, dropping the shard flag so both legs ran the full unsharded ~50-spawn suite — re-arming the Windows watchdog timeout with no signal. parseShardArg now throws an actionable error on any --shard/--shard=… arg that fails the strict regex (unrelated flags like --shardx= pass through), and the call site in run-cross-platform.ts catches it into console.error + exit 1, kept outside the execFileSync try so the message isn't swallowed by that catch's watchdog-only branch. Addresses PR #2394 tri-review finding F4. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(test): fail loud on an unknown GITNEXUS_E2E_CLI value (#2394) computeSpawnPrefix silently degraded any unknown GITNEXUS_E2E_CLI value to tsx-on-source, so a typo (e.g. `dsit`) would make CI believe it tests the dist entry point while actually running src. Throw on any value other than 'dist'/'src'/unset (the safe tsx default is preserved for unset/''/'src', so it still never selects dist without an explicit opt-in). Flip the unknown-mode unit test to assert the throw and add the missing {mode:undefined, distExists:true} case. Only ci-tests.yml sets the var (=dist), so no existing suite is affected. Addresses PR #2394 tri-review findings minor-a/b. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ci): run cli-entry.test.ts on the cross-platform matrix (#2394) cli-entry.test.ts resolves CLI_SPAWN_PREFIX from a real path, and its last assertion (cli[/\\]index) has a Windows backslash branch that only Ubuntu exercised. Register it in PLATFORM_LOGIC so it runs on the Windows/macOS matrix too. (shard-arg.test.ts stays out — pure string logic, OS-independent.) List grows 73 -> 74; the generated shard matrix keeps coverage complete. Addresses PR #2394 tri-review finding minor-c. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(test): share tsxLoaderUrl(), dedup the last tsx-loader boilerplate (#2394) bridge-cache-reopen.test.ts carried its own copy of the tsx-loader-resolution boilerplate (createRequire -> resolve('tsx/package.json') -> pathToFileURL) — the one site the PR's CLI_SPAWN_PREFIX migration didn't cover (it spawns a seed script, not the CLI). Export the existing tsxLoaderUrl() from cli-entry.ts and reuse it here; the resolved loader URL is byte-identical. Addresses PR #2394 tri-review finding minor-d. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(test): make skipUnlessFtsAvailable install FTS on miss so shards are self-sufficient (#2394) Sharding the platform-sensitive suite into 3 exposed a latent test-isolation bug: load-only FTS primitives (test/integration/lbug-core-adapter.test.ts) only passed because a sibling installer test happened to co-locate in the same shard and install FTS into the shared ~/.lbdb first. At 3 shards, lbug-core-adapter landed in a shard with no installer sibling, so its load-only loadFTSExtension() failed deterministically on macOS+Windows shard 2/3 under GITNEXUS_REQUIRE_FTS=1. Make the gate self-sufficient: on a load-only miss under REQUIRE_FTS, install FTS with `auto` (LOAD-first, then one bounded network INSTALL) before treating it as a hard failure — mirroring withTestIndexedDB. A pre-installed extension still costs no network (auto is LOAD-first); offline/local runs (no env var) still skip gracefully. Verified: with a fresh HOME (no pre-installed FTS) + REQUIRE_FTS=1, lbug-core-adapter now passes 15/15 (previously threw). Addresses the 3-shard CI failure surfaced while validating PR #2394's F1 fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: warm-cache the LadybugDB FTS extension across platform shards (#2394) Follow-up to the FTS self-install fix: cache ~/.lbdb/extension per OS + lockfile so a warm run skips the network install entirely and the parallel shards share one download across runs. Pure reliability/speed — on a cache miss the tests still self-install FTS on demand (test/helpers/fts-availability.ts), so this is never a correctness dependency, just a way to cut the network-install surface that made the sharded FTS tests flaky. Keyed by lockfile hash (a LadybugDB version bump re-installs); per-OS since the extension is a native binary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ci): pass shard via env to clear zizmor template-injection (#2394) Interpolating ${{ matrix.shard }} (now sourced from the shard-plan job output) directly into the run: shell tripped zizmor's template-injection audit (code-scanning alert #824, ci-tests.yml:147). Move the value into a SHARD env var — assigned via ${{ }} but referenced as "$SHARD" in the shell, which is not an injection sink — and set shell: bash so the expansion is uniform across the windows + macOS matrix (the default run shell is pwsh on Windows, where $SHARD would be empty and trip the new malformed-shard fail-loud). Verified locally with zizmor: the :147 template-injection finding is gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: shard the ubuntu coverage job and merge blobs before the threshold gate (#2394) The coverage job ran the full suite unsharded (~16 min). Shard it like the cross-platform matrix, then merge the per-shard coverage before enforcing the threshold gate: - shard-plan now also single-sources the coverage shard count (cov_total / cov_shards), so the coverage matrix + /N denominator can't drift. - The `tests` job becomes a coverage shard matrix: each shard runs `vitest run --shard --coverage --reporter=blob` with thresholds forced to 0 (a single shard's partial coverage can never meet the gate) and uploads its blob. FTS self-installs per shard, so sharding the full suite is safe. - New `coverage-merge` job (needs: tests) reduces the blobs with `vitest --mergeReports`, enforcing the REAL config thresholds on the combined ('new') coverage — this is the gate. It also emits the merged test-results.json and runs the unsharded web + docker suites, so the `test-reports` artifact keeps the exact shape ci-report.yml consumes for its base-branch ('baseline') vs new coverage delta. The shard arg goes through a SHARD env var + shell: bash (no template-injection). Validated locally: shard blobs write and merge into a coverage-summary.json + merged test-results.json; the merge enforces thresholds on the union. CI Gate still aggregates the coverage-merge result via the reusable-workflow call. Note: the coverage check names change (ubuntu / coverage 1/3 … + merge) — update any pinned branch-protection required checks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ci): include hidden files when uploading the coverage blob (#2394) The coverage shards write their blob to gitnexus/.vitest-reports/ (a dotdir). actions/upload-artifact excludes hidden files by default, so the coverage-blob-* artifacts uploaded empty — the merge job then downloaded 0 artifacts and vitest --mergeReports failed with ENOENT scandir '.vitest-reports'. Set include-hidden-files: true on the blob upload so the blobs actually ship. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ci): group shard-plan GITHUB_OUTPUT writes to satisfy shellcheck SC2129 (#2394) Adding the coverage shard outputs (cov_shards/cov_total) made the shard-plan gen step write four individual `>> "$GITHUB_OUTPUT"` redirects, which shellcheck (run by the actionlint check) flags as SC2129. Group the echoes into a single `{ …; } >> "$GITHUB_OUTPUT"` block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(test): cost-balanced shard sequencer to cut CPU contention (#2394) vitest's default --shard hashes file paths and splits by file COUNT, which clustered the spawn-heavy suites onto one runner (Windows platform shard 1 ran ~4x the others). Add a custom sequence.sequencer that overrides only shard() and balances by estimated WORK instead: - specWeight() weights the fileParallelism:false spawn-heavy suites (cli-e2e, lbug-db — already isolated to run sequentially) far above the parallel default files, plus file size as a cheap finer signal. Deterministic per checkout. - assignShards() does greedy longest-processing-time bin-packing (heaviest file into the currently-lightest shard). The partition stays complete and disjoint — verified: on the 74-file cross-platform set the three shards weigh 7611/7610/8064 (the sequential-heavy files spread ~7/7/8) with zero overlap and no file dropped, vs the hash split's count-only balance. sort() is left to the base sequencer so project groupOrder / duration-cache ordering is untouched. Pure logic split into shard-balance.ts with a unit test locking the disjoint+complete, balance, and determinism properties. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ci): install + cache FTS up front on the coverage (and cross-platform) shards (#2394) coverage 3/3 failed on extension-binary-real.test.ts: it uses the file-path FTS gate (requireFtsResourceOrSkip), which resolves ~/.lbdb/extension at MODULE LOAD and cannot self-install the way the load-path gate (skipUnlessFtsAvailable, U8) does. The coverage job had no FTS cache and relied on an installer test running first in the shard — the balancing sequencer reshuffled the shards and dropped extension-binary-real into a shard with no installer, so FTS was absent. Remove the ordering dependency: add scripts/ensure-fts.ts (init a throwaway lbug db, loadFTSExtension with policy:auto → LOAD-first, INSTALL on miss) and run it up front on every coverage AND cross-platform shard, after restoring the per-OS FTS cache. The coverage job now shares that same cache key (it previously had none — this is the "share the cached FTS with coverage" the failure pointed at). Cold cache installs once; warm cache is a no-network load. Verified locally: ensure-fts installs FTS into a fresh HOME and is a no-op when already present. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Gergő Magyar
|
76a1c90b02
|
fix(fts): diagnose Windows FTS missing-dependency load failures (#2374, Phase 1) (#2383)
Some checks are pending
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Scorecard / Scorecard analysis (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
* feat(lbug): classify FTS extension load errors with Windows missing-dependency guard (#2374) Add classifyExtensionLoadError() — a pure-string, lbug-free four-way classifier (missing_file / corrupt_file / missing_dependency / unknown). The Windows catch-all guard keys missing_dependency strictly on the error-126 signal, never LadybugDB's generic 'Failed to load library … needed by extension' wrapper, so 127/5/1114 and truncated (193) files route correctly. * feat(fts): surface classified missing-dependency remedy in doctor, repair-fts, and degrade warnings (#2374) Route the FTS load reason through classifyExtensionLoadError at all four surfaces (doctor, --repair-fts error, analyze degrade log, ftsDegradedWarning). For the Windows missing-dependency class, emit the runtime-install remedy (VC++ redist, then OpenSSL) instead of the wrong reinstall-over-network guidance; other classes keep their existing routing. Path redaction preserved on the client-facing warning. * test(fts): assert doctor surfaces the classified remedy end-to-end (#2374) Extend the broken-file e2e: doctor now prints the corrupt-file re-download remedy through the real CLI, and the Windows missing-dependency remedy (VC++/OpenSSL) must not misfire on a corrupt file — the catch-all guard, verified end-to-end. Also assert the repair path does not misfire. * style(fts): apply prettier formatting to #2374 diagnosis files * feat(fts): language-independent hedged fallback for Windows load failures (#2374) The Windows OS-error tail is localized, so matching only en/zh 126 text left other locales on the generic 'run doctor' remedy. lbug's 'Failed to load library' wrapper is English on every platform and present for all load failures, so use it as a fallback: when the localized tail matches no specific class, emit a hedged remedy that points the user at their own OS error and offers both branches (install runtime / --repair-fts) without prescribing the wrong single fix. Precise en/zh 126 keeps its definite remedy. * feat(fts): language-independent structural classifier via binary inspection (#2374) Add diagnoseExtensionLoad: pull the extension's file path out of lbug's own English wrapper and inspect the binary header (PE/ELF/Mach-O magic + arch) directly, so corrupt-vs-valid is decided by the file itself, not the localized OS-error tail. A valid binary that still failed to load ⇒ missing_dependency (runtime dep), decided in any OS display language and on all three platforms. Falls back to the string classifier (with its hedged fallback) when the file can't be read. Wire all four surfaces to it. Event Viewer / GetLastError-via-FFI were dead ends (lbug catches the failure — no crash event; no native FFI dep). * test(fts): exercise the structural classifier on real binaries (#2374) Add an integration suite that runs inspectExtensionBinary/diagnoseExtensionLoad against genuine binaries — the running node executable, the real lbugjs.node addon, and the installed FTS extension (valid); a truncated real binary and a real text file (corrupt). Registered in cross-platform-tests PLATFORM_LOGIC so it runs on the Windows + macOS matrix, proving the PE and Mach-O header parsing on real PE/Mach-O files (ubuntu covers ELF). * fix(fts): honor a corrupt_file verdict over a structurally-valid header (#2374) The structural probe in diagnoseExtensionLoad inspects only the first 4 KB, so a download truncated after its header reads 'valid' and was routed to the "install VC++, reinstalling will NOT help" remedy — the exact loop #2374 exists to kill, for the truncated-download case the module docstring claims it handles. Honor the loader's own corruption report ("file too short" / Windows error 193 "not a valid Win32 application") before defaulting to the dependency remedy; localized corrupt tails stay hedged missing_dependency, preserving language-independence. Addresses PR #2383 review finding F1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(fts): return indeterminate for a PE header beyond the read window (#2374) The structural probe reads only BINARY_HEADER_BYTES (4 KB). A valid PE with a large DOS stub whose e_lfanew points past that window was wrongly called 'corrupt', routing a fine DLL to "re-download". A garbage e_lfanew from a truly corrupt file is indistinguishable from here, so widen the header verdict with 'indeterminate' and return it in that case; the caller then defers to the loader's own report instead of asserting a false verdict. Fat Mach-O stays valid (LadybugDB ships thin per-arch binaries). Also covers the unmapped-arch and garbage-PE-signature branches. Addresses PR #2383 review finding F1-secondary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(fts): drop contradictory reinstall guidance from the analyze degrade log (#2374) For a missing runtime dependency the extension file is present, so appending FTS_UNAVAILABLE_MESSAGE (which tells the user to install it "with network access") to the remedy ("reinstalling will NOT help") produced self-contradictory guidance on the main analyze surface. Lead the missing_dependency degrade log with the class-neutral sentence (FTS_UNAVAILABLE_LEAD) and append only the classified remedy; other classes keep FTS_UNAVAILABLE_MESSAGE unchanged. Addresses PR #2383 review finding F2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(fts): cache the load diagnosis so the degraded warning does no per-request I/O (#2374) ftsDegradedWarning() runs on every degraded /api/search response and MCP query, and it was calling diagnoseExtensionLoad — a synchronous openSync/readSync of the extension file — on every call. Compute the diagnosis once at mark-unavailable time (the single load-failure sink, run per Database not per request), cache it on ExtensionCapability, and have the warning read the cached result (falling back to the pure, no-I/O string classifier if it is absent). Loader capability-shape assertions relax from toEqual to toMatchObject for the new optional field. Addresses PR #2383 review finding F3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(fts): cover the missing_dependency remedy on the --repair-fts path (#2374) The repair-fts error interpolates the classified remedy, but no test reached the missing_dependency branch — only the corrupt/invalid-ELF path. Add a Windows error-126 case asserting the thrown error carries the VC++ redistributable remedy and omits the old "retry the network install" tail, and that no index is dropped. Addresses PR #2383 review finding F6a (--repair-fts surface). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(fts): share the VC++ redistributable install hint (#2374) The Microsoft Visual C++ redistributable name and aka.ms URL were duplicated verbatim in WINDOWS_MISSING_DEPENDENCY_REMEDY and STRUCTURAL_MISSING_DEPENDENCY_REMEDY. Factor a single VC_REDIST_INSTALL_HINT constant so the pointer cannot drift between them; the composed remedy strings are byte-identical (existing exact-text assertions unchanged). Also adds a test covering the previously-unexercised structural remedy branch. Addresses PR #2383 review finding F5a. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(fts): guard FILE_CORRUPTION_SIGNATURES parity with the installer script (#2374) The corruption-signature list is deliberately duplicated between extension-load-error.ts and scripts/install-duckdb-extension.mjs (the .mjs cannot import the .ts), with nothing guarding against drift — a one-sided edit would desync the FORCE-INSTALL verb from remedy classification. Export the array from both and add a parity test that compares regex source + flags element-wise. Addresses PR #2383 review finding F5b. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(test): run extension-binary-real in the sequential lbug-db vitest project (#2374) extension-binary-real.test.ts imports @ladybugdb/core but ran in the parallel `default` project, contrary to TESTING.md's rule that native-LadybugDB tests live in the sequential `lbug-db` project. Add it to the lbug-db include list and the default exclude list; it now runs under lbug-db and no longer under default. Addresses PR #2383 review finding F6c. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(fts): fail loud, not silent-skip, on missing FTS artifacts under REQUIRE_FTS=1 (#2374) The real-binary structural tests gated on raw .skipIf(!lbugNative) / .skipIf(!installedFts), so under GITNEXUS_REQUIRE_FTS=1 a missing artifact would silently vanish from a green CI run (the #2299 trap). These tests inspect the extension file directly and need its path, not a loaded connection — so skipUnlessFtsAvailable (which needs an initialized LadybugDB) does not fit. Add requireFtsResourceOrSkip: skip gracefully offline, throw under REQUIRE_FTS=1. The always-on process.execPath assertion still runs everywhere. Addresses PR #2383 review finding F6d. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(fts): apply prettier formatting to the #2383 fix files (#2374) Line-wrapping only; the quality/format CI check flagged three files. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Gergő Magyar
|
177bbc89c3
|
fix: surface real FTS extension LOAD errors and self-heal broken extension files (#2374) (#2375) | ||
|
Gergő Magyar
|
35ebe37c42
|
fix(deps): pin Ladybug 0.18.0, validate the multi-writer deadlock fix (#2340)
* chore(deps): bump @ladybugdb/core to 0.18.0 Pins the release containing LadybugDB/ladybug#605 (TransactionManager lock-order-inversion deadlock fix). Checked for known post-release regressions specific to 0.18.0 via the Ladybug issue tracker — none found. * fix(lbug): re-validate version-coupled comments and regexes for 0.18.0 Extends the LADYBUGDB-CONTRACT re-validation to two spots the marker convention doesn't catch (bridge-db.ts's LBUG_OPEN_RETRY_PATTERNS, conn-lock.ts's serialization rationale). Confirms via upstream source diff (v0.16.1..v0.18.0) that every matched error-text string is unchanged; conn-lock.ts's rationale is unaffected by #612/#623 since neither addresses concurrent queries on one connection. Adds a stemmer-sweep test proving the bundled 0.18.0 FTS extension accepts every entry in SUPPORTED_FTS_STEMMERS, not just the default porter. A live-trigger test for isMissingShadowSidecarError was attempted but abandoned after empirical probing showed it isn't reliably reproducible (even a SIGKILL-simulated crash didn't reproduce the error on reopen) — documented as inspection-verified instead of overclaiming test coverage that doesn't exist. * test(lbug): add concurrent multi-connection deadlock stress test (#2338) Directly validates LadybugDB/ladybug#605 — the TransactionManager lock-order-inversion deadlock between a commit()-triggered checkpoint and a concurrent beginAutoTransaction() — under a shape close to GitNexus's real concurrent-writer load, independent of conn-lock.ts's app-level serialization. Comparison run against 0.17.1 (pre-fix): 1 of 4 runs hung for the full 60s timeout, a direct reproduction of the deadlock. 9 consecutive runs against 0.18.0 (post-fix) all passed cleanly. Production is unchanged — conn-lock.ts still serializes every write; this test validates the engine-level fix without shipping multi-writer as a default. * fix(test): address code review findings in multiwriter deadlock test - Reuse lbug-config.ts's createLbugDatabase (via GITNEXUS_WAL_CHECKPOINT_THRESHOLD) instead of a hand-duplicated 9-arg raw constructor call whose stated justification (needing to bypass createLbugDatabase for the threshold override) was incorrect — the env var already provides it. - Close every QueryResult via the existing closeQueryResults helper (write loop, read loop, verify query, setup query) instead of leaking native cursors, matching lbug-adapter.ts's established pattern. - Move all cleanup (timers, connections, db close, env var restore) into the outer finally block so it runs on every exit path, not just the happy path — a timeout or a writer exhausting its retry budget no longer leaves dangling timers/connections/abandoned query loops. Verified: 8 consecutive runs after the refactor, all passing cleanly. Found via 8-angle parallel code review (medium effort); the two other findings (isDbBusyError not recognizing LadybugDB's 'Only one write transaction' message, and shadow-file poll timing sensitivity) are noted in the PR description as residual — the first is a production-code change beyond this validation test's scope, the second is inherent to observing a transient native sidecar file and not cleanly fixable without overengineering. * fix(test): apply ce-code-review autofix findings Fixes from an 8-persona parallel review round (correctness/testing/ maintainability/project-standards/reliability/adversarial/agent-native/ learnings): - Extract the duplicated skipUnlessFtsAvailable/FTS_UNAVAILABLE_NOTE helper (previously copy-pasted between lbug-core-adapter.test.ts and fts-stemmer-sweep.test.ts) into a shared test/helpers/fts-availability.ts. - Fix a native connection leak: verifyConn in the deadlock test's final verification block is now pushed into the readers array the outer finally already closes, so it's cleaned up even if the count query throws. - Fix a latent TypeScript type error (tsconfig.test.json catches it, tsconfig.json doesn't): conn.query() types as QueryResult | QueryResult[]; narrow to the single-result case before calling .getAll() rather than assuming the array branch never happens. - Replace repeated inline InstanceType<typeof import(...)> expressions with local LbugDatabase/LbugConnection type aliases. Verified: 12 consecutive runs of the deadlock test all pass, full lbug-db project (336 tests) green. Cross-reviewer-confirmed but left as residual (design judgment calls, not mechanical fixes) for the PR description: isDbBusyError doesn't recognize LadybugDB's 'Only one write transaction' message (pre-existing production gap, confirmed independently by 3 reviewers); the deadlock test's timeout path doesn't cancel in-flight writer/reader loops before closing connections; the reader loop has no bounded retry for transient errors during the race window; pinning @ladybugdb/core with a caret range trades automatic patch updates for less re-validation certainty. * docs: trim task-referencing JSDoc artifacts, add operator notes The U2 re-validation pass left verbose 'Re-validated on the 0.17.0->0.18.0 bump (#2338): ...' paragraphs stacked onto 5 production files' docstrings, alongside the already-updated version numbers. That narrative (SIGKILL-probe methodology, diff commands run, issue cross-references) belongs in the PR description, not in code comments that will accumulate a new paragraph on every future bump and confuse readers who just want the current fact. Trimmed each to state only the durable, current-state fact: - lbug-config.ts, sidecar-recovery.ts, lbug-adapter.ts, bridge-db.ts: dropped the bump-narrative paragraphs; kept only genuinely durable notes (e.g., which matchers are inspection-verified vs live-tested, what upstream wording changed). - conn-lock.ts: compressed a 12-line, 3-issue-number enumeration into 2 lines stating the current conclusion (no upstream 0.18.0 fix addresses the same-connection-concurrent-query risk this lock guards against). Also added operator-facing notes to GUARDRAILS.md and RUNBOOK.md's existing 'LadybugDB lock' sections: an isDbBusyError gap found during this validation (LadybugDB's 'Only one write transaction...' message isn't recognized by our busy/lock retry matcher) means that specific error can surface unretried. Documented so it's recognized as the same single-writer conflict, not a new failure mode. * refactor(test): use gitnexus-shared's withRetry in multiwriter deadlock test Replaces the hand-rolled writeWithRetry/sleep loop with the existing gitnexus-shared retry helper (already used by embeddings/hf-env.ts) instead of duplicating the pattern. * fix(test): guarantee non-zero retry delay in deadlock test's writer loop withRetry's isRetryable previously returned {retry: bool} with no afterMs, so computeBackoffMs's exponential-jitter formula gave a deterministic zero-delay on the first retry (floor(random()*1) is always 0 at attempt=0). This contradicted the file's own documented tuning, which specifically needs a non-zero 1-3ms delay to avoid tripping a different native guard. Return an explicit afterMs override on the retryable branch instead. * docs(test): remove dangling doc references from deadlock test JSDoc The JSDoc pointed to a local-session-only docs/plans/2026-07-01-001-... path (docs/ is repo-gitignored, so this never existed for anyone but the implementing session) and to "the PR description" as a source of truth that stops being current once the PR merges. Replace both with self-contained prose and durable references (issue/PR numbers, commit SHAs, GUARDRAILS.md/RUNBOOK.md) that stay resolvable after merge. * fix(search): harden SUPPORTED_FTS_STEMMERS against external mutation Type as ReadonlySet<string> to match this codebase's established convention for exported validation allowlists (EVAL_SERVER_TOOLS, STRUCTURAL_LABELS). Type-only change — no behavior change; both the internal .has() check and the sweep test's spread-iterate pattern continue to work unchanged. * docs(guardrails): fold Known-gap note into the LadybugDB Sign's Why label GUARDRAILS.md's own convention is strictly Trigger/Do/Why per Sign entry (stated in the file's header, followed by all 5 other entries). The new isDbBusyError gap note introduced a 4th label; fold it into Why instead, which is what it's actually explaining. * fix(test): run the multi-writer deadlock test on Windows too itLbugMultiwriter mirrored lbug-core-adapter.test.ts's win32 skip, but that pattern exists for a close-then-reopen-same-path lock lingering bug (kuzudb/kuzu#3872). This test never reopens the database — it holds connections open for the whole run — so the skip excluded the one test validating issue #2338's deadlock fix from the platform conn-lock.ts actually ships native bindings for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Gergő Magyar
|
400cc6a440
|
feat(search): add opt-in CJK bigram segmentation for FTS search (#2339) | ||
|
Parafee41
|
316aaed928
|
fix(indexing): keep full text file content searchable (#2323)
* fix(indexing): keep full text file content searchable * fix(indexing): flush CSV chunks by byte size * fix(fts): flatten newlines/tabs in indexed content so multiline files are searchable (#2317) The end-to-end FTS test (review follow-up F2) exposed that removing the 10KB cap alone does NOT fix #2317: Ladybug's FTS tokenizer splits ONLY on the space character — \n, \r, and \t are not delimiters. So multiline file/symbol content indexes as a few giant cross-line tokens that no word query matches; full content is stored but stays unsearchable. (Verified: identical 8KB content is fully searchable when space-separated and entirely unsearchable when newline-separated.) The existing fts-description-search test never caught this because all its seed content is single-line. Collapse \r\n\t -> single space in the FTS-indexed text (extractContent's File and snippet content, plus the description column) via normalizeFtsText. This rewrites the stored column too, so File content returned via the graph API is space-flattened — an accepted trade for making file/symbol text searchable. Add the real end-to-end guard test/integration/fts-fullfile-search.test.ts: write a >16KB file, load it through the real streamAllCSVsToDisk -> COPY -> createSearchFTSIndexes path, and assert searchFTSFromLbug returns a needle past 10KB (plus a short-content no-regression and a stored-cell-not-truncated guard). It drives the COPY path a Cypher-seed test would bypass, reusing withTestLbugDB's FTS-availability gating via a new before-FTS load hook. * docs(lbug): note the deliberate File-unbounded / snippet-capped asymmetry The File branch returns full content (whitespace-normalized for FTS, bounded upstream by the walker cap) while the symbol snippet path 11 lines down stays MAX_SNIPPET-capped. Comment the intent so the uncapped File branch doesn't read as a forgotten guard. No behavior change. * test(lbug): update #2203 overlap round-trip for FTS whitespace normalization The newline/tab→space normalization (a170915a, #2317) flattens stored File content, so the #2203 overlap test's "File content == original multiline source" assertion no longer holds. The test's actual invariant — overlap path == serial path, byte-for-byte — is unchanged and still asserted; BasicBlock text (not FTS-indexed) still round-trips raw. Update only the File-content expectation to the whitespace-flattened form and document why. * fix(lbug): collapse CSV flush to a single byte threshold BufferedCSVWriter flushed on row-count (FLUSH_EVERY=500) OR byte-count (FLUSH_BYTES=8MB) — two independent triggers for one job. Byte count is the only one tied to the actual risk (an unbounded buffer.join('\n') string), so drop FLUSH_EVERY and make shouldFlushCSVBuffer single-arg. Rather than tune FLUSH_BYTES by guesswork or expose it as an env knob, derive its safety margin from constants the codebase already hard-enforces: a single row is capped at TREE_SITTER_MAX_BUFFER (32MB, clamped regardless of GITNEXUS_MAX_FILE_SIZE) and at most doubled by escapeCSVField's quote-escaping, so the worst-case joined chunk (FLUSH_BYTES + 2 * TREE_SITTER_MAX_BUFFER ≈ 72MB) sits >7x under Node's MAX_STRING_LENGTH (~512MB) — the ceiling that throws RangeError: Invalid string length. A new test pins that margin numerically so it can't erode unnoticed, which covers the "configurable" alternative better than a knob would: there's no evidence any deployment needs a different value, and an unbounded env var would let an operator silently walk the margin back into the danger zone. Also updates the two tests tied to the removed row-count path: the FLUSH_EVERY-boundary integration test now crosses FLUSH_BYTES with real oversized File content instead of relying on row count, and the shouldFlushCSVBuffer unit test drops to the new single-arg signature. --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> |
||
|
Gergő Magyar
|
e148bc089a
|
fix(group): replace LadybugDB-incompatible multi-label Cypher (#2325) (#2327)
Some checks are pending
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Scorecard / Scorecard analysis (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
* fix(group): use labels(n) IN allowlist instead of LadybugDB-incompatible multi-label Cypher (#2325) manifest-extractor and http-route-extractor built Cypher with the openCypher label disjunction `MATCH (n:A|B|C)`, which LadybugDB's parser rejects. The error was swallowed by try/catch, so manifest contracts silently fell back to synthetic UIDs with empty filePath and http-route cross-file handler resolution silently returned null. Replace all 7 queries with `MATCH (n) WHERE labels(n) IN [...]`. LadybugDB returns labels(n) as a single string, so this is an exact allowlist — a 1:1 behavior-preserving syntax translation (validated against LadybugDB 0.17.1). Export the two http-route query constants so integration tests can run the exact production strings against a real DB, and add per-branch real-DB regression coverage (the bug shipped because no test exercised these queries). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(group): import CypherExecutor from contract-extractor in #2325 test The new manifest regression test imported `CypherExecutor` from `group/types.js`, which does not export it — the type is defined only in `group/contract-extractor.js` (as all production extractors import it). This was a real TS2305 under `tsc -p tsconfig.test.json`, masked from CI because the default tsconfig excludes `test/` and `import type` is erased at runtime. Split the import so the type resolves from its real module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(group): run #2325 native-LadybugDB tests in the lbug-db project Per TESTING.md, every test that opens a real `@ladybugdb/core` handle must be registered in the sequential `lbug-db` Vitest project (and excluded from `default`) to avoid native-mmap file-lock conflicts across parallel forks on Windows. The two new group integration tests use `withTestLbugDB`/pool-adapter but were in neither list, so they ran under the parallel `default` project. Add both to `lbug-db.include` and `default.exclude`, matching every sibling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(group): export custom-contract resolve query for #2325 test The #2325 integration test hand-copied the 21-label `custom`-branch resolve query into a local `LABELS_CUSTOM_QUERY` constant, so editing the production allowlist would silently desync the canary. Promote the query to an exported `CUSTOM_CONTRACT_RESOLVE_QUERY` (mirroring http-route-extractor's exported query strings) and import it in the test, so the canary always runs the exact production query. Behavior unchanged — same query string. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(group): de-brittle the #2325 custom-query label assertion The unit test asserted a fixed 7-label ordered substring of the 21-label custom-branch allowlist, coupling it to label order and no-space formatting — a harmless reorder would have broken it. Replace with order/spacing-tolerant membership checks for a spread of individual labels, keeping the unconditional `not.toContain('Function|Method')` guard as the real regression check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(group): correct #2325 http-route docstring + add real-trigger canary The http-route test claimed `MATCH (n:Function|Method|CodeElement)` "which LadybugDB rejects" — but that 3-label disjunction actually PARSES. Verified against the real parser, the genuine #2325 trigger is a *reserved-keyword* label in the disjunction: `Macro` and `Union` both are, and only the manifest custom branch (21-label list) and the lib branch (missing `Package` table) actually threw. The http-route conversion to `labels(n) IN [...]` was a consistency change, not a parser fix. Correct the misleading docstring and add a rejection canary pinned to the real cause (`MATCH (n:Function|Macro|Union)` rejects), so a future query that reintroduces a reserved-keyword disjunction is caught. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(group): cover the thrift package-strip path against a real LadybugDB The thrift-only branch of resolveSymbol strips a `package.` prefix from the service name (`com.example.AuthService` -> `AuthService`) before the Class/Interface lookup — previously exercised only with a mocked executor. Add a service-contract integration case (no method, so it takes the package-strip path, not the grpc-identical method path) that resolves the real `cls:AuthService`. Without the strip the lookup matches nothing and falls back to a synthetic uid, so this is a non-vacuous guard for the strip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(group): drop vestigial 'Package' label from lib contract lookup The `lib` branch allowlisted `labels(n) IN ['Package','Module']`, but there is no `Package` node table (see NODE_TABLES) — the entry only ever matched nothing. Restrict to `['Module']`, the label libraries actually resolve to. Behavior-neutral: the lib integration case still resolves its Module symbol. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(group): update PIPELINE label-scoped queries to labels(n) IN form The resolveSymbol label-scoping bullets still showed the banned `MATCH (n:A|B)` disjunction; a contributor copying them would reintroduce #2325. Rewrite them in the actual `labels(n) IN [...]` form, note the real trigger (LadybugDB rejects a disjunction naming a reserved keyword such as `Macro`/`Union`), and reflect the lib allowlist as `['Module']` after dropping the vestigial `Package` label. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(group): correct #2325 root-cause comments in the extractors The production comments claimed LadybugDB rejects the `MATCH (n:A|B)` disjunction "outright". Verified against the real parser, it rejects only when a label is a reserved keyword (`Macro`, `Union`) or names a missing node table. So only the manifest `custom` branch (reserved keywords in its 21-label list) and the `lib` branch (missing `Package` table) actually threw; the http-route/grpc/thrift/topic disjunctions parse fine and were converted to `labels(n) IN [...]` for consistency and future-proofing, not because they were broken. Rewrite the comments to say so accurately. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(group): make #2325 test prose name the real reserved-keyword trigger The manifest test docstring/title and the unit-test comment said LadybugDB rejects the `MATCH (n:A|B)` disjunction generally. It rejects only when a label is a reserved keyword (`Macro`/`Union`) or a missing table. Reword the docstring (custom + lib branches threw; others parsed), retitle the rejection canary to "its list names reserved keywords Macro/Union", and correct the unit-test comment. The rejection canary still passes — the custom 21-label list does contain Macro/Union. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Gergő Magyar
|
576e81442e
|
fix(search): index description field for FTS so doc comments are keyword-searchable (#2300)
Some checks failed
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Scorecard / Scorecard analysis (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
Devcontainer Smoke / Config-transform unit tests (push) Has been cancelled
Devcontainer Smoke / Build devcontainer image (push) Has been cancelled
* fix(search): index description column for FTS so doc comments are keyword-searchable Closes #2299. descriptionExtractor (#2286) populates the `description` column for every symbol table, but FTS only indexed name+content on 5 tables, so doc-comment keywords (Javadoc/KDoc/godoc/Rust ///) were invisible to BM25 keyword search. - Add `description` to the Function/Class/Method/Interface FTS indexes (File has no description column, left as name+content). - Add FTS indexes for the remaining EMBEDDABLE_LABELS symbol tables (Struct, Enum, Trait, Impl, Macro, Namespace, Constructor, TypeAlias, Typedef, Const, Property, Record, Union, Static, Variable). - createSearchFTSIndexes now drops-then-creates each index so the schema change reaches existing DBs on incremental re-analyze and --repair-fts (createFTSIndex is idempotent-by-name and would otherwise skip stale indexes). Tests: fts-schema column-subset + coverage guards; drop-before-create order; e2e doc-comment keyword search (Java class + Rust struct found by description-only terms). bm25-search assertions derive from FTS_INDEXES. * fix(review): apply autofix feedback - Guard the --repair-fts path on FTS-extension availability before createSearchFTSIndexes drops-then-creates indexes (P1 regression: without the gate, an unavailable extension could drop existing indexes then fail to recreate them, leaving the DB index-less). Mirrors the analyze path's ftsAvailable gate and fails loudly first. - Add a re-analyze upgrade integration test: seed an old name+content-only DB (no Struct index), run the real createSearchFTSIndexes(), and assert description keyword search + the previously un-indexed Struct now resolve. Proves drop-then-create upgrades a live stale index end-to-end. * fix(ci): add loadFTSExtension to --repair-fts test mocks The R3 review fix added a loadFTSExtension availability gate to the --repair-fts path, but run-analyze-fts-repair.test.ts mocked the lbug adapter without that export, so both repair tests threw `No "loadFTSExtension" export`. Add loadFTSExtension to the two mocks (returning true to preserve their original intent) and add a dedicated test proving the guard fails loudly — and does NOT drop any index — when the extension is unavailable. * test(fts): run fts-description-search in the sequential lbug-db project It was the only FTS-index-creating integration test left in the parallel `default` vitest project; every other ftsIndexes-using test (search-core, search-pool, augmentation, …) runs in the `lbug-db` project, which forces fileParallelism: false to avoid LadybugDB native mmap file-lock conflicts in parallel forks (Windows). Add it to the lbug-db include list and the default exclude list to match the convention and remove the flake risk. * test(ci): fail loudly when FTS extension is unavailable, never silently skip FTS-dependent lbug integration suites (search-core, search-pool, augmentation, fts-description-search, …) self-skip via ctx.skip() when the LadybugDB FTS extension can't load, emitting only a console.warn while the job stays green. That means a broken/missing FTS extension in CI would make these integration tests silently vanish with no signal — false confidence. withTestLbugDB now honors GITNEXUS_REQUIRE_FTS=1: when set and the extension is unavailable, setup() throws instead of skipping, so the suite fails loudly. The CI test jobs (ubuntu coverage + windows/macOS cross-platform) set the flag; local/offline runs leave it unset and keep skipping gracefully. (Verified the extension currently loads on all three runners, so this is a guard against regression, not a behavior change today.) * test(ci): run fts-description-search on macOS/Windows cross-platform jobs The new FTS description-search suite was registered in the sequential lbug-db vitest project (ubuntu/coverage) but absent from LBUG_NATIVE, so the macOS/Windows platform-sensitive jobs (which run only the explicit ALL_CROSS_PLATFORM allowlist via run-cross-platform.ts) never executed it. The GITNEXUS_REQUIRE_FTS=1 hardening on those jobs guarded the old FTS fixtures but not the new 20-index/description path. Add the suite to LBUG_NATIVE so the new path is validated cross-platform too. Refs #2299. * fix(search): verify FTS indexes cover description, not just queryability verifySearchFTSIndexes probed each index with QUERY_FTS_INDEX and treated 'queryable' as 'present'. A stale name+content-only index left on a pre-#2299 DB stays queryable yet silently misses the description column, so verification would pass green while doc-comment search stayed broken. Switch to a single CALL SHOW_INDEXES() that exposes property_names per index, and report an index as missing when it is absent OR does not cover its configured columns. Return contract (string[] of table.indexName) is unchanged, so both run-analyze.ts call sites are untouched. The per-index string interpolation is gone, so the now-dead safeIdentifier helper is removed. The real caller of the live function in tests is bm25-search.test.ts (the repair test mocks verifySearchFTSIndexes wholesale); its two probe-shaped cases are rewritten to feed SHOW_INDEXES rows and now assert column coverage, plus an absent-index case. Refs #2299. * test(search): assert description search via the public query surface The #2299 integration suite only exercised the searchFTSFromLbug helper. Add a third block that drives the public LocalBackend.callTool('query') path — which resolves the repo via the registry and routes BM25 through the pool adapter (a different connection context than the core-adapter helper) — and asserts a description-only keyword returns the seeded class. Reuses the existing description-only SEED and production FTS_INDEXES; partial-mocks repo-manager so listRegisteredRepos points at the test DB while cleanupOldKuzuFiles and the rest stay real. Refs #2299. * test(search): make lbug-core-adapter FTS gate honor GITNEXUS_REQUIRE_FTS lbug-core-adapter.test.ts has its own per-test FTS gate (skipUnlessFtsAvailable) that called ctx.skip() whenever the extension could not load — bypassing the GITNEXUS_REQUIRE_FTS=1 hardening that withTestLbugDB already honors. Since this file is in LBUG_NATIVE it runs on the ubuntu/macOS/windows jobs that all set GITNEXUS_REQUIRE_FTS=1, so an FTS regression on a runner would have let these FTS-primitive tests silently vanish from a green run — the exact gap #2299's test-infra hardening set out to close. Make the helper mirror withTestLbugDB: when GITNEXUS_REQUIRE_FTS=1 and the extension is unavailable, throw (hard fail) instead of skipping. Offline/local runs (no env var) still skip gracefully. Refs #2299. |
||
|
Gergő Magyar
|
aa8c567126
|
fix(lbug): stop --pdg analyze double-free (skip LadybugDB close-destructor crash) + harden connection serialization (#2264)
* fix(lbug): serialize singleton connection to stop --pdg analyze double-free
LadybugDB is single-writer and its Connection is NOT safe for concurrent
query execution. The WAL-checkpoint driver (5s setInterval) issued
`conn.query('CHECKPOINT')` on the same module-singleton `conn` the analyze
pipeline used for COPY. With --pdg the extra BasicBlock / REACHING_DEF / CDG /
POST_DOMINATE / TAINTED / CALL_SUMMARY / TAINT_PATH table COPYs outlast the
5s tick, so a checkpoint executed concurrently with an in-flight COPY on one
connection -> two libuv workers mutate shared native state -> heap corruption
("double free or corruption (out)" / SIGABRT, detected at the final
"Saving metadata..." free).
Fix: add conn-lock.ts (`withConnLock`, a promise-chain mutex) and run every
singleton-`conn` helper's full query + result-drain inside it: queryAndDrain
(when targetConn === conn), executePrepared, executeWithReusedStatement,
flushWAL, tryFlushWAL, getLbugStats, deleteAllInterprocTaintPaths,
deleteAllCallSummaries. Add an `if (inflight) return` reentrancy guard to the
driver tick so overdue ticks don't stack checkpoints. streamQuery is
intentionally NOT wrapped (read path, re-entrant per-row callback).
Reproduced the crash with concurrent queries on one raw Connection (serial =
stable); verified the fix drives the same overlap through the locked adapter
without crashing.
Tests: conn-lock serialization (no overlap / FIFO / throw-releases) and
driver reentrancy guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): lock deleteAllCommunitiesAndProcesses against the WAL driver (#2264)
The count + DETACH DELETE ran raw conn.query on the singleton connection during
incremental --pdg writeback while the WAL-checkpoint driver was live — the same
concurrent CHECKPOINT-vs-write double-free this branch fixes elsewhere. Wrap the
body in withConnLock, mirroring the already-wrapped deleteAllInterprocTaintPaths.
Adds test/integration/lbug-conn-serialization.test.ts (call-through withConnLock
spy) asserting the helper now acquires the lock, wired into the lbug-db vitest
project (and excluded from the default project so it doesn't run twice).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): lock queryImporters against the WAL driver (#2264)
queryImporters issued a raw conn.query on the singleton connection inside the
importer-BFS loop of incremental --pdg writeback, while the WAL-checkpoint driver
could fire a concurrent CHECKPOINT — the same double-free class. Wrap the read
(query + getAll + drain) in withConnLock.
Extends lbug-conn-serialization.test.ts with a routing assertion.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): lock deleteNodesForFile count query on the singleton path (#2264)
The per-table count read used a raw targetConn.query while the sibling DETACH
DELETE already routed through the locked queryAndDrain — an asymmetry that left
the count racing the WAL-checkpoint driver during incremental --pdg writeback.
Gate the count through withConnLock when targetConn === conn (the singleton),
matching queryAndDrain; per-query/temp connections stay lock-free.
Test asserts the count loop takes the lock once per filePath-bearing node table.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): drain DELETE results in the deleteAll* helpers (#2264)
deleteAllInterprocTaintPaths, deleteAllCallSummaries, and
deleteAllCommunitiesAndProcesses awaited conn.query(...DELETE...) but dropped the
returned QueryResult (only the count result was closed), leaking a native result
handle and violating the helpers' own "query + drain inside the lock" contract.
Close each delete result via closeQueryResults, matching the count handling.
Adds a seeded drain test (closeQueryResults fires for the DELETE result).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): make the conn-lock non-reentrancy invariant enforced, not just documented (#2264)
A future withConnLock-wrapped helper calling another wrapped helper would await
its own holder's tail and hang silently. Add an AsyncLocalStorage-based re-entry
guard: withConnLock throws a clear error when invoked from within a holding fn's
async context. A boolean flag can't do this — a legitimately-queued top-level
caller also runs while the lock is held; only AsyncLocalStorage distinguishes a
true nested call from normal contention.
Tests: re-entry throws (not deadlocks); sequential and concurrent top-level calls
do NOT false-fire; the lock releases after a re-entry throw.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* refactor(lbug): rename __resetConnLockForTests to _resetConnLockForTests (#2264)
Match the repo's single-underscore test-seam convention (_initLockPathForTest).
Pure rename of the @internal export and its sole importer.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* test(lbug): fix stale CHECKPOINT guard regex after the c.query refactor (#2264)
lbug-checkpoint.test.ts asserted exactly two CHECKPOINT sites by grepping the
literal `conn.query('CHECKPOINT')`. The connection-serialization refactor changed
flushWAL/tryFlushWAL to capture `const c = conn` and call `c.query('CHECKPOINT')`
inside withConnLock, so the literal grep found 0 and the test failed (expected 2).
Make the regex receiver-agnostic (`.query('CHECKPOINT')`) — preserves the guard's
intent (exactly two authorized CHECKPOINT sites; a third is a regression) while
tolerating the captured-receiver form.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): skip native close on CLI exit to dodge LadybugDB destructor double-free (#2264)
THE actual fix for the `analyze --pdg` crash. gdb shows the abort is a double-free
inside LadybugDB's own destructor during conn.close():
"double free or corruption (out)" -> abort
lbug::main::ClientContext::~ClientContext()
lbug::main::Connection::~Connection()
NodeConnection::Close(...) <- conn.close() from safeClose
It reproduces with the WAL driver OFF and with serial load, so it is NOT the
checkpoint/COPY concurrency the rest of this branch serialized — it's a native
LadybugDB engine bug (@ladybugdb/core 0.17.1, latest stable) triggered by the
larger --pdg write set, firing during teardown AFTER a fully-written, checkpointed
index.
Fix: closeLbug({ skipNativeClose }) CHECKPOINTs for durability (flushWAL) then
skips conn.close()/db.close(), leaving the handles referenced so no GC finalizer
re-runs the destructor. The CLI analyze command (success, error, and SIGINT paths
all process.exit) opts in via skipNativeCloseOnExit; long-lived callers (MCP
server, tests) keep the real close. Mirrors the pool adapter's fire-and-forget
native close and the ONNX native-cleanup philosophy.
Validated end-to-end: `analyze --pdg --force` now exits 0 with a 193,876-node
index; re-opening it (no --force) reads clean and reports up-to-date, proving the
CHECKPOINT-only persistence is durable without db.close().
Workaround for an upstream LadybugDB bug (ClientContext destructor double-free).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): keep conn.close()/db.close() literals out of the closeLbug comment (#2264 review P1-1)
The skipNativeClose comment in closeLbug contained the literal `conn.close()`/
`db.close()`, which the structural guard test (lbug-checkpoint.test.ts:52-53 —
"closeLbug must not inline conn.close()/db.close()") greps for and fails on.
Reword the comment to describe the native close without the literal tokens; the
code already delegates close exclusively to safeClose, so the guard's intent holds.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): real close on the analyze error path to avoid a hang under skipNativeClose (#2264 review P1-2)
The CLI error handler soft-returns (process.exitCode = 1) instead of forcing
exit, relying on the released native handles to let Node terminate. The earlier
commit made runFullAnalysis's error-path closeLbug skip the native close, leaving
live LadybugDB handles that keep the event loop alive forever — a post-init
analyze failure would hang. Only the SUCCESS path (which guarantees a following
process.exit) skips the native close; the error path now always closes for real.
A late-error close could still abort in the destructor, but that terminates the
process — it does not hang.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(server): skip native close in the analyze worker to avoid the LadybugDB destructor crash (#2264 review P2-3)
The forked server analyze worker runs runFullAnalysis then force-exits
(process.exit(0)). With a real native close inside runFullAnalysis, the LadybugDB
ClientContext destructor can double-free after --pdg writes and abort the worker
BEFORE it sends 'complete', failing the parent's analyze. Pass
skipNativeCloseOnExit: true so the worker checkpoints for durability and lets its
process.exit reclaim the handles — same about-to-exit contract as the CLI.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(cli): force exit on a soft error-return when LadybugDB handles are open (#2264 review P1)
The full-analysis success path skip-closes LadybugDB (handles left open, reclaimed
by process.exit). If a post-finalize step (assertAnalysisFinalized) then throws,
the outer catch soft-returns (process.exitCode = 1) — and with native handles
open the event loop never drains, so the process HANGS instead of exiting 1.
Guard once at the analyzeCommand wrapper, after the try/finally: if isLbugReady()
(handles still open) the analyze actually ran and we must force the exit. The
success path never reaches here (analyzeCommandImpl process.exit(0)s itself);
early-validation errors and unit tests that mock runFullAnalysis never open the DB
(isLbugReady() false), so the soft return is preserved.
Adds analyze-finalize-failure-exits.test.ts (force-exits when handles open; does
NOT when they aren't). The analyze-*.test.ts that mock lbug-adapter now also mock
isLbugReady (vitest throws on accessing an undefined export of a mocked module).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): skip the native close on the analyze error path too (#2264 review P2)
A real conn.close() on the error path after large --pdg writes can itself hit the
LadybugDB ClientContext destructor double-free → SIGABRT, degrading an actionable
exit-1 error into a raw native abort. Switch the error-path close to
skipNativeClose (mirroring the success path). Safe now that the CLI catch
force-exits when isLbugReady() (the prior commit): handles left open are reclaimed
by that guaranteed process.exit, so the process terminates without the abort and
without hanging. flushWAL keeps the partial index durable.
Depends on the prior commit (CLI force-exit guard).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): run loadCachedEmbeddings reads under withConnLock (#2264 review P2)
loadCachedEmbeddings issued raw conn.query reads on the singleton connection
outside withConnLock — safe today only because it runs before the WAL-checkpoint
driver starts, an ordering invariant not enforced by code. Wrap the whole read in
withConnLock so a future reorder can't race a CHECKPOINT on the connection. Leaf
read; no nested wrapped helpers.
Adds a routing assertion to lbug-conn-serialization.test.ts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* test(lbug): de-brittle the close/CHECKPOINT structural guard (#2264 review P2)
lbug-checkpoint.test.ts grepped the adapter SOURCE (comments included) for
conn.close()/db.close()/.query('CHECKPOINT') literals, coupling a passing test to
comment wording — a prior commit had to reword a comment just to keep it green.
Strip comments from the read source before the structural assertions so they
reflect code only; the invariant (exactly two CHECKPOINT sites; close calls only
in safeClose) is preserved and no longer breaks on a comment edit.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* refactor(lbug): rel COPY uses the captured writeConn, matching node COPY (#2264 review P3)
The relationship COPY passed the module-level `conn` to copyCsvWithRetry while the
node COPY uses the captured `writeConn`. Use `writeConn` for both — one captured
reference for the whole bulk load, removing the latent identity dependency. Same
object during analyze (`conn` is only reassigned at open/close under the session
lock), so the queryAndDrain `targetConn === conn` lock gate still engages.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(server): make the analyze worker's IPC send() failure-safe (#2264 review P3)
The worker's send() used `process.send?.(msg)` — the `?.` guards an undefined
channel but not a throw from an already-closed one (ERR_IPC_CHANNEL_CLOSED). A
throw in the catch-branch send() would escape the message handler and skip the
scheduled `setTimeout(process.exit(0))`, stranding the worker (with skip-close
leaving native handles open, #2264). Wrap process.send in try/catch so the exit
always fires; a vanished child is a failure to the parent regardless.
Not unit-tested: send() is module-private and importing the worker registers
process signal handlers; the change is a defensive try/catch around one call.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(cli): bound the SIGINT cleanup CHECKPOINT so Ctrl-C stays responsive (#2264 review P3)
The SIGINT handler calls closeLbug({skipNativeClose:true}), whose flushWAL
CHECKPOINT queues behind the connection lock held by an in-flight COPY — so a
single Ctrl-C during a long --pdg COPY appeared hung until the COPY released.
Race the cleanup against a 2s timeout before process.exit(130); the WAL replays
on the next analyze. The double-Ctrl-C escape hatch is unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(server): report analyze-worker errors over IPC, never swallow (#2264 P3)
The worker's send() swallowed IPC failures (and a prior pass logged them to
stderr). Per review, all worker errors must be reported back to the parent over
the existing IPC channel (send({ type: 'error' })) and nothing silently dropped.
- send() no longer catches: a dead channel (ERR_IPC_CHANNEL_CLOSED) throws
instead of being swallowed.
- Every handler (uncaughtException, unhandledRejection, SIGTERM, the analysis
message handler) reports its error via send() in try and schedules process.exit
in finally, so a throw from send() can no longer skip the exit and wedge the
worker — the P3 'schedule the exit so it always fires' fix, without a swallow.
- SIGTERM cleanup failures are now reported to the parent instead of an empty
catch {}.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(workers): report every caught parse-worker error over IPC (#2264)
The parse worker swallowed or only-locally-logged several caught errors, so they
never reached the pool: the per-language-group catch was an empty catch {} that
silently dropped the whole group on any throw (not just an unavailable grammar),
the per-file parse/query-execution catches only logger.warn'd (worker-thread
local), and the C++ template-constraint catch swallowed silently.
Route all work-path catches through a new reportWarning() helper that posts
{ type: 'warning', message } to the pool (which logs it on the main thread AND
resets the worker idle timer, so a worker grinding through failing files isn't
falsely idle-evicted), with a logger.warn fallback for the non-worker path. The
existing inline warning sites (query-compilation, the extractParsedFile callback,
CFG build) are migrated to the same helper.
The 4 optional-grammar module-load guards (Swift/Dart/Kotlin/C) stay silent: they
run before the 'ready' handshake and their absence is already surfaced via
result.skippedLanguages + the isLanguageAvailable gate. Fatal/group-aborting
errors continue to flow through the message handler's { type: 'error', errorStack }.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* test(cli): harden finalize-failure test against forked-worker death (#2264)
analyzeCommand calls installFatalHandlers(), which registers global
unhandledRejection/uncaughtException handlers that call the REAL process.exit(1).
Across this file's vi.resetModules() reimports they accumulate on `process`, and
under CI timing a stray async rejection fired one while no process.exit spy was
active — killing the forked vitest worker ("Worker exited unexpectedly"), which
only surfaced once the full test lanes finished (they were pending at review time).
Keep process.exit spied for the whole file (beforeAll/afterAll) so a fatal handler
can never really exit mid-run, strip the handlers installFatalHandlers added in
afterAll (preserving vitest's own, snapshotted up front) before restoring the real
process.exit, and reset process.exitCode so the worker exits clean. Passes in
isolation and grouped; behavior under test is unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(analyze): don't take the up-to-date fast path for an unregistered repo (#2264)
A prior 'analyze --name X' that hit a registry name collision writes meta.json
(meta-save runs before registerRepo) but fails before registering — leaving the
index up-to-date but UNREGISTERED. A later 'analyze --name X --allow-duplicate-name'
then matched the up-to-date gate and early-returned WITHOUT registering, so the
repo stayed invisible to list_repos/MCP and the CLI's assertAnalysisFinalized
rejected it. --allow-duplicate-name could never heal it.
This was latent on main, masked by the very close-hang this PR fixes: the lingering
process pushed the cli-e2e #829 step-3 analyze past its 60s spawn timeout
(status===null → the test's vacuous early-return). With the hang gone the analyze
exits promptly, exit 1 surfaces, and the bug becomes deterministic on all platforms.
Fix: the up-to-date fast path now short-circuits only when the repo is actually
registered (new isRepoRegistered helper, sharing assertAnalysisFinalized's exact
canonical/case-folded membership check). An indexed-but-unregistered repo falls
through to the pipeline, which registers it honoring allowDuplicateName. Already
registered repos keep the fast path unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* chore: trigger CI re-run
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(analyze): gate up-to-date self-heal on --allow-duplicate-name (#2264)
The prior commit healed every up-to-date-but-unregistered repo by falling through
to register it — which broke the #1169 guard: a plain `analyze` of an up-to-date
repo whose registry entry is missing MUST fail loudly ("Analysis did not finalize")
rather than silently register a possibly half-finalized index.
Distinguish the two causes of "unregistered":
- collision-rejected + user re-runs with --allow-duplicate-name → explicit intent
to register, so fall through to the pipeline and register it (#829).
- plain analyze, registry missing/wiped → keep the #1169 fail-loud behavior.
So self-heal is gated on options.allowDuplicateName; isRepoRegistered is only read
on that opt-in branch, so the common fast path keeps its single-stat cost. Both
cli-e2e guards (#1169 fail-loud, #829 heal) now pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(server): skip the native close on analyze-worker SIGTERM cancellation (#2264 P2)
cancelJob() / the 30-min timeout (analyze-job.ts) send SIGTERM to the forked
analyze worker, but its SIGTERM handler still did a full `await closeLbug()`
(native conn/db teardown) — even though normal completion now skips it via
skipNativeCloseOnExit. A cancelled or timed-out --pdg server analyze could
therefore still hit the LadybugDB ClientContext destructor double-free, or block
behind the in-flight COPY's connection lock before exiting.
Mirror the CLI SIGINT path: a best-effort CHECKPOINT with
closeLbug({ skipNativeClose: true }) bounded by a 2s Promise.race timeout, then
process.exit(0) (which reclaims the handles). A CHECKPOINT failure is reported to
the parent over IPC rather than swallowed; the exit is in .finally so it always
fires.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* test(cli): import analyze once in the finalize-failure test (#2264 CI)
The test failed deterministically only on the ubuntu coverage lane (2/2 runs)
while passing locally and in isolation, incl. with --coverage. Cause: the
vi.resetModules() + per-test `await import('analyze.js')` re-instrumented the
ENTIRE analyze module graph on every test; under --coverage on the
memory-constrained CI runner that OOM/crashed the forked worker ("Worker exited
unexpectedly" → the assertion never ran).
Import analyzeCommand ONCE and drive the mocks per-test via mockReturnValue
(resetModules wasn't needed — the hoisted mocks are controllable per-test). Keeps
the whole-file process.exit spy + afterAll fatal-handler strip from the prior pass.
Behavior under test is unchanged; passes in isolation and with --coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* test(cli): pre-set NODE_OPTIONS heap cap so ensureHeap can't re-exec (#2264 CI)
Root cause of the ubuntu-coverage-only failure (3/3 CI runs, passing locally):
analyzeCommand calls ensureHeap() (analyze.ts:715), which RE-EXECS the process —
spawning `node <heap-flags> <argv>` with vitest's argv — unless NODE_OPTIONS
already carries --max-old-space-size (analyze.ts:498). That re-exec killed the
forked vitest worker ("Worker exited unexpectedly" → the assertion never ran).
It only reproduced on the memory-constrained CI runner because locally a high V8
heap-size-limit also short-circuits ensureHeap (analyze.ts:501).
Reproduced locally with NODE_OPTIONS="--no-warnings" (no heap cap) → same failure;
fixed by pre-setting --max-old-space-size in beforeAll (restored in afterAll), the
same workaround cli-e2e uses. Verified: passes under the repro condition, normally,
and with --coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(server): worker asserts finalization before reporting complete (#2264 P2)
The forked analyze worker reported {type:'complete'} straight after runFullAnalysis,
so a server/web analyze of a half-finalized repo (meta.json written but the global
registry entry missing — a prior collision-aborted run, or a wiped registry) was
reported successful while the repo stayed unregistered/invisible to list_repos. The
CLI already guards this with assertAnalysisFinalized; the worker did not.
Extract the run -> finalize -> report contract into a side-effect-free
analyze-worker-core seam (the entry module's top-level process.on handlers make it
untestable directly) and call assertAnalysisFinalized before sending complete — a
failure is reported as {type:'error'} instead of a false success. The seam is
dependency-injected and unit-tested with fakes; the entry module wires the real deps
and keeps owning process.exit.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(server): coordinate worker SIGTERM cancellation with completion (#2264 P3)
The worker SIGTERM handler unconditionally sent {type:'error','Analysis cancelled'}
and didn't coordinate with the message handler that sends complete, so a cancel near
the finish line could report a cancelled job complete, or a late SIGTERM could flip
an already-complete job to failed.
Add a single terminal-outcome claim (createTerminalClaim) shared by the message
handler and the SIGTERM handler: whoever claims it first reports its terminal
message; the other skips its terminal send. Single-threaded JS makes the
check-and-set atomic. The cleanup + process.exit still run regardless.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(server): make a job's terminal outcome immutable on the parent side (#2264 P3)
Defense-in-depth complement to the worker terminal-claim: the launcher's message
handler and the job manager's updateJob both lacked a terminal-state guard, so a
late worker IPC message (a SIGTERM-driven 'error' after 'complete', or vice versa)
could re-release the repo lock and flip the reported status. (Touches parent-side
files outside the original PR diff — deliberate, clearly-scoped.)
- analyze-job.ts updateJob: drop any update once the job is already terminal (the
transition INTO terminal still applies, since status isn't terminal yet then).
- analyze-launch.ts message handler: return early when the job is already terminal,
mirroring its sibling exit handler.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* test(lbug): assert deleteAllInterprocTaintPaths + deleteAllCallSummaries route through withConnLock (#2264)
The lock-routing suite covered 4 singleton-conn helpers but not these two
withConnLock-wrapped delete helpers (lbug-adapter.ts), which also run during the
incremental --pdg writeback window — so a revert of either wrapper would have gone
uncaught. Add the two routing assertions to complete the coverage the file's header
claims (every singleton-conn helper reachable during the WAL-driver window).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* test(lbug): assert temp-conn deleteNodesForFile skips withConnLock (negative gate, #2264)
The positive case (singleton deleteNodesForFile locks each per-table count) was
covered, but not the negative branch of the targetConn === conn gate: a per-file/temp
connection (dbPath provided) must NOT take the singleton lock, or temp-conn callers
would needlessly contend with it. Add the negative-gate assertion so a regression
that unconditionally locks is caught.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* test(cli): assert the force-exit forwards process.exitCode, not a hardcoded 1 (#2264)
The existing cases asserted process.exit(1), but since the error catch always sets
exitCode=1 they couldn't distinguish forwarding (process.exit(process.exitCode ?? 1))
from a hardcoded 1. Add a case on the alreadyUpToDate path — which returns without
setting exitCode or calling process.exit — with a pre-set exitCode=2 and isLbugReady
forced true, asserting the wrapper force-exits with 2. Proves the exitCode-forwarding
branch.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* refactor(lbug): replace skipNativeClose flag with a dedicated closeLbugBeforeExit() (#2264)
The "skip the native close only when a process.exit is guaranteed to follow"
invariant was enforced by convention across ~4 call sites via a boolean option on
closeLbug — the exact foot-gun the review flagged. Encode the contract in the name
instead:
- New closeLbugBeforeExit() (CHECKPOINT via flushWAL, then return without the native
close); closeLbug() drops the option and is the plain real-close again.
- run-analyze success + error paths: options.skipNativeCloseOnExit ?
closeLbugBeforeExit() : closeLbug(). CLI SIGINT + worker SIGTERM call
closeLbugBeforeExit() directly. skipNativeCloseOnExit stays on AnalyzeOptions as the
caller's "I will exit" signal.
- lbug-checkpoint.test: assert closeLbugBeforeExit exists + has no native close, and
match `closeLbug =` precisely so it doesn't prefix-match the new function.
- Retarget the conn-serialization integration case to closeLbugBeforeExit(); add the
new export to the 12 analyze-*.test.ts lbug-adapter mocks.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* refactor(lbug): extract isSharedSingletonConn predicate for the lock gate (#2264)
The targetConn === conn object-identity gate (decides whether an op takes
withConnLock) was duplicated inline in queryAndDrain and deleteNodesForFile with
its own explanatory comments. Extract a single isSharedSingletonConn(c) predicate
with the rationale in one place; both sites route through it. Behavior unchanged —
covered by the lock-routing tests' positive (singleton locks) and negative
(temp-conn skips) cases.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* refactor(lbug): share the bounded checkpoint-then-exit cleanup (SIGINT/SIGTERM) (#2264)
The CLI SIGINT handler (analyze.ts) and the worker SIGTERM handler (analyze-worker.ts)
had near-identical Promise.race([closeLbugBeforeExit, timeout]).finally(exit) blocks
with separately-hardcoded 2s timeouts. Extract boundedCheckpointBeforeExit into a
shared shutdown-helpers module — parameterized by exit code, an optional flush-error
reporter (worker reports over IPC), and an optional beforeExit hook (CLI flushes the
logger). checkpoint + exit are injectable test seams, so it's unit-tested without the
real LadybugDB close or process.exit.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* refactor(storage): extract registryPathEquals for the registry case-fold compare (#2264)
The Windows case-insensitive / POSIX case-sensitive registry-path comparison was
duplicated across 6 sites (registerRepo dedup, the fresh-merge findIndex,
removeRepo/removeBranchIndex local 'matches' helpers, isRepoRegistered, and the
path-match lookup). Extract a single registryPathEquals(a, b) predicate so every
registry lookup/dedup/finalize check answers identically; route all 6 through it.
No behavior change — repo-manager + finalize-invariant suites pass (incl. the
Windows case-fold case).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* feat(lbug): runtime-guard streamQuery against the WAL-checkpoint driver (#2264)
streamQuery is deliberately not wrapped in withConnLock (its per-row callback can
re-enter the adapter), so its unlocked per-row reads could race a CHECKPOINT on the
shared connection — the corruption window the lock serializes everything else
against. That invariant was comment-only, safe today only because the serve/read
path forks analyze workers. Make it enforced:
- lbug-adapter: a walDriverActive flag + markWalDriverActive(bool); streamQuery
throws an actionable error when the driver is active.
- wal-checkpoint-driver: arm the flag on start, disarm in stop() AFTER the in-flight
CHECKPOINT drains (clearing earlier would briefly allow a race).
A future in-process analyze overlapping a stream now fails loud instead of
corrupting native state. (reentrancy test's lbug-adapter mock gains the new export.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* docs(lbug): explain why closeLbugBeforeExit skips finalizeLbugSidecarsAfterClose (#2264)
Document the deliberate trade-off: the skip-close path intentionally does NOT run
the sidecar-finalize step that safeClose runs after a real close. It's designed for
released WAL handles; running it with the connection still open risks a Windows
file-lock on the in-use WAL. The CHECKPOINT already made the index durable and the
next run's preflightLbugSidecars reconciles residual WAL — the deferral is the
accepted cost of skipping the native close to dodge the destructor double-free.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm
* fix(lbug): move WAL-driver-active flag to its own module (fix mock ripple, #2264)
The streamQuery guard (
|
||
|
Gergő Magyar
|
d4449b4ec8
|
fix(lbug): resolve non-ASCII paths for KuzuDB on Windows (#1811) (#1817)
* fix(lbug): resolve non-ASCII paths to 8.3 short form on Windows (#1811) KuzuDB's native C++ layer uses ANSI file APIs (fopen) on Windows. When the repo path contains CJK or other non-ASCII characters, the UTF-8 bytes from Node.js are misinterpreted as the system's Active Code Page (e.g. GBK), producing a garbled path — "Error 3: The system cannot find the path specified." Add `toNativeSafePath()` which converts non-ASCII paths to their Windows 8.3 short-name form (all-ASCII) before passing them to the native layer. Applied to both the database open path and the COPY CSV paths. No-ops on non-Windows and on all-ASCII paths. Closes #1811 * test(lbug): add unit + integration tests for non-ASCII path handling (#1811) - Unit tests for toNativeSafePath: ASCII passthrough, non-Windows no-op, Windows short-path conversion, nonexistent-path fallback - Integration test: full initLbug + loadGraphToLbug round-trip with CJK characters in the storage path — runs on all platforms - Fix toNativeSafePath to reject cmd.exe output containing '?' chars (replacement for unrepresentable Unicode in the console code page) - Register integration test in vitest lbug-db project and cross-platform-tests.ts matrix * chore(autofix): apply prettier + eslint fixes via /autofix command * feat(lbug): junction fallback, tmpdir CSV staging, pool-adapter coverage (#1811) U1+U4: toNativeSafePath now tries 8.3 short path → NTFS junction fallback → diagnostic warning. Junctions target path.dirname(p) and reconstruct the leaf. Handles EEXIST races. Registers cleanup on exit/SIGTERM/SIGINT. Orphan scan on first call removes stale junctions from prior crashes. U2: loadGraphToLbug redirects csvDir to os.tmpdir() when storagePath contains non-ASCII on Windows, avoiding non-ASCII characters in COPY FROM paths entirely. U3: All 4 createLbugDatabase call sites in pool-adapter.ts now wrap dbPath with toNativeSafePath. * fix(test): fix CI failures from toNativeSafePath addition (#1811) - Fix lbug-non-ascii-path integration test: use CodeRelation (actual relationship table name) instead of CALLS - Add toNativeSafePath to lbug-config.js mocks in pool-wal-recovery and lbug-pool-win-fts-probe tests — pool-adapter now imports it * fix(lbug): sanitize path before cmd.exe shell expansion (CodeQL) Reject paths containing cmd.exe metacharacters (" % | & < > ^) before interpolating into the `for %I` short-path command. Prevents command injection via crafted path names. * fix(lbug): address code review findings in non-ASCII path implementation - U1: Use process.exit(0) on Windows instead of process.kill re-raise (SIGTERM forcefully kills on Windows, handlers never fire) - U2: Pass safePath to openWithLockRetry so sidecar sweep targets the path KuzuDB actually opened, not the original non-ASCII path - U3: Skip junction creation in worker threads (isMainThread guard) to prevent junction leaks from pool-adapter workers - U4: Replace existsSync with lstatSync in orphan scan to avoid 30s blocking on unreachable UNC network targets * chore(autofix): apply prettier + eslint fixes via /autofix command * fix(lbug): correct SIGTERM exit code and run Prettier (#1811) - Use exit code 143 (SIGTERM) / 130 (SIGINT) on Windows instead of 0 so termination is not masked as success - Run Prettier to fix formatting (CI Gate blocker) * fix(lbug): eliminate CodeQL command-injection taint in tryShortPath Pass the path via GITNEXUS_SP environment variable instead of interpolating it into the cmd.exe command string. The FOR loop reads %GITNEXUS_SP% from the environment, so the command text is entirely static — no user-controlled data in the shell command. Also removes CMD_UNSAFE_RE since the env var approach makes character-level sanitization unnecessary. --------- Co-authored-by: Test <test@example.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
Gergő Magyar
|
ac9a2ee12f
|
chore(ci): consolidate parity shards and narrow cross-platform matrix (#1798)
* chore(ci): reduce CI runner-minutes by consolidating parity and narrowing cross-platform
Scope-resolution parity previously spawned 9 separate GitHub Actions jobs
(one per migrated language), each doing full checkout + npm ci + build for
a single test file. Consolidate into one job running scripts/run-parity.ts
which loops through all migrated languages sequentially — same coverage,
~45 fewer runner-minutes of redundant setup per PR.
Cross-platform (Windows/macOS) previously ran the full 373-file test suite.
Narrow to 45 platform-sensitive files (native LadybugDB, process spawning,
path separators, worker threads, filesystem behavior). Full suite still runs
on Ubuntu with coverage.
Also adds 2 missing lbug integration tests (lbug-orphan-sidecar-recovery,
lbug-readonly-init) to the sequential lbug-db vitest project where they
belong, and rewrites TESTING.md to document all test lanes.
* fix: address code review findings on parity and cross-platform scripts
- Capture stderr in run-parity.ts (vitest writes diagnostics to stderr)
- Lower per-invocation timeout from 5min to 60s to stay within CI job limit
- Add --language flag validation (error on missing value)
- Add timeout diagnostic to run-cross-platform.ts catch block
- Add analyze-wal-checkpoint-failure.test.ts to lbug-db sequential project
- Expand cross-platform list: parser-loader, pipeline, pipeline-graph-golden,
setup-skills, cli/tool-no-index-stderr (51 files, was 45)
* fix: add shell:true for Windows npx resolution and simplify fs import
execFileSync('npx', ...) fails with ENOENT on Windows because npx is
npx.cmd — shell:true resolves this. Also replaces dynamic await
import('fs') with static import, and fixes timeout detection to use
err.killed instead of err.code.
* fix(ci): raise parity per-invocation timeout to 120s and job timeout to 30min
TypeScript and C++ resolver tests take 60-90s on CI runners, exceeding
the 60s per-invocation timeout. Raise to 120s. Also bump the job-level
timeout from 25 to 30 minutes for margin (realistic total is ~11 min).
* fix(ci): raise parity per-invocation timeout to 180s for C++ resolver
C++ resolver tests take 130-150s on CI runners due to template
metaprogramming, ADL, and SFINAE fixture volume. 120s was still too
tight. Realistic total across all 9 languages is ~12 min, well under
the 30-min job timeout.
* fix(ci): use stdio inherit for parity — no per-invocation timeout
Switch from piped stdio with per-invocation timeouts to stdio: 'inherit'.
Vitest output streams to CI console in real time, making failures
immediately visible. The CI job-level timeout (30 min) is the only
guard — no more artificial per-invocation timeouts that cut off slow
resolver tests like C++ (which genuinely takes 3+ minutes).
---------
Co-authored-by: Test <test@example.com>
|
||
|
Gergő Magyar
|
1d46200c47
|
fix(lbug): robust Windows lock acquisition for CI integration tests (#1430)
* fix(lbug): robust Windows lock acquisition for CI integration tests
LadybugDB's `new Database()` raises `Could not set lock on file` from
local_file_system.cpp synchronously inside the constructor — before any
query is issued, so `withLbugDb`'s query-time retry never sees it. On
Windows CI this surfaces as flaky integration tests due to AV-scanner
holds, libuv handle-release lag, and stale `.wal` sidecars from aborted
prior runs.
This change closes the gap at *open time*:
- `openLbugConnection` now wraps `new lbug.Database()` in a bounded
busy-retry (5x100ms back-off) inside `lbug-config.ts`. Errors that
exhaust the budget are tagged via `LBUG_OPEN_RETRY_EXHAUSTED` so
`withLbugDb`'s outer 3x retry skips re-retrying a freshly-exhausted
path (eliminates the 3x5=15-attempt / ~6s tail latency).
- For recognized test fixtures only (immediate-parent dir matches a
known prefix AND resolves under `os.tmpdir()`), one final stale-
sidecar sweep removes `.wal`/`.lock` and retries once. Production
paths never enter this branch.
- `safeClose` on Windows runs a bounded `fs.open` probe to absorb
native handle-release lag; logs a warning if the probe exhausts so
operators can spot AV interference.
- `isDbBusyError` is now defined in `lbug-config.ts` as the single
source of truth, re-exported from `lbug-adapter.ts` for compatibility.
- New tests cover open-time retry (happy/retry/exhaust/non-busy/tag),
stale-sidecar sweep (test-fixture-only, production-rejection,
preserves-original-error), `isTestFixturePath` direct unit suite
(accept/reject/traversal/nested/trailing-sep), and
`waitForWindowsHandleRelease` (openable/ENOENT/no-leak).
- The two new test files are added to vitest's existing serialized
`lbug-db` project (already `fileParallelism: false`).
Closes the chronic Windows CI flake on lbug-touching integration tests
while preserving the existing single-writable-Database-per-process
LadybugDB contract. No public API surface changed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(lbug): drop isDbBusyError re-export, import from lbug-config directly
The re-export from lbug-adapter.ts was a transitional convenience — with
the matcher now living in lbug-config.ts, having two import paths for the
same symbol invites future drift. Updated the two real consumers
(lbug-lock-retry.test.ts, lbug-open-retry.test.ts) to import from
lbug-config directly, removed the re-export equality test (now vacuous),
and refreshed the explanatory comment so it no longer references a
re-export pattern that doesn't exist.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(lbug): silence benign LadybugDB v0.16.1 schema-init lock warnings on Windows
doInitLbug logs "⚠️ Schema creation warning: ... Could not set lock on
file" on every CREATE NODE TABLE call after the first init on a given
dbPath, on Windows. The lock is internal to LadybugDB v0.16.1 and is
resolved before the table is created — same tolerance pattern as the
existing "already exists" filter. Genuine cross-process lock contention
still surfaces on the next operation through withLbugDb's retry, so
filtering at the schema-init catch only suppresses noise, not signal.
Also extend the safeClose Windows handle-release probe to cover the
.wal sidecar (the previous Database's WAL handle was the slowest to
release, surfacing as the schema-query lock contention) and switch the
probe back to 'r+' so it actually detects exclusive locks.
Test loop in lbug-close-handle-release.test.ts simplified to 10 plain
iterations now that the underlying noise is filtered upstream.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(lbug): isDbBusyError review fixes
- Drop redundant `could not set lock` term — already subsumed by `lock`.
- Document the intentionally-broad matcher: graph-DB lock-shaped errors
("deadlock", "unlock failed", "lock contention", "could not open lock
file") are all treated as transient. If a non-transient surfaces,
tighten the matcher rather than raise the retry budget.
- Add positive test cases covering those lock-shaped strings so the
intent is visible and a future tightening would deliberately break
these.
- Fix the open-retry back-off comment: max sleep is 100+200+300+400 =
1000ms (no sleep after the final attempt), not 1.5s.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Gergo Magyar
|
b5316c2df1 |
test(ci): isolate native LadybugDB and CLI e2e flakes
createFTSIndex now short-circuits on the in-process cache before issuing the native CALL CREATE_FTS_INDEX, so a prior writable session cannot trigger the macOS WAL/checkpoint duplicate-create path observed on main. The cache is also primed on the "already exists" recovery and cleared on re-init/close/drop, keeping ensureFTSIndex semantics identical for read-only fallbacks. The lbug-core-adapter close+reopen test moves to the end of the suite so its native handle churn cannot corrupt later assertions in the same fixture. skills-e2e moves into its own sequential vitest project so the heavy spawnSync-driven CLI fixtures stop competing with the parallel default project on Windows runners, fixing the C-fixture beforeAll timeout. Made-with: Cursor |
||
|
Gergő Magyar
|
3f0c74fea0
|
fix(deps): upgrade @ladybugdb/core to 0.16.0 to resolve native segfaults (#1235)
* fix(deps): upgrade @ladybugdb/core to 0.16.0 to resolve native segfaults Resolves the SIGSEGV / access-violation (0xC0000005) / exit-139 crashes that have been reported widely since 1.6.3. The native crashes originate in @ladybugdb/core 0.15.x — primarily during FTS index creation, VECTOR extension load, and concurrent query teardown — and are reproducible on Linux, macOS and Windows. The maintainer-confirmed fix is to bump the runtime to 0.16.0, which ships nodejs async + memory-management fixes, extension ABI bump, and macOS Intel binaries. Adopting 0.16.0 cleanly required three supporting changes; without them the upgrade itself regresses other paths: 1. maxDBSize must be passed explicitly. 0.16.0 keeps the upstream JSDoc note that the default 0 is "introduced temporarily for now to get around with the default 8 TB mmap address space limit some environment". Constrained CI runners and laptops cannot reserve 8 TB and crash with "Buffer manager exception: Mmap for size 8796093022208 failed." A new gitnexus/src/core/lbug/lbug-config.ts centralises a 16 GiB default (overridable via GITNEXUS_LBUG_MAX_DB_SIZE) and every Database() construction site now passes it. 2. enableCompression default flipped from false to true in 0.16.0. Every Database() call site is updated to pass false explicitly so existing GitNexus indexes keep the same wire format. 3. Bridge DB sidecar files (.wal, .shadow). 0.16.0 enforces a database-id check on .wal / .shadow sidecars and rejects opens whose sidecars belong to a different base name. writeBridge now (a) cleans the full sidecar set when removing the tmp slot, (b) renames .wal / .shadow alongside the main file during the atomic .tmp -> .lbug swap, and (c) wraps openBridgeDbReadOnly in a bounded retry on transient Win32-Error-33 lock errors. Eager db.init() / conn.init() forces the lazy native handle to surface lock contention at the retry site. Known limitation (not a regression): on Windows the 0.16.0 native binary does not release the OS file lock until the process exits, so the close-then-reopen-same-process pattern raises Error 33 after the first close. Production paths (analyze / serve / mcp each open the DB exactly once per process) are unaffected, but eight tests that exercise the pattern are guarded with a process.platform === 'win32' skip; CI's Linux + macOS shards exercise them as before. Tracking upstream: kuzudb/kuzu#3872 / #3883 / #4730. Closes #1136 #1154 #1160 #1162 #1178 #1195 #1196 #1199 #1204 #1206 Refs #1209 (supersedes — Dependabot bump without the supporting fixes) Made-with: Cursor * fix(test): isolate LadybugDB native test state Use per-suite LadybugDB databases in integration helpers so test forks do not reopen a database created by Vitest global setup, and centralize Windows-tolerant native temp cleanup for bridge tests. * fix(lbug): avoid bridge existence reopen Reuse the built LadybugDB config in the extension installer and avoid native close/reopen cycles when checking bridge existence on Windows. Made-with: Cursor * chore(docs): exclude local lbug plan Keep the refactor planning note out of the PR while leaving the ignored local copy on disk. Made-with: Cursor * refactor(lbug): centralize database construction Route LadybugDB opens through shared helpers so native constructor defaults stay consistent across core, pool, bridge, and extension install paths. Made-with: Cursor --------- Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> |
||
|
Gergő Magyar
|
aa7bacd48b
|
fix(search): load FTS during core DB init (#1123)
* fix(search): load FTS during core DB init Made-with: Cursor * test(lbug): rely on core init for FTS extension loading Made-with: Cursor |
||
|
Gergő Magyar
|
bf09eab95b
|
feat: configure prettier with pre-commit hook (#563)
* feat: configure prettier with pre-commit hook integration Add prettier, lint-staged, and prettier-plugin-tailwindcss at the repo root with husky pre-commit hook integration. Moves husky from gitnexus/ to root package.json for reliable hook installation. - Root package.json with prepare/format/format:check scripts - .prettierrc with endOfLine:lf and tailwindStylesheet for TW v4 - .prettierignore excluding fixtures, vendor, generated, *.d.ts, *.md - .gitattributes enforcing LF line endings for Windows consistency - Pre-commit hook uses direct node_modules/.bin/ paths (no npx) * style: apply prettier formatting to entire codebase One-time bulk format. No logic changes. Use .git-blame-ignore-revs to skip this commit in git blame. * chore: add .git-blame-ignore-revs for prettier format commit * perf: pre-commit hook runs only tests related to staged files Use vitest --related to scope test execution to tests that import the changed files, instead of running the full suite on every commit. * perf: remove vitest from pre-commit hook, keep in CI only Pre-commit now runs lint-staged + tsc only. Tests run in CI (ci-tests.yml) where they belong — keeps commits fast. * ci: add prettier format check to quality workflow PRs will now fail if code isn't formatted with prettier. |
||
|
Gergő Magyar
|
33225c2fce
|
fix(ci): move shape-check-regression test to lbug-db project (#518)
The shape-check-regression test uses withTestLbugDB but was running in the default vitest project with parallel forks, causing LadybugDB file-lock conflicts on Windows CI. Move it to the lbug-db project (sequential execution) and exclude from default. Follows up on #501. |
||
|
John R. Eakin
|
f0540b33fb
|
ci: E2E workflow, web typecheck job, pre-commit hook, test suite (#486) | ||
|
marxo126
|
c437acf6bb
|
feat: deep flow detection — consumer access tracking, middleware chains, error shapes, api_impact tool (#482) | ||
|
Shunsuke Hayashi
|
03156935cb |
fix(lbug): retry on DB lock with session-safe cleanup (#325)
When LadybugDB throws a BUSY/lock error (e.g. CLI and server running concurrently), withLbugDb retries up to 3 times with linear backoff. Addresses review feedback: 1. **Race condition fix**: Connection cleanup (close + state reset) now runs inside runWithSessionLock, preventing another operation from acquiring the lock between cleanup steps and having its connection closed from under it. 2. **Tests call withLbugDb directly**: Replaced simulateWithRetry helper with tests that invoke the real withLbugDb implementation, catching regressions in retry count, backoff, and lock interaction. Closes #325 |
||
|
hiromima
|
5b012c3351 |
test: add e2e tests for stale detection, sequential enrichment, stability (#396)
- Stale data detection: verify ensureInitialized() detects meta.json changes and re-opens pool without SIGSEGV or WAL corruption - Staleness throttle: verify 5s throttle window doesn't cause errors - Sequential enrichment: impact() enrichment queries complete on arm64 - Consecutive stability: 10+ sequential cypher calls, mixed tool cycles - Watchdog guard: parallel queries with activeQueryCount protection - stdout restoration: verify process.stdout.write is properly restored Covers test plan items from PR #396 (issues #285, #290, #292, #297) |