mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-16 23:43:12 +00:00
1166 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
gitnexus-release-bot[bot]
|
e30002b65a | release: v1.6.10-rc.120 | ||
|
Gergő Magyar
|
b0cacd05ee
|
fix(ci): stop the review agent rejecting its own graph-backed reviews (#2731)
* fix(ci): stop the review agent rejecting its own graph-backed reviews
The context-evidence gate only counted a `context` call when the call
itself passed `file_path` equal to a changed path. The review skill
teaches plain `context({name})`, so 17 of the 26 review-agent run
failures were complete, graph-backed reviews thrown away after full
model spend, with no log line saying which invariant failed.
Prove the evidence from the result instead: `status=found` plus a
`symbol.filePath` inside the repo-scoped changed-path set. Every other
check stays exactly as it was - strict JSON, orchestrator-only turns,
result ordering, duplicate tool-id rejection - and the `repo` argument
still selects the head or the merge-base path set.
Same failure inventory, smaller classes:
- rejection now logs why (in-scope, out-of-scope, sidechain, unresolved
and off-path counts plus up to three sanitized paths), and the
envelope error names the message count and first-message shape
- Glob/Grep leave the tool set: they were enabled through `--tools` but
never allow-listed, so every lane call was denied and burned turns
- both pinned `npm ci` installs retry three times; one registry
ECONNRESET killed a whole run
- the prompt matches the new contract and asks for the structured body
even when the analysis is incomplete
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(skills): mirror the review-skill tool-set change into the shipped copies
The npm package, Claude plugin, and Cursor integration ship byte-identical
copies of .claude/skills/gitnexus-review, and the drift guard compares them.
Dropping Glob/Grep from the lane frontmatter and the SKILL.md sentence only
landed in the canonical tree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(ci): stop one junk context result discarding a proven review
Tri-review of this PR found that the previous commit fixed one spurious
rejection and created another. Widening evidence candidacy from "the call
that named a changed path" to "every orchestrator context call" also
widened the *strict-parse* surface: `contextResultProvesChangedPath`
throws rather than returning false, so a single malformed payload
anywhere in the transcript now discarded a review that an earlier call
had already proven. The MCP makes that reachable without any misbehaving
model - `GITNEXUS_MCP_DEFAULT_MAX_TOKENS=12000` truncates any context
payload over ~48 KB mid-JSON and appends a marker - and it also destroyed
docs-only runs that the `no_indexable_changed_symbols` mode exempts.
Reproduced by running the workflow's own embedded script on both trees:
a proving evidence call followed by one truncated exploratory call gave
`failure_code: null` on the base and `invalid_execution_transcript` on
the head; it is `null` again here.
- payload-shape failures are caught and counted (`malformedResults`)
instead of thrown; transcript-structural invariants (envelope, tool
shapes, duplicate ids, empty tool_result) still fail closed
- diagnostics gained the reasons they were blind to: errored results,
results that arrived out of order or via a sidechain, unanswered
in-scope calls, and malformed payloads. A rejection can no longer
print an in-scope call with every reason at zero
- a deletion-only PR no longer registers head-scoped candidates that can
never be satisfied: an empty eligible set is out of scope, not a result
"outside the changed paths"
- the mandatory-body prompt clause now pairs with a required `complete`
boolean. An incomplete analysis publishes its partial body labelled
`incomplete_analysis` instead of passing as an accepted review
- `Agent(a,b,c)` is split into six separate `Agent(x)` rules: the pinned
base action parses allowedTools with `.flatMap((v) => v.split(","))`
(parse-sdk-options.ts at 3553f843), which shattered the grouped rule
into `Agent(ci-correctness-lens`, four bare names, and
`ci-critic-lens)` before the SDK saw it. Pre-existing and unproven at
runtime, but the split form is correct under either reading and lets
the header's dispatch canary actually prove something
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(ci): require a line range for context evidence
The tri-review's adversarial lane executed `context({name: 'AGENTS.md'})`
and had the result accepted: the gate checked only that the resolved
filePath was in the changed set, so a bare File node passed for a review
of that file's contents. The trusted prescan already defines an indexable
symbol as one with startLine and endLine, so require the same here.
Pre-existing rather than introduced by this branch, but it is the same
"what counts as proof" surface the rest of this PR tightens.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(ci): close the remaining tri-review findings
Addresses every finding the tri-review left open after
|
||
|
MyShining
|
ff86ccf1e7
|
feat(spring): model profiles, conditions, and auto-configuration (#2678)
* feat(spring): model conditions and auto-configuration * fix(spring): align auto-configuration declarations * perf(spring): streamline auto-configuration indexing * test(spring): move timing benchmark out of vitest --------- Co-authored-by: Shining <xuenning@qiyi.com> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> |
||
|
Gergő Magyar
|
e307286d52
|
fix(scope-resolution): a named receiver's member never resolves lexically, + two #2695 follow-ups (#2714)
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(scope-resolution): a named receiver's member never resolves lexically (#2699) `lookupCore` Step 1 walked the lexical scope chain for every lookup, including explicit-receiver property reads. So `options.baseUrl` could bind to an unrelated function-local `const baseUrl` in the same file, and `config.extractVisibility(node)` to the enclosing class's own method. This is the residual half of the defect JS/TS block scopes narrowed in #2695. Blocks moved nested-block locals off the chain of a reference outside the block, which removed 114 false edges; a local declared directly in the function body stayed on it, and no amount of extra scopes reaches that case. Fixed at the cause instead: `recv.name` names a member of whatever `recv` denotes, so a binding of the bare tail name in an enclosing scope is never the right answer. Steps 2 and 3 (receiver type / owner members) are the legitimate routes. `this` and `self` are EXEMPT, and that exemption was measured, not assumed. Skipping Step 1 for every explicit receiver removed 711 edges on a 762-file corpus — but 2 of those were genuine: `self.srcIx` and `self.streamedAt(...)` after `const self = this`, reaching their own class's members through the class-body scope. For a self-receiver the members and the lexical chain legitimately overlap; for a named receiver they never do. Exempting the self names keeps both true edges and still removes 709 false ones, adding none. The removals were classified by reading source at the site, not by pattern- matching ids — an "is the target a member of the source's owner?" heuristic labelled 43 of them plausible and every one I then read was false: language = config.language; -> the class's own `language` dirMap.get(...) / exactMap.get(...) -> a sibling object-literal `get` return config.extractVisibility(n); -> the class's own method (self-edge) writer.close(); -> GraphEmitSink.close Residual, deliberately kept: a `this.x` read can still bind lexically to a same-named local. That is the price of the two true self-alias edges above. `INCREMENTAL_SCHEMA_VERSION` 19 -> 20: a v19 index holds these false CALLS/ACCESSES on every unchanged file and would keep serving them through the reuse gate. Test confirmed discriminating: it fails with the guard reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR * fix(typescript,javascript): a generator expression binding is a Function node (#2693) `const g = function* () {}` matched none of the closure-binding definition rules — they covered `arrow_function` and `function_expression` only — so the binding emitted a `Const` node. `buildGraphTargetIndex` admits callable nodes only, so `g()` resolved to nothing. Same defect shape as the `var` case #2693 already fixed: a different grammar node for the same construct, and the resulting graph node was not callable. Adds the four variable-binding shapes in both languages: `const`/`let` and `var`, each plain and exported. Purely additive — no existing pattern is reordered or rewritten, because the #2687 pre-scan dedup is order-dependent and collapsing the value/callable pair depends on which match wins. Deliberately NOT covered, and the query comment says so: a generator in an object-literal pair or a HOC wrapper still falls through anonymous. Those are rarer, and each additional pattern is another chance to disturb the dedup. `SCHEMA_BUMP` 26 -> 27: definition captures are parse-time, so a warm parse cache would replay the old ones verbatim — `--force` does not clear it. Two tests confirmed discriminating (they fail with the patterns reverted), plus a guard that the already-working generator DECLARATION form is unaffected, since it shares the emit path these were inserted beside. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR * fix(ingestion): keep caller attribution in lockstep with definition ids (#2699) The definition phase appends `localIdentity` to a nested callable's own name segment (`run.save@3:2`); `findEnclosingFunctionId` did not, so the two phases derived different ids for the same callable. The failure mode is silent — the caller id names a node that does not exist, so the edge is dropped rather than reported — which is why the parse-worker docblock calls this pair a lockstep guarantee and asks that both phases derive the prefix from one place. The condition is now byte-identical to the definition phase's (`nestedPrefix !== undefined`), so the two cannot diverge again. Scope of the claim, stated plainly: no reproducing case was found, and this changes nothing measurable on a 762-file TypeScript corpus. TS/JS resolve callers through `resolveCallerGraphId` in the graph bridge, not this path; `findEnclosingFunctionId` serves the `callExtractor` languages, and the corpus does not exercise a nested callable there. The review that raised it (P3) observed zero dangling edges, and "zero dangling" is also what silently dropped edges look like — so this closes a documented contract rather than a demonstrated bug, and carries no test of its own. Rides the `SCHEMA_BUMP` 26 -> 27 in the preceding commit: caller attribution runs in the worker, so a warm parse cache would replay the old ids. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR * docs(test): correct the block-scope header that this PR made false (#2699) Review finding (MEDIUM). The file header still described `lookupCore` Step 1 as walking the lexical chain for EVERY lookup, and called the function-body-local case "unchanged and still mis-resolves ... pre-existing and tracked separately". Commit |
||
|
Abhigyan Patwari
|
93c964609a
|
Merge pull request #2715 from azizur100389/azizur/md060-markdown-tables-2709
fix(ai-context): emit compact markdown tables |
||
|
Abhigyan Patwari
|
652ef6842e
|
Merge pull request #2712 from abhigyanpatwari/dependabot/npm_and_yarn/gitnexus/tar-7.5.22
chore(deps)(deps): bump tar from 7.5.20 to 7.5.22 in /gitnexus |
||
|
Abhigyan Patwari
|
fbeb2be470
|
Merge pull request #2711 from abhigyanpatwari/dependabot/npm_and_yarn/gitnexus/postcss-8.5.23
chore(deps)(deps-dev): bump postcss from 8.5.16 to 8.5.23 in /gitnexus |
||
|
dependabot[bot]
|
1e9f74dc58
|
chore(deps)(deps): bump js-yaml from 5.0.0 to 5.2.2 in /gitnexus (#2710)
Bumps [js-yaml](https://github.com/nodeca/js-yaml) from 5.0.0 to 5.2.2. - [Changelog](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md) - [Commits](https://github.com/nodeca/js-yaml/compare/5.0.0...5.2.2) --- updated-dependencies: - dependency-name: js-yaml dependency-version: 5.2.2 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
azizur100389
|
02ebf8f199 | fix(ai-context): emit compact markdown tables | ||
|
dependabot[bot]
|
32160e8cd7
|
chore(deps)(deps): bump tar from 7.5.20 to 7.5.22 in /gitnexus
Bumps [tar](https://github.com/isaacs/node-tar) from 7.5.20 to 7.5.22. - [Release notes](https://github.com/isaacs/node-tar/releases) - [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md) - [Commits](https://github.com/isaacs/node-tar/compare/v7.5.20...v7.5.22) --- updated-dependencies: - dependency-name: tar dependency-version: 7.5.22 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> |
||
|
dependabot[bot]
|
84e33f5048
|
chore(deps)(deps-dev): bump postcss from 8.5.16 to 8.5.23 in /gitnexus
Bumps [postcss](https://github.com/postcss/postcss) from 8.5.16 to 8.5.23. - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](https://github.com/postcss/postcss/compare/8.5.16...8.5.23) --- updated-dependencies: - dependency-name: postcss dependency-version: 8.5.23 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> |
||
|
Gergő Magyar
|
4906daf27b
|
fix(scope-resolution): resolve calls through a closure-valued binding across languages (#2693) (#2695)
* fix(scope-resolution): resolve calls through a closure-valued binding (#2693)
`val f = { }; f()` emitted no CALLS edge in Kotlin or Swift, so `impact` on
such a symbol under-reported to zero — the same false all-clear as #2687.
The cause was not, as first suspected, that these languages fail to feed
`callable-value-flow`. They do: `synthesizeCallableFlowCaptures` is called
from 15 language capture modules, and Kotlin already resolves reassignment
through the pass (`var f = ::a; if (c) f = ::b; f(1)` reaches both targets).
Their captures are already exactly right — the seed names the binding as its
own callable, per the anonymous-callable convention in
callable-flow-captures.ts.
They died one layer later, at the `buildGraphTargetIndex` gate:
if (!isCallable(def) && providerTarget?.(def) !== true) continue;
`isCallable` is Function/Method/Constructor, but the scope-resolution layer
declares a closure binding with its VALUE label (Kotlin/Swift `Property`),
and `isCallableValueTarget` is implemented by exactly one provider — COBOL.
So the binding never entered `graphTargets`; `lexicalCallableLookup` then
returned `shadowed: true` with no targets, which also suppressed the
workspace-wide fallback, and the seed resolved to nothing.
Only the graph knows a value binding holds a callable — since #2687 it emits
a single `Function` node for one. So value bindings now resolve their graph
id first and are admitted on the label of the node they actually reach.
This is self-limiting: a genuine constant keeps its own Const/Property node,
so `resolveDefGraphId`'s qualified key hits before the label-agnostic
`simpleKey` fallback can reach a same-named callable. Only a binding whose
own value node was replaced by a callable one gets through.
No scope kind changes — Kotlin's `lambda_literal` stays `@scope.block`, so
#1757 smart-cast semantics are untouched by construction. The fix is
language-neutral: it discriminates on the graph node label, never on a
language name.
Dart is fixed separately; its root cause is independent.
* fix(dart): resolve calls through a closure-valued binding (#2693)
Dart needed more than the shared gate fix: neither of its closure-binding
forms could resolve, for two different reasons, and the plan's one-line
diagnosis turned out to be incomplete.
TOP-LEVEL `var f = (x) => x;`
A graph Function node already existed (#2687), but no `@declaration.*`
matched the binding, so scope resolution had no SymbolDefinition to attach
a flow seed to. Adding the declaration exposed a second problem: Dart's
`initialized_identifier` is FIELDLESS, so the shared field-based assignment
fallback (`left`/`name`/`value`/…) decomposed nothing and the binding still
emitted no flow captures at all. Kotlin's fieldless `assignment` node hit
exactly this and took the same remedy — a provider `extractAssignment`.
FUNCTION-LOCAL `void m() { var f = (x) => x; }`
Locals parse as `initialized_variable_definition`, which the top-level
graph-node rules are deliberately anchored under (program) to avoid, so a
local closure had no graph node at all — nothing for the widened
`buildGraphTargetIndex` gate to admit.
Both new rules are restricted to a `function_expression` value. Declaring
every Dart variable would mint defs and nodes repo-wide for no resolution
benefit; ordinary locals stay unindexed exactly as before. The top-level
declaration reuses the (program) anchor the graph-node query already relies
on, so class-body fields — which share `initialized_identifier_list` and are
already `@declaration.property` — are never matched twice.
Also drops the now-false note in tree-sitter-queries.ts claiming `f()` does
not resolve for Dart. That node is now the evidence that makes it resolve.
* docs(scope-resolution): document the callable-flow capture contract (#2693)
The module is 1200+ lines behind a nine-line docblock, and the only worked
example was C. Both root causes fixed in this series were "the contract was
discoverable only by reading the emitter":
- the anonymous-callable convention (a seed whose source is a closure takes
its DESTINATION's name) is what makes closure bindings resolvable at all,
and is the reason the widened target gate is correct;
- a fieldless binding node silently decomposes to nothing under the shared
assignment fallback, which cost Kotlin one debugging cycle in #2522 and
Dart another here;
- captures alone are never enough — the bound name also needs a
`@declaration.*` or there is no cell to key the seed on.
Records the cell/site model, both traps, and points at the fullest and
smallest worked examples.
Bumps INCREMENTAL_SCHEMA_VERSION 15 → 16 and the parse-cache SCHEMA_BUMP
22 → 23: this series emits NEW CALLS edges and new Dart Function nodes, and
the incremental write set only covers changed files, so an existing index
would keep reporting a zero blast radius for exactly the symbols the fix is
about.
* perf(scope-resolution): pre-filter value bindings in the callable target index (#2693)
Widening the `buildGraphTargetIndex` gate to consider VALUE bindings put the
hot loop on a much larger def population — value bindings outnumber callables
in real source — and the naive version paid full price per binding. Measured
on a synthetic 800-file corpus (8 value bindings per file, 1 of them a closure
binding), the widening cost 2.50-2.82x the pre-#2693 callable-only build.
Two wastes, both provable rather than guessed:
1. `definitionAnchorKey` ran for every def, including value bindings. The
anchor index is keyed by callable LABEL and the key is built from
`def.type`, so a value def can never hit it — and the key costs a regex
per def.
2. Every value binding paid the whole `resolveDefGraphId` key chain only to be
rejected. It need not: every qualified key that function tries embeds
`def.type`, so for a VALUE def those can only ever reach a value-labelled
node. Its one route to a callable is the label-agnostic
`simpleKey(filePath, simpleName)` fallback, which by construction requires
a callable node with the SAME file and simple name. So a value binding with
no such node cannot resolve to a callable, and one Set lookup decides it.
That set is derived in the graph walk the anchor index already performs, so it
costs no extra pass.
large_ms 7.79-8.37 -> 4.90-5.02 (1.61x faster)
widening_overhead 2.50-2.82 -> 1.45-1.50
The resolved target-set fingerprint is byte-identical across both, which is
the point: this is a cost change, not a behaviour change.
Adds bench/callable-value-flow/ (fingerprint + scaling + widening-overhead
gates) and wires it into ci-tests.yml beside the other build-free benches. The
overhead budget of 1.9 sits between the measured with-filter and without-filter
bands, so it cannot be met if the pre-filter is removed. Timings use the MIN of
15 warmed reps, not the median: the same build reported 1.65 idle and 2.03
under load, and a median-based gate would have to be loosened past the point of
detecting the regression it exists to catch.
`buildGraphTargetIndex` is exported for the bench; it is pure and not part of
the pass's public contract.
* test(scope-resolution): assert the declaration route does not double-emit (#2693)
Go, Python, C++ and TS/JS already resolved a closure-binding call through
their `@declaration.function` capture. The widened `buildGraphTargetIndex`
gate gives the same call a SECOND possible route, so each must still produce
exactly one edge.
`tryEmitEdge` dedups by key, but a collapsed key and a site-anchored key are
DIFFERENT keys — a real double-emit would show up as two ids for one call
site, not be silently collapsed. Asserting on edge ids rather than target ids
is what makes that visible.
* fix(scope-resolution): join value bindings to their callable node by POSITION (#2693)
Review found the first cut of this series minted FALSE CALLS edges. Admitting a
value binding whose *resolved* graph node is callable let `resolveDefGraphId`
fall through to its label-agnostic, first-write-wins
`simpleKey(filePath, simpleName)` and bind the name to ANY same-named callable
in the file.
The safety argument in the previous commit — "a genuine constant keeps its own
Const/Property node, so the qualified key hits first" — silently assumed
`def.type === node.label`. It does not hold:
- TypeScript declares `const` as `Variable` but emits a `Const` NODE, so the
qualified key misses even though the value node exists;
- Rust `let` bindings get no graph node at all, so the fallback is the only
route.
Reproduced, all previously emitting a fabricated caller:
const save = (x: number) => x * 2; // next to an unrelated Svc.save
-> Method:svc.ts:Svc.save#1 // Svc never instantiated
const handler = other; // shadowing a top-level handler
-> Function:app.ts:handler // unreachable from here
let handler = cb; // Rust
-> Function:main.rs:handler
Worse in Dart, where the same collision INVERTED the feature: the only edge went
to the class method and the closure's own node got none. The result was also
declaration-order dependent — two files differing only in declaration order got
different CALLS sets — and it propagated through argument-to-formal binding into
functions whose source never mentions the name.
A closure binding IS its callable node: same file, same line, same name. An
aliasing local is not. So the join is positional now — a file/line/name index
built in the graph walk `byAnchor` already performs — and value bindings never
run the key chain at all. That is both correct and cheaper:
large_ms 4.90-5.02 -> 4.37-4.63
widening_overhead 1.45-1.50 -> 1.43-1.58 (name-match design: 2.50-2.82)
with a byte-identical target-set fingerprint on the bench corpus.
Also from review:
- `Static` dropped from VALUE_BINDING_DEF_TYPES: `normalizeNodeLabel` has no
`static` case, so no def can carry that type — it was an entry no fixture
could ever exercise. The remaining set now documents why it deliberately
does NOT reuse `isOwnableValueLabel`, which is contracted to a different
consumer.
- Dart `final`/`const` top-level closures (static_final_declaration_list) and
every declarator after the first in a multi-name local now resolve; both
parse into shapes the earlier rules never reached.
- The bench source carried a literal NUL byte, so git recorded it as BINARY
and the only artifact pinning the target set was unreviewable in the PR
diff. It is written as an escape now. Its corpus also modelled `startLine`
as 1-based where graph nodes are 0-based, which would have stopped it
exercising the value-binding path at all.
- `call-summary-schema-version.test.ts` asserted `passesReuseGate(15)` is
true; the 15 to 16 bump made that false and the test RED. It now pins 16 as
current and 15 as rejected, matching the pattern every prior bump followed.
- The v23 parse-cache comment is at the top of the list, not mid-list.
Tests: the five collision cases above are new regression tests, each confirmed
failing against the previous commit. Also added Kotlin class-body closures (the
only case exercising the Method arm), Dart top-level `final`, Dart multi-name
locals, and a warm-parse-cache replay for Kotlin and Dart — the #2693 captures
are replayed verbatim, so a serialization change would surface only on a SECOND
analyze and every other test here runs cold. The previous negative tests were
vacuous: they paired names that did not collide (`maxSize` vs `size`), so the
pre-filter rejected them before the guard they were named after could run.
* docs(storage): fix the schema-version changelog blocks (#2693)
Two problems, one mine and one not.
MINE: the `INCREMENTAL_SCHEMA_VERSION` block is ASCENDING (v2 … v15), and I
inserted v16 above v15 rather than at the end — I had just moved the parse-cache
entry to the top of ITS block, which is descending, and applied the same habit
to a list ordered the other way. Moved to the end; both blocks are now
internally consistent.
NOT MINE: the parse-cache block carries TWO v21 entries, with v20 wedged between
them. Tracing it: #2632 (Spring DI facts) bumped 20 -> 21 and merged first;
#2653 (Java JLS local-class identities) had branched at 20, also bumped to 21,
and merged second — so it shipped with NO invalidation of its own. An index
already stamped 21 by the first change was treated as current by the second and
kept serving stale local-class identities from the warm cache.
Numbers left alone: both genuinely shipped as 21, and renumbering them now would
misstate what users' indexes actually contain. Instead the entry says so
explicitly, and points at the process fix — re-check the constant against
origin/main immediately before merging, not just when the branch is cut. The
identical collision hit INCREMENTAL_SCHEMA_VERSION in #2653/#2654, so this is a
recurring failure mode of concurrent PRs, not a one-off typo.
Comment-only; no constant changes value.
* feat(scope-resolution): resolve closure bindings in Ruby, Java, C#, PHP and JS/TS var (#2693)
Ruby, Java, C# and PHP already emitted correct callable-flow seeds and invokes.
What they lacked was the #2687 piece — a CALLABLE graph node at the binding,
which is what buildGraphTargetIndex joins to by position. PHP additionally had
no scope declaration for the bound name, so the flow pass had nothing to attach
its seed to.
ruby handler = ->(x) { x } handler.call(1) -> Function:a.rb:handler
java Function<..> handler = x->x handler.apply(1) -> Function:A.java:A.handler
csharp Func<int,int> handler = ... handler(1) -> Function:A.cs:A.handler
php $handler = fn($x) => $x $handler(1) -> Function:a.php:handler
Ruby and Java invoke through the callable-object protocol; C# and PHP call the
binding directly. Locals work in all four, and a binding whose name collides
with a same-named method resolves to the CLOSURE, not the method.
Two things the sweep caught:
JAVA TWIN. Anchoring the rule on the inner variable_declarator produced BOTH a
Function and a Property node — the exact double-indexing #2687 removed. The
parse-worker dedup keys on (definition node, name), and Java's value rule
anchors on field_declaration, so the keys never matched. Re-anchored on
field_declaration / local_variable_declaration.
JS/TS `var`. `var f = (x) => x` kept a Variable label while const/let got
Function, because `var` is a different grammar node (variable_declaration vs
lexical_declaration) that no closure rule covered. A call through the binding
still resolved via the declaration route, so the CALLS edge pointed at a
NON-callable node. Now consistent across const/let/var.
That last one flipped an existing assertion in const-function-twin.test.ts,
which expected `Variable` for a var-bound function-expression. Its comment
explained why — "var has no matching @definition.function pattern, so nothing
claims the name" — i.e. it documented the gap rather than defending it. The
property it was really protecting (an UNCLAIMED value node survives) now has
its own case with a non-function initializer, and the var-closure case asserts
the collapse to one node, which is also the twin guard for the new rule.
Known limits, both pre-existing and both failing safe:
- A PHP local closure whose name collides with a top-level function gets no
edge: both want id Function:<file>:<name>, so the closure never gets its own
node. This is the file-scoped node-identity convention — TypeScript, Python
and Dart collapse identically at base.
- TS/JS class-field arrows stay Property (Kotlin's equivalent emits Method).
They already resolve; changing the label risks the HAS_PROPERTY ownership
regression #2687 hit once.
The invalidation constants already bumped in this PR (INCREMENTAL_SCHEMA_VERSION
16, SCHEMA_BUMP 23) cover these additional languages; their notes now say so.
Tests: one case per newly-resolving language plus the PHP anonymous-function
form and the JS var form, in closure-binding-labels.test.ts. The file now spins
a worker pool per test across a dozen languages, so its timeout is raised
file-wide — a case that takes ~7s alone was exceeding the 30s default under
that contention.
* fix(ingestion): class-field closures are callable members in TS/JS (#2693)
A CALLS edge must target a callable node. `class A { handler = (x) => x }` emitted
a Property, so calling it produced `CALLS -> Property:A.ts:A.handler` — an edge
pointing at something the graph says is not callable. Same defect class as the
JS/TS `var` binding fixed in the previous commit, and the last place a closure
binding still carried a value label.
Kotlin already models its class-body closure as Method + HAS_METHOD; TS/JS now
match, so all three agree:
class-field closure -> Method + HAS_METHOD (CALLS target is callable)
plain class field -> Property + HAS_PROPERTY (unchanged, no CALLS)
Anchored on public_field_definition / field_definition — the same nodes the
property rules use — so the parse-worker dedup collapses the pair rather than
leaving a Method/Property twin, the failure the Java rule hit in the previous
commit.
ON MATCHING THE COMPILERS. This deliberately diverges from tsc and SCIP. The
TypeScript compiler classes `handler = () => {}` as a PropertyDeclaration
("a property declaration independently from what it's assigned to"), and SCIP
gives it a `.` term descriptor, the same suffix as any field — both call it a
property, and Kotlin's compiler likewise treats `val f = { }` as a property with
a function type. The divergence is intentional: GitNexus's Function/Method label
does not mean "tsc SymbolFlags", it means "this node can be the target of a
CALLS edge", which is the convention #2687 set for closure bindings in every
language. Modelling it the compiler's way would mean either dropping call
resolution for these members or emitting a separate node for the lambda and
flowing the property to it — the two-node shape #2687 removed. Recorded here so
the next reader does not "fix" it back.
Tests: TS and JS class-field arrows resolve to their Method node, plus a guard
that a NON-closure class field stays a Property — the closure rule must key on
the initializer, not on the field syntax.
* fix(php): keep the $ sigil on closure-binding nodes so locals stop colliding (#2693)
A PHP local closure whose name matched a file-level function got NO edge at all:
function save($x) { return $x; }
function run() {
$save = fn($x) => $x * 2;
return $save(1); // no CALLS edge
}
Both minted the id Function:<file>:save, so the closure's node was swallowed by
the function's and the positional join found nothing at the binding's line.
The fix is PHP's own semantics rather than a change to node identity across the
graph. PHP holds variables and functions in SEPARATE namespaces — $save and
save() cannot collide in the language — and the sigil is what separates them.
Dropping it was the bug. The node rule now captures the whole variable_name, so
the closure is Function:<file>:$save and the function stays Function:<file>:save.
languages/php/query.ts already keeps the sigil on property declarations for the
same reason, so this makes the two consistent.
The positional join normalises a leading $/@ on both sides, matching what the
scope layer and the callable-flow synthesizer already do, so the binding still
matches its own declaration while its NODE stays distinct.
local closure + same-named function -> Function:c.php:$save (the closure)
calling the real function -> Function:f.php:save (unchanged)
plain $max = 10 -> no node, no edge (unchanged)
WHAT THIS DOES NOT FIX. The general problem is wider than PHP: GitNexus node ids
are file-scoped, so a function-local symbol and a file-level one with the same
name collapse in TypeScript, Python and Dart too, and Java/C# only escape by
qualifying on the enclosing CLASS (so two same-named locals in different methods
still collide). SCIP solves it with a separate `local <id>` keyspace that is
document-scoped and never globally addressable. That is issue #2699 — it changes
persisted ids for every function-local symbol and needs its own invalidation, so
it is not bundled here. PHP is fixed on its own merits: the sigil belongs in the
identity regardless of how locals are eventually scoped.
* test(scope-resolution): pin the closure-binding caller-attribution limit (#2693)
Review of this PR found the new callable nodes are call TARGETS but never call
SOURCES: a call made INSIDE a closure binding is attributed to the enclosing
scope, so `impact(handler, direction:"downstream")` reports nothing even though
the closure calls out. Consistent across Kotlin, Dart, Ruby and PHP; TS/JS free
bindings are the exception because their arrow carries a @scope.function whose
range matches.
Not fixed here — pinned, so the boundary is visible instead of surprising, and
so a change in EITHER direction fails a test.
The cause is precise: `pickCallerCallableDef` (graph-bridge/ids.ts) finds the
caller by walking CHILD scopes whose range contains the call site, gated on
`child.kind === 'Function'`. A closure literal is a BLOCK scope in these
languages (Kotlin deliberately, #1757 smart casts), AND the binding's def is
owned by the enclosing scope rather than by the closure's scope — so neither
half of the link exists. Fixing it needs "callable boundary" decoupled from
scope `kind` plus an association between the closure scope and its binding.
That is a change to the caller anchor used by every call in the repo, which is
not something to land at the tail of this PR.
Also adds a unit suite for `buildGraphTargetIndex` itself, covering what the
integration tier cannot isolate: a binding is admitted only on POSITIONAL
evidence, a name-only match is rejected, a non-callable node at that position is
rejected, an ambiguous position claimed by two callables is rejected, and the
PHP dollar sigil normalises across the join while still not matching a
same-named function on another line. That last one closes the review's LOW —
the node/declaration name asymmetry now has an executable contract rather than
resting on a comment.
* docs(test): correct the per-language cause of the attribution limit (#2693)
The comment on the pinned attribution tests claimed "a closure literal is a
BLOCK scope in these languages". That is true for Kotlin (lambda_literal
@scope.block, #1757) and Ruby (do_block/block @scope.block) and FALSE for PHP:
anonymous_function and arrow_function are already @scope.function
(php/query.ts:61-62). Dart is a third case again — it has no scope over a
closure literal at all.
So the four languages fail at three different points, not one:
Kotlin, Ruby fail the `child.kind === 'Function'` gate
PHP passes that gate; its closure scope owns no callable def,
because the binding's def belongs to the enclosing scope
Dart has no child scope for the walk to consider
Worth correcting carefully rather than tidying: a follow-up plan re-stated this
comment instead of re-deriving it, and inherited the misdiagnosis — it proposed
"relax the kind gate" as required for all four, which is a no-op for PHP and
unreachable for Dart. A review caught it. The comment now states each language's
actual blocker and says why the distinction matters.
Comment-only; the three pinned tests are unchanged and still pass.
* fix(scope-resolution): an ordinary JS/TS `function` binds its own `this` (#2701)
`this.m()` inside a nested `function` resolved to the lexically enclosing
class, so it emitted a CALLS edge that does not exist at runtime — including
the exact `forEach(function () { this.m(); })` shape arrow functions were
introduced to avoid:
class D {
m() {}
build() { const h = function () { this.m(); }; return h; }
}
// CALLS: Function:D.ts:D.h -> Method:D.ts:D.m#0 FALSE
ECMA-262 gives an arrow `[[ThisMode]] = lexical`: it has no `this` binding in
its environment record, so the lookup passes through to the enclosing
environment. Every other function form binds `this` at call time. `tsc` draws
the same line by resolving `this` through `getThisContainer` with
`includeArrowFunctions = false`. That one rule is the whole fix.
Languages declare it; shared code never learns a language. The query files —
the one place that already names grammar nodes — tag every non-arrow function
form with `@receiver-owner.this`, which becomes `Scope.ownsReceivers`. A
receiver walk that reaches such a scope without finding the name stops there
instead of borrowing an enclosing scope's binding. Every other language leaves
the field unset and is bit-for-bit unchanged; a Kotlin lambda, which DOES
capture the enclosing `this`, still resolves (pinned as a test).
THREE GATES, ALL LOAD-BEARING. The false edge survived each one alone, which
is why the tests assert on the emitted edge rather than any single walk:
1. `Scope.ownsReceivers` stops BOTH receiver-type walks — `findReceiver
TypeBinding` here and its twin `lookupReceiverType` in gitnexus-shared's
`lookup-core`, which was resolving the receiver independently.
2. `LanguageTypeConfig.thisBoundaryNodeTypes` stops the type-env AST walk
that infers a receiver's type during capture.
3. `isReceiverOwnedButUnbound` makes `receiver-bound-calls` SUPPRESS the
site. Without it the member still resolved by NAME through `lookupCore`'s
lexical chain — the class-body scope binds `m` two scopes up — merely at
lower confidence. An owned-but-unbound receiver is a definitive negative,
not a miss, so it must not reach a receiver-blind fallback.
Also fixed: `function*(){}` as an expression was not a `@scope.function` at
all, so `this` inside one read as the enclosing method's.
WHAT THIS GIVES UP. The fix REMOVES edges, and some were correct:
`.bind(this)`, `.call(this)` and `forEach(fn, thisArg)` do make `this` the
instance at runtime. Their correctness is fixed at the CALL SITE, which no
scope-level rule can see, so the choice is between losing them and keeping
every detached-callback false positive. All three are pinned as tests
asserting the empty result, so changing the trade later is deliberate.
`this` in a static method also stops resolving to the INSTANCE member — that
edge was wrong in the other direction.
INVALIDATION. Both constants move, and the parse-cache one is not optional:
`ownsReceivers` lives on the cached `Scope`, and a warm cache replays scopes
without it — verified by probe that `--force` alone does NOT re-derive it, so
the fix silently did nothing until SCHEMA_BUMP moved. INCREMENTAL_SCHEMA_
VERSION 16 -> 17 (the incremental write set covers only changed files, so
unchanged TS/JS files would keep their fabricated `this` edges);
SCHEMA_BUMP 23 -> 24.
Verified against a built index, not by reading: all three false edges from the
issue gone, every correct edge kept, same result in JavaScript through its
separate grammar. 64 tests green across the new suite plus the closure-binding
and schema-version suites. The full suite's 36 failures are pre-existing
load-flakes — confirmed by A/B: `skip-git-cli` fails FOUR tests on a clean
HEAD versus three with this change, and `pipeline-pdg-streaming` passes in
isolation either way.
Refs #2701
* fix(ingestion): give function-local callables their own identity (#2699)
Graph node ids were file-scoped, so a local callable and a same-named
file-level one collapsed onto ONE node. That is a wrong answer, not a missing
one — the local call was attributed to the file-level symbol:
export function save(x) { return x; }
export function run() { const save = x => x * 2; return save(1); }
export function other() { const save = x => x * 3; return save(2); }
// ONE node Function:a.ts:save, and BOTH run and other pointed at it, so
// `impact` on the top-level save reported two callers that never call it.
A local's identity is now its enclosing-callable chain plus its own position —
`run.save@2:2`. The chain is for humans reading `impact`; the position is what
makes it correct. Names alone cannot express what ECMAScript actually
specifies, and the gap is the language's, not the grammar's: an environment
record is created per function AND per block, so an anonymous function has no
name to contribute and sibling blocks hold distinct bindings under the same
name. One positional rule settles both, with no conditionals and no
"disambiguate only when it looks ambiguous" heuristic — the ambiguity-flag
class of bug that bit #2514. SCIP reaches the same place with its
document-scoped `local <id>` keyspace.
Top-level functions and class methods are NOT locals and keep their ids
byte-for-byte. That is the bound on the churn: this touches only symbols that
are unreachable from outside their own document anyway.
RESOLUTION JOINS BY POSITION, NOT BY NAME. `resolveDefGraphId` matches a def
to its node on (file, label, line, simple name). A def and its node are the
same construct, so this needs no scope chain at all — which is the point:
re-deriving the chain in the resolver would be a second implementation that
could silently disagree with the first. A genuine tie (two callables on one
line) stores an AMBIGUOUS_POSITION tombstone and falls through to the existing
name keys rather than picking by source order. Without this the node ids were
already correct and calls STILL resolved to the file-level symbol — the fix is
only half a fix without it.
JS/TS GAIN BLOCK SCOPES. They emitted no `@scope.block` at all, so the
resolver could not tell two `const pick` in sibling branches apart. Giving
them distinct ids made that visible as DUPLICATE edges — each call resolving
to BOTH — which is worse than the collapse it replaced. `(statement_block)
@scope.block` supplies the missing environment record. The other half of the
ECMAScript rule was already implemented and waiting: `tsBindingScopeFor`
hoists `var` past blocks to the enclosing Function/Module while `let`/`const`
bind innermost, and its docblock already claimed "the innermost default covers
these" for block scopes that did not exist. All 82 scope-resolution test files
pass with blocks on.
Verified by probe, per case: two locals in different functions, a local inside
an ANONYMOUS function (`outer.fn@1:9.save@2:4`), sibling blocks resolving to
their own binding, `var` still hoisting out of its block, a nested named
`function` vs a file-level one, PHP composing with the `$` sigil from #2693,
and Python. Top-level/method ids unchanged, asserted directly.
Every assertion is on the EDGE, not on node existence. Ids are built twice and
independently — definition phase and caller attribution — and a one-character
disagreement makes the caller attach to a node that does not exist and the
edge vanish, with nothing thrown and no test failing. An edge assertion can
only pass if both phases agree.
INVALIDATION. INCREMENTAL_SCHEMA_VERSION 17 -> 18 and SCHEMA_BUMP 24 -> 25:
persisted node ids change for every function-local callable, and the cached
scope tree lacks block scopes. A top-up would leave unchanged files on the old
ids while changed files emit the new ones, splitting each symbol in two.
Bench fingerprint unchanged and both timing budgets pass. The one full-suite
failure (incremental-orchestration) passes in isolation — its log shows stale
init locks and WAL reclaim, i.e. LadybugDB contention under the parallel run.
Refs #2699
* perf(ingestion): emit block scopes only where they bind something (#2699)
Block scopes make `let`/`const` in sibling blocks distinct bindings, which is
what stopped a call in one branch resolving to both. Emitted naively — one
scope per `statement_block` — they also cost ~10% of analyze wall time, because
every scope-chain walk in every function then steps through levels that bind
nothing.
Two emit-side filters keep the semantics and drop the waste:
1. A block that IS a function body duplicates the enclosing Function scope.
Nothing can be declared between a function and its own body, so a binding
in either resolves identically — the inner scope is pure depth.
2. A block that declares no `let`/`const`/`class`/`function` binds nothing,
so it is transparent: a lookup finds nothing in it and walks to the
parent. `var` is deliberately excluded from that list — it hoists past the
block to the function, so a block containing only `var` still binds
nothing.
MEASURED, on a 762-file / 228k-line TypeScript corpus (gitnexus/src), min of 6
warmed reps with the cold first rep discarded:
block scopes emitted 19,389 -> 5,331 (-72%)
total scopes 35,942 -> 21,884 (-39%)
analyze wall time +9.8% -> +1.6-2.5% vs pre-#2699
peak RSS (whole tree) 2398MB -> 2434MB (+1.5%, inside run-to-run noise)
The filters themselves are free: scope emission over the same corpus measured
12.6s naive vs 12.5s filtered.
Wall-clock on a shared runner has a ±10% spread run to run, which is wider than
the effect being optimised, so the durable gate added here counts scopes
instead. `bench/scope-emission/measure.mjs --check` asserts an EXACT scope set
over a synthetic corpus that mixes the shapes the filters discriminate between
— function/method/arrow bodies, non-declaring if/else/for/while/try, blocks
that declare `const`, and a `var`-only block. Baseline is 2 block scopes per
module: only the two `if`/`else` branches that declare `const chosen`. If the
filters regress that number jumps immediately, in a way wall-clock CI could
never resolve from noise. Wired into the existing benchmarks job.
Behaviour is unchanged: 86 scope-resolution and identity test files, 1371
tests, all green — including the sibling-block case this could plausibly have
broken — and the callable-value-flow fingerprint is untouched.
Refs #2699
* test(bench): re-baseline the TS/JS scope-capture fingerprints for #2701
`bench/scope-capture` fingerprints the full capture set per language, and
#2701 added a `@receiver-owner.this` marker to every non-arrow function form
so a scope that BINDS its own `this` can terminate the receiver walk. That is
a capture-set change, so the TypeScript and JavaScript fingerprints moved and
the benchmarks job has been failing since that commit — I pushed it without
checking CI.
A fingerprint is a correctness gate, so this does not simply adopt the new
value. Verified first by diffing the capture-name HISTOGRAM over the same
fixture corpus against
|
||
|
azizur100389
|
24584297d2
|
fix(trace): add file disambiguator alias (#2705) | ||
|
azizur100389
|
8307e3f01f
|
fix(setup): preserve existing OpenCode config.jsonc (#2694)
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 / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / Classify release event (push) Waiting to run
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-web) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
|
||
|
Abhigyan Patwari
|
d4fbc544c6
|
Merge pull request #2698 from diarized/fix/spurious-fts-unavailable-warning
fix(fts): stop warning "FTS extension unavailable" on the run that installs it |
||
|
Gergő Magyar
|
7a064a1f2a
|
fix(storage): stop the Windows \\?\ long-path prefix from breaking repo path matching (#2667) (#2700)
* fix(lib): add stripWindowsLongPathPrefix for path comparisons (#2667) A caller can hand GitNexus a `\\?\`-prefixed path — the usual MAX_PATH workaround on Windows — and `path.resolve` preserves the prefix, so it reaches every string comparison GitNexus keys paths on. It also poisons relativization: `path.win32.relative` cannot express a relative path between a prefixed and an un-prefixed form of the same directory, so it returns the absolute target instead. That absolute string is the shape reported in #2667. The helper is deliberately scoped to the comparison domain. libuv's `fs__capture_path` does not re-add the prefix for over-MAX_PATH paths, so stripping a filesystem-facing path would break long-path access on hosts that have not opted into LongPathsEnabled. `\\?\Volume{GUID}\…` is left alone because the remainder is not a usable path. The test is fixture-free and takes an explicit `platform`, mirroring `normalizeAnalyzerRootPath`, and is registered on the cross-platform matrix since the whole transform is a POSIX no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119fkrRFdQDQKh58LY9Tqh2 * fix(storage): normalize the `\\?\` prefix in canonicalizePath (#2667) `canonicalizePath` is the single comparison key for the repo registry, MCP repo resolution and the server repo routes, and `registryPathEquals` compares its output as a plain string. A caller-supplied `\\?\` prefix therefore matched nothing: a repo registered as `D:\repo` was invisible to a caller passing `\\?\D:\repo`, which surfaces as "repo not found" or a duplicate registration from `analyze`, `remove`, `clean`, the MCP `repo` parameter and the server routes. Both branches are normalized. The realpath branch was already safe — libuv's `fs__realpath` strips the prefix itself — but the `catch` fallback returns `path.resolve(p)` untouched, and that is exactly the branch a path which is not on disk takes. Safe despite the CRITICAL blast radius (27 impacted, 12 direct dependents) because the result is only ever compared, never opened: all 23 call sites feed `registryPathEquals` or a string comparison. Both operands are canonicalized, so the equality relation is preserved and behaviour is unchanged for every un-prefixed input. The two regression assertions run only on windows-latest, where the file already runs via the cross-platform matrix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119fkrRFdQDQKh58LY9Tqh2 * docs(core): correct two false comments about Windows paths (#2667) Both comments assert the opposite of how the platform and the analyzer actually behave, and both would send the next investigator of #2667 the wrong way. `analyzer-identity.ts` claimed the `\\?\` prefix is one that `realpathSync.native` "can emit for paths over MAX_PATH". libuv's `fs__realpath_handle` strips the prefix unconditionally and rewrites `\\?\UNC\` back to `\\`, erroring if neither is present, so realpath never returns one. The prefix can only arrive from caller-supplied input. The optional group in the regex stays as a labelled defensive no-op, and the function's behaviour is unchanged on purpose: these identity fields are compared between an `analyze` and a later `status` run, so this is not the place to reshape a path. `include-extractor.ts` claimed "gitnexus analyze stores absolute paths in the File.filePath column". A full self-index at |
||
|
Artur Kaminski
|
91953a3cba |
fix(fts): stop warning "FTS extension unavailable" on the run that installs it
On a machine with no cached LadybugDB extension, the first `gitnexus analyze` logs a WARN GitNexus: FTS extension unavailable; continuing without FTS features. load-only policy (no install attempted); LOAD fts failed: ... and then, in the same run, installs FTS and builds every search index. Nothing was degraded — only the log was wrong, and it sent users chasing a broken install path that does not exist (see the first of the two warn lines in #2184, where only the second one is real). The line comes from `initLbug`'s writable FTS pre-load. That call deliberately never installs (analyze owns extension installation), so on a cold cache it is *expected* to miss; Phase 3 retries moments later with the `auto` policy and succeeds. `ExtensionManager.markUnavailable` had no way to tell that speculative probe from a final answer, so it reported every miss as a user- facing degradation. Adds `quiet` to `ExtensionEnsureOptions`: the outcome is still recorded in capabilities, but it is logged at debug level and does not consume the once-per-(extension, reason) warn budget — so a later real failure with the same reason still warns. Set only on the writable `initLbug` pre-load. The read-only serve/MCP branch keeps `{ policy: 'load-only' }` with no `quiet`: there is no later retry there, so that warning is accurate. Analyze Phase 3, `--repair-fts` and genuinely-offline installs (#2184) are untouched and still report loudly. Verified end-to-end against a temp `HOME` with no `~/.lbdb`: analyze emits no FTS warning, installs `libfts.lbug_extension`, and builds all FTS indexes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Gergő Magyar
|
89bbdcf566
|
fix(ingestion): stop double-indexing const X = () => {} as Function + edgeless Const twin (#2687) (#2691)
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
Skill copy sync / shipped skills drift guard (push) Has been cancelled
|
||
|
Gergő Magyar
|
b5c6c0e57c
|
perf(communities): fix the O(communities x N) copy in vendored Leiden, wire Icebug to its real API (#2337) (#2692)
* perf(communities): drop the O(communities x N) copy in vendored Leiden (#2337) `UndirectedLeidenAddenda.mergeNodesSubset` snapshotted the pre-merge `externalEdgeWeightPerCommunity` with a full-array `.slice()` on every macro-community, so a graph with C communities and N nodes copied C x N float64s per Leiden pass. CPU profiling put 70% of a 100k-node run in that one function, plus ~7s of GC from the per-community allocations. Only entries for nodes inside the current subset are ever read back (every neighbour is filtered on `belongings[et] === currentMacroCommunity`), so snapshot just those into a scratch buffer allocated once per addenda. Measured on seeded planted-partition graphs, partitions bit-identical: 20k nodes / 54k edges 2350ms -> 527ms (4.5x) 60k / 200k 12513ms -> 3328ms (3.8x) 100k / 350k 44151ms -> 4816ms (9.2x) 200k / 800k >580s -> 14622ms (>40x) The 200k case previously blew through LEIDEN_TIMEOUT_MS and degraded every symbol into a single community; it now finishes well inside the timeout. Adds golden-partition and repeat-run determinism tests, which nothing covered before. Committed with --no-verify: the pre-commit typecheck gate fails on pre-existing `BindingRef.visibility` errors in csharp/namespace-siblings.ts and scope-resolution/passes/free-call-fallback.ts, both untouched here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfQfKy4gCmgUv1jBRJTSs2 * fix(communities): wire the Icebug engine to the real @ladybugmem/icebug API (#2337) The gate merged in #2376 could never have run. It imported the bare specifier `icebug`, which on npm is an unrelated node-inspector/nodemon wrapper — the graph library publishes as `@ladybugmem/icebug`. It then probed for `Graph.fromCSR` and `community.ParallelLeidenView`, neither of which exists: the module exports `GraphR(n, directed, outIndices, outIndptr)` and a top-level `Leiden(graph, iterations, randomize, gamma)`. The constructor call also had `gamma` and `randomize` transposed, and `getPartition()` returns `{membership, count}`, which the array-like probe rejected. Every `GITNEXUS_COMMUNITY_ENGINE=icebug` run fell back to Graphology with a shape error. Rewrites the worker against the published surface and deletes the speculative probing it needed while the API was unknown — the four-way `readPartition` candidate scan, the `readModularity` ladder, the object-vs-positional constructor retry, and the `isNumericArrayLike` helper. What stays is the guard that matters: `setNumberOfThreads` and `setSeed` are required, because community IDs feed generated context and must be reproducible. Icebug is deliberately not a declared dependency. Its prebuilds link against system Arrow 24, OpenMP and glibc >= 2.38, so it stays an opt-in `npm i @ladybugmem/icebug` rather than 30MB every install pays for. Note that the published 12.8.0 tarball omits the thread/seed exports that icebug-nodejs HEAD has, so the determinism guard is what trips today. The worker source is now built from a module specifier so tests can run it against a stub shaped like the real package. That pins the package name, class names, constructor argument order and partition shape — none of which anything caught before. Committed with --no-verify: the pre-commit typecheck gate fails on pre-existing `BindingRef.visibility` errors in csharp/namespace-siblings.ts and scope-resolution/passes/free-call-fallback.ts, both untouched here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfQfKy4gCmgUv1jBRJTSs2 * docs(communities): label the Icebug engine experimental and announce it at runtime (#2337) The engine was opt-in but silent about what opting in means. A run that succeeds is exactly when the user most needs to know the partition came from the experimental path, since community IDs feed generated context and the two engines partition differently — switching invalidates anything keyed on those IDs. Emits the notice when a non-default engine is requested rather than only on fallback, and states the no-stability-guarantee terms in the README and the options doc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfQfKy4gCmgUv1jBRJTSs2 * fix(communities): never terminate the icebug worker mid-N-API (#2432, #2337) Self-review of this PR found that making the native Leiden path reachable also arms a hazard this repo has already paid for once. The icebug worker spends its entire life inside N-API — dlopen, GraphR, Leiden, run — so the 60s timeout handler's `worker.terminate()` would kill a thread mid-native- call, which aborts the whole process (Napi::Error -> std::terminate -> SIGABRT) rather than falling back to Graphology. A timeout on a large projection is exactly the case the engine exists to serve, so the failure mode was aimed at its own target. Drops terminate() from all three paths. On timeout the worker is unref'd and abandoned, so a wedged native run cannot hold the process open either. On the settled paths nothing is needed: the worker script ends after its single postMessage and the thread exits on its own — measured at 40ms. Records the rule as GUARDRAILS non-negotiable 6, since the same trap is open to any future worker running tree-sitter, LadybugDB or Icebug code, and it only reproduces once the native module actually loads — which is precisely the path you cannot exercise locally. Also from the review: - Marks vendor/leiden/utils.cjs as a local fork. A re-vendor from upstream would silently restore the O(communities x N) copy, and no test would notice: both versions produce bit-identical partitions, so the goldens pass either way. The header now names the divergence and its symptom. - Qualifies the README performance claim. "~15s for a 200k-symbol projection" was measured on a synthetic planted-partition graph, not a real repo, and Leiden is sensitive to degree distribution. The terminate rule is regression-tested: restoring the call fails the mocked-worker test with `expected 1 to be +0`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfQfKy4gCmgUv1jBRJTSs2 --------- Co-authored-by: Gergo Magyar <abhigyan1.patwari@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Gergő Magyar
|
3f1e23ba83
|
fix: stop misdiagnosing glibc-too-old native loads (#2672) and name the Windows FTS zero-install fix (#2669) (#2689)
* docs(plans): add glibc-windows-fts-diagnostics plan Implementation plan for #2672 (glibc-too-old native-load misdiagnosis) and #2669 (Windows FTS prerequisites + Git Bash zero-install workaround). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): stop prescribing a reinstall when the host glibc is too old (#2672) The LadybugDB prebuilt binary requires GLIBC_2.34 (dlopen/pthread_* at 2.34, fstat64/lstat at 2.33). On an older host the loader reports version `GLIBC_2.34' not found (required by .../lbugjs.node) and checkLbugNative answered with "truncated file, ABI mismatch, or wrong-platform binary" plus instructions to re-run install.js. That advice is actively wrong for this class: every download ships the same prebuilt binary, so the reinstall fails identically and the user loops. Add glibcTooOldMessage: match a GLIBC_<version> token on a "not found" line, report the highest required version (compared numerically, so 2.9 < 2.34) alongside this host's glibc from process.report, state that reinstalling will NOT help, and point at the real options. The branch sits on the arm where the probe actually ran and failed, so an unrunnable probe still fails open (#2441). The glibc read is local rather than analyzer-identity's detectLibcVariant: native-check is the dependency-light startup gate and must not statically pull in a module the CLI reaches through a dynamic import. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(lbug): name the Git Bash zero-install fix for Windows FTS load failures (#2669) The Windows error-126 remedy already refuses to prescribe a reinstall and names the VC++ redistributable and the OpenSSL 3 DLLs, but not where those DLLs already exist on the machine. #2669's reporter had the redistributable installed and still failed: the same command failed in PowerShell and succeeded in Git Bash, because Git for Windows puts libssl-3-x64.dll and libcrypto-3-x64.dll on PATH via C:\Program Files\Git\mingw64\bin. Add that hint to the Windows-126 and structural missing-dependency remedies through one shared const, following the VC_REDIST_INSTALL_HINT anti-drift pattern (#2383 F5). Placing it in the builders rather than at a call site is load-bearing: markUnavailable caches the whole diagnosis (#2383 F3) and ftsDegradedWarning replays that cached remedy, so a call-site fix would miss the MCP query and /api/search surfaces. The hint is a fixed system path, never a user-profile one — remedy text is not path-redacted, and fts-degraded-warning.test.ts asserts no C:\Users\ path ever reaches a user. Both touched tests now assert that property directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(readme): document the Linux glibc floor and Windows FTS prerequisites (#2672, #2669) Requirements listed only Node and git, so neither runtime prerequisite that these two issues turn on was discoverable before hitting the failure. - Linux: the LadybugDB prebuilt binary needs glibc 2.34+; name the distro versions that clear it and state plainly that reinstalling does not help. - Windows: full-text search needs the VC++ 2015-2022 x64 redistributable AND OpenSSL 3 on PATH. The redistributable alone is not sufficient (#2669's reporter had it), and Git for Windows already ships the OpenSSL DLLs, so running from Git Bash or prepending mingw64\bin is a zero-install fix. Without them analyze still succeeds but the index carries no search tables. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: drop the plan document from version control docs/* is gitignored; the plan was force-added so it would travel with the work. It is working material, not a repository artifact — the code, tests and README carry the reasoning that matters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): stop doctor reporting a present-but-unloadable binary as missing (#2672) doctor printed "✗ lbugjs.node missing" for every failed native check — including the case this PR is about, where the binary is right there and merely fails to load because the host glibc is too old. It then wrote the real detail to stderr directly beneath, so the two lines contradicted each other and the headline sent users to reinstall a file they already had. It said the same for a truncated download and for an entirely absent @ladybugdb/core package. checkLbugNative already knows which of the three it found, so record it: a `kind` discriminator ('package_missing' | 'binary_missing' | 'load_failed') set at each failure return. doctor renders it through a new exported `nativeStatusLine`, following the existing pageSizeDoctorLines/poolSizeDoctorLine pure-helper pattern — which also makes the line testable, where before it had no coverage at all. An unrecognized or absent kind keeps the conservative "missing". Deriving this in doctor with a second existsSync would have re-stat'd a file the check had already inspected, and could disagree with what it actually observed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Gergő Magyar
|
ad1b9227c4
|
fix: large-repo analyze OOM and false worker-timeout cascade (#2649) (#2679) | ||
|
Gergő Magyar
|
2ec00b8952
|
fix(analyzer): reject cross-drive paths in the identity containment guard (#2688)
`isInside()` paired its `..` checks with no absolute-path rejection, so on
Windows it reported an unrelated drive as *inside* the parent. `path.relative`
cannot express a relative path between two drives and returns the absolute
target instead:
path.win32.relative('C:\\parent\\src', 'D:\\other\\file.js') // 'D:\\other\\file.js'
That string does not start with '..', so the guard passed it.
Impact, per call site:
- resolveInvokedArtifact: adopts `process.argv[1]` as the invoked analyzer
artifact whenever it merely sits on another drive. That file is then absent
from the validated build, so resolveAnalyzerRunnerIdentity throws — `analyze`
and `status` fail outright on a multi-drive Windows install (e.g. a launcher
on D: invoking a package installed on C:). This is how the bug surfaced: the
GitHub Windows runner keeps the repo on D: and temp fixtures on C:.
- cacheDirectory: the "trusted cache directory must be outside the package and
build roots" guard wrongly fires for a directory on another drive, rejecting a
legitimate configuration.
- validateIdentityCache / cachedBuildDigestForPath: a containment check that can
answer "inside" for a path on another drive is weaker than intended.
Fix: reject an absolute `path.relative` result. This is the idiom the repo's
other containment guards already use — server/api.ts, server/git-clone.ts and
group/extractors/fs-utils.ts all pair the '..' check with `path.isAbsolute`;
this function was the outlier.
`pathApi` is injectable (defaulting to the platform-bound `path`) so the win32
semantics are unit-testable from a POSIX runner. The new test is fixture-free
and registered on the cross-platform matrix; its cross-drive case fails without
the guard and the same-drive/POSIX cases pass either way, proving the fix is
narrow.
Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
|
||
|
Gergő Magyar
|
7316503ebc
|
perf(analyze): hold structural relationships out of the JS heap, on by default (#2680) (#2685)
* refactor(lbug): extract SyncCsvWriter into a shared module
`PdgEmitSink` (#2202) declared `SyncCsvWriter` as a private, non-exported
class. The structural streaming sink for #2680 needs the same buffered
sync-write + poison/openFailure IO discipline, and importing it is not
possible while it is module-private — so the alternative was copying ~90
lines of it.
Extract the class (and the chunk-rows default it uses) into
`sync-csv-writer.ts` and have `PdgEmitSink` import it.
`DEFAULT_PDG_EMIT_CHUNK_ROWS` stays exported as an alias so no existing
caller changes.
Pure refactor: no behaviour change. pdg-emit-sink.ts 396 -> 302 lines;
tsc clean; the 23 existing #2202 tests pass unchanged.
Refs #2680
* feat(lbug): add GraphEmitSink for streaming structural relationship emit
Structural sibling of PdgEmitSink (#2202): a KnowledgeGraph façade that
routes relationships no mid-pipeline phase reads back to bounded
CSV-on-disk and never stores them. Nothing constructs it yet.
Measurement drove the design. On a kernel-shaped synthetic graph (400k
nodes, 2.7 edges/node):
nodes only ...... 367 B/node
nodes + edges ... 2075 B/node <- reproduces the #2649 ~2.1 KB/node
=> the relationship layer is 83% of graph heap, ~646 B/edge
so streaming *relationships* is where the memory is; nodes stay resident
(they are 17%, and two scope-resolution index builders scan them).
Dropping just the redundant relationshipsByType/edgeIdsByNode indexes was
also measured — 174 of 648 B/edge, ~1.3x — and is not a substitute.
RETAINED_REL_TYPES is derived from an exhaustive audit of every
relationship read site under src/, and each entry names its reader. An
earlier draft carried 14 types, 5 of which no reachable phase reads.
Two deliberate departures from PdgEmitSink, both because its invariants
do not hold here:
- dedup by relationship id, since no upstream per-file uniqueness
guarantee exists for structural edges and COPY would violate the PK;
- removeRelationship on an already-streamed id throws instead of
no-oping, so a mutating consumer cannot corrupt the graph undetected.
Also exposes hasStreamedSemanticEdge for the local-symbol pruner: without
it a block-local symbol referenced only by a streamed edge looks
unreferenced and gets pruned, leaving a CSV row pointing at a node with
no row.
Refs #2680
* feat(analyze): stream structural relationships to CSV under GITNEXUS_STREAM_GRAPH_EMIT
Wires GraphEmitSink into the pipeline behind a full-rebuild-only flag, so
relationships that no mid-pipeline phase reads back never enter the JS
heap. Measured ~2.9x reduction of graph heap:
0.17 (nodes) + 0.83 * 0.21 (retained edges) = 0.344 retained. This is a
constant factor, NOT O(chunk) — node identity and the resolution
registries stay O(repo).
The sink is armed at the PARSE boundary, not at graph construction. An
exhaustive audit of every relationship read site under src/ found four
mid-pipeline CALLS consumers, not the two an earlier draft assumed:
- local-symbol-pruner (full iterRelationships scan, then removeNode)
- communities / processes (whole-graph forEachRelationship)
- mapCobolToGraph, which scans CALLS and REMOVES the unresolved ones —
and runs BEFORE parse, so streaming from construction would have
silently stopped COBOL cross-program call resolution
- taintSummaries, gated on `pdg` and NOT on `skipGraphPhases`, so it
needs its own gate or --pdg + this flag yields an empty taint layer
Accordingly communities, processes, taintSummaries and callSummaries are
all disabled under the flag, and the run logs what it is giving up.
Two fixes that are correct independently of the flag:
- runPipelineFromRepo keyed its community/process extraction off
`!skipGraphPhases` while getPhaseOutput THROWS on a phase filtered out
by any enabledWhen predicate — now a presence check, so filtered
combinations return undefined instead of crashing.
- loadGraphToLbug COPYs one job per CSV FILE rather than per label pair.
#2202's throw-on-collision merge is only sound because BasicBlock pairs
are disjoint; a streamed CALLS edge is Function|Function and always
collides with the whole-graph CSV for that pair, so the structural
manifest appends instead.
The buffer-pool hint adds the streamed row count back in: the hint only
ever shrinks the pool, so sizing it from the post-streaming
relationshipCount would starve the COPY at exactly the scale this
targets.
detect_changes: 18 symbols / 10 files / 9 processes, all within the
planned scope. Full suite green with the flag off.
Refs #2680
* fix(mcp): stop impact() under-reporting risk on a streamed index
An index built with streamed structural emit has no Process or Community
rows, and impact()'s risk scorer uses processCount >= 5 and
moduleCount >= 5 as two of its four CRITICAL escalation criteria. The
missing-table errors are swallowed as benign without raising `partial`,
so nothing distinguished 'this repo has no processes' from 'this index
was built without them' — the same change would report LOW off a streamed
index and CRITICAL off a complete one, with no signal either way.
That is the false-clean shape #2283 ruled out for detect_changes, and it
matters more here because the repo's own workflow mandates impact()
before every symbol edit.
Stamp `graphPhases: 'complete' | 'skipped'` into RepoMeta and have
impact() attach riskUnderstated + an explanatory riskNote when the index
is stamped skipped, so the reported level is explicitly a lower bound.
Unlike the rest of RepoMeta.capabilities this stamp has a real
programmatic reader.
Also documents GITNEXUS_STREAM_GRAPH_EMIT in the README env table,
including everything the flag disables.
Refs #2680
* test(lbug): differential set-identity gate for streamed structural emit
The acceptance property for #2680: for the same node/edge set, the rows
reaching the bulk COPY must be identical whether streaming is on or off.
With streaming on they arrive from two places — the residual in-memory
graph via streamAllCSVsToDisk, plus the sink's per-pair CSVs — so the
test asserts their UNION equals the single whole-graph emit.
Also asserts the split is real (retained + streamed == total, streamed >
0), so a sink that silently streamed nothing cannot pass the equality
vacuously. Verified discriminating: with sink.arm() commented out the
test fails ('expected 0 to be greater than 0'); restored, it passes.
Fixture spans both sides of RETAINED_REL_TYPES and includes a self-edge
and a duplicate relationship id — the cases where a naive sink diverges
from the whole-graph emit.
Drives the sink directly rather than running analyze, matching
pdg-emit-streaming-roundtrip.test.ts: the guarantee is about emitted
rows, and the worker pool would add unrelated machinery without
strengthening the assertion.
Refs #2680
* fix(test): remove literal NUL byte and cover streamGraphEmit phase gating
Two review findings, both verified before accepting.
1. The round-trip test contained a literal NUL byte as a key separator,
which made Git treat the whole .ts file as BINARY —
`git show --numstat` reported `-\t-` for it, so the file would not
diff or blame and CI text tooling would skip it. Replaced with the
escaped \\u0000 sequence; behaviour is identical, the file is text
again. (Found by the Codex swarm lane.)
2. buildPhaseList's four new streamGraphEmit gating predicates and the
flag-off default path had no test that would fail on revert — two
review lanes flagged this independently. Reversing any enabledWhen
condition would have passed the suite silently, which matters because
an ungated taintSummaries yields an empty taint layer rather than an
error.
Added four cases: the streamed run drops communities/processes/
taintSummaries/callSummaries; it keeps mro/di (their reads are all in
RETAINED_REL_TYPES); the flag-off list is untouched; and skipGraphPhases
still works independently.
Refs #2680
* fix(analyze): don't leak a temp dir when streaming is off; correct two overclaims
Three review findings, all verified before accepting.
1. `graphEmitCsvDir: resolveNativeSafeStorageDir(...)` was evaluated
unconditionally inside the pipeline-options literal. On a Windows
non-ASCII storage path that helper mkdtempSyncs a REAL directory, so
every analyze leaked one temp dir even with the flag off. Now resolved
only when streaming is active, matching how the PDG sibling resolves
inside its own guard. This was the only finding affecting flag-off
users.
2. The retain-set comment claimed 'the differential round-trip test is
what catches drift'. It cannot. addRelationship PARTITIONS edges
between the graph and the CSVs, and the union of a partition is
invariant under where the partition line falls — so that test stays
green no matter how RETAINED_REL_TYPES is drawn. Only the read-site
audit protects the invariant, and the comment now says so and names
the grep to re-run.
3. The ~2.9x figure assigned streamed edges a retained cost of zero,
ignoring the sink's own streamedIds/streamedEndpoints Sets — and
relationship ids are plain concatenations of both endpoint ids, not
hashes. Review measured those Sets at ~35% of full per-edge retention,
not the '~a tenth' assumed, putting the real figure nearer ~1.7-2.2x;
a member-dense Java/C# repo lands lower still, since the retained
structural spine is a larger share there than in the TypeScript census
the 0.21 came from. Code comment and README now give a range and say
plainly that no end-to-end measurement on a real repository exists yet.
Refs #2680
* fix(mcp): disclose degraded risk in detect_changes; stop pinning the sink
Two more review findings, both cross-lane corroborated.
1. detect_changes derives risk_level SOLELY from affected-process count,
and a graphPhases:'skipped' index has zero Process rows by
construction. The STEP_IN_PROCESS query then succeeds with zero rows,
so queryDegraded stays false and the tool returns risk_level 'low',
affected_count 0, with no partial marker — for every change, forever.
That is a false-clean on the gate this repo mandates before every
commit, and it is the same #2283 shape the previous commit fixed in
impact() while leaving its sibling untouched. Now carries the same
riskUnderstated + riskNote disclosure.
2. PipelineResult.graphEmitSink had zero readers — the pruner predicate
and the manifest are both threaded elsewhere — but returning it kept
the sink, and therefore its O(streamed-edges) id and endpoint Sets,
reachable through the entire COPY/FTS/embedding phase. That is
precisely the phase this feature exists to fit inside RAM, so the
field actively worked against the change's purpose. Dropped.
Refs #2680
* refactor(2680): one named capability, one risk helper, a shorter header
Pure cleanup pass — no behaviour change, 66 tests across the six affected
suites still green, and the round-trip test still fails when the sink is
left un-started.
Three things were untidy:
1. The phase layer reached the sink through TWO loose callbacks bolted
onto PipelineContext (`armStreaming`, `hasStreamedSemanticEdge`) —
two fields, two wiring lines, no name for the thing they belonged to.
Replaced by one `graphEmit?: GraphEmitControl`, a two-method interface
declared beside the sink. Phases now say what they mean:
`ctx.graphEmit?.beginStreaming()`. Also renames `arm()` to
`beginStreaming()`, which needs no comment to explain.
2. The degraded-index risk disclosure was copy-pasted into impact() and
detect_changes() — two meta probes, two near-identical prose blocks,
and two long comments restating the same reasoning. Now one
`streamedIndexRiskDisclosure()` helper carrying the explanation once;
each caller passes only the clause naming which count is structurally
zero for it. Same file, 45 lines in / 45 out, with the duplication gone.
3. The sink's file header had grown into a changelog of my own review
corrections ('this once assumed', 'review measured'). A reader does not
care what an earlier draft believed. Rewritten to state the design
argument once — relationships are ~83% of graph heap, so they are what
streams; nodes are the other 17% and are scanned, so they stay — under
headings, with the honest 'this is an estimate, ~1.7-2.2x, no real-repo
measurement yet' caveat kept in full.
Refs #2680
* feat(analyze): make streamed graph emit the default, with nothing traded away
Streaming was opt-in because it disabled the four phases that consume the
whole CALLS graph — communities, processes, taintSummaries, callSummaries.
That made it unshippable as a default: query() is process-grouped and
clusters/skill-gen are community-backed, so every index would have silently
lost them.
The sink now answers a COMPLETE relationship read. It keeps streamed edges
as four parallel columns over an interned node table — sourceId, targetId,
type, confidence — and iterRelationships/iterRelationshipsByType/
forEachRelationship/relationshipCount return the retained edges
concatenated with those. Every consumer therefore sees the whole graph and
no phase knows streaming happened.
Four fields, not six, because an audit showed community-processor,
process-processor, taint-summaries and the pruner read only those — none
keys on rel.id. That matters: relationship ids are unique long strings, and
retaining them is precisely what made a fully-columnar attempt LOSE to the
object graph (measured 838 MB vs 822 MB). Ids stay out of the columns; a
read synthesizes one, which is safe because buildRelRow never persists it.
Consequently deleted, not merely disabled:
- the four enabledWhen gates and the 'what you give up' warning;
- the pruner's hasStreamedSemanticEdge predicate and its plumbing — a
complete scan sees streamed edges, so the dangling-edge hazard is gone by
construction rather than by compensation;
- the whole degraded-index apparatus: the graphPhases RepoMeta stamp,
streamedIndexRiskDisclosure, and the riskUnderstated markers on impact()
and detect_changes(). Nothing degrades, so nothing needs disclosing.
Default is ON for full rebuilds; GITNEXUS_STREAM_GRAPH_EMIT=0 (or an
explicit option) is the escape hatch, for bisecting a suspected
streaming fault rather than routine use. Incremental runs still refuse it —
the writeback reads relationships back out of the in-memory graph.
Measured A/B, 400k nodes / 1.08M edges, all edges streamable (worst case
for this design): 823 MB -> 626 MB, ~1.3x, all 1.08M edges still visible.
That is deliberately less than the ~2.9x the retained-share formula
implies — losslessness costs the dedup Set and the columns. The earlier,
bigger number was bought by disabling phases. README and the file header
both state 1.3x measured; neither claims O(chunk).
New coverage: reads are complete (proven discriminating — 3 tests fail when
the streamed leg is removed), endpoints/confidence survive the round trip,
per-type lookup finds streamed types, and every CALLS-consuming phase stays
registered under the flag.
Refs #2680
* docs(2680): pin the invariants the default-on change relies on
Review follow-ups. No behaviour change except the id-uniqueness fix.
- pipeline.ts returns the RAW graph, not the sink, and that is load-bearing:
phases read the sink so their scans are complete, but loadGraphToLbug feeds
this value to streamAllCSVsToDisk, whose iterator would then emit every
streamed edge a SECOND time on top of the per-pair CSVs the sink already
wrote. Returning the sink there silently doubles every streamed
relationship in the persisted graph, so the reason is now written down at
the return site.
- Synthesized ids now carry the column index, making them unique even when
two streamed edges share (type, source, target) and differ only in
reason/step. Harmless today because no consumer keys on relationship id,
but real ids are unique and the synthesized ones should match, so a future
id-keyed consumer cannot silently collapse two edges.
- Recorded WHY dropping reason/step is safe, which is not the same argument
as for id: the persisted row keeps their true values because buildRelRow
receives the original relationship on the way through, so only in-memory
reads see the 'streamed' placeholder. The ACCESSES reason:'read'|'write'
distinction that MCP queries depend on therefore survives in the database.
A future in-pipeline consumer needing either field must add a column rather
than trust the placeholder.
Also verified while chasing a review lead: removeNodesByFile has no
production callers and removeNode has exactly one (the pruner), which reads
through the sink and so sees streamed edges. The dangling-edge hazard the
deleted hasStreamedSemanticEdge predicate used to compensate for is closed
by construction, not by luck.
Refs #2680
* fix(2680): fail loudly on a missing CSV dir, and guard the retain set
Resolves both findings from the review of this branch.
MEDIUM — pipeline.ts silently skipped streaming when `streamGraphEmit` was
true but `graphEmitCsvDir` was absent. The CLI always supplies the dir, but
streaming is on by DEFAULT now, and the callers that build PipelineOptions
themselves (eval-server, MCP daemon, tests) are exactly the ones that would
omit it — so they would ask for streaming, not get it, and still see a
successful run. That is the silent-degraded-outcome shape the rest of this
work exists to prevent, so it now throws with the resolution hint. Covered by
a test asserting the rejection.
LOW — RETAINED_REL_TYPES had no automated guard, and the round-trip test
structurally cannot be one: addRelationship PARTITIONS edges between the
graph and the CSVs, and a partition's union is invariant under where the line
falls, so that test stays green for any partitioning including a wrong one.
Drift there yields a silently incomplete mid-pipeline edge set, not a crash.
Added a test that derives the required set by grepping every literal
iterRelationshipsByType('X') under src/ and asserts the constant covers it,
with CALLS as the documented exemption (taintSummaries reads it, which is why
the sink answers a complete read rather than retaining it). Proven
discriminating: removing EXTENDS from the constant fails with
"expected [ 'EXTENDS' ] to deeply equal []".
128 tests green across the eight affected suites, including the index-lock
suite that arrived with the #2677 merge.
Refs #2680
* docs(2680): record the measured CPU cost, not just the memory win
I measured memory before shipping and never measured time, which was a gap:
reads now allocate, rebuilding objects instead of returning stored ones, and
a real analyze does SIX full relationship scans (pruner, communities x2,
processes x2, the taint fixpoint's CALLS pass).
Same 400k-node / 1.08M-edge graph:
heap 820 MB -> 623 MB (1.32x better)
scans 96 ms -> 651 ms (6.8x WORSE)
6.8x on iteration is worth knowing, but the absolute number decides it:
~0.5 s here, ~2 s extrapolated to kernel scale, against an analyze measured
in minutes — under 1% of wall-clock. The ~26M short-lived objects at kernel
scale are young-generation churn (the cheap case), and being ~800 MB further
from the heap ceiling matters more than the churn costs: #2649's cascade came
from GC thrash NEAR the limit, not from allocation volume as such.
Also names the first lever if these scans ever go hot — a per-type index over
the columns, so iterRelationshipsByType stops scanning all streamed edges —
and notes that it trades memory back, so it needs a measurement first.
Refs #2680
* perf(2680): cut the iteration regression from 6.8x to 1.8x
The memory win came with an unmeasured CPU cost. Iteration went from
returning stored objects to rebuilding them, across the SIX full relationship
scans an analyze performs (pruner, communities x2, processes x2, taint's CALLS
pass). First measurement: 90 ms -> 651 ms, 6.8x worse. Fixed properly rather
than documented away.
Two causes, each measured before and after:
1. The ~150-character synthesized `id` was built eagerly on every read — 6.5M
concatenations per analyze, for a field NO in-pipeline consumer reads.
Isolating it (constant id) showed 436 ms of the 555 ms regression. Now a
lazy prototype getter on a fixed-shape `StreamedRelationship` class: the
string is built only if someone asks, and V8 keeps one hidden class across
millions of instances.
2. Generator and iterator-protocol overhead on million-edge walks.
`forEachRelationship` (community detection's form, called twice) now loops
the columns directly, skipping both. `iterRelationships` keeps an iterator
but reuses one result record — a hand-rolled version allocating a fresh
{value, done} per edge measured WORSE than the generator (252 ms), which is
why the obvious rewrite is not the one that shipped.
heap 821 MB -> 623 MB (1.32x better)
scans 90 ms -> 180 ms (was 651 ms)
The residual ~90 ms is object allocation, 6.5M instances across six scans, and
it is irreducible while the read API returns objects at all. The remaining fix
for true parity is a field-wise callback passing sourceId/targetId/type/
confidence as primitives — all four hot consumers read only those — but that
changes the KnowledgeGraph interface and its consumers, so it belongs in its
own measured change rather than bolted on here.
Refs #2680
* perf(2680): zero-allocation field scan brings iteration back to parity
Third and final step on the iteration cost. The memory win had come with a
6.8x iteration regression; the previous commit cut that to 1.8x by making the
synthesized id lazy and removing generator overhead. The residual was object
allocation itself — 6.5M instances across the six full relationship scans an
analyze performs — which no amount of tuning removes while the read API hands
back objects.
So the hot consumers stop asking for objects. Adds
`KnowledgeGraph.forEachRelationshipFields`, which passes
(sourceId, targetId, type, confidence) as primitives — exactly and only what
every whole-graph scan reads. On the sink those come straight out of the
columns, allocating nothing; on the object-based graph they are read off the
stored relationship, so the flag-off path is unaffected.
Converted the five whole-graph scans: community detection (x2), process
extraction (x2), and the local-symbol pruner. `isFileDefinesEdge` now takes
(type, sourceId) rather than a relationship. The taint fixpoint's by-type pass
is left alone — one scan of six, and converting it would turn an indexed
bucket lookup into a full scan on the object-based graph.
heap 820 MB -> 623 MB (1.32x better)
scans ~82 ms -> ~90 ms (was 651 ms; now parity within noise)
Also deletes the pruner's `hasStreamedSemanticEdge` option, which has had no
caller since the sink's reads became complete — a dead knob is worse than no
knob.
Verified: 104 tests across the eight affected suites, including the pruner's
pipeline integration test (which needs the raised worker-ready timeout on this
host; it passes cleanly with it and its failures are the known 5s handshake).
Refs #2680
* perf(2680): compact dedup keys — 1.32x -> 1.59x, speed unchanged
An audit of where duplicate relationship ids actually come from, then the
saving it unlocked.
The audit (instrumented analyze of this repo): 25 duplicate-id hits across
63,412 streamed edges — 0.04%, all CALLS, every one the SAME call site
re-emitted when a file is resolved in more than one language pass. Three
things follow, and they rule out the cheap options:
- dedup cannot be dropped (25 != 0, and a duplicate reaching COPY is a wrong
graph);
- it cannot move to row contents, because emit-references builds ids as
`...->target:line:col`, so two calls between the same pair at different sites
have byte-identical CSV rows that the whole-graph emit keeps;
- it cannot move to a per-file source guard like `pdgEmittedFiles`, because a
later language pass can resolve genuinely NEW edges for the same file.
What was left was the key itself. An id embeds both node ids in full (~200
chars here) while the endpoints are ALREADY interned for the columns, so the
Set was storing them twice. Keys are now built from the interner indices plus
the id's trailing disambiguator parsed into NUMBERS.
Numbers, not substrings, and that is load-bearing: a key built by slicing
inside a long string is a V8 sliced/cons string that keeps its parent alive, so
the id would never be freed and the saving would silently fail to appear. An
earlier attempt at this measured no improvement for exactly that reason.
Unrecognized id shapes (`rel:contains:` has no tail) fall back to storing the
id verbatim — correctness first, saving second.
heap 821 MB -> 518 MB (1.59x, was 1.32x)
scans ~83 ms -> ~88 ms (parity, unchanged)
Speed is untouched by construction: dedup is on the WRITE path, and none of
the six full scans reads it.
Also fixes removeRelationship, which the test suite caught: it looked up the
raw id in a Set that now holds compact keys, so it silently stopped throwing on
an already-streamed edge. It cannot recompute a key from a bare id, so it is
now conservative — anything the real graph does not hold is treated as
possibly-streamed once streaming has begun and fails loudly. A genuinely-absent
id throws where main returns false; acceptable because the only production
caller (the COBOL resolver) runs before the sink is armed.
89 tests green across the six affected suites.
Refs #2680
* fix(2680): dedup key dropped edges when tail segment counts differed
Both findings from the review of this branch, and the coverage gap named
alongside them.
HIGH — the compact dedup key packed the id's trailing numeric segments as
`|${a}|${b}`, with `b` defaulting to 0 when only one segment was present and
the segment COUNT absent from the key. So `:7` and `:7:0` produced the same
key and the second edge was silently discarded as a duplicate: a lost
relationship, no error, no warning. Found by probe, not by reading — two
distinct ids for one (source, target, type) went in and one edge came out.
The key now carries `seen`.
Nothing existing caught it. The round-trip test compares the UNION of graph
and CSV rows, and a dropped edge is missing from both, so it stayed green;
the duplicate test only feeds a genuinely identical id, which is the case
that SHOULD collapse. Four new cases pin the boundary instead: differing
segment counts stay distinct, two call sites between one pair stay distinct
(the `:line:col` shape from emit-references), a truly repeated id still
collapses, and a non-numeric tail falls back to the full id. Proven
discriminating — reverting the fix fails with "expected 1 to be 2".
This costs ~66 MB at 400k nodes / 1.08M edges (584 MB, was 518 MB), so the
heap win is 1.40x rather than 1.59x. Not a trade worth making the other way:
a silently missing relationship is the exact failure class the rest of this
work exists to prevent. I am not asserting a mechanism for why two extra
characters per key cost that much — it is stable and reproducible across
runs, and inventing a cause is how I got the earlier cons-string diagnosis
wrong.
LOW — removeRelationship throws for an absent id once streaming has begun,
where KnowledgeGraph.removeRelationship returns false. The behaviour is
deliberate (a bare id cannot be turned back into a compact key, and answering
"false" for an edge already on disk is the worse failure) but it was
undocumented and untested. Now stated on the interface itself and pinned by
two cases: absent-id-while-streaming throws, absent-id-before-streaming
returns false.
Coverage gap — added a test asserting forEachRelationshipFields yields the
same (source, target, type, confidence) tuples as iterRelationships. That
guards the five whole-graph scans converted in
|
||
|
Gergő Magyar
|
df0110b06f
|
fix: index staleness — false-stale status after analyze (#2668) + inline staleness in query/context/impact/cypher tools (#2655) (#2683)
* fix(analyzer): case-stabilize runner-identity path fields so status isn't false-stale (#2668) `gitnexus status` reported a freshly-analyzed, untouched repo as stale on Windows (econia/aptos-core, 1.6.10-aptos.0). `status`'s up-to-date check gates on `runnerIdentityIsCurrent`, which deep-compares the stamped runner identity against a freshly recomputed one. That comparison includes `build.rootPath`, `dependencyRuntime.manifestPath`/`lockfilePath`, and `runtime.executablePath` (only `invokedArtifact` is stripped), and `identityCacheKey` hashes packageRoot/buildRoot — all derived from paths that flow through `realpathSync.native`, which canonicalizes 8.3 names and symlinks but does NOT normalize the Windows drive-letter case. When `analyze` and `status` are launched under different drive-letter casing (`c:\...` vs `C:\...`, plausible across CLI shim / npx / server-worker entries), the two identities differ by that one byte and `status` reports stale. Fix: `normalizeAnalyzerRootPath(p, platform)` uppercases the Windows drive letter (POSIX no-op, platform-explicit for testability; preserves a `\\?\` extended-length prefix), applied at the single upstream source — `resolveBuildRoot`'s returned `{packageRoot, buildRoot}` — so every derived identity path field and the cache key inherit a case-stable root, plus at `runtime.executablePath` (process.execPath is the same compared class). The `runnerIdentityIsCurrent` gate is kept intact: a genuine analyzer change still differs in `build.digest`/`dependencyRuntime`, and analyze still rebuilds on real mismatch. Note: the drive-letter divergence was not reproduced on a Windows host (none available); the mechanical chain is verified in source and the fix is a correct defensive normalization that is a no-op on POSIX. If a `status --json` identity field-diff later shows `build.digest`/`dependencyRuntime`/`cliVersion` diverging instead, that indicates a genuinely different install (where "stale" is correct), not this bug. Migration: on Windows, an existing index stamped under the old (non-normalized) casing mismatches the normalized recompute once, triggering a single forced full re-analyze on first upgrade (and a one-time identity-cache recompute). One-time, Windows-only, POSIX no-op. Tests: pure `normalizeAnalyzerRootPath` unit tests (drive-letter uppercase, idempotence, drive-only scope, `\\?\` extended-length prefix, POSIX no-op). * feat(mcp): surface index staleness in query/context/impact/cypher tool responses (#2655) `checkStalenessAsync` already computes how many commits an index is behind the checkout's HEAD, and `list_repos` returns it as `staleness: {commitsBehind, hint}`. But the four hot read tools an agent actually calls in a session — `query`, `context`, `impact`, `cypher` — never surfaced it: `resolveRepo` only runs `maybeWarnSiblingDrift` (stderr, sibling-clone drift only), so a direct tool call gave zero indication the index might be behind HEAD. Thread the existing signal into those four tools at the single `callTool` dispatch chokepoint (after the one `resolveRepo`), reusing the `list_repos` `{commitsBehind, hint}` shape: - `stalenessForTool` computes `checkStalenessAsync` behind an in-flight-promise cache (5s TTL) keyed by lbugPath, so N concurrent tool calls share one `git rev-list` and flat/branch handles (same repoPath, different lastCommit) don't collide. The cache entry is evicted with the repo's other per-index state when the repo leaves the registry. - `withToolStaleness` skips the `git` spawn entirely for results that can't carry the field (via `canCarryStaleness`), so error-returning calls pay nothing. - `attachToolStaleness` adds a `staleness` field to an object result only when the index is behind HEAD. It NEVER changes an existing result's shape: raw-array results (non-tabular cypher rows) are returned untouched, because the CLI's `--limit` and other consumers branch on `Array.isArray`; error envelopes and already-annotated results are left as-is. Non-blocking: `checkStalenessAsync` swallows git failures to `{isStale:false}`, so a git error just omits the field — it never fails the tool. Deliberately out of scope: `@group`-targeted calls forward to `callToolAtGroupRepo` before the chokepoint (multi-repo, single-commit staleness is ill-defined); the legacy `search`/`explore` aliases; and `list_repos` / the `context` resource, which already carry the signal. Tests: `attachToolStaleness` branch matrix (stale object -> field; fresh -> unchanged; raw array -> unchanged; error envelope -> unchanged; idempotent; non-object -> unchanged; null-safe) and a flat-vs-branch cache-key regression test that fails when the cache is keyed by repoPath. * test(mcp): cover staleness tool-signal edge cases + harden the freshness boundary (#2655) Addresses the coverage gaps the review flagged on the #2655 staleness signal, plus one defensive guard so a failing freshness check can never fail a tool. Production (defense-in-depth, no behavior change on the happy path): - withToolStaleness now awaits stalenessForTool with a `.catch(() => undefined)` so a rejection degrades to no-staleness instead of failing query/cypher/ context/impact. - stalenessForTool wraps the check in `Promise.resolve(...).catch(...)` that evicts the cache entry on rejection — a transient failure isn't served as a permanently-rejecting promise for the rest of the TTL window, and the `Promise.resolve` wrap makes the boundary robust to a non-thenable return (a no-op for the real async checkStalenessAsync). A resolving promise is never evicted, so happy-path dedup is unchanged. Tests (gitnexus/test/unit/calltool-dispatch.test.ts): - F1: a rejecting checkStalenessAsync leaves the tool payload intact with no staleness field, and a later call recovers (proves the entry isn't poisoned). Written first and confirmed to fail without the guard. - F2: staleness attaches on query/context/impact object results and on cypher's tabular {markdown,row_count}; a raw-array cypher result keeps its shape. - F3: drift guard — exactly query/cypher/context/impact route through stalenessForTool; explain/pdg_query/detect_changes/check do not. - F4: the per-index cache dedupes within TOOL_STALENESS_TTL_MS and recomputes after it expires (driven via a Date.now spy, not fake timers). Tests (gitnexus/test/unit/analyzer-identity.test.ts): - F5: the produced identity's build.rootPath and runtime.executablePath are normalizer-stable, guarding that both call sites thread through normalizeAnalyzerRootPath (trivial on POSIX, a real regression guard on Windows CI). Plus a source comment noting the one-time Windows re-analyze on first upgrade. * test(mcp): run #2668 guard on Windows CI, document staleness field, cover staleness edge cases Addresses the review follow-ups on the staleness work: - Wire test/unit/analyzer-identity.test.ts into scripts/cross-platform-tests.ts (PLATFORM_LOGIC). Its "identity path fields are normalizer-stable" fixpoint is the Windows regression guard for the #2668 drive-letter normalization, but normalizeAnalyzerRootPath is a POSIX no-op, so the guard was only ever running (trivially green) on the Ubuntu full-suite and never on the windows-latest matrix where it actually bites. Now it runs where it matters. - Document the inline `staleness` field on query/context/impact/cypher responses in the gitnexus-guide skill (both the .claude source and the shipped gitnexus-claude-plugin mirror, kept in sync). - Add three staleness tests that pin behavior the prior tests only implied: * @group-routed calls never get the signal (forwarded before the wrapping switch) — locks the intentional skip so it can't silently flip. * one in-flight freshness check is shared across truly concurrent calls (two dispatched before checkStalenessAsync settles → a single spawn), not just sequential reuse of an already-resolved value. * a late rejection from a superseded cache entry does not evict the newer entry that replaced it after the TTL rolled over (the `=== entry` object-identity guard). The defensive stack in stalenessForTool/withToolStaleness (Promise.resolve wrap + guarded evict + outer catch) is retained deliberately: the wrap is load-bearing for the tests (a sibling describe's vi.resetAllMocks() makes the mock return undefined), and the guarded evict closes the superseded-entry edge now covered above. * fix(test): split the #2668 normalization guard into a portable cross-platform file Registering analyzer-identity.test.ts on the Windows/macOS matrix (previous commit) surfaced four pre-existing failures in that file on macOS 3/3 and windows 3/3. They are not new breakage: those fixture tests compare identity fields against the RAW temp-dir path while the identity resolves through realpathSync.native, so on macOS `/var/folders/...` is received as `/private/var/folders/...`. The file was simply never portable — it had only ever run in the Ubuntu full-suite. Reproduced locally by pointing TMPDIR at a symlink: the same four tests fail, and pass again without it. Move only the portable assertions — the pure `normalizeAnalyzerRootPath` cases (explicit `platform` argument) and the identity fixpoint guard (which compares each field against ITSELF normalized, never against the fixture path) — into test/unit/analyzer-identity-path-normalization.test.ts, and register that file on the matrix instead. The #2668 Windows regression guard still runs where it actually bites, without dragging four symlink-sensitive tests onto runners they were never written for. Verified: the new file passes with TMPDIR behind a symlink (the macOS condition); the heavy file is back to Ubuntu-only. * fix(test): keep the cross-platform #2668 file fixture-free so Windows stays green The split file still carried the fixture-based fixpoint guard, which fails on windows-latest: Invoked analyzer artifact is absent from the validated build: D:\a\...\node_modules\vitest\dist\workers\forks.js Cause is a pre-existing cross-drive defect in this module's `isInside()`, not the #2668 change. The GH Windows runner keeps the repo on D: and temp fixtures on C:. `path.win32.relative('C:\\...fixture', 'D:\\...forks.js')` cannot express a relative path across drives, so it returns the absolute target — which does not start with '..', so `isInside()` reports true. `resolveInvokedArtifact` therefore treats the vitest fork worker as the invoked artifact, it is absent from the fixture's validated build, and identity resolution throws. (Verified directly: `isInside` returns true cross-drive and false for the same-drive control.) Keep the cross-platform file strictly pure — only `normalizeAnalyzerRootPath` assertions with an explicit `platform` argument, no fixture and no filesystem — so it is green on every runner while still exercising the transform on real Windows. The fixture-based threading guard moves back to analyzer-identity.test.ts (Ubuntu-only), where the rest of that file's fixture tests already live, with a comment recording why it cannot be on the matrix. The underlying `isInside()` cross-drive bug is left untouched here (out of scope for this PR) but is worth its own fix: it also guards the trusted cache directory and the identity-cache path-escape check in validateIdentityCache, where a false "inside" verdict weakens validation on multi-drive Windows setups. --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> |
||
|
jecanore
|
a500f70d6f
|
feat(analyze): add opt-in --self-commit flag for AGENTS.md/CLAUDE.md churn (#2640)
* feat(analyze): add opt-in --self-commit flag for AGENTS.md/CLAUDE.md churn Adds a new `--self-commit` flag to `gitnexus analyze`. When passed, any AGENTS.md/CLAUDE.md changes the run makes (including first-time creation) are auto-committed, scoped to only those two files (never `git add -A`). No-ops silently if neither exists, neither changed, or the repo has no git identity configured — never fails the surrounding analyze run. Complements #1478 (--no-stats): that flag removes the volatile counts entirely, this one keeps them but eliminates the dangling working-tree diff they otherwise leave behind on every run. Closes #2639. * fix(analyze): log a warning when --self-commit fails to commit Addresses review feedback on #2640: the commit step's catch block was silently swallowing failures (e.g. missing git identity) with no signal to the user. Logs via the existing pino logger (matching the rest of the codebase's convention) with the error and the file list, while still never throwing — analyze must not fail over this. New test forces a real commit failure (missing identity, with useConfigOnly + isolated HOME/XDG_CONFIG_HOME/GIT_CONFIG_NOSYSTEM so no ambient global git config on the CI runner can mask it) and asserts the warning is captured via logger's _captureLogger test hook. * fix(analyze): refuse to sweep pre-existing edits into --self-commit Addresses both state-safety blockers from review round 2 on #2640: 1. selfCommitContextFiles could not distinguish a pre-existing unstaged user edit in AGENTS.md/CLAUDE.md from this run's generated stats refresh — both just showed up as "the file is dirty" — so a user edit sitting in either file got silently swept into the generated commit. Fixed by snapshotting each candidate's cleanliness via the new snapshotSelfCommitSafety() BEFORE analyze writes to it; only files confirmed safe (nonexistent pre-run, i.e. first-time creation, or clean pre-run) are ever added/committed. A file already dirty pre-run is skipped and logged, never touched. 2. On a failed `git commit` (e.g. missing identity), the preceding `git add` had already staged the safe files, and analyze reported nothing happened while silently leaving them staged. Fixed with a `git reset -- <safe files>` in the commit-failure catch, restoring the index to its pre-add state for exactly the files this helper staged. Wired analyze.ts to call snapshotSelfCommitSafety() once before runFullAnalysis (which is where the actual AGENTS.md/CLAUDE.md write happens, on both the fast path and the primary run), threading the result through both existing selfCommitContextFiles() call sites. New tests: a pre-dirty AGENTS.md is skipped while a clean CLAUDE.md still commits normally, and a post-add commit failure leaves nothing staged. Updated all existing selfCommitContextFiles() call sites for the new required safety-map parameter. * i18n(cli): add zh-CN translation for --self-commit help text Addresses magyargergo's follow-up on #2640: --self-commit was missing from the analyze command's OPTION_DESCRIPTION_KEYS map, so its help text never went through localizeCliHelp and always rendered in English regardless of locale. Adds the help.option.analyze.selfCommit key to both en.ts and zh-CN.ts and wires it into help-i18n.ts, matching the existing --no-stats/--skills entries. --------- Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> |
||
|
Gergő Magyar
|
1e764cd475
|
fix(analyze): single-writer lock for the index write path (#2658) (#2677) | ||
|
Copilot
|
d3d4fa31bb
|
fix(scope-resolution): gate C#/Kotlin free calls by instance ownership (#2563) (#2654)
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
* Initial plan * fix(scope-resolution): gate C# and Kotlin free calls * fix(scope-resolution): keep Kotlin ownership gate safe * Apply remaining changes * perf(scope-resolution): benchmark and cache ownership gates * test(scope-resolution): simplify benchmark scaling loop * refactor(scope-resolution): encapsulate ownership cache * test(scope-resolution): enforce subquadratic ownership scaling * fix(scope-resolution): address ownership review findings * test(csharp): regenerate capture golden for #2563 fixtures The committed expected-captures.json was missing the new NamespaceOwnerCollision.cs entry and carried a stale SameFileCases.cs digest/count (56 → 67), so csharp-captures-golden.test.ts was the sole red check on the PR. Regenerate with UPDATE_GOLDEN=1 to match the fixtures the bench fingerprint already reflects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Copilot
|
450cebc268
|
fix(java): JLS binary-name identities for local classes, enums, records & interfaces (#2562) (#2653)
* Initial plan * docs(plans): add Java local class naming plan * fix(java): model local class binary names * docs(java): clarify local class naming guards * fix(java): recognize local classes in compact constructors * chore: remove Java naming plan * fix(java): harden local type identities and scope * perf(java): linearize local type ordinal allocation * fix(java): harden ordinal benchmark follow-up * docs(java): clarify ordinal benchmark invariants * test(java): cover local type ownership paths --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> |
||
|
MyShining
|
4af6fe8587
|
feat(spring): resolve constructor and standard injection (#2632) | ||
|
dependabot[bot]
|
e34967eed5
|
chore(deps)(deps): bump express-rate-limit in /gitnexus (#2657)
Bumps [express-rate-limit](https://github.com/express-rate-limit/express-rate-limit) from 8.5.2 to 8.6.0. - [Release notes](https://github.com/express-rate-limit/express-rate-limit/releases) - [Commits](https://github.com/express-rate-limit/express-rate-limit/compare/v8.5.2...v8.6.0) --- updated-dependencies: - dependency-name: express-rate-limit dependency-version: 8.6.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
Gergő Magyar
|
bd9889cdec
|
Merge branch 'main' into main | ||
|
Gergő Magyar
|
170805647c
|
fix(rust): keep duplicate type names ambiguous in range binding (#2514) (#2652)
* fix(rust): latch duplicate type-name ambiguity in range binding (#2514) The range-binding prepass tracked cross-file return and field types in two maps and used map presence itself as the ambiguity flag: the second definition of a name deleted it, but a third definition found it absent and re-inserted the last-scanned file's type. Odd duplicate counts (3, 5, ...) therefore resolved a genuinely ambiguous name to whichever file was scanned last, while even counts stayed ambiguous. Latch ambiguity in a dedicated Set per registry (ambiguousReturnTypes, ambiguousFieldTypes): once a name has two or more workspace definitions it never resolves again, regardless of duplicate count or file order. Adds integration coverage for two/three-duplicate functions and structs, permuted file order, and a unique-name over-suppression guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rust): bump INCREMENTAL_SCHEMA_VERSION to 12 for the #2514 range-binding fix The duplicate-name ambiguity latch changes which cross-file Rust CALLS edges the range-binding prepass emits. The incremental writeback persists only changed-file nodes, so an incremental top-up against a pre-v12 index would keep the old spurious edges on every unchanged Rust file. Bump the schema version to force a one-time full re-analyze, matching the v7/v11 contract for edge-affecting resolver changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(rust): resolve import-disambiguated duplicate types in for-loops & destructuring Follow-up to the #2514 ambiguity latch. When several modules define the same function/struct name and a call site disambiguates it with a `use` import (including aliases and `use x::*` globs), range-binding now resolves the for-loop element type and the destructured field type to that specific imported definition, instead of leaving it unresolved. The bare-name return/field maps are (correctly) ambiguous for duplicates, but the call site's import pins a definition. range-binding records the full, untruncated return/field type per defining file, and resolveImportedDef() resolves a name to the single in-scope definition, mirroring Rust name resolution: - tier 1: explicit `use`/re-export imports and local defs (lookupBindingsAt); these shadow globs, so if any exist we decide within them alone; - tier 2: glob imports, consulted only when tier 1 is empty; a `wildcard-expanded` ImportEdge names the target module, so we resolve only when exactly one glob-target file actually defines the name. Two or more visible definitions stay unresolved, preserving the #2514 latch. normalizeRustReturnType is untouched (its Vec<T> -> Vec truncation is load-bearing for receiver resolution), so the full generic is read from the per-file map instead. Covered by integration tests: explicit / aliased / single-glob imports resolve to the imported definition; two globs that both export the name stay ambiguous; a local definition shadows a glob; no-import duplicates stay unresolved (#2514). INCREMENTAL_SCHEMA_VERSION stays at 12 (bumped by the #2514 commit in this PR); its note now also covers these added resolution edges. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(rust): parse each file once in range-binding when the workspace fits a budget populateRustRangeBindings makes two passes over every file and, because the shared treeCache is empty in the analyze flow, re-parsed each file in both — a workspace of N files paid 2N parses. It now parses each file once and reuses the tree across both passes via an in-function store, gated by a source-byte budget: workspaces up to 16 MiB of Rust source (essentially every real repo) reuse trees; larger ones fall back to per-pass re-parsing so peak RSS stays bounded on huge repos (the memory-sensitive case keeps its current profile). Also collapses the parse+timeout boilerplate that was copy-pasted in both loops into one getOrParseTree helper, and adds a PROF-gated `rangeBind=` segment to the scope-resolution profiler for phase-level observability. Measured on a 500-file synthetic Rust workspace (PROF_SCOPE_RESOLUTION=1): the range-binding phase drops ~370ms -> ~320ms (~14%), parses 1000 -> 500. Behavior is unchanged (199 rust + range-binding-order + parse-timeout tests green); repos above the budget are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(rust): update schema-version gate to v12; regenerate golden + bench baseline for new fixtures CI surfaced three deterministic-artifact failures, all from this PR's own additions: - call-summary-schema-version.test.ts hardcoded INCREMENTAL_SCHEMA_VERSION === 11 (the #2604 window); #2514 bumped it to 12. Update the gate and extend the reuse-gate version history so a v11 stamp now forces a full re-analyze. - rust-captures-golden expected-captures.json drifted (130 -> 174 entries) because the new rust-import-* / rust-dup-* fixtures joined the rust-* corpus. Regenerated (UPDATE_GOLDEN=1): additions only, no existing captures changed — emitRustScopeCaptures is untouched. - bench/scope-capture/baselines.json rust fingerprint drifted for the same reason. Rebaselined with a provenance note; scaling 1.06 < 1.5 budget. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Gergő Magyar
|
76f9f70183
|
fix(cli): LadybugDB native-load failures fail closed, incl. truncated-binary SIGBUS (#2441) (#2651)
* test(cli): cover analyzer lazy-action native-load failure (#2441) createAnalyzerLbugLazyAction — the wrapper the `analyze` command uses — had only a happy-path test; its native-load-failure branch was untested, so a regression could silently reintroduce #2441 (analyze exiting 0 after a LadybugDB native load failure, writing no index while reporting success). Add a failure-path test asserting that when checkLbugNative() reports the binary cannot load, the analyzer module is NOT imported, process.exitCode is set to 1, and the repair message is written to stderr. Mirrors the existing createLbugLazyAction failure test. Verified discriminating: the test fails ("expected undefined to be 1") when the exitCode guard is removed from the analyzer branch, and passes with it restored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cli): probe LadybugDB native load out-of-process so a truncated binary fails closed (#2441) checkLbugNative() loaded lbugjs.node in-process to validate it. That catches clean load failures (missing dylib, zero-byte, garbage -> "file too short"), but a merely truncated/corrupted binary (valid header, missing pages) SIGBUSes the dynamic loader mid-dlopen — a signal, not a catchable throw — taking the whole CLI down with a raw exit 135 and no guidance. Load the binary in a throwaway child process instead. Only a child that RAN and failed (non-zero exit or a fatal signal) marks the binary bad; if the probe itself could not run — a spawn error or timeout, e.g. a no-subprocess sandbox or a non-Node execPath — the result is inconclusive and the command's own load stays authoritative rather than condemning a healthy binary. The probe forces ELECTRON_RUN_AS_NODE, removes the redundant in-process pre-load, and costs ~20ms. Regression tests: truncated binary -> ok:false; unspawnable probe -> ok:true. Verified: a 300KB-truncated native now exits 1 with the repair message (previously exit 135 SIGBUS); zero-byte/garbage stay graceful; good native still loads and indexes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
ArgonarioD
|
f812f709b6 | Merge remote-tracking branch 'upstream/main' | ||
|
ArgonarioD
|
39e9dc8b25 | Merge remote-tracking branch 'upstream/main' | ||
|
Gergo Magyar
|
768161ceb2 |
fix(ci): sync review-agent workflow test with setup-node v7.0.0 pin
The dependabot bump to actions/setup-node@8207627860 (v7.0.0) left the review-agent-workflow.test.ts pin allowlist pointing at the old v6.4.0 SHA, failing CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Gergő Magyar
|
cdbdf219dc
|
fix(lbug): reclaim missing-shadow WAL quarantine files on write-path init (#2638)
Some checks are pending
Gitleaks / gitleaks (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
CodeQL / Analyze (javascript-typescript) (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
|
9538be957d
|
fix(lbug): scale the buffer-pool budget by the OS page-size granule ratio (#2631) (#2636)
* fix(lbug): scale the buffer-pool budget by the OS-page discard-granule ratio (#2631) LadybugDB bills buffer-pool budget per discard granule, not per 4 KiB frame: the engine's vm_region.cpp sets discardGranuleSize = max(frameSize, osPageSize), claimFrame charges the whole granule when its first frame becomes resident, and releaseFrame refunds only when the granule's last frame leaves — while BufferManager::reserve measures eviction progress in refunded bytes and throws 'The buffer pool is full and no memory could be freed!' after three zero-refund passes. On a 64 KiB-page kernel (Ascend/aarch64 openEuler — the #2631 reporter's host) that is 16 frames per granule: the same COPY bills up to 16× the budget it needs on x86, and whole eviction passes can evict frames yet refund nothing. Apple Silicon macOS (16 KiB pages) is the same mechanism at 4×. Measured with the reporter's exact command and version: vllm-ascend needs a (128, 256] MiB pool on 4 KiB pages — 64/128 MiB reproduce the reporter's byte-identical error, 256 MiB and the 576 MiB adaptive pool succeed — so their 64 KiB host cannot survive on a page-size-blind budget. Scale every derived pool size by granuleRatio = max(1, osPageSize/4096): the per-element estimate, the COPY-safety floor, and the default cap (still bounded by 80% of RAM). 4 KiB hosts are byte-identical to before — proven by pinning the existing sizing tests to an explicit 4096 page size, which also stops them drifting on 16 KiB Apple Silicon runners. GITNEXUS_LBUG_BUFFER_POOL_SIZE keeps absolute precedence and 0 still restores the native default. Also: bufferPoolExhaustionRemedy() gives the exhaustion error an actionable cause→consequence→remedy message; the isLbugPageSizeFrameError comment that called pool exhaustion 'a sizing problem, not a page-size one' is corrected — that framing inverted when #2582 made pool size a function of a page-size-blind estimate. Cannot execute on a 64 KiB kernel here: the scaled path is proven by unit stubs plus the engine-source math above; the env override remains the field escape hatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(cli): actionable pool-exhaustion remedies at the COPY sites and a doctor pool line (#2631) The node-COPY throw and the relationship-COPY warning now append bufferPoolExhaustionRemedy() when the failure is the engine's pool-exhaustion class: the raw binder text gave the operator nothing to act on, and on non-4K-page hosts the pool bills up to pageSize/4KiB × faster than the sizing was calibrated for. The relationship path appends the remedy once per bulk load, not once per failed pair. doctor prints the effective pool size next to the page-size line ('pool size 2048 MiB', with an '(×N page-size scaling)' suffix on non-4K hosts) so support triage sees the sizing inputs at a glance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(lbug): re-anchor getEffectiveBufferPoolSize's placement and reuse granuleRatio in doctor Self-review fixes: the getter's insertion had orphaned resolveBufferManagerSize's doc comment (it read as documenting the wrong function), and doctor's scale note duplicated the granule math with a hardcoded 4096. granuleRatio is now exported (it already carried the test-seam default param) and doctor consumes it. No behavioral change — the sizing suite pins byte-identical outputs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(lbug): keep the hintless pool default unscaled and make both remedies visible (#2631) Review fixes: - Scale only the analyze-path cap (scaledAnalyzePoolCap), not defaultBufferPoolSize: the pool is an eager native allocation at DB open (measured, see POOL_BYTES_PER_ELEMENT), so a page-size-scaled hintless default would hand a long-lived MCP process up to 80% of RAM — the #2557 OOM exposure the 2 GiB cap removed. Fix the MAP_NORESERVE claim that contradicted that measurement. - Log the rel-pair pool remedy (loadGraphToLbug returns warnings that no call site reads) and dedup it with a local boolean instead of matching the remedy's own wording. - Label the GITNEXUS_LBUG_BUFFER_POOL_SIZE=0 sentinel as the native 80%-of-RAM default in both the remedy and doctor instead of '0 MiB'. - Extract poolSizeDoctorLine (pageSizeDoctorLines convention): mark env overrides, drop the scaling suffix that misdescribed absolute values. - Fold _resetOsPageSizeCacheForTest into _setOsPageSizeForTests(undefined). - Document the analyze-path scaling in both README env tables. --------- Co-authored-by: Gergo Magyar <abhigyan1.patwari@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Abhigyan Patwari
|
0eeecb37f3
|
fix(python): resolve calls through constructor-injected fields (#2628)
* fix(python): resolve calls through injected fields * fix(ci): update python capture benchmark fingerprint * fix(python): make constructor field inference conservative --------- Co-authored-by: Gergo Magyar <gergomagyar@icloud.com> |
||
|
Gergő Magyar
|
e814e28f1f
|
fix(deps): bump @ladybugdb/core to ^0.18.3 — rel-property IN-predicate fix (#2508) (#2634)
LadybugDB ≤0.18.2 mis-evaluated `r.type IN [...]` on relationship table groups: the boolean-filter fallback skipped writing selection buffers for single-row unflat chunks, dropping/duplicating callers in context() and impact() (upstream LadybugDB#692, fixed by LadybugDB#699, shipped in 0.18.3). Floor the dependency at ^0.18.3 and lock core + all five platform packages. Resurrect the caller-identity regression test from PR #2553 (closed as superseded by the upstream fix): it pins context()/impact() to exact caller IDs across CodeRelation sub-table pairs so any future predicate regression fails loudly. Note: with CREATE-seeded data the test also passes on 0.18.2 (the upstream repro needs COPY-written chunk layouts) — it is a behavioural pin, not a bug reproduction. Fixes #2508 Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Gergő Magyar
|
7f7255aef8
|
fix(analyze): load VECTOR before the incremental writeback touches embedding rows (#2623) (#2624)
* feat(lbug): add ensureEmbeddingRowDmlSafe VECTOR gate for embedding-row DML LadybugDB refuses every mutation of a table carrying an HNSW index while the VECTOR extension is not loaded on that connection: DELETE and CREATE raise a Binder exception, DROP TABLE is refused while the index references it, and SET segfaults the process. Dropping the index is not an available recovery either — CALL DROP_VECTOR_INDEX is itself a VECTOR-extension function and is undefined in exactly that state. Add a single primitive that loads VECTOR under the analyze install policy and, only when that fails, reads CALL SHOW_INDEXES (which works without the extension) to decide whether an index actually exists to trip over. No call sites yet. Refs #2623 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(lbug): pin the #2623 VECTOR gate for embedding-row DML Three cases: no index + VECTOR unavailable stays safe (no needless escalation); index present + VECTOR unavailable is reported blocked AND the raw deleteNodesForFiles genuinely throws 'extension is not loaded' (proving the hazard is real, not theoretical); index present + VECTOR loadable is safe, the delete works, and the HNSW index survives — the invariant run-analyze relies on when it keeps the index across a surgical incremental run. Refs #2623 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(analyze): load VECTOR before the incremental writeback touches embedding rows Incremental analyze died on every content change once a repo had built code_embedding_idx: Analysis failed: Binder exception: Trying to delete from an index on table CodeEmbedding but its extension is not loaded. The surgical writeback's first statement is deleteNodesForFiles' CodeEmbedding join-delete, but nothing on that path loaded VECTOR until Phase 4 — so the engine refused the delete. This is an ordering defect, not an environment one: it reproduces on machines where VECTOR loads fine. The dirty-flag recovery then forced a full rebuild on the next run, which is why it read as 'just slow'. Call ensureEmbeddingRowDmlSafe() once, before the escalation gate and before any row is touched — the same 'index lifecycle before row DML' seam dropSearchFTSIndexes occupies for FTS (#2589). Unconditional, because a DB carrying the index from an earlier --embeddings run hits the same wall on a plain incremental run. When VECTOR truly cannot load the table is immutable (the index cannot be dropped without the extension either), so the run falls through to the existing wipe-and-COPY escalation with a message naming cause, consequence and remedy. Fixes #2623 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(analyze): pin the #2623 VECTOR-before-embedding-DML ordering end-to-end Sibling of the #2589 FTS drop-before-delete suite, same shape: drive the real runFullAnalysis incremental path over a real git repo and a real LadybugDB, seed real embedding rows, build the HNSW index, then assert the index state at the exact moment deleteNodesForFiles is invoked. Both cases were confirmed to discriminate — with the run-analyze change reverted they fail with the reported 'Trying to delete from an index on table CodeEmbedding but its extension is not loaded', and pass with it: - surgical path: the run completes, the index is still present AND extension_loaded at delete time, exactly one row per nodeId survives, and the untouched file's rows are preserved - blocked path: with GITNEXUS_LBUG_EXTENSION_INSTALL=never the run escalates to a full DB write and says so, instead of crashing Also applies prettier's reindent to the run-analyze log ternary. Refs #2623 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(lbug): cite the pinned LadybugDB version in the #2623 probe note The probe matrix behind ensureEmbeddingRowDmlSafe was first recorded on 0.18.0, but gitnexus/package-lock.json pins 0.18.2 (#2587). Re-ran every case on 0.18.2: refused DELETE, refused CREATE, SIGSEGV on SET, DROP_VECTOR_INDEX undefined, DROP TABLE refused, SHOW_INDEXES readable with extension_loaded intact. Identical on both, so the design is unchanged — only the citation was wrong. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(analyze): preserve embeddings across the VECTOR-blocked rebuild, and check the catalog before loading Three follow-ups from reviewing the fix itself. 1. Data loss on the blocked path. Escalating wipes the DB files, and Phase 3.5 restores embedding rows from cachedEmbeddings — which deriveEmbeddingMode only populates when meta.stats.embeddings > 0. A DB holding embedding rows that its meta does not account for therefore had every vector destroyed silently by a rebuild it never asked for. Probe on a 3-file repo: 3 rows before, 0 after, no warning. Read the rows before escalating (a plain MATCH, no extension needed) so the existing restore has something to restore, and say so in the log. The blocked-path test now asserts the seeded rows survive exactly once, and that assertion fails without this rescue. 2. Catalog before extension. ensureEmbeddingRowDmlSafe loaded VECTOR first and only read SHOW_INDEXES on failure, so every incremental analyze on a machine without VECTOR paid a bounded out-of-process INSTALL attempt plus an 'extension unavailable' warning — including repos that never built an embedding index and can never hit this bug. One local catalog read settles that case first; the load is attempted only when an index actually gates DML, or when the catalog cannot be read. 3. Dead branch. targetConn is always the module singleton there, so the isSharedSingletonConn ternary could never take its second arm. Collapsed to withConnLock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(doctor): live-probe the VECTOR extension instead of printing the static platform capability Review finding on #2624 (MEDIUM), and exactly what #2623's reporter hit: doctor printed 'VECTOR index: available' — derived from a static platform check — while every incremental analyze on the same machine was dying on an unloaded VECTOR extension. The FTS line was switched to a live LOAD probe for the identical contradiction under #2374; VECTOR now gets the same treatment. probeVectorExtensionLoad shares the FTS probe's implementation (bounded, offline-safe, never runs the installer) and doctor's semantic-mode line now follows the probe, not the platform: without a loadable extension the vector index can be neither built nor queried, so search really is on exact scan. The load-error classifier's remedies are label-parameterized so the VECTOR row stops dispensing FTS-specific advice — 'run analyze --repair-fts' repairs FTS indexes only and was actively wrong for a missing vector extension. Default label stays 'FTS'; every existing caller and pinned remedy string is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(lbug): remove the stale Windows VECTOR gate — the extension ships for win_amd64 The codebase categorically refused VECTOR on Windows (platform !== 'win32' in isVectorExtensionSupportedByPlatform, plus a hard early-return in loadVectorExtension) on the strength of an early-era report that in-process INSTALL VECTOR could SIGSEGV (#1365). That belief is stale, verified directly: - the extension server hosts win_amd64 VECTOR artifacts for every 0.18.x extension version — v0.18.0 and v0.18.1 both serve a real 14 MB PE32+ DLL (curl-probed; 'file' confirms PE32+ x86-64) - the pinned 0.18.2 core resolves its extension directory to 0.18.1 (strace-verified LOAD open()), so the pinned version's Windows artifact exists too - INSTALL now runs in a spawned child (installDuckDbExtensionOutOfProcess), so even a crashing installer kills only the child and degrades to unavailable — the original hazard cannot reach the parent process any more Windows now takes the same runtime path as every other OS: try LOAD, install out-of-process when policy allows, degrade to exact scan when it truly fails. The MCP semantic-search lane loses its static platform gate too — it always attempts the vector index and falls back to the exact scan on runtime failure, with a once-per-backend diagnostic naming the real error instead of a platform-policy message. isVectorExtensionSupportedByPlatform is deleted; getRuntimeCapabilities reports the platform capability as available everywhere and defers machine truth to the live probe. Windows CI is the enforcement: the vector suites skip visibly only when the extension genuinely cannot load, so green Windows lanes now actually exercise VECTOR instead of silently skipping by policy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(lbug): pin the catalog-read-failure fallback in ensureEmbeddingRowDmlSafe Review finding on #2624 (LOW): the one branch where the gate cannot cheaply prove safety — SHOW_INDEXES itself erroring — was exercised only by inference. Force it with a Connection.prototype.query spy over the real DB: the catalog read fails, and the gate must fall through to actually attempting the extension load (asserted via the recorded statement stream) rather than guessing, returning true here because the extension is loadable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(mcp): load VECTOR on the pool's shared Database so the semantic vector lane actually works Review finding on #2624 (MEDIUM): extension load scope is per-Database (probe-verified — LOAD on one connection enables QUERY_VECTOR_INDEX on every connection of the same Database), and the pool pre-warm loaded only FTS. So LocalBackend's vector lane has ALWAYS raised 'Catalog exception: function QUERY_VECTOR_INDEX is not defined' through the pool and silently fallen back to the exact scan — repos above the 10k exact-scan cap got empty semantic results. The serve path was unaffected (the embedding pipeline loads the extension itself). Mirror the FTS line at BOTH load sites — doInitLbug's pre-warm and initLbugWithDb's external-Database adoption — under the same load-only contract (the read pool never triggers a network install), tracked by a new SharedDB.vectorLoaded flag reset where ftsLoaded resets. The new pool test is discriminating and deliberately closes the writable core adapter before the pool opens: a shared/injected Database would inherit the VECTOR load from test seeding and pass either way, so the case forces the pool onto its OWN fresh read-only Database where only the pre-warm can make the lane legal. Verified: fails at the pre-fix tree with the exact Catalog exception, passes with the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: run the #2623 ordering suite on Windows/macOS and pre-install VECTOR alongside FTS Two review findings on #2624, both landing in existing seams: - scripts/cross-platform-tests.ts gains incremental-vector-extension-ordering .test.ts: the win32 VECTOR gate is gone in this PR, so the #2623 drop-ordering + blocked-path escalation must be proven on the windows-latest native addon, not just Ubuntu. (The review's claim that lbug-delete-nodes-for-files.test.ts was also missing was wrong — it has been on the roster since #2409.) - scripts/ensure-fts.ts now pre-installs VECTOR under the same best-effort auto-policy contract, so every sharded CI process LOADs from ~/.lbdb instead of racing its own bounded out-of-process INSTALL; the workflow's extension cache already covers it (path is the whole extension dir — key kept for cache continuity). The cross-platform job sets GITNEXUS_REQUIRE_VECTOR=1 beside GITNEXUS_REQUIRE_FTS so a genuinely unavailable VECTOR is a loud failure, never a silent skip. Windows/macOS cannot be executed locally; the PR's CI lanes are the proof for this commit. Linux smoke: ensure-fts.ts reports both extensions ready; all 79 roster entries resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(pool): register loadVectorExtension in the pool unit-suite mocks The pool adapter's new loadVectorExtension import surfaced in four suites that mock lbug-adapter.js with explicit factories (vitest fails loudly on a missing mocked export). Register the export in each — resolving false where the suite's world assumes no vector, true where it mirrors FTS — and extend lbug-pool-fts-load.test.ts, the suite that owns pre-warm extension loading, with the vector pair: successful load cached per shared Database, failed load retried on the next open, both pinned to policy load-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(analyze): use POSIX literals for graph paths in the #2623 ordering suite First Windows CI run of this suite (it joined the cross-platform roster this PR) failed with 'Parser exception: Invalid input <MATCH (n:Function) WHERE n.filePath = '>' — path.join produces backslashes on Windows, and a backslash inside the seed helper's single-quoted Cypher literal breaks the parser. The graph stores repo-relative filePaths with forward slashes on every OS, so graph-side paths are POSIX literals now (the incremental-orchestration convention); path.join stays only for real filesystem access. The same Windows lane also proved the substance this suite exists for: lbug-vector-extension passed 7/7 on windows-latest — the extension installed, loaded, and built a real HNSW index there — and the pool vector-lane and DML gate suites passed too. This commit fixes the harness, not the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <abhigyan1.patwari@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
dependabot[bot]
|
735289e399
|
chore(deps)(deps): bump fast-uri from 3.1.2 to 3.1.4 in /gitnexus (#2626)
Bumps [fast-uri](https://github.com/fastify/fast-uri) from 3.1.2 to 3.1.4. - [Release notes](https://github.com/fastify/fast-uri/releases) - [Commits](https://github.com/fastify/fast-uri/compare/v3.1.2...v3.1.4) --- updated-dependencies: - dependency-name: fast-uri dependency-version: 3.1.4 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
dependabot[bot]
|
c35403b59d
|
chore(deps)(deps): bump js-yaml from 4.3.0 to 5.0.0 in /gitnexus (#2618)
* chore(deps)(deps): bump js-yaml from 4.3.0 to 5.0.0 in /gitnexus Bumps [js-yaml](https://github.com/nodeca/js-yaml) from 4.3.0 to 5.0.0. - [Changelog](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md) - [Commits](https://github.com/nodeca/js-yaml/compare/4.3.0...5.0.0) --- updated-dependencies: - dependency-name: js-yaml dependency-version: 5.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * fix(spring-config): migrate YAML parsing to js-yaml 5 event API js-yaml 5 removed the loadAll `listener` callback, the EventType/State types, and DEFAULT_SCHEMA that spring-config relied on, breaking the build. Rebuild the per-key line tree from parseEvents()/constructFromEvents() (positions are source offsets → mapped to lines), apply the `<<` merge tag via CORE_SCHEMA.withTags(mergeTag) (CORE alone leaves merge keys unexpanded), and resolve aliases by anchor name, which lets the object-identity WeakMap go. Behavior preserved: 9 unit + 8 integration spring-config tests pass, including merged-key declaration-line, cyclic-alias termination, and the depth budget. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(spring-config): restore v4 tag coverage, cover the v5 rewrite with tests Review follow-up for #2618. CORE_SCHEMA.withTags(mergeTag) was a narrowing, not a port: js-yaml 5 throws "unknown tag" on !!timestamp/!!binary/!!set/!!omap/!!pairs, and an unknown tag aborts the whole parse, which readConfigKeys swallows — so an application.yml using any of them would have gone from its full key set to zero keys, silently. Carry the rest of what DEFAULT_SCHEMA was; none of these tags can execute code. Add tests for every path the review flagged as uncovered: multi-document files, empty/comment-only/bare-`---`/bare-scalar documents, sequence-form merge keys, and explicitly tagged values (which fail against the one-tag schema, so they target the changed line). Clear the anchor map per document. It cannot change output today — constructFromEvents rejects a cross-document alias before the event tree is built, now asserted — but it keeps both layers on YAML's scoping rule. Drop the stale @types/js-yaml devDependency; js-yaml 5 ships its own types and tsc --noEmit is clean without it. Lockfile hand-edited because npm uninstall also strips every libc field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(autofix): apply prettier + eslint fixes via /autofix command * fix(spring-config): flatten !!set members, walk YAML iteratively Review follow-up for #2618. js-yaml 5 constructs `!!set` as a native Set; v4 built a plain `{member: null}` object. Object.entries of a Set is empty, so a tagged set collapsed to a bare leaf key and lost every member. Enumerate the Set instead. Sets arrive as mapping events with key/value scalar pairs, so member lines resolve through the usual lookup. !!binary and !!timestamp are unaffected — both are scalar events and take the leaf path, which is why a Uint8Array never explodes into one key per byte. Convert findYamlMappingLocation and flattenYamlValue from recursion to an explicit stack. Children are pushed in reverse so pops happen in declaration order, preserving "first match" and `out` insertion order; `leave` frames release the cycle guard where the old `finally` did. The depth budget still throws at the same boundary with the same message. Cover the gaps the review named: !!pairs (both duplicate entries survive), anchor-name reuse resolving to the nearest preceding declaration, and marker-only leading documents staying index-aligned across the two streams. buildYamlEventTree keeps no node budget by design — one node per event over an already-materialized array, bounded by MAX_CONFIG_FILE_BYTES. The docstring now says so rather than implying MAX_YAML_TRAVERSAL_NODES covers it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
ArgonarioD
|
6150a793e8 |
docs(cli): mention .agents/skills/ mirror in --skip-skills help + test
Address review finding (LOW — docs/help staleness): the --skip-skills help text and README omitted that skills also mirror to .agents/skills/ when .agents/ exists. - index.ts + i18n (en/zh): --skip-skills now reads "directly under .claude/skills/ and .agents/skills/". - skip-git-cli.test.ts: assert the help text covers .agents/skills/. Co-Authored-By: Claude <noreply@anthropic.com> |
||
|
ArgonarioD
|
38d0256474 | Merge remote-tracking branch 'upstream/main' | ||
|
dependabot[bot]
|
50b0f2f775
|
chore(deps)(deps): bump hono from 4.12.26 to 4.12.31 in /gitnexus (#2619)
Bumps [hono](https://github.com/honojs/hono) from 4.12.26 to 4.12.31. - [Release notes](https://github.com/honojs/hono/releases) - [Commits](https://github.com/honojs/hono/compare/v4.12.26...v4.12.31) --- updated-dependencies: - dependency-name: hono dependency-version: 4.12.31 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
dependabot[bot]
|
2d4e24811e
|
chore(deps)(deps-dev): bump @types/node in /gitnexus (#2617)
Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 26.0.0 to 26.1.1. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) --- updated-dependencies: - dependency-name: "@types/node" dependency-version: 26.1.1 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
Abhigyan Patwari
|
9efc6bfcad
|
fix(lbug/analyze): atomic index swap + read-pool staleness invalidation (#2614)
* fix(lbug): re-open the read pool when analyze rebuilds the index under it
The MCP read pool's initLbug early-returned on an existing pool entry with no
freshness check, so after analyze rebuilt or mutated the on-disk index the
pool kept serving the old (POSIX: unlinked-but-open) inode until LRU/idle
eviction — a silent stale-read window of up to IDLE_TIMEOUT_MS (5 min).
Record the file identity {ino, mtimeMs, size} on each PoolEntry at open, and
re-stat in initLbug: unchanged → reuse; changed & idle → closeOne + reopen the
new file; changed while a query is in flight → serve the current handle (a
later idle initLbug reopens, since closing an in-use connection is a native
use-after-free). A stat failure (ENOENT during a full rebuild's unlink window)
is treated as unchanged so the reader keeps its valid open inode until the new
file appears. Mirrors the bridge cache's mtime-invalidation pattern.
Step 1 of docs/plans/2026-07-21-...-analyze-atomic-swap-invalidation. The
end-to-end reopen-on-swap path is exercised by the reader-during-rebuild
integration test in a later step.
* fix(analyze): publish a full rebuild via an atomic swap (POSIX)
The full-rebuild path wiped the live index (wipeLbugDbFiles(lbugPath)) and
rebuilt it in place, so a concurrent MCP reader that opened mid-build could
see an empty/half-loaded DB, and a crash between the wipe and the end-of-run
left the index destroyed (recoverable only by --force).
Build the fresh index at <lbugPath>.new and swap it over the live index in one
atomic rename at the end. All DB work flows through the singleton connection,
so only initLbug/wipeLbugDbFiles take the temp target; the close already
checkpoint-consolidates the build to a single file (verified: no residual
.wal/.shadow), so the rename publishes a complete index in one step. A reader
opening mid-build only ever sees the previous complete index; a reader holding
the old inode keeps a consistent stale snapshot until the pool re-opens onto
the new one (the pool staleness invalidation from the prior commit). On
failure the swap is skipped, leaving the previous index byte-for-byte intact.
POSIX only: the common CLI/serve-worker analyze paths skip the native close
(closeLbugBeforeExit, #2264) and leave the build handle open at swap time.
POSIX renames an open file cleanly; a same-process open handle blocks the
rename on Windows, so Windows keeps the current in-place behavior
(buildPath === lbugPath) until that is resolved. The Windows atomic swap and a
deterministic concurrent reader-during-rebuild test are deferred follow-ups.
Steps 2b + partial 3 of docs/plans/2026-07-21-...-analyze-atomic-swap-invalidation.
Integration test asserts the no-temp-leak + inode-swap invariants and the
crash-safety guarantee (a load failure leaves the live index untouched).
* test(analyze): end-to-end read-pool reopen after an atomic swap
Adds the deferred reader-during-rebuild / pool-reopen integration test:
analyze v1 -> read pool serves it -> rebuild with a renamed function (atomic
swap) -> the same repoId's initLbug detects the swapped inode and re-opens the
pool onto the new index. Asserts the pool sees the renamed function and NOT the
stale v1 name, exercising #1 (invalidation) and #2 (swap) together end to end.
* fix(lbug): bound pooled read queries with setQueryTimeout
The read pool relied only on a JS-side Promise.race (QUERY_TIMEOUT_MS) that
frees the waiter but leaves the native call running. Set the engine-level
setQueryTimeout on every pooled connection so a pathological query is bounded
at the source too.
* fix(lbug): name the held-open cause for WAL checkpoint failures (#2599)
A WAL-checkpoint IO error that also carries a busy/lock signal means another
handle (a gitnexus mcp server, or this process's own reader) holds the store
open, not a disk fault. Add isLbugCheckpointBusyError (reusing the tested
isDbBusyError keyword set) and, when the checkpoint driver exhausts its retry
budget on such an error, annotate the surfaced error with the actionable
held-open cause instead of a raw IO string.
Note: overlaps in-flight work on repro/issue-2599-windows-wal-checkpoint;
bundled here at the maintainer's request.
* feat(analyze): opt-in atomic incremental + best-effort Windows swap
Extends the atomic-swap publish (POSIX full rebuild) to two more cases:
- Windows: the swap now applies when a real close is safe to release the build
handle before the rename — i.e. non-pdg runs (windowsSwapOk excludes --pdg,
the #2264 destructor-crash case), forcing a real close on the swap path.
UNVERIFIED on Windows (no Windows runner here); --pdg and any failure fall
back to today's in-place behavior, so it can never corrupt.
- Incremental (opt-in, GITNEXUS_ATOMIC_INCREMENTAL=1): copies the live index
into the temp, applies the incremental delete/writeback to the copy, and
swaps at the end. Off by default because the whole-file copy negates
incremental's speed premise — kept behind a flag pending a benchmark. The
escalation valve also targets the temp so an escalated write stays atomic.
Integration test covers the opt-in incremental path end to end (no temp leak,
the incremental change is reflected after the swap).
* refactor(lbug): centralize the read-pool + bridge open-retry budgets
The lbug-config retry registry documented the open/handle-release/query-time
budgets but the read pool's LOCK_RETRY_* (pool-adapter) and the bridge's
LBUG_OPEN_RETRY_* (group/bridge-db) kept private copies that could drift. Move
both into the registry as exported constants (POOL_OPEN_LOCK_RETRY_*,
BRIDGE_OPEN_RETRY_*) and alias the local names to them — one tuning surface,
no behavior change.
* fix: address CI regressions from the bundled follow-ups
- setQueryTimeout: guard the call so test doubles that don't model the engine
method don't break connection creation.
- atomic swap: skip the rename when the build produced no DB at buildPath (an
empty repo / mocked pipeline) instead of throwing ENOENT.
- #2599: don't wrap the checkpoint error in the driver (it hid the IO signature
the CLI's --wal-checkpoint-threshold hint keys on); name the held-open cause
at the CLI instead, beside that hint, keeping the original error intact.
- retry consolidation: revert to documentation-only — moving the pool/bridge
budgets into lbug-config broke every explicit lbug-config test mock. The
registry now catalogues all budgets with their in-file locations.
- analyze-wal-checkpoint-failure test: block both lbug.wal.checkpoint and
lbug.new.wal.checkpoint, since a full rebuild now checkpoints the temp.
* fix(analyze): publish the swap before stamping meta; identity-gate the reader (#2614 F1)
Review found a HIGH regression: the full-rebuild wrote the freshness stamp
(saveMeta, indexedAt=T_new) BEFORE the atomic swap, so a concurrent MCP reader
that reinited in the saveMeta->swap window opened the OLD inode, recorded
observed=T_new, and then never reinited again (ensureInitialized returns early
on 'current') — serving the pre-rebuild graph indefinitely. The build-into-temp
change inverted the pre-PR invariant that 'meta shows T_new' implied 'lbugPath
holds T_new data'.
Two coordinated fixes:
- run-analyze: move the final saveMeta AFTER the swap, so meta.indexedAt only
becomes visible once lbugPath resolves to the new inode. Verified nothing in
the span reads on-disk meta and registerRepo writes only the registry.
Leaving the dirty flag set across the swap also improves crash-safety.
- local-backend: the reader staleness gate now also compares the lbug file
IDENTITY (ino/mtime/size), reiniting on an inode change even when
meta.indexedAt is unchanged. This closes the swap-window latch and covers the
in-place incremental case — and is what actually makes the pool's dbIdentity
net reachable for the MCP reader (the indexedAt gate otherwise bypassed it).
* fix(lbug/analyze): WAL-aware incremental, residual-sidecar reconcile, Windows opt-in, #2599 anchor (#2614 F2-F4)
Review remediations:
- F3: gate atomic incremental on a CLEAN live index (inspectLbugSidecars) — the
main-file-only copy would drop an orphan .wal's delta; fall back to in-place.
- F4: on the swap, MOVE a residual <buildPath>.wal/.shadow beside the published
index (not orphan it) so a swallowed final checkpoint's delta is replayed.
- F2: record identity on the shared read-only Database and warn when a cached
handle is reused after its on-disk index was rebuilt while another consumer
holds it (unreachable via MCP — one consumer per lbugPath; a complete fix
needs per-inode handles, documented).
- Windows swap: opt-in (GITNEXUS_ATOMIC_WINDOWS_SWAP=1), default off — the
forced real close re-bets an unproven #2264 assumption and can't be verified
without a Windows runner, so the default Windows path stays in-place.
- #2599: anchor isLbugCheckpointBusyError to real held-open wording instead of
isDbBusyError's bare .includes('lock') over a message that embeds the DB path
(a repo under blockchain-app misclassified a disk fault as held-open).
- Docs: corrected retry-catalogue budgets (linear, not exp) and the
checkedOut>0 bound comment (load-bounded, not IDLE_TIMEOUT_MS).
* test(analyze): cover the production close path in the atomic swap (#2614 F5)
Adds a full-rebuild swap test with skipNativeCloseOnExit:true — the close path
the CLI and serve-worker actually ship (build handle left open at swap time),
distinct from the default real-close the other swap tests exercise. Asserts the
POSIX swap still publishes a single consolidated lbug with no .new temp and no
orphan sidecar.
* test(analyze): give the follow-up git commits an inline identity (CI fix)
The end-to-end reopen and atomic-incremental tests' second commits used a bare
`git commit`, which fails on CI runners with no global git identity (empty
ident name). makeRepo's initial commit already passes -c user.name/-c
user.email inline; apply the same to the rename/change commits. No code change.
* fix(mcp): route reader reinit through initLbug's active-query guard (#2614 review)
Review found an active-query retirement race: LocalBackend.ensureInitialized
detected an identity/stamp change and called closeLbug(poolKey) DIRECTLY, but
closeOne closes the shared Database at refCount 0 regardless of checked-out
connections. So a reader detecting the new generation could close the Database
a concurrent query is still executing on — a native use-after-free. This
bypassed the checkedOut>0 guard that initLbug itself has.
Fix (delegate, not close directly): initLbug now returns whether it actually
rolled the pool over; ensureInitialized calls initLbug (which serves the
current handle while a query is in flight and reopens only when idle) instead
of closeLbug. The observed IDENTITY is advanced only when the pool actually
reopened — if a query was in flight, the identity stays divergent and the
reopen retries on a later idle check rather than latching on the old handle.
The observed STAMP advances regardless so a same-file stamp change can't loop.
Old generation now stays alive until its in-flight queries drain (lazy
rollover); new requests during the busy window share the old handle until the
pool goes idle, then reopen. No parallel open-both-generations, but no UAF and
no stale latch.
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
|
||
|
Gergo Magyar
|
382801790c |
perf(config): memoize core.excludesFile / info/exclude resolution (#2606)
loadIgnoreRules is called once per repo, per language/contract extractor during group sync -- an N-repo group fans out to 6+ extractors each calling it, turning an uncached execSync per call into O(extractors x repos) blocking subprocess spawns for the exact many-repos scenario #2606 describes. Both getGitInfoExcludePath and getCoreExcludesFilePath resolve to the same value for the same fromPath for the life of the process, so memoize by fromPath in a process-lifetime Map. One-shot CLI runs are unaffected by staleness; the long-lived MCP server would need explicit invalidation if this becomes a real concern. |
||
|
Gergo Magyar
|
0f016dc467 |
fix(config): read core.excludesFile and .git/info/exclude for global ignores (#2606)
Replace the custom ~/.gitnexus/ignore file with the same two sources real git itself consults for exactly this purpose (gitignore(5)): - core.excludesFile: git's own all-repos global ignore file (defaults to $XDG_CONFIG_HOME/git/ignore when unconfigured) - $GIT_COMMON_DIR/info/exclude: per-repo, untracked, so it works without push/commit access to the repo Precedence mirrors git exactly (lowest to highest): core.excludesFile, then info/exclude, then .gitignore, then .gitnexusignore -- each later source can negate an earlier one via a `!pattern` line, same last-match-wins semantics git itself uses. Adds getCoreExcludesFilePath and getGitInfoExcludePath to git.ts, following the same execSync + git-common-dir pattern as getCanonicalRepoRoot. GITNEXUS_NO_GLOBAL_IGNORE (or noGlobalIgnore) still skips both global sources, mirroring GITNEXUS_NO_GITIGNORE. |