fix(ingestion): close Ruby mixin heritage gaps on sequential path + relabel 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).
This commit is contained in:
Gergo Magyar 2026-04-17 11:12:49 +01:00
parent 2cea3eba76
commit fbac43815b
15 changed files with 380 additions and 12 deletions

View file

@ -369,6 +369,15 @@ export const processHeritageFromExtracted = async (
* {@link ExtractedHeritage} rows without mutating the graph. Used on the
* sequential pipeline path so `buildHeritageMap(..., ctx)` can run before
* `processCalls` (worker path defers calls until heritage from all chunks exists).
*
* This prepass extracts BOTH capture-based heritage (`@heritage.*` extends /
* implements / trait-impl) AND call-based heritage (`@call.name` routed through
* `heritageExtractor.extractFromCall` Ruby `include` / `extend` / `prepend`).
* Without the second pass, sequential-mode `sequentialHeritageMap` would not
* know about Ruby mixin ancestry before `processCalls` resolves calls against
* it, silently dropping mixed-in methods from the graph. This function stays
* read-only `processCalls` still owns emission of heritage graph edges via
* its `rubyHeritage` return path.
*/
export async function extractExtractedHeritageFromFiles(
files: { path: string; content: string }[],
@ -408,6 +417,8 @@ export async function extractExtractedHeritageFromFiles(
continue;
}
const callBasedEnabled = !!provider.heritageExtractor?.extractFromCall;
for (const match of matches) {
const captureMap: Record<string, any> = {};
match.captures.forEach((c) => {
@ -429,6 +440,31 @@ export async function extractExtractedHeritageFromFiles(
});
}
}
continue;
}
// Call-based heritage (e.g. Ruby include/extend/prepend). Matches the
// routing the worker path performs inline in parse-worker.ts — see the
// `provider.heritageExtractor?.extractFromCall` branch there. We only
// need call-based records here; other @call captures are consumed by
// processCalls later in the sequential loop.
if (callBasedEnabled && captureMap['call'] && captureMap['call.name']) {
const calledName: string = captureMap['call.name'].text;
const heritageItems = provider.heritageExtractor!.extractFromCall!(
calledName,
captureMap['call'],
{ filePath: file.path, language },
);
if (heritageItems) {
for (const item of heritageItems) {
out.push({
filePath: file.path,
className: item.className,
parentName: item.parentName,
kind: item.kind,
});
}
}
}
}
}

View file

@ -30,6 +30,23 @@ import { rubyCallConfig } from '../call-extractors/configs/ruby.js';
import { createHeritageExtractor } from '../heritage-extractors/generic.js';
import { rubyHeritageConfig } from '../heritage-extractors/configs/ruby.js';
/**
* Ruby label override. Applied to:
* - `definition.module` captures in the structure phase remaps to `Trait`
* so Ruby modules are registered in the class-like type registry and are
* therefore resolvable by `lookupClassByName` during mixin heritage
* resolution (`include`/`extend`/`prepend`).
* - `definition.function` captures Ruby has no bare "function" construct
* (top-level `def` is a method on `main`); return the default so generic
* logic continues to apply.
*
* Returning `null` means "skip this definition"; we never do that here.
*/
const rubyLabelOverride = (_node: SyntaxNode, defaultLabel: NodeLabel): NodeLabel | null => {
if (defaultLabel === 'Module') return 'Trait';
return defaultLabel;
};
/** Ruby method/singleton_method: extract name from 'name' field, label as Method. */
const rubyExtractFunctionName = (
node: SyntaxNode,
@ -140,5 +157,6 @@ export const rubyProvider = defineLanguage({
variableExtractor: createVariableExtractor(rubyVariableConfig),
classExtractor: createClassExtractor(rubyClassConfig),
heritageExtractor: createHeritageExtractor(rubyHeritageConfig),
labelOverride: rubyLabelOverride,
builtInNames: BUILT_INS,
});

View file

@ -108,6 +108,7 @@ export async function runChunkedParseAndResolve(
allORMQueries: ExtractedORMQuery[];
bindingAccumulator: BindingAccumulator;
resolutionContext: ReturnType<typeof createResolutionContext>;
usedWorkerPool: boolean;
}> {
const ctx = createResolutionContext();
const symbolTable = ctx.model.symbols;
@ -173,9 +174,11 @@ export async function runChunkedParseAndResolve(
stats: { filesProcessed: 0, totalFiles: totalParseable, nodesCreated: graph.nodeCount },
});
// Don't spawn workers for tiny repos — overhead exceeds benefit
const MIN_FILES_FOR_WORKERS = 15;
const MIN_BYTES_FOR_WORKERS = 512 * 1024;
// Don't spawn workers for tiny repos — overhead exceeds benefit.
// Test suites may lower the thresholds via `options.workerThresholdsForTest`
// to exercise the worker-pool path with small fixtures; see PipelineOptions.
const MIN_FILES_FOR_WORKERS = options?.workerThresholdsForTest?.minFiles ?? 15;
const MIN_BYTES_FOR_WORKERS = options?.workerThresholdsForTest?.minBytes ?? 512 * 1024;
const totalBytes = parseableScanned.reduce((s, f) => s + f.size, 0);
// Create worker pool once, reuse across chunks
@ -588,5 +591,9 @@ export async function runChunkedParseAndResolve(
allORMQueries,
bindingAccumulator,
resolutionContext: ctx,
// Whether a worker pool was actually live for this run. False means the
// sequential fallback handled every chunk (either due to `skipWorkers`,
// the file-count/byte thresholds, or a pool-creation failure).
usedWorkerPool: workerPool !== undefined,
};
}

