feat(ingestion): language-agnostic heritage extractor with config+factory pattern#890
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…er-language configs - Create heritage-types.ts with HeritageExtractionConfig, HeritageExtractor, HeritageInfo interfaces - Create heritage-extractors/generic.ts with createHeritageExtractor() factory - Create 11 per-language config files in heritage-extractors/configs/ - Add heritageExtractor field to LanguageProviderConfig interface - Wire createHeritageExtractor() in all 16 language providers - Replace inline heritage capture handling in parse-worker with provider.heritageExtractor - Add extractFromCall support for Ruby call-based heritage (include/extend/prepend) - Replace inline heritage logic in heritage-processor extractExtractedHeritageFromFiles - Ruby heritage config absorbs mixin routing logic from CallRouter Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/b55df80d-758a-48cd-96de-a7657bdc2acb Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
…actory - 36 tests covering factory construction, capture-based extraction, Go named field filtering, Ruby call-based heritage, and all 14 config validations Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/b55df80d-758a-48cd-96de-a7657bdc2acb Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
- Simplify makeNode test helper parameter type - Use !== null check in parse worker for extractFromCall result Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/b55df80d-758a-48cd-96de-a7657bdc2acb Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
|
@claude Act as a senior GitNexus implementation engineer. Your job is to implement the requested feature in a way that is correct, scoped, maintainable, and safe for this repo. You are working in the GitNexus monorepo:
Your task has 4 phases, in this exact order: PHASE 1 — LOAD CONTEXT Always read:
Also read any files directly related to the feature area before editing.
PHASE 2 — DEFINE THE IMPLEMENTATION SHAPE
Do not start coding until you have identified the likely extension points. PHASE 3 — IMPLEMENT PHASE 4 — VALIDATE
PRIMARY OBJECTIVE Do not produce speculative redesigns. IMPLEMENTATION RULES
REPO-SPECIFIC CONSTRAINTS 1. Correctness & functional completeness
2. Code clarity & maintainability
3. Test coverage & change safetyImplement or update tests per TESTING.md.
Assertions should be meaningful, not tautological. At minimum, think in terms of:
4. PerformanceBe careful in hot paths, especially in ingestion/group sync / graph-related code:
Do not optimize blindly, but do not introduce obvious scale hazards. 5. Operational riskPreserve:
If the feature changes any of these, state it explicitly. NON-NEGOTIABLES
WHEN IMPLEMENTING
If something cannot be safely completed from available context, say exactly what is missing instead of guessing. OUTPUT FORMAT Implementation plan
Changes madeFor each meaningful change, include:
ValidationUse these exact headings:
What I could not fully verifyList anything that could not be proven from the available code/tests/context. Final statusChoose one:
If follow-up is required, list the blocking items clearly. IMPORTANT BEHAVIOR
|
|
Claude finished @magyargergo's task in 5m 53s —— View job PR Review: Heritage Extractor Config+Factory PatternTasks:
Implementation PlanFeature summary: Migrates heritage extraction (extends/implements/mixins) from ad-hoc inline logic in the parse worker and heritage processor to the same config+factory pattern used by method, field, call, and variable extractors. Introduces Files/packages that changed: Existing contracts preserved:
Risks / edge cases: Go named field filtering, Ruby missing enclosing class, Ruby multi-arg mixin calls. All handled and tested. Changes Made
Fix applied locally (push blocked): ValidationCorrectness & functional completeness✅ Fully wired end-to-end. Both consumer paths (parse-worker and heritage-processor) delegate to ✅ Go named field filtering is correct. ✅ Ruby mixin interception is correct.
Code clarity & maintainability✅ Follows the established extractor pattern exactly. ✅ No cross-phase coupling introduced. Heritage extraction still lives entirely within the
✅
Test coverage & change safety✅ Factory construction: All 14 configs tested for clean construction. ✅ Generic extraction: All three capture kinds tested (extends, implements, trait-impl). Empty capture map, partial captures, and all-three-at-once are covered. ✅ Go shouldSkipExtends: Anonymous embedding (pass), named field (skip), orphan node without parent (pass) — all three cases tested. ✅ Ruby call-based: ✅ Language config validation: All 14 languages have their ❌ Missing: integration/wiring test for parse-worker path. The unit tests exercise the factory and configs directly but don't test the ❌ Missing: regression test for heritage-processor.ts path. The ❌ Missing: Vue provider wiring test. Vue uses Performance✅ No performance regressions. Operational risk✅ No graph schema changes. ✅ No MCP contract changes. No tool/resource signatures modified. ✅ Cross-platform safe. No path manipulation in new code. ✅ No LadybugDB invariant violations. Extraction is read-only from the database perspective. ✅ No embeddings semantics changes. Not in the extraction/indexing path. What I Could Not Fully Verify
Final StatusThe core implementation is architecturally correct, well-patterned, and safe. The ruby.ts comment fix has been applied. Blocking items before merge:
|
- parse-worker.ts: drop redundant captureMap['call']! non-null assertion at the Ruby heritage interception site; reuse the existing callNode local from the enclosing if (captureMap['call']) guard (matches the unasserted read on the callRouter line below). - parse-worker.ts: document the intent of the heritageItems.length > 0 guard and its heritage.extends/implements/trait fallback so readers see why both continues are needed. - ruby.ts: rewrite self-contradictory JSDoc. Clarify that class inheritance (class A < B) goes through standard @heritage.extends captures while only mixin calls (include/extend/prepend) use callBasedHeritage. - Add test/integration/heritage-extractor-wiring.test.ts exercising the Ruby callBasedHeritage and TypeScript heritage extract hooks against real tree-sitter ASTs. Covers single/multi-arg include, scope_resolution extend, nested module/class enclosing-class resolution, top-level no- enclosing-class fallthrough, and TS extends + implements co-occurrence.
|
@claude Act as a senior reviewer for GitNexus. Your job is to determine whether this PR is production-ready for this repo, not to give a generic code review. You are reviewing a PR in the GitNexus monorepo:
Your task has 2 phases, in this exact order: PHASE 1 — DEFINE THE BAR PHASE 2 — REVIEW THE PR AGAINST THAT BAR CONTEXT TO LOAD FIRST
Additional context:
PRIMARY OBJECTIVE Do not optimize for completeness at the expense of signal. REVIEW RULES
For each finding, assign one severity:
REPO-SPECIFIC REVIEW CHECKLIST 1. Correctness & functional completenessCheck:
2. Code clarity & clean codeCheck:
3. Test coverage & change safetyEvaluate against TESTING.md:
4. PerformanceInspect for:
5. Operational riskCheck:
6. MaintainabilityCheck:
NON-NEGOTIABLES TO VERIFY
OUTPUT FORMAT Production-ready bar for this PRGive a concise repo-specific definition in 5–10 bullets. FindingsFor each finding, use this structure exactly: [BLOCKING|NON-BLOCKING|NIT] Short title
Only include findings that are real and supported.
What I could not fully verifyList any behavior you could not prove from the diff, tests, or available context. Final verdictChoose exactly one:
If verdict is not ready, list the blocking items in priority order. IMPORTANT REVIEW BEHAVIOR
|
|
Claude finished @magyargergo's task in 5m 52s —— View job PR Review: Heritage Extractor Config+Factory PatternTasks:
Production-ready bar for this PRFor a heritage extractor refactor touching the ingestion pipeline in GitNexus, "production-ready" means:
Findings1. Correctness & functional completenessNo issues found for the worker path. The new Go named field filtering is correct. [NON-BLOCKING] Dead heritage code in
|
magyargergo
left a comment
magyargergo
left a comment
There was a problem hiding this comment.
There are comments from claude that need to be addreu!
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 6506 tests passed 97 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
… file Addresses CI format check failure: npx prettier --check flagged 3 files with code style issues. Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/ff7b0282-ec10-433d-8286-a47c152fa66c
Fixed the CI formatting failure in Regarding Claude's review findings: those two reviews (comment IDs 4258797672, 4259331218) were about the call extractor feature on a previous iteration of this branch, not the heritage extractor changes. The specific findings (C#/Kotlin The heritage extractor changes have separate coverage via 36 unit tests + the integration tests you added in |
Addressed in The factory
All 11 providers with default behaviour now pass the enum directly: heritageExtractor: createHeritageExtractor(SupportedLanguages.Java),
This was addressed in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@claude Act as a senior reviewer for GitNexus. Your job is to determine whether this PR is production-ready for this repo, not to give a generic code review. You are reviewing a PR in the GitNexus monorepo:
Your task has 2 phases, in this exact order: PHASE 1 — DEFINE THE BAR PHASE 2 — REVIEW THE PR AGAINST THAT BAR CONTEXT TO LOAD FIRST
Additional context:
PRIMARY OBJECTIVE Do not optimize for completeness at the expense of signal. REVIEW RULES
For each finding, assign one severity:
REPO-SPECIFIC REVIEW CHECKLIST 1. Correctness & functional completenessCheck:
2. Code clarity & clean codeCheck:
3. Test coverage & change safetyEvaluate against TESTING.md:
4. PerformanceInspect for:
5. Operational riskCheck:
6. MaintainabilityCheck:
NON-NEGOTIABLES TO VERIFY
OUTPUT FORMAT Production-ready bar for this PRGive a concise repo-specific definition in 5–10 bullets. FindingsFor each finding, use this structure exactly: [BLOCKING|NON-BLOCKING|NIT] Short title
Only include findings that are real and supported.
What I could not fully verifyList any behavior you could not prove from the diff, tests, or available context. Final verdictChoose exactly one:
If verdict is not ready, list the blocking items in priority order. IMPORTANT REVIEW BEHAVIOR
|
|
Claude finished @magyargergo's task in 6m 16s —— View job PR Review: Heritage Extractor Config+Factory PatternTasks:
Production-ready bar for this PRFor a heritage extractor refactor touching the ingestion pipeline in GitNexus, "production-ready" means:
Findings1. Correctness & functional completenessAll three execution paths are now fully wired to the extractor:
Go named field filtering is correct. Ruby mixin interception is correct. Graph schema preserved: No issues found in this area. 2. Code clarity & clean code
[NON-BLOCKING] Integration test uses non-standard
|
magyargergo
left a comment
magyargergo
left a comment
There was a problem hiding this comment.
Integration test uses non-standard SupportedLanguages import path
Evidence: test/integration/heritage-extractor-wiring.test.ts:16 — import { SupportedLanguages } from '../../src/config/supported-languages.js'
Why it matters: CONTRIBUTING.md mandates "direct imports from gitnexus-shared". Every other test file, including the unit test added in this same PR (heritage-extraction.test.ts:10), imports from 'gitnexus-shared' directly. The re-export file exists so it compiles, but it creates an inconsistency reviewable by the next contributor.
Recommendation: Change '../../src/config/supported-languages.js' → 'gitnexus-shared'.
Confidence: High
Fix this
language variable shadowing in processHeritage
Evidence: heritage-processor.ts:191 — const language = getLanguageFromFilename(file.path) (SupportedLanguages | null); heritage-processor.ts:226 — const language = parser.getLanguage() (tree-sitter Language object, inside a try {} block)
Why it matters: Two language variables in the same function with entirely different types. The inner one is correctly scoped to the try block and used only for new Parser.Query(language, ...), but a future contributor editing the heritage processing block could accidentally use the wrong binding. The outer language is the enum value that flows into heritageExtractor.extract() and resolveAndAddHeritageEdge().
Recommendation: Rename the inner variable: const treeSitterLang = parser.getLanguage(); and update the Query constructor call.
Confidence: High
Fix this
…age-processor Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/96a027ec-b115-48aa-8556-a3b60145bb6d Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Both fixed in
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…elabel modules as Trait Fixes two correctness bugs in Ruby mixin heritage resolution identified by a Codex adversarial review, plus the test-integrity issues the follow-up code review flagged on the regression suite. Sequential ingestion fallback now extracts call-based heritage (Ruby include/extend/prepend) during the prepass, so sequentialHeritageMap is populated with mixin ancestry before processCalls resolves calls against it. extractExtractedHeritageFromFiles also runs heritageExtractor.extractFromCall on @call captures, mirroring the worker-path routing. The prepass stays read-only with respect to the graph -- processCalls still owns edge emission via rubyHeritage -> processHeritageFromExtracted. Ruby module declarations are now relabeled to Trait during the structure phase so they participate in lookupClassByName and buildHeritageMap. Unlike the inert Module label, Trait is already a dispatch-category class-like label, so mixin owners resolve through the type registry. The relabel lives in one site (rubyLabelOverride consulted by getLabelFromCaptures) plus the CONTAINER_TYPE_TO_LABEL mapping for enclosing-class-id lookup -- both paths switch in lockstep so method-owner IDs stay consistent with structure-phase IDs. Regression suite adds ruby-sequential-mixin.test.ts with fixtures covering include/extend/prepend across files. Plan 002 Units 1+2 harden it further: - Worker-vs-sequential parity is now real, not a sham. PipelineOptions gains an @internal workerThresholdsForTest override so the tiny fixture can force the worker pool to spawn. PipelineResult.usedWorkerPool proves which path actually executed. - The prepend-override assertion uses a non-shadowed method (prepended_marker) defined only on PrependedOverride, so asserting owner = [PrependedOverride] proves the prepend provider entered the MRO without depending on the still-deferred kind-aware MRO ordering. Guard verification: - Reverting the Module -> Trait relabel (plan 001 Unit 2) breaks 4 of 7 tests in the regression suite. - Reverting the sequential prepass (plan 001 Unit 1) does NOT currently break the suite because processCalls independently extracts call-based heritage and the resolver has a global-name fallback that bypasses MRO ancestry. Documented as a residual limitation -- a stronger guard needs cross-chunk or name-shadowed scenarios (plan 002 Unit 3 scope). Plans: - docs/plans/2026-04-17-001-fix-codex-adversarial-ruby-mixin-heritage-plan.md - docs/plans/2026-04-17-002-fix-ce-review-ruby-mixin-followups-plan.md (Units 1+2 complete; Units 3-7 still pending) - docs/plans/2026-04-17-003-fix-ruby-mro-ordering-heritage-fallback-plan.md (MRO kind-ordering + Struct fallback, all 6 units pending) Test suite: 151 passed (4 files) for ruby.test.ts, ruby-sequential-mixin.test.ts, resolve-enclosing-owner.test.ts, heritage-extractor-wiring.test.ts. Full gitnexus suite: 6120 passed; 2 pre-existing LadybugDB worker-exit flakes unrelated to this change (java-class-impact, lbug-vector-extension).
…not Struct Ruby `include`/`extend`/`prepend` records share a processHeritageFromExtracted branch with Rust `trait-impl`. When the mixin child name cannot be resolved through the type registry, the fallback synthesized a `Struct:filepath:X` id for both. Rust structs emit under the `Struct` label -- correct. Ruby classes emit under `Class` and Ruby modules emit under `Trait` (since plan 001's module relabel), so the synthesized `Struct:` id would never match any real node and the IMPLEMENTS edge would dangle off a phantom. The resolved case -- the common path now that Ruby modules register normally -- is unaffected; only the fallback for truly unresolved references changes. `Class` is the right default for Ruby mixin kinds because the dominant shape is `class X; include M; end` where X is a Class. Split is kind-aware: Rust `trait-impl` keeps `Struct`, Ruby mixin kinds get `Class`. Applied at both call sites (processHeritageFromExtractedItem + processHeritageFromExtracted).
Adds kind-aware MRO infrastructure for Ruby's prepend/include/extend semantics.
The heritage map now preserves declaration kind and exposes split views for
instance vs singleton dispatch; `lookupMethodByOwnerWithMRO` gets a new
`'ruby-mixin'` MroStrategy that walks prepend providers before the direct
owner lookup, honoring Ruby's rule that prepended modules beat the class's
own method of the same name.
Changes:
- gitnexus-shared: `MroStrategy` union gains `'ruby-mixin'` with doc block
- model/heritage-map: `directParents` internal shape changes from `Set<parentId>`
to ordered `ParentEntry[]` with `{ parentId, kind }`. Public
`getParents`/`getAncestors` contract preserved (still returns flat string[]
via dedup). New methods: `getParentEntries`, `getInstanceAncestry`,
`getSingletonAncestry`. Insertion order mirrors tree-sitter match order,
which for Ruby matches source declaration order.
- model/resolve: new `ruby-mixin` branch walks prepend parents first
(reverse declaration — last-prepended wins), then direct owner, then
extends + include parents (reverse declaration). Non-Ruby strategies
(`first-wins`, `c3`, `leftmost-base`, `implements-split`, `qualified-syntax`)
unchanged — they still do direct-owner-first short-circuit. Walker also
accepts optional `ancestryOverride` for singleton dispatch.
- languages/ruby: sets `mroStrategy: 'ruby-mixin'`
Known limitation (documented in test TODO): Ruby bare-identifier calls
inside methods flow through `resolveFreeCall` today, not owner-scoped
`resolveMemberCall`. That means `ruby-mixin`'s shadow-name behavior
(prepend > self) is not yet observable in the `call_serialize → serialize`
case; it resolves via global lookup. The strategy is correctly wired and
will apply as soon as Ruby self-call inference is added (threading bare
calls as `self.method` with `receiverTypeName = enclosing class`).
The `prepended_marker` test (non-shadowed method only reachable via the
prepend provider) keeps working as the narrow-guard: it proves the prepend
parent enters the ancestry, even though the MRO ordering effect isn't
reachable yet from free-call paths.
No regressions: 840 tests pass across Ruby, Python (C3), Rust (qualified),
Java, Kotlin, heritage-extractor-wiring, resolve-enclosing-owner.
…ng language logic Refactors the call-resolution pipeline inside the parse phase into a typed 6-stage DAG where language-specific behavior lives entirely behind provider hooks. Shared pipeline code names no languages; adding a new language's implicit-receiver or dispatch semantics is a provider-method change, not a shared-code edit. DAG stages: extract-call → classify-form → infer-receiver → select-dispatch → resolve-target → emit-edge Stages 1-2 run in the parse worker; stages 3-6 run main-thread in call-processor. Provider hooks plug in at stages 3 and 4. New provider hooks on LanguageProvider: - inferImplicitReceiver: synthesizes receiver when language makes it implicit (Ruby bare serialize → self.serialize). Returns ImplicitReceiverOverride or null. Runs after the shared receiver-inference chain (TypeEnv → constructor-map → class-as-receiver → mixed-chain) so explicit receivers always take precedence. - selectDispatch: returns a DispatchDecision with primary (owner-scoped / free / constructor), optional fallback (free-arity-narrowed), optional ancestryView (instance / singleton). Defaults come from defaultDispatchDecision when the hook returns null. DAG types in gitnexus/src/core/ingestion/call-types.ts: - ReceiverEnriched: stage-3 output; carries receiverSource discriminant (none / typed-binding / constructor-map / class-as-receiver / mixed-chain / implicit-self). Only variants with live writers are declared. - ImplicitReceiverOverride: hook return shape for implicit-self synthesis. - DispatchDecision: stage-4 output; language-agnostic dispatch contract. resolveCallTarget now dispatches on decision.primary (was: callForm ladder with receiverTypeName gate). Singleton-ancestry miss MUST NOT degrade to resolveMemberCallByFile — the file-scoped fallback is ownerId-keyed and would leak instance methods onto class-constant receivers. Singleton miss null-routes or honors decision.fallback. lookupMethodByOwnerWithMRO accepts ancestryOverride for singleton dispatch and the existing 'ruby-mixin' strategy (prepend → direct → include walk). Ruby implementation (languages/ruby.ts): - inferImplicitReceiver wraps the new pure helper maybeRewriteRubyBareCallToSelf (utils/ruby-self-call.ts) to detect bare calls inside class/module bodies and synthesize self.method with the enclosing class as receiverTypeName. Tags singleton dispatch via the hint field for def self.foo and class << self bodies. - selectDispatch returns fallback=free-arity-narrowed for implicit-self so unresolved self-calls preserve the cross-class arity-narrowing heuristic (Service#run_task → OneArg#write_audit still works). Returns ancestryView=singleton for class-as-receiver calls (Account.log → LoggerMixin#log via extend). - rubyResolveEnclosingOwner extracted to module-level const, shared by provider.resolveEnclosingOwner and inferImplicitReceiver. Shared code changes that needed to accompany the DAG: - class-as-receiver inference filter now accepts Trait (Ruby modules as receivers reach the singleton branch). - Local ReceiverSource union in call-processor replaced with an import from call-types (single source of truth). - defaultDispatchDecision helper and DAG-stage labels make the pipeline grep-discoverable. Observable wins: - Ruby call_serialize → PrependedOverride#serialize: prepend wins over class's own method (kind-aware MRO is now reachable from bare calls). - Usage#run → LoggerMixin#log: class-method dispatch via extend resolves through singleton ancestry. - Service#run_task → OneArg#write_audit: arity-narrowing still works; fallback preserves the existing free-call heuristic for implicit-self. Tests: 1787 integration tests pass across all 12 languages (12 new unit tests for maybeRewriteRubyBareCallToSelf, 1 new integration assertion for Usage#run → LoggerMixin#log, tightened shadow-name assertion for call_serialize → PrependedOverride#serialize). Docs: - ARCHITECTURE.md gains a Call-Resolution DAG section documenting stages, hook signatures, DispatchDecision semantics, the "how to add language behavior" recipe, and non-goals. - AGENTS.md adds a rule enforcing "shared ingestion pipeline code does not name languages — use LanguageProvider hooks." - CLAUDE.md mirrors the pointer; legacy duplicate MCP block trimmed. - JSDoc across 7 touched code files documents DAG stages, hook contracts, invariants (notably the singleton-miss no-fallback guarantee), and canonical vocabulary. Token-optimized via follow-up pass to preserve all load-bearing facts while cutting ~47% of doc tokens.
|
@claude Act as a senior reviewer for GitNexus. Your job is to determine whether this PR is production-ready for this repo, not to give a generic code review. You are reviewing a PR in the GitNexus monorepo:
Your task has 2 phases, in this exact order: PHASE 1 — DEFINE THE BAR PHASE 2 — REVIEW THE PR AGAINST THAT BAR CONTEXT TO LOAD FIRST
Additional context:
PRIMARY OBJECTIVE Do not optimize for completeness at the expense of signal. REVIEW RULES
For each finding, assign one severity:
REPO-SPECIFIC REVIEW CHECKLIST 1. Correctness & functional completenessCheck:
2. Code clarity & clean codeCheck:
3. Test coverage & change safetyEvaluate against TESTING.md:
4. PerformanceInspect for:
5. Operational riskCheck:
6. MaintainabilityCheck:
NON-NEGOTIABLES TO VERIFY
OUTPUT FORMAT Production-ready bar for this PRGive a concise repo-specific definition in 5–10 bullets. FindingsFor each finding, use this structure exactly: [BLOCKING|NON-BLOCKING|NIT] Short title
Only include findings that are real and supported.
What I could not fully verifyList any behavior you could not prove from the diff, tests, or available context. Final verdictChoose exactly one:
If verdict is not ready, list the blocking items in priority order. IMPORTANT REVIEW BEHAVIOR
|
|
Claude finished @magyargergo's task in 5m 21s —— View job PR Review: Heritage Extractor Config+Factory Pattern
|
…solution DAG Addresses four issues flagged in the PR 890 review: 1. CI failure — processCallsFromExtracted non-callable symbol tests Two unit tests in test/unit/call-processor.test.ts failed after the DAG refactor: - 'refuses non-callable symbols even when the name resolves' - 'refuses Class target without callForm=constructor (existing behavior)' Root cause: defaultDispatchDecision mapped callForm === undefined to primary: 'free', which routes through resolveFreeCall's class-target fast path (Swift/Kotlin `User()` without `new`). Pre-DAG, undefined callForm fell through to singleCandidate via the module-alias branch — no class-target fast path. Fix: route callForm === undefined to primary: 'owner-scoped' (same as 'member'). Without receiverTypeName, the owner-scoped branch falls through to resolveModuleAliasedCall + singleCandidate, restoring pre- DAG behavior: bare-identifier calls where the name resolves only to a non-callable symbol (Class / Interface / etc.) null-route cleanly. 2. [NON-BLOCKING] Array.includes() inside BFS loop (O(n²)) model/resolve.ts ruby-mixin strategy step 4 used `prependParents.includes(id)` and `otherParents.includes(id)` inside the BFS loop over getAncestors() — latent O(n²) on deeply-mixed Ruby hierarchies. Fix: build a `walkedDirect` Set once before the loop for O(1) `.has()` lookups. 3. [NON-BLOCKING] getInstanceAncestry / getSingletonAncestry per-call allocation model/heritage-map.ts filtered `directParents` entries on every call, allocating a new array per dispatch site. For Ruby codebases with thousands of bare-call dispatches, this is a per-call-site allocation. Fix: lazy per-owner split cache. First call to getInstanceAncestry / getSingletonAncestry for a given nodeId partitions entries once and memoizes; subsequent calls return the cached arrays. Shared EMPTY_PARENT_ENTRIES sentinel avoids allocating `[]` when a view is empty (common: most classes have no extend providers, so singleton view is always empty). 4. [NON-BLOCKING] No direct unit test for ruby-mixin MRO walk model/resolve.ts ruby-mixin branch was exercised only indirectly via ruby-sequential-mixin integration tests, leaving no regression guard for the prepend-before-direct / include-after-direct / singleton- override / transitive-BFS invariants. Added 7 unit tests in test/unit/symbol-table.test.ts § lookupMethodByOwnerWithMRO: - prepend provider beats class's own method (shadow) - class's own method wins over include provider (shadow) - include provider used when class lacks the method - extend providers excluded from instance-dispatch walk - singleton ancestryOverride routes to extend provider - transitive mixin (Account includes Outer; Outer includes Inner) - stacked prepends — last-prepended wins Surfaced an additional walker bug during the extend-exclusion test: step-4 BFS fallback visited `extend` providers during instance dispatch via getAncestors() (flat parent list, no kind filter). Fix in resolve.ts: also skip `getSingletonAncestry` parents from the step-4 BFS — instance dispatch must never reach extend-provided methods, even transitively. Tests: 330 unit tests pass (was 323, +7 for ruby-mixin walk coverage), 1787 integration tests pass (unchanged).
|
@claude Act as a senior reviewer for GitNexus. Your job is to determine whether this PR is production-ready for this repo, not to give a generic code review. You are reviewing a PR in the GitNexus monorepo:
Your task has 2 phases, in this exact order: PHASE 1 — DEFINE THE BAR PHASE 2 — REVIEW THE PR AGAINST THAT BAR CONTEXT TO LOAD FIRST
Additional context:
PRIMARY OBJECTIVE Do not optimize for completeness at the expense of signal. REVIEW RULES
For each finding, assign one severity:
REPO-SPECIFIC REVIEW CHECKLIST 1. Correctness & functional completenessCheck:
2. Code clarity & clean codeCheck:
3. Test coverage & change safetyEvaluate against TESTING.md:
4. PerformanceInspect for:
5. Operational riskCheck:
6. MaintainabilityCheck:
NON-NEGOTIABLES TO VERIFY
OUTPUT FORMAT Production-ready bar for this PRGive a concise repo-specific definition in 5–10 bullets. FindingsFor each finding, use this structure exactly: [BLOCKING|NON-BLOCKING|NIT] Short title
Only include findings that are real and supported.
What I could not fully verifyList any behavior you could not prove from the diff, tests, or available context. Final verdictChoose exactly one:
If verdict is not ready, list the blocking items in priority order. IMPORTANT REVIEW BEHAVIOR
|
|
Claude finished @magyargergo's task in 12m 23s —— View job PR Review: Heritage Extractor Config+Factory PatternTasks:
Production-ready bar for this PRFor a heritage extractor refactor + Ruby MRO wiring touching the ingestion pipeline in GitNexus:
Findings1. Correctness & functional completenessAll three execution paths are confirmed wired:
Go named field filtering ( Ruby mixin interception: Ruby MRO walk ( Graph schema preserved: [BLOCKING] Two pre-existing tests fail in CI run
|
Bumps gitnexus to v1.6.2 and adds the matching CHANGELOG entry. Highlights since v1.6.1 (61 commits): - Docker support (#848) - Language-agnostic heritage / call / variable extractors (config+factory pattern, #877 #878 #890) - AST-aware embedding chunking (#889) - jQuery / axios HTTP consumer detection (#887) - SemanticModel wired as first-class resolution input, SM-20 (#885) - ImportSemantics split into per-strategy hooks (#886) - Python dotted-import fix (#899); worker warnings non-terminal (#900 / #261); global-install ENOTEMPTY fixes (#843 #846); embeddings staleness fix (#831) See gitnexus/CHANGELOG.md for the full list. After merge, tag `v1.6.2` triggers publish.yml which runs CI, verifies tag↔package.json match, publishes to npm with provenance, and creates the GitHub Release using the extracted CHANGELOG body.
Bumps gitnexus to v1.6.2 and adds the matching CHANGELOG entry. Highlights since v1.6.1 (61 commits): - Docker support (#848) - Language-agnostic heritage / call / variable extractors (config+factory pattern, #877 #878 #890) - AST-aware embedding chunking (#889) - jQuery / axios HTTP consumer detection (#887) - SemanticModel wired as first-class resolution input, SM-20 (#885) - ImportSemantics split into per-strategy hooks (#886) - Python dotted-import fix (#899); worker warnings non-terminal (#900 / #261); global-install ENOTEMPTY fixes (#843 #846); embeddings staleness fix (#831) See gitnexus/CHANGELOG.md for the full list. After merge, tag `v1.6.2` triggers publish.yml which runs CI, verifies tag↔package.json match, publishes to npm with provenance, and creates the GitHub Release using the extracted CHANGELOG body.
Bumps gitnexus to v1.6.2 and adds the matching CHANGELOG entry. Highlights since v1.6.1 (61 commits): - Docker support (abhigyanpatwari#848) - Language-agnostic heritage / call / variable extractors (config+factory pattern, abhigyanpatwari#877 abhigyanpatwari#878 abhigyanpatwari#890) - AST-aware embedding chunking (abhigyanpatwari#889) - jQuery / axios HTTP consumer detection (abhigyanpatwari#887) - SemanticModel wired as first-class resolution input, SM-20 (abhigyanpatwari#885) - ImportSemantics split into per-strategy hooks (abhigyanpatwari#886) - Python dotted-import fix (abhigyanpatwari#899); worker warnings non-terminal (abhigyanpatwari#900 / abhigyanpatwari#261); global-install ENOTEMPTY fixes (abhigyanpatwari#843 abhigyanpatwari#846); embeddings staleness fix (abhigyanpatwari#831) See gitnexus/CHANGELOG.md for the full list. After merge, tag `v1.6.2` triggers publish.yml which runs CI, verifies tag↔package.json match, publishes to npm with provenance, and creates the GitHub Release using the extracted CHANGELOG body.
Heritage extraction (extends/implements/mixins) was handled inline in the parse worker, call-processor, and heritage-processor via ad-hoc
@heritage.*capture map checks with language-specific switches (Go named field skip, Ruby mixin routing). This migrates it to the same config+factory pattern used by method, field, call, and variable extractors, and removes all legacy inline heritage paths.Types & factory
heritage-types.ts—HeritageExtractionConfig,HeritageExtractor,HeritageInfointerfacesheritage-extractors/generic.ts—createHeritageExtractor(config)factory converts declarative config to runtime extractor. Accepts either a fullHeritageExtractionConfig(for languages with custom hooks) or a bareSupportedLanguagesenum value for default capture-based extraction — no per-language config file needed for the default case.Per-language configs (2 files in
heritage-extractors/configs/)Only languages with actual custom extraction logic have dedicated config files:
go.ts) —shouldSkipExtendsfilters named struct fields from embedding capturesruby.ts) —callBasedHeritageabsorbsinclude/extend/prependmixin routing previously inrouteRubyCallAll other languages use the default extraction path by passing the
SupportedLanguagesenum directly to the factory.Provider wiring
heritageExtractoradded toLanguageProviderConfig. All 16 providers wired:Parse worker, call-processor & heritage-processor
All inline heritage capture logic replaced with
provider.heritageExtractor?.extract():buildTypeEnvnow usesheritageExtractor.extract(). Call-based heritage checked viaextractFromCallbefore the call router. Heritage case removed from callRouter dispatch.extractFromCallcheck added before callRouter (was previously missing). Heritage case removed from callRouter switch.processHeritageinline captureMap checks (extends/implements/trait-impl with Go named field skip) replaced withheritageExtractor.extract()+ newresolveAndAddHeritageEdgehelper that bridgesHeritageInfoto graph edge resolution.extractExtractedHeritageFromFilesupdated identically.Legacy path removal
routeRubyCall— Heritage routing removed (include/extend/prepend →{kind:'heritage'}).RubyHeritageIteminterface and heritage variant removed fromRubyCallRoutingunion. Router now returnsskipfor these calls sinceheritageExtractor.extractFromCallintercepts them first.processHeritage— ~100 lines of inline captureMap checks replaced with extractor delegation.field_declarationskip removed, now handled bygoHeritageConfig.shouldSkipExtends.What did NOT change
heritage-processor.tscross-file resolution logic (resolveHeritageId, resolveExtendsType)model/heritage-map.ts/ MRO strategiesExtractedHeritageinterfaceTests
27+ unit tests covering factory construction (both enum and full config paths), Go named field filtering, Ruby call-based heritage extraction (include/extend/prepend with constants, scope resolution, modules, multiple args, missing enclosing class). Default language extractors verified to have no custom hooks. Provider wiring test ensures all 15 tree-sitter providers have
heritageExtractordefined. Integration tests exercise RubycallBasedHeritageand TypeScript heritage against real tree-sitter ASTs. Call-routing tests updated for new skip behavior.