Addresses all 5 blocking findings from production-readiness review:
1. meta.json capabilities: Track ftsIndexed flag and write
status='degraded' (not 'available') when FTS indexes are missing.
Prevents false-positive capability reporting (#1715 regression).
2. Progress event gap: Emit progress('fts', 90, 'Search indexes
degraded (BM25 unavailable)') in the verification-failure branch
so progress never stalls at 85%.
3. Narrow catch scope: Only suppress errors containing
'FTS extension unavailable'. DB connection failures, schema errors,
and programming errors (e.g. safeIdentifier) now propagate as before.
4. (Finding 4 resolved by Finding 3): Non-FTS errors still throw in
all analyze modes, so only the intended FTS-unavailable case degrades.
5. Stronger test assertions: Verify warning log content, verify
progress does not report 'Search indexes ready', verify degraded
progress message is emitted.
* Initial plan
* fix(analyze): add WAL auto-checkpoint CLI control and default-off behavior
* test(analyze): share lbug auto-checkpoint parsing and align validation
* fix(analyze): always enable lbug auto-checkpoint and expose threshold control
* refactor(lbug): inline always-on auto-checkpoint constructor arg
* fix(analyze): guide checkpoint-threshold on Ladybug WAL checkpoint IO failures
* test(analyze): cover checkpoint IO guidance and add integration guard
* fix(analyze): tighten checkpoint IO detection and remove test hook
* fix(analyze): remove checkpoint test hook and tighten error matching
* fix(analyze): rename to wal-checkpoint-threshold, raise default, add manual checkpoint driver with retry
Address review feedback on PR #1772:
- Rename CLI flag, env var, AnalyzeOptions field, recovery-hint tag, and
parser/constants from lbug-* to engine-neutral wal-* (matches the existing
WAL_RECOVERY_SUGGESTION / isWalCorruptionError convention).
- Raise default threshold from -1 (Ladybug stock ~16 MiB) to 64 MiB so users
on the default config no longer hit the original rename/remove race.
- Align both READMEs to publish 67108864 (64 MiB) instead of 65536 (which
would have made the crash more frequent).
- Add wal-checkpoint-driver.ts: a periodic manual CHECKPOINT driver wrapped
in a 3-attempt jittered retry (50/200/500 ms), driven from runFullAnalysis.
Opt-out via GITNEXUS_WAL_MANUAL_CHECKPOINT=0. Moves the race window into a
JS-controllable retry surface while keeping native auto-checkpoint on.
- Move LBUG_CHECKPOINT_RENAME_RE / REMOVE_RE plus the predicate (renamed to
isLbugCheckpointIoError) into lbug-config.ts alongside isWalCorruptionError.
Predicate is now exported. Add a permissive fallback matcher and pin the
matched Ladybug version in comments.
- Warn instead of silently defaulting when GITNEXUS_WAL_CHECKPOINT_THRESHOLD
is set to a non-empty unparseable value (closes the CLI-vs-env asymmetry).
- Add a typed RecoveryHint string-literal union in cli-message.ts so future
hint tags can't drift.
- Add a real integration test under test/integration/ that triggers a
Ladybug checkpoint IO failure via a pre-existing directory at the rename
target (portable across platforms; no test-only injection hook).
- Add small-disk / CI caveat (32 MiB secondary suggestion) to the recovery
hint and README env-var rows.
- Document CLI/env precedence in the analyze --help block.
- Help placeholder: <value> -> <bytes>.
- Rename analyze-lbug-auto-checkpoint.test.ts to use the new wal-* token.
* chore(lbug): remove dead jitteredDelay helper and apply prettier
- Drop unused `jitteredDelay` function flagged by CodeQL in PR #1772; the
retry loop already inlines the same calculation with the injectable
`randomImpl` so the helper was dead. Move the non-cryptographic-by-design
comment next to the actual jitter site.
- Apply `prettier --write` to wal-checkpoint-driver.ts and the new
integration test to absorb the PR autofix bot's formatting findings.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Co-authored-by: Test <test@example.com>
The scope-resolver header comment claimed forced-mode passed 154/175
(88%) and listed smart casts, cross-file iterables, method chains,
overload selection, virtual dispatch, and interface defaults as
"remaining gaps". All six landed in PRs #1774-#1779. Forced mode now
passes 175/175 (verified post-merge against `main`).
Update the header to:
- state the current forced-mode result accurately,
- enumerate the closed sub-issues so future readers can trace each
capability back to its PR,
- and explicitly name the remaining flip blockers (#1755, #1756,
#1757) so the next maintainer to look at this file knows exactly
what's required before adding `Kotlin` to `MIGRATED_LANGUAGES`.
Docs-only — no behavioral changes.
Refs #1746.
Co-authored-by: Test <test@example.com>
* feat(ingestion): log deferred resolution progress when verbose
Add [deferred-profile] timing logs for post-chunk import, heritage, heritage-map, and legacy call resolution. Enabled on GITNEXUS_VERBOSE / analyze -v (and optionally GITNEXUS_PROFILE_DEFERRED) to diagnose analyze stalls on large repos (issue #1741).
Co-authored-by: Cursor <cursoragent@cursor.com>
* chore(autofix): apply prettier + eslint fixes via /autofix command
* fix(ingestion): address PR #1773 production-readiness review
Move deferred call progress logs after the registry-primary skip so sites= counts match files actually resolved. Only time buildHeritageMap when heritage records exist; otherwise log an explicit skip. Add wiring tests that assert [deferred-profile] emission from buildHeritageMap and processCallsFromExtracted. Snapshot GITNEXUS_PROFILE_DEFERRED env vars in analyze CLI isolation.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(ingestion): address PR #1773 code-review findings
P0
- Replace forbidden toBeGreaterThanOrEqual/toBeLessThan in
profileElapsedMs test with exact-arithmetic vi.spyOn(hrtime.bigint)
asserting .toBe(2.5) and .toBe(0). DoD §2.7 compliance.
P2
- Use Number() (not parseInt) when parsing
GITNEXUS_PROFILE_DEFERRED_SLOW_MS so scientific notation like '1e9'
doesn't silently parse to 1 and turn the slow-file log into a per-file
log storm.
- Introduce startTimer(enabled): bigint | null and endTimer(start,
format) helpers in deferred-resolution-profile.ts; refactor 6+
timing blocks in parse-impl.ts and call-processor.ts to use them.
Removes the 0n sentinel that conflated 'disabled' with 'zero
elapsed time' and let TS narrow correctly.
- Split the call-processor file counter: filesProcessed (all iterated)
vs resolvedFiles (post registry-primary skip). Key the every-N
progress log and the start-of-phase log on resolvedFiles so mixed
Python+JVM repos where the skipped language sorts first still emit
'calls 1/1 file=...' on the first non-skipped file. Adds a wiring
test for the mixed-language ordering case.
P3
- Restore the original isDev '🔗 E1: Seeded ...' logger.info line so
log scrapers keyed on the emoji marker still match; emit the
[deferred-profile] variant only when deferredProfile && !isDev.
- Move tFile = startTimer(profileCalls) below the registry-primary
skip so skipped files don't trigger an hrtime.bigint() call.
- Document GITNEXUS_PROFILE_DEFERRED and
GITNEXUS_PROFILE_DEFERRED_SLOW_MS in the README env-var table.
* refactor(ingestion): extract parseTruthyEnv to shared utils (U5)
Three narrow-form env-var truthy checkers (verbose.ts, registry-primary-flag.ts,
deferred-resolution-profile.ts) each had their own `'1' | 'true' | 'yes'` parser
with subtle divergences (trim or no trim, set vs disjunction). Consolidate on a
single `parseTruthyEnv(raw)` helper in utils/env.ts — the module already serves
as the centralization point for shared ingestion env constants.
logger.ts's broader `isTruthyEnv` (negative-list, pino-debug convention) stays
untouched — different intent, different semantics.
New table-driven test at test/unit/env.test.ts covers case variants,
whitespace, and rejection of falsy / unknown tokens.
* refactor(ingestion): named constants for deferred-profile log gates (U6)
Replace magic literals 10 / 100 / 3_000 / 5_000 in
deferred-resolution-profile.ts with module-private named constants
LOG_EVERY_N_VERBOSE, LOG_EVERY_N_PROFILE, DEFAULT_SLOW_MS_VERBOSE,
DEFAULT_SLOW_MS. Not exported — internal tuning knobs. Pure refactor;
existing tests assert the exact values and still pass unchanged.
* fix(ingestion): pre-pass denominator for deferred call progress (U1, A1)
The live per-file denominator in processCallsFromExtracted previously
read `totalFiles - skippedRegistryPrimaryFiles` at log time. On mixed
Python+JVM repos where the skipped language interleaves with the
resolved one, the denominator drifts upward as the loop iterates —
files iterated before later skips have been seen carry an inflated
denominator. The live ratio only self-corrects after the final file
has been classified.
Fix: one-pass pre-count over byFile.keys() before the work loop
computes resolvedTotal once. The denominator is then stable from the
first emission onward. The pre-pass runs only on the enabled path
(profileCalls=true) so the disabled path keeps zero extra work.
Adds a wiring test exercising the alternating [ts, py, ts, py, ...]
order that triggered the drift, asserting every emitted line uses
`/4` and no other denominator slips through.
* fix(ingestion): E1 enrichment log emits on both dev and profile flags (U2, A2)
The post-chunk E1 enrichment log used `if (isDev) {...} else if
(deferredProfile) {...}` which is mutually exclusive. On combined runs
(NODE_ENV=development + GITNEXUS_PROFILE_DEFERRED=1) the [deferred-
profile] line was silently swallowed — operators grepping that prefix
saw a gap between wildcard-synth and heritage timings, while the
inline comment promised dual emission.
Fix: two independent `if` statements so both branches fire when both
flags are set. The original emoji-prefixed `🔗 E1: Seeded` line keeps
its phrasing for any dev-mode log scrapers that depend on the marker.
Pinning test (parse-impl-e1-emission-shape.test.ts) reads the source
and asserts (a) both branches exist as standalone `if` statements and
(b) the closing `}` of the isDev branch is followed by `if`, not
`else if`. Source-shape pins are the right test scope for a purely
structural change — the regression we are guarding against is exactly
how a future reader greps for it.
* feat(ingestion): unresolved-side counters in heritage-map profile (U7)
The existing maxNameCartesian / ambiguousHeritageRecords counters in
buildHeritageMap only observed records where BOTH the child and parent
name lookups resolved. On JVM monorepos the actual pathological case is
one side empty (typically an unresolved external supertype with many
same-named children, or vice versa) — those records were silently
dropped from the metric.
Add `unresolvedChildLookups` and `unresolvedParentLookups` in a
separate `if (profileHeritage)` block placed immediately after the two
`lookupClassByName` calls (so it observes the unresolved cases the
length-guarded ambiguity block below cannot see). Both counters reuse
the existing childDefs / parentDefs values — no additional lookups.
Done-summary log extended to include the two new counters. Wiring test
covers both directions (unresolved parent, unresolved child) plus the
existing "both resolved" baseline now asserts the new counters report
zero for that case.
* fix(ingestion): endTimer formatter exception safety (U3)
Wrap the format callback in endTimer in a try/catch so a throwing
formatter (custom toString, JSON.stringify on a circular object,
future heavier serializers) cannot abort the deferred resolution
band. Observability code must never escalate to a load-bearing
failure mode.
On catch we emit a single `[deferred-profile] formatter error: …`
line via logDeferredProfile and return; the caller's stage continues
as if profiling had no-op'd for this timer. DoD §2.8 is satisfied —
the failure is surfaced, not silently swallowed.
Tests cover the four cases: happy path emits the formatted line, null
start no-ops without invoking the formatter, throwing formatter is
caught and surfaces one error line, non-Error throws are coerced via
String() in the message.
* fix(ingestion): defensive wrap + dropped-line counter for logDeferredProfile (U4)
Wrap logger.info inside logDeferredProfile in a try/catch so a throwing
underlying logger cannot abort the deferred resolution band. Pino with
sync:false (the current SonicBoom destination) does not throw
synchronously for `info(string)` calls, but first-use construction
paths (pino-pretty resolve, level validation) and any future transport
reconfiguration could. The wrap is belt-and-suspenders coverage; the
counter makes silent failures visible.
A module-private droppedLogLines counter accumulates dropped lines.
Two helpers — getDeferredProfileDroppedCount() and
resetDeferredProfileDroppedCount() — expose the counter. The handler
deliberately does NOT call the failing logger; that would risk an
infinite loop if the failure is steady-state.
processCallsFromExtracted resets the counter at entry (so each analyze
run gets a fresh count rather than accumulating across the process
lifetime — relevant for the MCP server, eval harness, integration
tests), and surfaces the count in the done-summary as `note: N profile
log lines dropped (logger errors)` when greater than zero. DoD §2.8
(no silent diagnostic catches) is satisfied.
Tests cover the helper API (zero at entry, idempotent reset) and the
happy path; the catch arm is pinned via source-shape assertion since
the logger Proxy can't be vi.spyOn'd directly (lazy `get` trap, no
own-property to wrap).
* docs(readme): clarify GITNEXUS_PROFILE_DEFERRED_SLOW_MS coercion (U8)
The env-var row mentioned integer / scientific notation only, but the
underlying parser (`Number(raw)` since the U2 fix in PR #1773) also
accepts decimals like `.5` and hex like `0x10`. Document the actual
acceptance set plus the non-finite / non-positive fallback so operators
setting unusual values know what to expect.
---------
Co-authored-by: Test <test@example.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
The test previously asserted that FTS verification failure throws during
full analyze. Since FTS failure is now non-fatal (graceful degradation),
update the test to verify analyze completes successfully with a warning
instead of throwing.
The repair-mode test (--repair-fts) still asserts a throw, which is
correct: if the user explicitly requests FTS repair, failure should be
reported.
Closes#1762. `val animal: Animal = Dog(); animal.speak()` resolved
to `Animal.speak` (or no edge to Dog at all) under
`REGISTRY_PRIMARY_KOTLIN=1` because the Kotlin scope query emits BOTH
an annotation type-binding (`animal -> Animal`) and a constructor-
inferred type-binding (`animal -> Dog`). The generic scope-extractor
ranks annotation sources higher than constructor-inferred sources (see
`typeBindingStrength` in scope-extractor.ts), so the annotation
always won and `animal.speak()` dispatched against the static type.
Kotlin's virtual dispatch semantics expect the dynamic type — the
overriding `Dog.speak` should win when the RHS is a constructor call,
because that's what runs at runtime.
Fix: in `emitKotlinScopeCaptures`, suppress the `@type-binding.
annotation` capture when the underlying `property_declaration` has a
`call_expression` value sibling. The constructor-inferred capture
remains, becomes the sole binding for the variable, and receiver-bound
resolution dispatches against the constructed class (and walks its
MRO).
This is intentionally Kotlin-specific — flipping precedence globally
would change behavior for other languages whose static-type
annotations are still the right binding when present. Kotlin is the
language where the constructor RHS is the dispatch target by design.
Verification (REGISTRY_PRIMARY_KOTLIN=1):
- Forced-mode: 21 -> 20 failing of 175 (1 fewer; test 1715 in
`test/integration/resolvers/kotlin.test.ts` now green).
- Default-mode Kotlin: 175/175 unchanged.
- Full resolver suite: 2216/2216 unchanged.
- Remaining 20 failures are tracked by sibling sub-issues
(#1758, #1759, #1760, #1761, #1763).
Does NOT add Kotlin to MIGRATED_LANGUAGES per parent #1746 flip criteria.
Closes#1762. Refs #1746.
Co-authored-by: Test <test@example.com>
Closes#1760. Multi-step intra-file chains like
val user = getUser()
val addr = user.address
val city = addr.getCity()
city.save()
produced no `CALLS` edge for `city.save()` because the Kotlin extractor
only inferred property types for `simple_identifier` values (`val x = y`)
and call expressions with simple-identifier callees (`val x = fn()`).
Navigation expressions (`val addr = user.address`) and call expressions
with navigation-expression callees (`val city = addr.getCity()`)
returned null, leaving `addr` and `city` unbound — the chain broke
two hops before `city.save()`.
Implementation:
- `collectKotlinClassMembers(rootNode)` indexes per-file class fields
(primary-constructor `val`/`var` params + body property declarations)
and method return types. Per-file scope matches the existing
extractor design.
- `inferKotlinPropertyType` gains two new cases:
1. `navigation_expression` value — receiver type via `localTypes`,
field type via `classMembers.fields`.
2. `call_expression` with `navigation_expression` callee — receiver
type via `localTypes`, method return type via `classMembers.methods`.
Both return null when any link is unknown (safe / over-conservative).
Verification (REGISTRY_PRIMARY_KOTLIN=1):
- Forced-mode: 21 -> 20 failing of 175 (1 fewer; test 1491 in
`test/integration/resolvers/kotlin.test.ts` now green).
- Default-mode Kotlin: 175/175 unchanged.
- Full resolver suite: 2216/2216 unchanged.
- Remaining 20 failures are tracked by sibling sub-issues
(#1758, #1759, #1761, #1762, #1763).
Does NOT add Kotlin to MIGRATED_LANGUAGES per parent #1746 flip criteria.
Closes#1760. Refs #1746.
Co-authored-by: Test <test@example.com>
Two related bugs surfaced in REGISTRY_PRIMARY_KOTLIN=1 forced mode:
1. `import models.getRepo` silently resolved to `models/User.kt` (the
first `.kt` file inside `models/` by iteration order) when no file
was named after the symbol. `findKotlinFile` returned a single
directory child as a fallback, so the importer's module-scope mirror
only ever picked up the first arbitrary candidate — `getUser → User`
landed but `getRepo → Repo` never did, and downstream `repo.save()`
resolution fell through to no edge.
2. `for (x in importedCallable())` produced no for-loop type binding
when the callee's return type lived in another file, because
`inferKotlinIterableElementType`'s call-expression arm consulted
only the local file's `returnTypes` map.
Fix:
- Split `findKotlinFile` into `findKotlinExactOrSuffix` (exact / suffix
match only) and `findKotlinDirectoryChild` (legacy single-child
fallback). Add `findKotlinPackageFiles` returning every `.kt`/`.kts`
file inside a package directory. The resolver now fans out the
stripped path through `findKotlinExactOrSuffix → findKotlinPackageFiles`,
returning a `readonly string[]` candidate set. The finalize pass
walks each candidate and picks the one whose `localDefs` actually
export the imported name — exactly the multi-target contract
`FinalizeHooks.resolveImportTarget` already supports.
- `inferKotlinIterableElementType` for `call_expression` now falls
back to the callee's identifier text when the local return-type map
has no entry. `propagateImportedReturnTypes` chain-follows
`loopvar → callee → ElementType` once the imported `callee → Element`
mirror lands at module scope (which now works thanks to fix#1).
Verification (REGISTRY_PRIMARY_KOTLIN=1):
- Forced-mode: 21 -> 18 failing of 175 (3 fewer; tests 487, 1242, 1251
in test/integration/resolvers/kotlin.test.ts now green).
- Default-mode Kotlin: 175/175 unchanged.
- Full resolver suite: 2216/2216 unchanged (incl. `kotlin-calls`
`util.OneArg.writeAudit` regression check at line 176).
- Remaining 18 failures are tracked by sibling sub-issues
(#1758, #1760, #1761, #1762, #1763).
Does NOT add Kotlin to MIGRATED_LANGUAGES per parent #1746 flip criteria.
Closes#1759. Refs #1746.
Co-authored-by: Test <test@example.com>
Closes#1763. `user.validate()` on `class User : Validator` resolved
to no edge under REGISTRY_PRIMARY_KOTLIN=1 when validate() was a
default method declared on the Validator interface:
class User(val name: String) : Validator
interface Validator { fun validate(): Boolean = true }
fun run() { val user = User("alice"); user.validate() }
The generic `buildMro` walks EXTENDS edges only. Kotlin classes
implement interfaces via IMPLEMENTS edges (per the parsing-processor),
so the implementor's MRO never picked up the interface's default
methods — `findOwnedMember(User, validate)` returned undefined and
no fallback walked to Validator.
Fix: replace `defaultLinearize` with a Kotlin-specific MRO builder
modeled after PHP's `buildPhpMro` (trait composition):
1. Run the generic `buildMro` (EXTENDS-only).
2. Collect direct IMPLEMENTS edges as class -> interface[] map.
3. For each class, walk its EXTENDS-MRO ancestors AND its own
IMPLEMENTS edges to seed interface candidates, then BFS-close to
pick up transitive interface inheritance (interface A : B).
4. Append the interface closure to the class's MRO (after the EXTENDS
chain — Kotlin requires explicit override on conflict, so this
ordering is a safe approximation for method lookup).
5. Classes with no EXTENDS but with IMPLEMENTS edges (the #1763
fixture shape) get their MRO seeded directly from their interfaces.
Verification (REGISTRY_PRIMARY_KOTLIN=1):
- Forced-mode: 21 -> 20 failing of 175 (1 fewer; test 2062 in
`test/integration/resolvers/kotlin.test.ts` now green).
- Default-mode Kotlin: 175/175 unchanged.
- Full resolver suite: 2216/2216 unchanged.
- Remaining 20 failures are tracked by sibling sub-issues
(#1758, #1759, #1760, #1761, #1762).
Does NOT add Kotlin to MIGRATED_LANGUAGES per parent #1746 flip criteria.
Closes#1763. Refs #1746.
Co-authored-by: Test <test@example.com>
When the LadybugDB FTS extension cannot be loaded (e.g. macOS 12 where
libc++ lacks std::to_chars(double), or container environments without
the native extension), `gitnexus analyze --embeddings` fails at 85%
and aborts before embedding generation can run.
This is inconsistent with the MCP server's behavior: pool-adapter.ts
already treats FTS load failure as a graceful degradation (ftsLoaded =
false, BM25 search degrades, other features continue).
Change: wrap createSearchFTSIndexes() in try/catch so that:
- FTS creation failure logs a warning instead of throwing
- Embedding generation (Phase 4) proceeds normally
- `--repair-fts` remains available for explicit FTS retry
- BM25 keyword search degrades gracefully (same as MCP read path)
Affected users: macOS 12 (Monterey), any platform where the FTS
extension binary references symbols missing from the system libc++.
Adds tree-sitter @scope.block captures for Kotlin when-arm bodies and
if-then bodies, plus a synthesizer that emits narrowed type-bindings
anchored on those bodies. The receiver-bound calls pass then resolves
`obj.member()` inside `is T` arms against `T` without leaking the
narrowing to sibling arms, `else` branches, or the enclosing function.
Implementation:
- query.ts: @scope.block on `(when_entry (when_condition (type_test))
(control_structure_body))` and `(if_expression (check_expression)
(control_structure_body))`.
- captures.ts: synthesizeKotlinSmartCastBindings walks `when_expression`
and `if_expression` nodes; emits `@type-binding.annotation` with a
`@type-binding.narrowed` marker so kotlinBindingScopeFor in
simple-hooks.ts overrides the scope-extractor's auto-hoist (which
would otherwise promote unbraced-arm bindings to the function scope
because the body anchor coincides with the Block scope's range).
- simple-hooks.ts: kotlinBindingScopeFor checks the marker and pins the
binding to the innermost (Block) scope.
Verification (REGISTRY_PRIMARY_KOTLIN=1):
- Forced-mode: 21 -> 9 failing of 175 (12 fewer; all 12 when/is tests
now green: lines 957, 966, 975, 1096, 1107, 1118, 1131, 1142, 1153,
1164, 1182, 1195 in test/integration/resolvers/kotlin.test.ts).
- Default-mode: 175/175 unchanged.
- Full resolver suite: 2216/2216 unchanged.
- Remaining 9 failures are tracked by sibling sub-issues (#1759-#1763).
Does NOT add Kotlin to MIGRATED_LANGUAGES per parent #1746 flip criteria.
Closes#1758. Refs #1746.
Co-authored-by: Test <test@example.com>
Same-arity Kotlin class-method overloads collapsed onto whichever node
was registered first. `lookup("alice")` resolved to `lookup(Int)` —
not because the picker chose wrong, but because `resolveDefGraphId`
fell through to the simple-name fallback after its parameter-typed key
lookup missed.
Root cause: `populateKotlinOwners` (which calls
`populateClassOwnedMembers`) assigned `ownerId` and qualified names to
class-owned function defs but left `def.type === 'Function'`. The
graph parsing-processor, in contrast, emits a `Method` node label for
class members. `resolveDefGraphId`'s parameter-typed key lookup is
gated on `def.type === 'Method'` (graph-bridge/ids.ts:108-116), so it
was skipped for every Kotlin class method. With the type-keyed lookup
skipped, the resolver fell through to `simpleKey`, which is
first-wins by registration order — and the Int overload always
registered first in these fixtures.
Fix: `populateKotlinOwners` now upgrades `def.type` from `Function`
to `Method` after `populateClassOwnedMembers` assigns `ownerId`. This
aligns the scope-resolution model with the graph's node labels so
parameter-typed key registration and lookup operate in the same
keyspace.
Picker logic in `pickImplicitThisOverload` / `narrowOverloadCandidates`
was already correct — verified by trace: it narrowed `lookup("alice")`
to the `[String]` def. Only the graph-id lookup was broken.
Verification (REGISTRY_PRIMARY_KOTLIN=1):
- Forced-mode: 21 -> 18 failing of 175 (3 fewer; tests 1620, 1659,
1692 in `test/integration/resolvers/kotlin.test.ts` now green).
- Default-mode Kotlin: 175/175 unchanged.
- Full resolver suite: 2216/2216 unchanged.
- Remaining 18 failures are tracked by sibling sub-issues
(#1758, #1759, #1760, #1762, #1763).
Does NOT add Kotlin to MIGRATED_LANGUAGES per parent #1746 flip criteria.
Closes#1761. Refs #1746.
Co-authored-by: Test <test@example.com>
* fix(cli): apply --no-stats to keep-marker stats line (#1706)
The keep-marker branch of upsertGitNexusSection rebuilt the index-summary
line on every analyze and always re-injected the volatile counts,
ignoring --no-stats. For teams that commit a trimmed AGENTS.md/CLAUDE.md
with a gitnexus:keep marker, that produced recurring no-value merge
conflicts — exactly what --no-stats exists to prevent.
Thread noStats into upsertGitNexusSection. Under --no-stats the keep-path
stats line becomes "Indexed as **<name>**" with no (N symbols, ...)
parenthetical; the project name still refreshes so renames propagate.
The statsPattern parenthetical is now optional so a count-free line left
by a prior --no-stats run still matches.
* test(cli): cover count-return and AGENTS.md parity for --no-stats keep path
Addresses review findings F1 and F2 on PR #1765:
- F1: add a test that counts RETURN when --no-stats is dropped after a
prior count-free run — guards against the flag becoming sticky.
- F2: extend the noStats+keep "drops the volatile counts" test to assert
AGENTS.md alongside CLAUDE.md, so a future asymmetry between the two
upsertGitNexusSection call sites is caught.
---------
Co-authored-by: Emmanuel Alawode <platforms@chowbea.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
* fix(mcp): disambiguate duplicate-name repo resolution for worktrees
When multiple indexed repos share the same registry name (main checkout plus linked worktrees), MCP tools no longer silently pick the first sibling. Resolution prefers the repo matching process.cwd()'s git root, throws RegistryAmbiguousTargetError when still ambiguous, and uses canonical path matching aligned with the CLI registry.
Fixes#1658. Complements worktree detect_changes fixes in #1654/#1691.
* fix(mcp): refresh registry on duplicate-name ambiguity before failing
resolveRepo now retries resolveRepoFromCache after RegistryAmbiguousTargetError so stale in-memory siblings clear when the registry changes. Adds detect_changes callTool ambiguity test, registry-refresh regression test, pickRepoHandleForCwd MCP cwd doc, and temp-dir cleanup in #1658 fixtures.
* chore(autofix): apply prettier + eslint fixes via /autofix command
* fix(mcp): PR #1753 review follow-ups + collision-id case bug
Address Findings 3-6 from the production-readiness review on PR #1753,
plus a latent bug surfaced while writing the F5 regression test:
- F3: drop the no-op `try { ... } catch (err) { throw err; }` wrapper
around the miss-path retry in `resolveRepo`; the catch only re-threw.
- F4: rewrite the misleading "child/repo" example on the relative-path
tier — `child/repo` would be classified as path-like and never reach
this branch. Comment now describes bare, separator-free names
resolved against `process.cwd()`.
- F5: add regression test for the stable hashed-id tier so a duplicate
sibling can be reached by its `<name>-<hash>` id. Writing this test
exposed that `repoId()` produced a mixed-case base64url suffix while
`resolveRepoFromCache` lowercased the param before the Map lookup, so
collision ids with any uppercase byte in the hash were unreachable.
Fix: lowercase the hash in `repoId` so it survives `paramLower`.
- F6: add regression test asserting two repos sharing a name prefix
(`project-a`, `project-b`) cause `resolveRepo("project")` to reject
as not-found rather than silently returning the first partial match.
* refactor(mcp): tighten PR #1753 follow-up tests + pin hash length
Address three P2 maintainability findings from the ce-code-review pass
on commit aa7f2050:
- Export `REPO_ID_HASH_LENGTH` from local-backend.ts and use it in both
`repoId()` and the hashed-id test. Closes the silent-drift hole where
the test's inline formula could fall out of sync with the source
without any signal.
- Extract `makeSharedPrefixFixture(nameA, nameB)` next to
`makeDuplicateNameFixture`. Centralises the temp-dir + `.gitnexus`
scaffolding + `duplicateFixtureDirs.push()` cleanup contract so
future callers can't drop the cleanup step.
- Reorder the hashed-id test's comment block so the intentional-coupling
rationale leads, before the description of the formula being mirrored.
* chore(autofix): apply prettier + eslint fixes via /autofix command
* chore: re-run CI
---------
Co-authored-by: Test <test@example.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* fix(group): detect httpx AsyncClient alias imports
* fix(group): anchor httpx dotted imports and skip shadowed aliases
Addresses Findings 1-3 of the production-readiness review on PR #1687.
- F1: the `(dotted_name (identifier) @module)` capture matches every
segment of a dotted module path, so `import package.httpx as hx` and
`from package.httpx import AsyncClient` would falsely populate the
alias sets. Anchor the check on `moduleNode.parent?.text === 'httpx'`
so the full dotted_name must equal `httpx`.
- F2: `moduleAliases` and `asyncClientAliases` were file-global and
unaware of Python scope. A function-local rebind like
`AsyncClient = lambda: MockClient()` left the alias entry intact and
any subsequent `client = AsyncClient(); client.get(...)` emitted a
false-positive consumer contract. Walk every
`(assignment left: (identifier) @name)` whose name matches an alias,
record the enclosing function/class scope as poisoned, and skip
direct- and module-attribute matches when the call site is inside
that scope chain.
- F3: extend the existing fixture with dotted-package look-alikes and
three local-shadow cases (`shadow_direct_alias`, `shadow_module_alias`,
`shadow_direct_context`) and assert the would-be FP contractIds are
not emitted.
- F6: refresh the module-level docstring to mention the supported
import-alias forms and the shadow-exclusion behavior.
* refactor(group): tighten httpx alias shadow detection and broaden tests
Follow-up addressing the residual review findings on PR #1687.
- Replace inline scope-key construction in isAliasShadowed with a
getScopeKey call so the two helpers cannot drift apart (M1).
- Collapse the double tree traversal in collectHttpxAsyncClients: build
one combined alias set and pass it to a single
collectAliasShadowScopes call (perf, P2).
- Add a `shadowScopeKey` helper that returns the scope a rebind actually
shadows under Python LEGB rules: function scope for in-function
rebinds, 'module' for top-level rebinds, and `null` for class-body
rebinds (class attributes do not shadow bare-name lookups in methods).
Removes the previous blanket `scopeKey === 'module'` skip and now
correctly poisons module-level rebinds (correctness #1).
- Extend `ALIAS_SHADOW_PATTERNS` to cover tuple, list, and pattern_list
destructuring targets (correctness #2).
- Rename `ALIAS_REBIND_PATTERNS` to `ALIAS_SHADOW_PATTERNS` and update
the block comment to say "shadowed" rather than "poisoned" (M4).
- Collapse `callScopeKeys` to a single-line return; the dead Set wrap
was misleading future readers (M2).
Tests:
- New negative fixtures for 3-segment dotted import
(`import a.b.c.httpx as deep_evil`), relative import
(`from .httpx import AsyncClient as rel_evil_async`), tuple
destructuring rebind, and an isolated file exercising the module-level
rebind path (T1, correctness #2, expanded F2).
- New positive fixture confirming that a class-body assignment of
`AsyncClient` does NOT poison the surrounding methods.
- Add a positive control assertion for `module_direct_client` so the
dotted-package negative assertions cannot pass vacuously (T3).
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Co-authored-by: Test <test@example.com>
* fix: link object literal methods to exported bindings
* fix(ingestion): bridge object-literal value receivers in scope-resolution (PR #1718 review)
Addresses adversarial production-readiness review on PR #1718 / issue #1358:
- F1 (caller resolution) — setting `ownerId` on object-literal method symbols
alone is not sufficient; the scope-resolution receiver-bound resolver only
consults class-like or type-annotated bindings, so lowercase value receivers
(`export const fooService = {...}; fooService.getUser(...)`) never reach the
owner-indexed lookup. Adds a Case 5 value-receiver bridge in
receiver-bound-calls.ts that resolves the receiver name as a Const/Variable
binding, translates its def to the canonical graph node id, and emits the
CALLS edge via the owner-indexed method registry.
- F2 (boundary guard) — rewrites findObjectLiteralBindingInfo as an explicit
two-phase AST walk: Phase A tracks object-literal depth (returns null for
nested literals and pre-declarator function/class boundaries — IIFE
patterns); Phase B walks the declarator's ancestors and rejects function,
class, and block-statement containers (if / for / while / try / catch /
switch / etc.) before reaching program/export_statement. Prevents false
HAS_METHOD edges for locally-scoped or block-scoped object literals.
- F4 — drops the dead `ownerName` field from ObjectLiteralBindingInfo.
Constraint: TS/JS are scope-resolution migrated per RFC #909; the legacy
Call-Resolution DAG (call-processor.ts) is intentionally left untouched.
Tests:
- test/integration/ast-helpers-object-literal-binding.test.ts (13 cases) —
pins helper semantics: happy paths, function/arrow/class-ctor boundaries,
nested literals, block scope (if / for-of / try), IIFE, assignment
expressions without declarator.
- test/integration/object-literal-owner-resolution.test.ts (9 cases) —
drives the full pipeline against an on-disk fixture: sequential CALLS edge
emission (issue #1358 proof), worker-mode parity, negative local binding,
and nested-literal attribution boundary.
Full sweep: 2958/2958 integration + 6056/6056 unit tests pass.
* refactor(ingestion): address code-review findings on object-literal owner resolution
Multi-agent code review on the prior commit surfaced 7 actionable findings,
all walked through and applied here. None change observable behavior for
issue #1358's fix; all harden correctness, predicate stability, and test
signal.
- #1 (P1 / 3-reviewer corroboration): Case 5 in receiver-bound-calls.ts no
longer hand-builds graph.addRelationship + a dedup key. New
tryEmitEdgeWithExplicitTargetId in edges.ts takes a pre-resolved target
id (the canonical Method nodeId from the parser) and reuses every
invariant of tryEmitEdge: dedup-key format, collapse-flag honoring,
caller-id resolution, rel-id shape, mapReferenceKindToEdgeType for
read/write ACCESSES. This also lands the adversarial reviewer's "F2"
follow-up (hardcoded type: 'CALLS' for non-call sites) for free.
- #2 (P2 cross-reviewer): findValueBindingInScope's predicate inverted
from denylist ("not class-like and not callable") to explicit allowlist
matching reconcileOwnership's registration set:
Const | Variable | Property | Static. Extracted as isOwnableValueLabel
so future NodeLabel additions require an explicit opt-in.
- #6 (P2): walkScopeChain<T>() extracted; both findClassBindingInScope
and findValueBindingInScope now route through it. Local scope.bindings
are exhausted BEFORE lookupBindingsAt (imported/augmented) at every
scope level — preserves JavaScript lexical scoping where a local const
shadows an imported binding of the same name. Behavior was already
correct in findClassBindingInScope but was implicit; now it is the
walker's explicit, documented contract.
- #7 (P2): scope-walker duplication closed. findClassBindingInScope and
findValueBindingInScope reduce to thin wrappers over walkScopeChain
with their respective predicate. findClassBindingInScope keeps its
qualifiedNames + dotted-name fallback tail.
- #3 (P2): parse-worker.ts hoists `const ownerId = enclosingClassId ??
objectLiteralOwnerInfo?.ownerId` once before the symbol push, dropping
the duplicated coalesce + `as string` cast. Matches the cast-free
pattern at parsing-processor.ts:793. HAS_METHOD emit site reuses the
same hoisted local.
- #4 (P2): object-literal-owner-resolution.test.ts Test A's CALLS-edge
assertion no longer matches by name alone. .toEqual now pins the
canonical target id (Method:src/service.ts:getUser#1 via generateId),
confidence (0.85), and reason ('import-resolved'). A regression that
emits the edge at confidence=0, with the wrong reason, or against a
phantom Method node now fails the test.
- #5 (P2): worker-parity test adds a CI tripwire — when CI=1 and
dist/parse-worker.js is missing, throw at module top with a clear
message. Locally, skipIf(!hasDistWorker) keeps the fast-iteration
experience; CI cannot pass with U3 (worker-path ownerId) unverified.
Verification: tsc --noEmit clean. Targeted regression sweep on
ast-helpers-object-literal-binding (13), object-literal-owner-resolution
(9), has-method (60), cross-file-binding (40) — 122/122 pass. Full unit
sweep: 6056/6056. Integration suite: 1 pre-existing Windows-flake in
worker-pool.test.ts (passes 28/28 in isolation) unrelated to this diff.
* refactor(scope-resolution): align Const label emission with legacy DAG (PR #1718 review F1)
Eliminates the architectural fragility surfaced by PR #1718's adversarial review
Finding 1. Previously, normalizeNodeLabel('const') returned 'Variable' while
the legacy DAG parse phase emits 'Const' graph nodes (via @definition.const
capture for lexical_declaration). PR #1718's Case 5 value-receiver bridge
resolved correctly only because resolveDefGraphId happened to fall back to
simpleKey after the qualified-key miss — accidental correctness.
After this change, scope-resolution defs for `const x = ...` declarations
report def.type === 'Const', matching the graph node label. resolveDefGraphId's
qualified-key path now hits on the first try; the simple-key fallback is no
longer load-bearing for value receivers and can be tightened in future without
silently breaking Case 5.
Audit completeness verification:
- Grep `\bVariable\b` across src/core/ingestion/scope-resolution/ surfaced two
consumer sites that already accept both labels: reconcile-ownership.ts:101+168
(`def.type === 'Variable' || def.type === 'Const' || ...`) and
walkers.ts:207 isOwnableValueLabel (`Const | Variable | Property | Static`).
No language hook in src/core/ingestion/languages/ branches on
`def.type === 'Variable'` for what's actually a const declaration.
- Sentinel stress test (the full unit + integration suite run with the
renamed label in place): 6137/6137 unit tests pass; 2967/2967 integration
tests pass. One pre-existing Windows-only flake on worker-pool.test.ts when
run alongside the full integration suite (passes 28/28 in isolation,
unrelated to scope-extractor — same flake observed before this diff).
The variable mapping (`'variable' → 'Variable'`) is preserved for `var`
declarations, matching the legacy DAG's `@definition.variable` capture for
variable_declaration. The split now mirrors the parse-phase capture
distinction exactly.
Per plan docs/plans/2026-05-21-002-feat-pr1718-followups-class-instance-and-label-normalization-plan.md
U4 + U5. T1 (class-instance singleton resolution from issue #1358's second
sub-case) is deferred to a standalone pre-plan investigation, not shipped
here.
* test(ingestion): add regression coverage for issue #1358 singleton sub-cases
Closes the remaining sub-cases of issue #1358 surfaced by PR #1718's
adversarial review (Finding 4, NOTED): the class-instance singleton
(`export const fooService = new FooService();`) and the factory-pattern
singleton (`export const fooService = makeFooService();`).
Pre-plan investigation (per docs/plans/2026-05-21-002 § "Pre-Plan
Investigation Task (T1)") confirmed Outcome A for both patterns — they
already resolve end-to-end through scope-resolution's
`@type-binding.constructor` capture (languages/typescript/query.ts:489-511)
+ `propagateImportedReturnTypes` chain-follow
(scope-resolution/passes/imported-return-types.ts:114) + receiver-bound
Case 4 simple typeBinding lookup (receiver-bound-calls.ts:625). The
mechanism was wired correctly before this session; the regression-net
wasn't.
This test pins the behavior:
- Pattern 1: `caller → FooService.getUser` CALLS edge with
confidence 0.85 and reason 'import-resolved'
- Pattern 2: same edge shape via factory chain-follow (the
`@type-binding.alias` capture for `const u = find()` style)
Both assertions use exact `.toEqual([{...}])` shape pinning so a future
regression that targets a phantom Method node, emits at lower confidence,
or drops the cross-file import-resolved reason fails loudly.
Verification: 5/5 pass, 127/127 in targeted regression sweep including
object-literal-owner-resolution.test.ts, ast-helpers-object-literal-
binding.test.ts, has-method.test.ts, and cross-file-binding.test.ts.
No production code change. The class methods get a class-qualified node id
(`Method:src/service.ts:FooService.getUser#1`) distinguishing them from
same-name methods on other classes — distinct from the bare-name node id
shape PR #1718's object-literal case uses.
* test(resolvers): add class-instance + factory-pattern singleton coverage for TS/JS (issue #1358)
Closes the remaining sub-cases of issue #1358 surfaced by PR #1718's
adversarial review (Finding 4). PR #1718 fixed object-literal-shorthand
singletons (`export const fooService = { getUser() {} }`); this commit adds
parallel coverage for the two other singleton shapes that resolve through
the existing scope-resolution chain:
// Pattern 1 — class-instance singleton
export class FooService { getUser(id) { ... } }
export const fooService = new FooService();
// Pattern 2 — factory-pattern singleton
export class FooService { getUser(id) { ... } }
export function makeFooService() { return new FooService(); }
export const fooService = makeFooService();
Pre-plan investigation (per local plan docs/plans/2026-05-21-002 § "Pre-Plan
Investigation Task (T1)") confirmed Outcome A — both patterns already
resolve end-to-end through:
- `@type-binding.constructor` capture (languages/{typescript,javascript}/
query.ts) seeds `fooService → FooService` at parse time
- `propagateImportedReturnTypes` (scope-resolution/passes/
imported-return-types.ts:114) mirrors the typeBinding cross-file
- Receiver-bound Case 4 simple typeBinding lookup
(scope-resolution/passes/receiver-bound-calls.ts:625) MRO-walks
FooService and emits the CALLS edge to getUser
Tests added per language × pattern (5 each, 10 total):
- node existence (Class, Method, Function, Const, plus Function for the
factory pattern's `makeFooService`)
- HAS_METHOD edge from class to method (class-instance variant)
- CALLS edge from caller to `getUser` with `targetFilePath: 'src/service.{ts,js}'`,
`reason: 'import-resolved'`, `confidence: 0.85` — exact `.toEqual([{...}])`
shape pinning so a regression that emits at lower confidence or drops the
cross-file reason fails loudly
Fixtures placed under the existing `test/fixtures/lang-resolution/` convention.
Tests appended to `test/integration/resolvers/{typescript,javascript}.test.ts`,
matching the in-file pattern of every other resolver scenario.
Also supersedes and removes the standalone
`test/integration/class-instance-and-factory-singleton-resolution.test.ts`
introduced earlier in this PR session (`0df91b77`) — the proper home for
language-resolver scenarios is the per-language resolver test file alongside
similar fixtures (`javascript-self-this-resolution`, `javascript-cross-file`,
`typescript-tsconfig-paths`, etc.). One canonical location for the scenario,
not two.
Verification: 10/10 new singleton tests pass; 297/297 full TS+JS resolver
suite pass (no regression in any existing resolver test).
* test(resolvers): gate TS/JS singleton tests behind scope-resolution parity (CI run 26223603426)
The class-instance and factory-pattern singleton CALLS-edge resolution
tests added in c8e573bc rely on scope-resolution-only mechanisms
(`@type-binding.constructor` capture + `propagateImportedReturnTypes`
mirror + receiver-bound Case 4). The `scope-parity / typescript parity`
and `scope-parity / javascript parity` CI jobs run with
`REGISTRY_PRIMARY_TYPESCRIPT=0` / `REGISTRY_PRIMARY_JAVASCRIPT=0` and
exercise the legacy DAG path, which has no cross-file constructor-derived
typeBinding propagation. Verified by job 77202610819 (TS parity) and
77202610869 (JS parity) failing with:
× resolves caller.fooService.getUser() to FooService.getUser via constructor-inferred typeBinding
× resolves caller.fooService.getUser() through the factory chain to FooService.getUser
Note: my local Windows shell-prefix env-var invocation did not propagate
the flag into vitest workers correctly (the cpp parity gate's 47-skipped
behavior masked the issue when I ran an ad-hoc comparison), so the
empirical "both modes pass" finding I posted earlier was wrong. CI is the
source of truth.
Changes:
- test/integration/resolvers/helpers.ts: add `typescript` and `javascript`
entries to `LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES` for the 2 CALLS-edge
resolution tests in each language. Node-existence and HAS_METHOD
assertions are NOT excluded — those pass under legacy DAG (parser-level
emission is intact).
- test/integration/resolvers/typescript.test.ts: drop the `it` import from
vitest; replace with `const it = createResolverParityIt('typescript');`
shadow (matches the c/cpp/csharp/go pattern at the top of those files).
- test/integration/resolvers/javascript.test.ts: same shadow with
`createResolverParityIt('javascript')`.
Verification:
- Default mode (registry-primary): 297/297 TS+JS resolver tests pass.
- Legacy DAG mode: the 4 listed singleton CALLS-edge tests will skip; all
other singleton assertions (node existence + HAS_METHOD edge) continue
to run and pass under both modes.
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
* fix(install): materialize vendored grammars to fix Windows EPERM (#1728)
Stop using file: optionalDependencies for tree-sitter-dart/proto/swift,
which made npm symlink vendor paths on install and fail on Windows without
symlink privileges. Copy vendor trees into node_modules at postinstall
instead; keep native builds and #836 vendor hygiene.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(install): atomic materialize swap + fail-soft tests (#1728, #836)
Hardens PR #1729 against two issues the original implementation could
still hit:
1. Torn-state on rmSync→cpSync. The previous loop deleted the
destination before copying. If cpSync threw — the exact Windows EPERM
scenario this PR targets — a previously-working grammar was silently
wiped. Now we copy to {dest}.materialize-tmp first and renameSync into
place, so an interrupted copy leaves the prior materialization intact.
2. Fail-soft try/catch had no test coverage. Adds two POSIX-only tests
(chmod 0o555 to deterministically force cpSync to throw) that verify
(a) a single grammar failure does not abort the other two, and (b) an
existing materialization survives a partial-copy failure. Skipped on
Windows where chmod doesn't enforce write restriction; runs on Linux
CI.
Other test improvements locking in the install-hygiene invariants:
- All three vendored grammars (dart/proto/swift) checked, not just dart.
- GITNEXUS_SKIP_OPTIONAL_GRAMMARS=1 short-circuit is exercised.
- Vendor cleanliness (#836): no node_modules/build under vendor/.
- Idempotent re-runs (clean overwrite verified via sentinel file).
- Missing-vendor warn+continue path now has explicit coverage.
- Vendored package manifests asserted to carry no install script or
runtime dependencies.
- package.json optionalDependencies asserted free of vendored grammars.
- package-lock.json assertion tightened from `if (entry !== undefined)
{ expect(entry.link).not.toBe(true); }` (vacuous when entry is absent,
i.e. the expected post-fix state) to `expect(...).toBeUndefined()`.
Verified locally:
- npx tsc --noEmit: clean
- vitest test/unit/materialize-vendor-grammars.test.ts: 8 pass + 2
POSIX-only skipped on Windows
- npm pack tarball: no vendor/*/node_modules or vendor/*/build entries
- Isolated global install (clean + upgrade + SKIP env) into temp prefix:
succeeds; gitnexus --version → 1.6.5; vendor stays clean post-install.
* fix(install): address review feedback — Swift parity, atomicity, CI smoke
Resolves all findings from the automated production-readiness review on
verify/issue-1728-symlink.
Swift warning parity (review #2):
Add tree-sitter-swift to OPTIONAL_GRAMMARS in src/cli/optional-grammars.ts
alongside Dart and Proto. Before this commit, Swift was materialized at
postinstall and probed by build-tree-sitter-swift.cjs but the runtime
warnMissingOptionalGrammars() never warned when it failed to load —
users got silent Swift degradation from the optional-grammars surface
(parser-loader's separate unavailableNote only fires on demand). Now
the warning path matches the materialize path.
README env-var table (review #1):
Update the GITNEXUS_SKIP_OPTIONAL_GRAMMARS row at README.md line 248 to
list all three vendored grammars (dart, proto, swift). The quick note
earlier in the README already mentioned all three; only the table row
was stale.
Atomicity hardening (review #3):
materialize-vendor-grammars.cjs now copies to {dest}.materialize-tmp,
renames the existing dest to {dest}.materialize-bak (if present), then
renames the partial into dest, then removes the backup. If the
partial→dest rename fails (e.g. Windows AV scanner racing the swap),
the catch block restores from backup so the previously-materialized
grammar is preserved. Closes the narrow torn-state window where the
prior implementation could leave dest deleted after rmSync succeeded
but renameSync failed.
Swift probe docs (review #4):
build-tree-sitter-swift.cjs script header rewritten to describe what
the script actually does — probe node-gyp-build at install time so
missing-prebuild failures surface as install-time warnings instead of
first-parse runtime errors. The script does not "activate" anything;
the runtime require() in parser-loader does the actual load. Console
warning text updated to match ("prebuild probe" not "activation").
Windows packaged-install smoke test (review #5):
New CI job `packaged-install-smoke` in .github/workflows/ci-tests.yml
matrices on windows-latest and ubuntu-latest. Runs npm pack, installs
the produced tarball globally into RUNNER_TEMP, then asserts:
* no vendor/*/node_modules or vendor/*/build (#836 invariant)
* tree-sitter-{dart,proto,swift} in node_modules are real
directories, not junctions/symlinks (#1728 invariant)
* gitnexus --version runs against the installed CLI
Closes the coverage gap where the existing windows-latest job only
ran `npm ci` in the source checkout — exercising postinstall but not
the tarball reify step that historically tripped EPERM.
Verified locally:
npx tsc --noEmit: clean
vitest test/unit/materialize-vendor-grammars.test.ts test/unit/cli-commands.test.ts:
18 pass + 2 POSIX-only skipped on Windows
prettier + eslint on all changed files: clean
* fix(ci): disable credential persistence on packaged-install-smoke checkout
GitHub Advanced Security (zizmor artipacked) flagged the new
packaged-install-smoke job's actions/checkout step as a potential
credential-persistence risk. The job runs `npm pack` + global install
and never pushes back, so the GITHUB_TOKEN that checkout would persist
in .git/config provides no value and only widens the leak surface (any
future artifact-upload step in this job would carry the token).
Disable persistence explicitly via `persist-credentials: false` on this
job's checkout. Scoped to the new job — pre-existing checkouts above
are left unchanged.
* fix(ci): use find instead of ls for tarball lookup (SC2012)
actionlint shellcheck SC2012 flagged `TARBALL=$(ls gitnexus-*.tgz | head -n1)`.
Switch to `find . -maxdepth 1 -name 'gitnexus-*.tgz' -print -quit` which
handles non-alphanumeric filenames safely. Also add an explicit
empty-result check so the failure mode is a clear error message instead
of a silent `npm install -g ""` later.
* fix(tests): sabotage vendor src (not partial path) in POSIX fail-soft tests
The fail-soft tests in materialize-vendor-grammars.test.ts pre-chmod'd
the destination's .materialize-tmp partial directory to 0o555 to force
cpSync to throw. After the atomicity rewrite (`fix(install): atomic
materialize swap + fail-soft tests`), the materialize script now starts
each grammar's loop with `fs.rmSync(partial, { force: true })`, which
deletes the chmod'd sabotage before cpSync runs — so cpSync succeeds and
the partial is then renamed into dest, leaving the test's `finally`
block with no path to chmod back (ENOENT) and the assertion that proto
remained unmaterialized failing because it materialized cleanly.
Fix: sabotage the *vendor source* directory (which the script reads from
but never modifies) by chmod'ing it to 0o000. cpSync then fails on
readdir, the catch block fires per-grammar, dart and swift still
materialize from their unaffected sources, and the existing-dest
preservation test verifies that a sabotaged second-run leaves the prior
materialization (and its sentinel file) intact.
Tests now pass locally (8 pass + 2 POSIX-only skipped on Windows) and
should pass on macOS/Ubuntu CI where the sabotage runs.
* fix(tests): restrict fail-soft tests to Linux (macOS Node cpSync abort)
Node 22 on macOS aborts the process with `libc++abi: terminating due
to uncaught exception filesystem_error` when fs.cpSync hits a source
directory it can't read — the abort happens at the C++ filesystem layer
and bypasses Node's JS try/catch entirely (nodejs/node#51399). My
chmod-0o000-the-source sabotage strategy triggers this SIGABRT on
macOS CI before the production script's `try { cpSync } catch` ever
runs, so the test sees a child-process crash instead of the fail-soft
warning it's verifying.
The production script's fail-soft is correct on Linux (where EACCES
surfaces as a normal JS exception) and effectively untestable on macOS
via permission sabotage. Real installs don't hit this — npm always
ships vendor/ with readable permissions — so the macOS gap is a test
artifact, not a behavior gap.
Restrict the two chmod-based tests to Linux only by replacing
`skipOnWin` with `linuxOnly`. Linux CI continues to verify both the
one-grammar-fails-others-succeed and existing-materialization-preserved
invariants. macOS and Windows runs skip these two scenarios; the other
8 tests still run on every platform.
* fix(tests): remove materialize unit tests, rely on CI smoke job
The materialize-vendor-grammars.test.ts file has been a recurring source
of platform-specific CI noise:
- Windows: chmod doesn't enforce read/write restrictions the way POSIX
does, so the fail-soft tests had to be skipped there.
- macOS Node 22: cpSync against an unreadable source aborts the process
with a libc++ filesystem_error (nodejs/node#51399) that bypasses JS
try/catch entirely — making the chmod-based fail-soft tests
unrunnable on macOS too.
- The "vendor-cleanliness" and "idempotency" tests on Windows
intermittently flake due to fs.cpSync timing on the GitHub runner.
The invariants these tests verified are now covered by stronger,
more realistic surfaces:
- packaged-install-smoke (ci-tests.yml): runs `npm pack` then
`npm install -g ./gitnexus-*.tgz` on windows-latest and
ubuntu-latest, then asserts no vendor/*/node_modules,
no vendor/*/build (#836), no junctions/symlinks on the
materialized grammar directories (#1728), and a working
`gitnexus --version`. This is the actual end-user install path.
- cli-commands.test.ts (kept, unmodified): asserts package.json
declares no `file:` optionalDependencies for vendored grammars,
the Swift vendor manifest carries no install script or
dependencies, and the postinstall chain runs
materialize-vendor-grammars.cjs + build-tree-sitter-swift.cjs.
These are static manifest checks — deterministic, fast, no
flake risk.
Removing the dynamic script-execution tests trades unit-level coverage
for end-to-end smoke coverage that actually exercises the
`file:` → cpSync change against a real npm install lifecycle, on
the platform the fix targets (windows-latest).
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(analyze): prevent cache-hit native workers from aborting
Delay parse worker startup until a cache miss requires it, fall back to sequential parsing when initial worker readiness fails, and preserve analyzer diagnostics/progress when heap respawn captures child output.
Constraint: Node 25 and tree-sitter/N-API worker initialization can abort before ready, while warm-cache analysis should not start workers at all.
Rejected: Treating status-134/SIGABRT as heap OOM unconditionally | native worker aborts require distinct recovery guidance and stderr/stdout evidence.
Rejected: cli-progress noTTYOutput for respawn progress | it appends newline frames instead of preserving one-line redraw UX.
Confidence: high
Scope-risk: moderate
Directive: Keep parse-worker creation behind confirmed cache misses and preserve TTY-style progress when respawn pipes stderr for crash classification.
Tested: GitNexus impact analysis for ensureHeap, runChunkedParseAndResolve, createWorkerPool, WorkerPool, walkRepositoryPaths; GitNexus detect_changes scoped to staged worktree; targeted vitest for analyze respawn, parse lazy cache, filesystem walker, worker pool; npx tsc --noEmit; npm run build; NODE_OPTIONS='--max-old-space-size=8192' npm test.
Not-tested: Windows terminal rendering and published npm package install path.
* ci(docker): tolerate slower arm64 TypeScript builds
Docker PR builds run gitnexus prepare under QEMU for linux/arm64, where the fixed 120s TypeScript timeout can kill otherwise healthy builds. Increase the default timeout and allow GITNEXUS_BUILD_TIMEOUT_MS to tune slower environments without changing the build steps.
Constraint: PR #1751 Docker Build & Push gitnexus failed with spawnSync /bin/sh ETIMEDOUT while running node_modules/.bin/tsc in scripts/build.js.\nRejected: Rerunning CI only | the failure was the build script's deterministic timeout boundary under arm64 emulation, not a code assertion.\nConfidence: high\nScope-risk: narrow\nDirective: Keep build timeout changes in scripts/build.js configurable; do not hide real compiler failures, only allow slower successful compiles to finish.\nTested: GitNexus impact for gitnexus/scripts/build.js reported LOW; gitnexus detect_changes reported 1 changed file, 0 affected processes, low risk; git diff --check; gitnexus npm run build.\nNot-tested: GitHub Docker arm64 build rerun before pushing; local Docker multi-platform build under QEMU.
* fix(analyze): truncate respawn progress safely
Preserve complete ANSI escape sequences and grapheme boundaries when the respawn progress terminal shim truncates wrapped output, so the shim does not emit dangling escape bytes or split surrogate pairs while keeping raw writes untouched.
Constraint: Claude review on PR #1751 flagged `s.slice(0, width)` in createAnsiPipeTerminal.write() as a latent terminal-corruption risk.
Rejected: Adding a display-width dependency | a local helper is sufficient for this narrow respawn terminal shim and avoids new dependency churn.
Rejected: Changing silent status-134 classification | current tests already document the output-less 134 fallback as heap guidance.
Confidence: high
Scope-risk: narrow
Directive: Keep respawn terminal writes ANSI-aware and preserve rawWrite bypass semantics for callers that intentionally write control sequences.
Tested: GitNexus impact for createAnsiPipeTerminal reported LOW; GitNexus detect_changes reported 2 changed files, 3 affected processes, medium risk; targeted vitest for analyze respawn progress and heap respawn; gitnexus npx tsc --noEmit; prettier check for changed files; eslint for changed files.
Not-tested: Full npm test suite; manual terminal rendering on Windows.
---------
Co-authored-by: wangxc <wangxc_a_bj@si-tech.com.cn>
* fix(lbug): keep serve stable when sidecars are missing
Shared missing-shadow WAL recovery prevents repeated read-only open warnings when LadybugDB sidecars are absent, while the Express preflight fix keeps `gitnexus serve` compatible with Express 5 route parsing.
Constraint: LadybugDB read-only replay can require a `.shadow` sidecar that may be absent after interrupted writes or checkpoint edge cases.
Rejected: keep reactive WARN-only quarantine in each adapter | it leaves repeated user-visible warnings and duplicate recovery behavior.
Confidence: high
Scope-risk: broad
Directive: Do not silently delete large orphan WALs; only quarantine tiny orphan WALs before open and keep large WALs for explicit recovery.
Tested: cd gitnexus && npx vitest run test/unit/sidecar-recovery.test.ts test/unit/lbug-adapter-wal-schema.test.ts test/unit/pool-wal-recovery.test.ts test/unit/web-ui-serving.test.ts && npx tsc --noEmit
Not-tested: full npm test in this split branch; full unit suite passed on the source branch before PR split.
Co-authored-by: OmX <omx@oh-my-codex.dev>
* fix(lbug): pool-caller ENOENT guard, symmetric size gate, permission-aware errors (PR #1747 review)
Addresses the production-readiness review of PR #1747 (Findings 1, 2, 3 of 6).
Findings 4, 5, 6 are deferred to follow-ups per the plan.
1. ENOENT-tolerance scoped to pool-adapter callers only
- `quarantineWalForMissingShadow` stays strict in `sidecar-recovery.ts`.
The direct adapter calls it inside `acquireInitLock` (cross-process
file lock) — ENOENT there means the file vanished under lock and
remains a real bug to surface.
- New `tryQuarantineForMissingShadow` local helper in `pool-adapter.ts`
returns a discriminated union { kind: 'quarantined', path } |
{ kind: 'peer-handled' }. Catches ENOENT, re-verifies via
statIfExists, and converts to 'peer-handled' only when WAL really
is gone. Defensive: if ENOENT but WAL still present, throws as
classified error rather than silently returning success.
2. Symmetric WAL-size gate on both recovery paths
- `refuseLargeWalQuarantine` applied in both
`reopenReadOnlyAfterMissingShadow` and
`reopenWritableAfterMissingShadow`. Closes the read-only data-loss
vector (large orphan WAL silently discarded would never be replayed
by a later writable open).
3. Permission-aware error classifier
- New `renameFailureMessage` and `isPermissionRenameError` in
`sidecar-recovery.ts`. EACCES / EPERM / EBUSY now surface a
permission-specific message pointing at ACLs, AV exclusions, and
file-locks. Other codes (ENOSPC, EROFS, EIO, ENOENT) fall through
to `shadowSidecarRecoveryMessage`.
- Used at both pool-adapter and direct-adapter caller catches around
`quarantineWalForMissingShadow`.
- `doInitLbug`'s pass-through classifier extended to include the new
permission message. The lock-retry substring match tightened so
"file-lock error" in the permission message is not mistaken for a
LadybugDB lock-retry trigger.
Tests
- sidecar-recovery.test.ts: 7 new tests for `renameFailureMessage` and
`isPermissionRenameError`.
- pool-wal-recovery.test.ts: 6 new tests covering ENOENT race,
EACCES/EPERM/EBUSY classification, ENOSPC fallthrough, and the
defensive "WAL still present after ENOENT" branch.
- lbug-adapter-wal-schema.test.ts: 5 new tests covering the symmetric
size gate on both recovery paths, including the boundary at exactly
TINY_ORPHAN_WAL_BYTES (4096) and the off-by-one at 4097.
Deferred (tracked as follow-up work)
- Brittle LadybugDB error-string matching (Finding 4).
- PNA header end-to-end coverage gap (Finding 5).
- warnedKeys module-global persistence (Finding 6).
- Cross-process init lock for pool-adapter.
* fix(lbug): dedup shadow-replay predicate + counter-based warn anti-spam (PR #1747 review, Findings 4 & 6)
Smallest viable response to the two remaining non-blocking findings from the
production-readiness review of PR #1747. An earlier-revision plan proposed
regex widening + a near-miss detector + per-dbPath warn scoping; an
adversarial doc-review found those defended against hypothetical strings
LadybugDB does not produce, added observability theater with no recovery
behavior change, and did not actually fix the long-running gitnexus serve
case for hot dbPaths (where finalizeLbugSidecarsAfterClose rarely fires).
Scope shrunk to dedup + counter-based — strictly behavior-changing and
fully testable.
Finding 4 — dedup + version-coupling markers
- `isReadOnlyShadowReplayError` was inlined in both `lbug-adapter.ts:451`
and `pool-adapter.ts:317`. Centralized as an export from
`sidecar-recovery.ts`. The two local copies are removed; both adapters
now import from the shared module.
- Both LadybugDB-coupled predicates (`isMissingShadowSidecarError` and
`isReadOnlyShadowReplayError`) gain a `// LADYBUGDB-CONTRACT:` marker
comment citing `@ladybugdb/core ^0.16.1`. When bumping LadybugDB,
`git grep "LADYBUGDB-CONTRACT"` enumerates every version-coupled spot.
- Strict matcher unchanged — when LadybugDB actually changes the error
format, the failure mode stays loud (raw native error propagates) and
the markers make every affected predicate trivially greppable.
Finding 6 — counter-based warn anti-spam
- `warnedKeys: Set<string>` → `warnedKeyCounts: Map<string, number>`.
`warnOnce` keeps its signature `(logger, key, message)` and keying
convention unchanged — the swap is internal.
- `WARN_MILESTONES = [1, 10, 100, 1000, 10000]`. Logarithmic spacing
gives O(log N) warns for a condition that fires N times. Past the
first occurrence the warn message is suffixed with "(Nth occurrence
of this condition)" so persistence is visible in the log line itself.
- Solves the long-running serve case: a hot dbPath hitting the same
condition 100 times now fires 3 warns (occurrences 1, 10, 100)
instead of 1 warn + 99 silent debug lines.
Tests (10 new in sidecar-recovery.test.ts, all green)
- Centralized isReadOnlyShadowReplayError: positive match, false-positive
guard, structural assertion that the duplicate regex is gone from both
adapter files, LADYBUGDB-CONTRACT marker count.
- Counter-based warnOnce: milestone-at-10 with suffix, milestone-at-100,
key isolation across dbPaths, reset zeroes the counter, first-occurrence
message does NOT carry the suffix.
Deferred (tracked separately)
- Finding 5 — PNA header end-to-end coverage gap (CORS boundary is sound).
- LadybugDB structured error codes (if/when the library exposes them).
- Per-call milestone configurability — re-open if tuning is needed.
* chore(autofix): apply prettier + eslint fixes via /autofix command
* ci: trigger CI rebuild
---------
Co-authored-by: wangxc <wangxc_a_bj@si-tech.com.cn>
Co-authored-by: OmX <omx@oh-my-codex.dev>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