View file

@ -56,6 +56,13 @@ export interface ParseOutput {
readonly allPathSet: ReadonlySet<string>;
/** Pass-through: total file count for progress reporting. */
totalFiles: number;
/**
* True if the parse phase spawned a live worker pool for this run.
* False means every chunk ran through the sequential fallback (skipWorkers,
* thresholds not met, or pool-creation failure). Primarily a test affordance:
* see `PipelineOptions.workerThresholdsForTest`.
*/
readonly usedWorkerPool: boolean;
}
export const parsePhase: PipelinePhase<ParseOutput> = {

View file

@ -43,6 +43,17 @@ export interface PipelineOptions {
skipGraphPhases?: boolean;
/** Force sequential parsing (no worker pool). Useful for testing the sequential path. */
skipWorkers?: boolean;
/**
* @internal Test-only override for worker-pool gating thresholds.
* When unset, production defaults apply (15 files OR 512 KB total bytes).
* Setting either field lowers the corresponding threshold so small test
* fixtures can still exercise the worker-pool path. Do not use from
* production call sites.
*/
workerThresholdsForTest?: {
minFiles?: number;
minBytes?: number;
};
}
// ── Phase registry ─────────────────────────────────────────────────────────
@ -99,7 +110,10 @@ export const runPipelineFromRepo = async (
});
// Extract final results for the PipelineResult contract
const { totalFiles } = getPhaseOutput<{ totalFiles: number }>(results, 'parse');
const { totalFiles, usedWorkerPool } = getPhaseOutput<{
totalFiles: number;
usedWorkerPool: boolean;
}>(results, 'parse');
let communityResult: CommunitiesOutput['communityResult'] | undefined;
let processResult: ProcessesOutput['processResult'] | undefined;
@ -123,5 +137,12 @@ export const runPipelineFromRepo = async (
},
});
return { graph, repoPath, totalFileCount: totalFiles, communityResult, processResult };
return {
graph,
repoPath,
totalFileCount: totalFiles,
communityResult,
processResult,
usedWorkerPool,
};
};

View file

@ -153,7 +153,14 @@ export const CONTAINER_TYPE_TO_LABEL: Record<string, string> = {
mixin_declaration: 'Mixin',
extension_declaration: 'Extension',
class: 'Class',
module: 'Module',
// Ruby `module` declarations map to `Trait` so they participate in the
// class-like type registry used by `lookupClassByName` / `buildHeritageMap`.
// This lets `include` / `extend` / `prepend` mixin heritage resolve to
// the providing module. Safe for non-Ruby languages: the only supported
// grammar that uses the bare `module` AST node type as a container is
// Ruby (Rust uses `mod_item`). Any new language adding a `module` node
// type must explicitly reclassify here.
module: 'Trait',
singleton_class: 'Class', // Ruby: class << self inherits enclosing class name
object_declaration: 'Class',
companion_object: 'Class',
@ -185,7 +192,17 @@ export function getLabelFromCaptures(
if (captureMap['definition.struct']) return 'Struct';
if (captureMap['definition.enum']) return 'Enum';
if (captureMap['definition.namespace']) return 'Namespace';
if (captureMap['definition.module']) return 'Module';
if (captureMap['definition.module']) {
// Let providers reclassify module captures (e.g. Ruby remaps `Module`→`Trait`
// so mixin heritage resolves through `lookupClassByName`). Returning null
// from labelOverride means "skip this symbol"; treat it as a no-op here so
// we keep the default label rather than dropping a real definition.
if (provider.labelOverride) {
const override = provider.labelOverride(captureMap['definition.module'], 'Module');
if (override && override !== 'Module') return override;
}
return 'Module';
}
if (captureMap['definition.trait']) return 'Trait';
if (captureMap['definition.impl']) return 'Impl';
if (captureMap['definition.type']) return 'TypeAlias';

View file

@ -11,4 +11,10 @@ export interface PipelineResult {
totalFileCount: number;
communityResult?: CommunityDetectionResult;
processResult?: ProcessDetectionResult;
/**
* True if the parse phase spawned a worker pool for this run. False means
* the sequential fallback handled every chunk. Primarily a test affordance
* so regression suites can prove which path executed.
*/
usedWorkerPool: boolean;
}

View file

@ -0,0 +1,25 @@
require_relative 'greetable'
require_relative 'logger_mixin'
require_relative 'prepended_override'
class Account
include Greetable
extend LoggerMixin
prepend PrependedOverride
def serialize
"account"
end
def call_greet
greet
end
def call_serialize
serialize
end
def call_prepended_marker
prepended_marker
end
end

View file

@ -0,0 +1,5 @@
module Greetable
def greet
"hello from greetable"
end
end

View file

@ -0,0 +1,5 @@
module LoggerMixin
def log(msg)
puts msg
end
end

View file

@ -0,0 +1,13 @@
module PrependedOverride
def serialize
"prepended"
end
# Unique method name not defined elsewhere in the fixture. Calling this
# from Account proves the prepend heritage edge adds PrependedOverride to
# the MRO at all. Shadowed-name resolution (prepend > self) is deferred —
# see plan 001's "Deferred to Separate Tasks: Ruby MRO kind-ordering".
def prepended_marker
"prepended-only"
end
end

View file

@ -0,0 +1,10 @@
require_relative 'account'
class Usage
def run
a = Account.new
a.call_greet
a.call_serialize
Account.log("from Usage")
end
end

View file

@ -0,0 +1,190 @@
/**
* Regression: Ruby mixin heritage resolution must work on the sequential
* ingestion fallback AND the worker-pool path, with identical output.
*
* Guards the two Codex adversarial review findings addressed by plan
* `docs/plans/2026-04-17-001-fix-codex-adversarial-ruby-mixin-heritage-plan.md`:
*
* 1. Sequential-mode `sequentialHeritageMap` must include Ruby `include` /
* `extend` / `prepend` mixin ancestry before `processCalls` resolves calls
* against it. `extractExtractedHeritageFromFiles` now also runs
* `heritageExtractor.extractFromCall` during its prepass.
*
* 2. Ruby `module` declarations are relabeled to `Trait` so they participate
* in `lookupClassByName` / `buildHeritageMap`.
*
* The follow-up plan `docs/plans/2026-04-17-002-fix-ce-review-ruby-mixin-followups-plan.md`
* Units 1 and 2 harden this suite:
* - Worker mode actually spawns a worker pool (verified via
* `PipelineResult.usedWorkerPool`) instead of silently falling back.
* - The prepend-only `prepended_marker` assertion checks the resolved
* method's OWNER, so reverting the ModuleTrait relabel (Unit 2 of
* plan 001) makes the test fail with a clear owner-mismatch instead
* of passing trivially on `Account`'s own method.
*
* Known guard limitation (documented residual): reverting plan 001 Unit 1
* alone (the sequential prepass extractFromCall) does NOT make these tests
* fail, because `processCalls` independently extracts call-based heritage
* into `rubyHeritage`, feeds it to `processHeritageFromExtracted` for graph
* edges, and the call resolver's global-name fallback can still locate
* mixin-provided methods without MRO ancestry. A stronger guard would need
* an ambiguous method name that only MRO can disambiguate; that requires
* cross-chunk or multi-class shadowing scenarios not covered by this
* fixture. Tracked as residual work in plan 002's Unit 3 (cross-chunk).
*/
import { describe, it, expect, beforeAll } from 'vitest';
import path from 'path';
import type { GraphRelationship } from '../../../src/core/graph/types.js';
import {
FIXTURES,
getRelationships,
getNodesByLabel,
runPipelineFromRepo,
type PipelineOptions,
type PipelineResult,
} from './helpers.js';
const FIXTURE = path.join(FIXTURES, 'ruby-sequential-mixin');
async function runMode(opts: PipelineOptions): Promise<PipelineResult> {
return runPipelineFromRepo(FIXTURE, () => {}, opts);
}
/** CALLS edges from `sourceName` whose target is a Method node. */
function methodCallEdges(result: PipelineResult, sourceName: string): Set<string> {
const edges = getRelationships(result, 'CALLS').filter(
(e) => e.source === sourceName && e.targetLabel === 'Method',
);
return new Set(edges.map((e) => `${e.source}${e.target}`));
}
/**
* Find the name of the node that `HAS_METHOD`s this target node, if any.
* Returns `undefined` when no owner edge exists (e.g., top-level function).
*/
function findMethodOwner(result: PipelineResult, methodNodeId: string): string | undefined {
for (const rel of result.graph.iterRelationships() as IterableIterator<GraphRelationship>) {
if (rel.type === 'HAS_METHOD' && rel.targetId === methodNodeId) {
return result.graph.getNode(rel.sourceId)?.properties.name;
}
}
return undefined;
}
/**
* Return the owner names of every `Method` target reached by a CALLS edge
* starting at `sourceName` whose target's name matches `targetMethodName`.
* Used to assert WHICH provider resolved a shadowed method name like
* `serialize` (provided by both Account and PrependedOverride).
*/
function resolvedMethodOwners(
result: PipelineResult,
sourceName: string,
targetMethodName: string,
): string[] {
const owners: string[] = [];
for (const e of getRelationships(result, 'CALLS')) {
if (e.source === sourceName && e.targetLabel === 'Method' && e.target === targetMethodName) {
const owner = findMethodOwner(result, e.rel.targetId);
if (owner) owners.push(owner);
}
}
return owners.sort();
}
describe('Ruby mixin heritage: sequential vs worker parity', () => {
let sequential: PipelineResult;
let workers: PipelineResult;
beforeAll(async () => {
sequential = await runMode({ skipWorkers: true });
// Force the worker pool to spawn even though the fixture is tiny.
// Without this override, the pipeline's MIN_FILES_FOR_WORKERS / MIN_BYTES_FOR_WORKERS
// gate would fall back to sequential and the "worker vs sequential" parity
// assertion below would degenerate into sequential-vs-sequential.
workers = await runMode({
skipWorkers: false,
workerThresholdsForTest: { minFiles: 1, minBytes: 0 },
});
}, 120000);
it('exercises both pipeline paths (sequential and worker)', () => {
// If either of these assertions fails, every downstream parity check
// below is meaningless — both modes would be running the same path.
expect(sequential.usedWorkerPool).toBe(false);
expect(workers.usedWorkerPool).toBe(true);
});
it('labels Ruby modules as Trait in both modes', () => {
const expected = ['Greetable', 'LoggerMixin', 'PrependedOverride'];
expect(getNodesByLabel(sequential, 'Trait').sort()).toEqual(expected);
expect(getNodesByLabel(workers, 'Trait').sort()).toEqual(expected);
// No Ruby modules leak through as the inert `Module` label.
// The 'lib' module node is the fixture's top-level directory node, which
// the ingestion pipeline emits for every fixture root — unrelated to
// Ruby `module` declarations. Filtering it keeps the assertion specific
// to Ruby-module relabeling without being coupled to how directory nodes
// are emitted.
expect(getNodesByLabel(sequential, 'Module').filter((n) => n !== 'lib')).toEqual([]);
expect(getNodesByLabel(workers, 'Module').filter((n) => n !== 'lib')).toEqual([]);
});
it('sequential mode resolves include-provided method: call_greet → greet', () => {
const edges = methodCallEdges(sequential, 'call_greet');
expect([...edges]).toContain('call_greet → greet');
// Stronger: the resolved `greet` must be owned by the `Greetable` module
// (relabeled to Trait). A regression in Unit 2 of plan 001 would either
// fail to resolve (owners = []) or resolve to some other owner.
const owners = resolvedMethodOwners(sequential, 'call_greet', 'greet');
expect(owners).toContain('Greetable');
});
it('sequential mode resolves prepend-only method: call_prepended_marker → PrependedOverride#prepended_marker', () => {
// `prepended_marker` is defined ONLY on PrependedOverride — not on
// Account, Greetable, or LoggerMixin. A resolver that fails to enter
// the prepend provider into the MRO (i.e., a regression in plan 001
// Unit 1's sequential prepass OR Unit 2's module relabel) would not
// find this method at all, and the owner list would be empty.
//
// We deliberately do NOT assert `call_serialize → PrependedOverride#serialize`
// here because `Account` also defines `serialize`; kind-aware MRO ordering
// (prepend wins over the class's own method) is a separate concern deferred
// by plan 001.
const owners = resolvedMethodOwners(sequential, 'call_prepended_marker', 'prepended_marker');
expect(owners).toContain('PrependedOverride');
});
it('sequential mode emits IMPLEMENTS edges for all three mixin kinds', () => {
// Ruby mixins (include / extend / prepend) flow through the IMPLEMENTS
// branch of processHeritageFromExtracted with the mixin kind recorded in
// rel.reason. See heritage-processor.ts L146-168.
const kinds = getRelationships(sequential, 'IMPLEMENTS')
.filter((e) => e.source === 'Account')
.map((e) => e.rel.reason ?? '')
.sort();
expect(kinds).toEqual(['extend', 'include', 'prepend']);
});
it('worker mode resolves the same include and prepend-only targets', () => {
// Cross-mode ownership parity for the mixin providers. If Unit 1 of
// plan 001 regressed on the sequential side only, the `greet` /
// `prepended_marker` owners would diverge between modes here — the
// sequential side would lose the mixin-provided edges while worker
// mode kept them (or vice versa).
expect(resolvedMethodOwners(workers, 'call_greet', 'greet')).toContain('Greetable');
expect(resolvedMethodOwners(workers, 'call_prepended_marker', 'prepended_marker')).toContain(
'PrependedOverride',
);
});
it('sequential and worker modes produce the same mixin-method CALLS edges', () => {
const seqEdges = methodCallEdges(sequential, 'call_greet');
const workerEdges = methodCallEdges(workers, 'call_greet');
expect([...seqEdges].sort()).toEqual([...workerEdges].sort());
const seqMarker = methodCallEdges(sequential, 'call_prepended_marker');
const workerMarker = methodCallEdges(workers, 'call_prepended_marker');
expect([...seqMarker].sort()).toEqual([...workerMarker].sort());
});
});

View file

@ -33,8 +33,12 @@ describe('Ruby require_relative, heritage & property resolution', () => {
expect(getNodesByLabel(result, 'Class')).toEqual(['BaseModel', 'User', 'UserService']);
});
it('detects 3 modules', () => {
expect(getNodesByLabel(result, 'Module')).toEqual(['Cacheable', 'Loggable', 'Serializable']);
it('detects 3 modules (labeled as Trait for class-like registry lookup)', () => {
// Ruby `module` declarations are relabeled to `Trait` during ingestion so
// they participate in `lookupClassByName` and `buildHeritageMap`. This is
// the single source of truth for Ruby module detection in the graph.
expect(getNodesByLabel(result, 'Trait')).toEqual(['Cacheable', 'Loggable', 'Serializable']);
expect(getNodesByLabel(result, 'Module')).toEqual([]);
});
it('detects methods on classes and modules', () => {
@ -431,9 +435,10 @@ describe('Ruby parent resolution', () => {
result = await runPipelineFromRepo(path.join(FIXTURES, 'ruby-parent-resolution'), () => {});
}, 60000);
it('detects BaseModel and User classes plus Serializable module', () => {
it('detects BaseModel and User classes plus Serializable module (Trait)', () => {
expect(getNodesByLabel(result, 'Class')).toEqual(['BaseModel', 'User']);
expect(getNodesByLabel(result, 'Module')).toEqual(['Serializable']);
// Ruby modules are labeled Trait — see the "detects 3 modules" test above.
expect(getNodesByLabel(result, 'Trait')).toEqual(['Serializable']);
});
it('emits EXTENDS edge: User < BaseModel', () => {

View file

@ -87,7 +87,10 @@ end
expect(info).not.toBeNull();
expect(info!.className).toBe('Helpers');
expect(info!.classId).toContain('Module');
// Ruby modules are labeled `Trait` so mixin heritage resolves through
// the class-like type registry; the enclosing class id switches labels
// in lockstep with the structure-phase label.
expect(info!.classId).toContain('Trait');
});
it('returns null for file-level singleton_class without enclosing class', () => {