mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-04 02:31:36 +00:00
review: address Claude review findings on PR #1520
- Findings 1-3 (BLOCKERS): restore include-extractor.ts and its test to the main baseline. Block-comment fallback regression, suffix-resolve false-positive suppression, and the four deleted regression tests (#3-#6) are now back. These changes were unrelated to C++ scope parity and should not have been in this PR. - Finding 4 (MAJOR, partial): revert COMPOUND_RECEIVER_MAX_DEPTH 6 to 4. No C++ test exercises depth > 4 (cpp-chain-call uses a 2-hop chain), so the bump risked silent regressions on other migrated languages without justification. The wildcard-origin propagation in imported-return-types.ts is retained — C++ #include and using namespace both emit wildcard-origin bindings (cpp/import-decomposer .ts:40,90), so wildcard propagation is causal to C++ parity. - Finding 6: tighten write-access dedup test with exact per-field counts (nameWrites = 2, addrWrites = 1) instead of total-count + sub string containment, so a regression in one of the two name writes can no longer be masked. - Finding 8: skipped. Box-drawing characters in cpp/query.ts comments match the established convention used in csharp/java/php query files. Finding 5 (int/long normalization tie-breaker) left as documented follow-up — proper fix requires resolver-level tie-breaker logic and risks regressing other arity-matching tests.
This commit is contained in:
Gergo Magyar
parent
9c1f6d731a
commit
c58187fe09
4 changed files with 587 additions and 64 deletions
|
|
@ -1,4 +1,5 @@
|
|||
import * as path from 'node:path';
|
||||
import * as fs from 'node:fs/promises';
|
||||
import { glob } from 'glob';
|
||||
import Parser from 'tree-sitter';
|
||||
import C from 'tree-sitter-c';
|
||||
|
|
@ -6,12 +7,11 @@ import Cpp from 'tree-sitter-cpp';
|
|||
import type { ContractExtractor, CypherExecutor } from '../contract-extractor.js';
|
||||
import type { ExtractedContract, RepoHandle } from '../types.js';
|
||||
import { readSafe } from './fs-utils.js';
|
||||
import {
|
||||
buildSuffixIndex,
|
||||
suffixResolve,
|
||||
type SuffixIndex,
|
||||
} from '../../ingestion/import-resolvers/utils.js';
|
||||
import { buildSuffixIndex, type SuffixIndex } from '../../ingestion/import-resolvers/utils.js';
|
||||
import { createIgnoreFilter } from '../../../config/ignore-service.js';
|
||||
import { getMaxFileSizeBytes } from '../../ingestion/utils/max-file-size.js';
|
||||
import { parseSourceSafe } from '../../tree-sitter/safe-parse.js';
|
||||
import { logger } from '../../logger.js';
|
||||
|
||||
/**
|
||||
* Cross-repo C/C++ `#include` dependency extractor.
|
||||
|
|
@ -33,20 +33,11 @@ import { parseSourceSafe } from '../../tree-sitter/safe-parse.js';
|
|||
|
||||
const HEADER_EXTENSIONS = new Set(['.h', '.hpp', '.hxx', '.hh']);
|
||||
|
||||
const _HEADER_GLOB = '**/*.{h,hpp,hxx,hh}';
|
||||
const SOURCE_GLOB = '**/*.{c,cpp,cc,cxx,h,hpp,hxx,hh}';
|
||||
|
||||
const STANDARD_IGNORES = [
|
||||
'**/node_modules/**',
|
||||
'**/.git/**',
|
||||
'**/vendor/**',
|
||||
'**/dist/**',
|
||||
'**/build/**',
|
||||
'**/.gitnexus/**',
|
||||
'**/third_party/**',
|
||||
'**/3rdparty/**',
|
||||
'**/external/**',
|
||||
];
|
||||
// Source = headers (provider-eligible) ∪ implementation files (.c/.cpp/.cc/.cxx).
|
||||
// Spread keeps the subset relationship explicit so a future contributor adding
|
||||
// a new header extension to HEADER_EXTENSIONS does not have to remember to
|
||||
// also add it here.
|
||||
const SOURCE_EXTENSIONS = new Set<string>([...HEADER_EXTENSIONS, '.c', '.cpp', '.cc', '.cxx']);
|
||||
|
||||
const INCLUDE_QUERY_SRC = '(preproc_include path: (_) @import.source) @import';
|
||||
|
||||
|
|
@ -213,10 +204,34 @@ const INCLUDE_REGEX = /^[ \t]*#\s*include\s*"([^"]+)"/gm;
|
|||
|
||||
// ---------- helpers ----------
|
||||
|
||||
/**
|
||||
* Normalize an include path to a canonical lowercase forward-slash form.
|
||||
*
|
||||
* IMPORTANT — case-folding caveat (PR #1156 review finding #3):
|
||||
* Header paths are lowercased so consumer `#include "Foo/Bar.h"` and
|
||||
* provider file `Foo/Bar.h` normalize to the same contract-id. This is
|
||||
* the right trade-off on case-insensitive filesystems (macOS, Windows)
|
||||
* but on case-sensitive Linux filesystems two distinct headers `Foo.h`
|
||||
* and `foo.h` in the same repo will collide onto the same provider
|
||||
* contract-id; only one survives `dedupe()`. The gain (reliable
|
||||
* cross-platform matching) outweighs the cost (extremely rare header
|
||||
* casing collisions inside a single repo).
|
||||
*/
|
||||
function normalizeIncludePath(raw: string): string {
|
||||
return raw.replace(/\\/g, '/').replace(/^\.\//, '').replace(/\/+/g, '/').toLowerCase();
|
||||
}
|
||||
|
||||
/**
|
||||
* Strip C/C++ block comments from a source blob. Used only by the
|
||||
* regex-fallback path to avoid emitting consumer contracts for
|
||||
* commented-out #include directives. Line comments (`// …`) cannot hide
|
||||
* #include directives because the regex anchors on start-of-line.
|
||||
* See PR #1156 review finding #5.
|
||||
*/
|
||||
function stripBlockComments(src: string): string {
|
||||
return src.replace(/\/\*[\s\S]*?\*\//g, '');
|
||||
}
|
||||
|
||||
function isAngleBracketInclude(rawNodeText: string): boolean {
|
||||
const trimmed = rawNodeText.trim();
|
||||
return trimmed.startsWith('<') && trimmed.endsWith('>');
|
||||
|
|
@ -252,11 +267,41 @@ function getLanguageForFile(filePath: string): unknown | null {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Check whether an include path resolves to a file inside the local repo.
|
||||
*
|
||||
* Uses *exact full-path* matching on the suffix index — we never accept a
|
||||
* truncated suffix match. For `#include "foo/bar.h"` this checks:
|
||||
* (a) a file whose path ends with the full `foo/bar.h`
|
||||
* (b) if the include omitted the extension, a file whose path ends with
|
||||
* the include + one of the C/C++ header extensions
|
||||
*
|
||||
* Returns `true` when a local file matches — caller should suppress the
|
||||
* cross-repo consumer contract.
|
||||
*
|
||||
* See PR #1156 review finding #4 (suffixResolve ambiguity).
|
||||
*/
|
||||
function isLocalInclude(cleaned: string, suffixIndex: SuffixIndex): boolean {
|
||||
const candidates = [cleaned];
|
||||
if (!/\.[a-zA-Z0-9]+$/.test(cleaned)) {
|
||||
for (const ext of ['.h', '.hpp', '.hxx', '.hh']) candidates.push(cleaned + ext);
|
||||
}
|
||||
for (const c of candidates) {
|
||||
if (suffixIndex.get(c) || suffixIndex.getInsensitive(c)) return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
// ---------- main class ----------
|
||||
|
||||
export class IncludeExtractor implements ContractExtractor {
|
||||
type = 'include' as const;
|
||||
|
||||
/**
|
||||
* Always returns `true`. NOT called by `sync.ts`, which gates extraction via
|
||||
* `config.detect.includes` instead (see `sync.ts:174`). Kept solely to satisfy
|
||||
* the `ContractExtractor` interface so the type stays uniform across extractors.
|
||||
*/
|
||||
async canExtract(_repo: RepoHandle): Promise<boolean> {
|
||||
return true;
|
||||
}
|
||||
|
|
@ -266,24 +311,86 @@ export class IncludeExtractor implements ContractExtractor {
|
|||
repoPath: string,
|
||||
_repo: RepoHandle,
|
||||
): Promise<ExtractedContract[]> {
|
||||
// 1. Build the local file list (for suffix resolution)
|
||||
const allFiles = await glob('**/*', {
|
||||
cwd: repoPath,
|
||||
ignore: STANDARD_IGNORES,
|
||||
nodir: true,
|
||||
});
|
||||
// 1. Build the local file list using the same discovery as ingestion
|
||||
// (createIgnoreFilter + getMaxFileSizeBytes). This guarantees the
|
||||
// universe of provider/consumer paths matches the universe of File
|
||||
// nodes in the LadybugDB graph — so no cross-link points at a UID
|
||||
// that group impact cannot fan out to.
|
||||
// (PR #1156 Codex follow-up: discovery aligned with ingestion.)
|
||||
const allFiles = await this.discoverIndexableFiles(repoPath);
|
||||
const normalizedFiles = allFiles.map((f) => f.replace(/\\/g, '/'));
|
||||
const suffixIndex = buildSuffixIndex(normalizedFiles, allFiles);
|
||||
|
||||
// 2. Provider: register all header files
|
||||
const providers = await this.extractProviders(dbExecutor, repoPath, allFiles);
|
||||
|
||||
// 3. Consumer: find unresolved #include directives
|
||||
const consumers = await this.extractConsumers(repoPath, normalizedFiles, allFiles, suffixIndex);
|
||||
// 3. Consumer: filter the shared discovery list for source extensions
|
||||
// and parse #include directives in those files.
|
||||
const sourceFiles = allFiles.filter((f) =>
|
||||
SOURCE_EXTENSIONS.has(path.extname(f).toLowerCase()),
|
||||
);
|
||||
const consumers = await this.extractConsumers(repoPath, sourceFiles, suffixIndex);
|
||||
|
||||
return this.dedupe([...providers, ...consumers]);
|
||||
}
|
||||
|
||||
/**
|
||||
* Discover repo-relative file paths using exactly the same rules the
|
||||
* ingestion pipeline uses (`walkRepositoryPaths` in
|
||||
* `gitnexus/src/core/ingestion/filesystem-walker.ts`):
|
||||
* - `createIgnoreFilter` honors `.gitignore`, `.gitnexusignore`, the
|
||||
* hardcoded ignore list, and `.gitnexusignore` last-match-wins
|
||||
* negation.
|
||||
* - `getMaxFileSizeBytes()` drops files larger than the cap so we
|
||||
* never emit `File:<rel>` UIDs for files ingestion would skip.
|
||||
*
|
||||
* Uses sequential stat — there is no `READ_CONCURRENCY` batching here
|
||||
* because group sync runs at startup-time, not the ingestion hot path,
|
||||
* and parallelism gains are not worth the import-graph weight.
|
||||
*
|
||||
* MAINTENANCE: if `walkRepositoryPaths` changes its glob options, ignore
|
||||
* filter shape, or size-cap logic, mirror those changes here. The two
|
||||
* implementations exist because the consumers need different return
|
||||
* shapes (string[] vs ScannedFile[]) and different concurrency, but
|
||||
* they MUST agree on which files are reachable — that is what makes
|
||||
* `File:<rel>` UIDs in cross-links correspond to graph File nodes.
|
||||
*/
|
||||
private async discoverIndexableFiles(repoPath: string): Promise<string[]> {
|
||||
const ignoreFilter = await createIgnoreFilter(repoPath);
|
||||
const maxFileSizeBytes = getMaxFileSizeBytes();
|
||||
|
||||
const candidates = await glob('**/*', {
|
||||
cwd: repoPath,
|
||||
nodir: true,
|
||||
dot: false,
|
||||
ignore: ignoreFilter,
|
||||
});
|
||||
|
||||
const survivors: string[] = [];
|
||||
for (const rel of candidates) {
|
||||
try {
|
||||
const stat = await fs.stat(path.join(repoPath, rel));
|
||||
if (stat.size > maxFileSizeBytes) continue;
|
||||
survivors.push(rel);
|
||||
} catch (err) {
|
||||
// ENOENT is the documented benign race (glob enumerated a file
|
||||
// that was deleted before we stat'd it — same race
|
||||
// walkRepositoryPaths absorbs via Promise.allSettled). Anything
|
||||
// else (EACCES, EMFILE, EIO) deserves a warning so an operator
|
||||
// can spot a permission/resource problem instead of silently
|
||||
// shipping fewer contracts than expected.
|
||||
const code = (err as NodeJS.ErrnoException | undefined)?.code;
|
||||
if (code !== 'ENOENT') {
|
||||
logger.warn(
|
||||
{ err: (err as Error).message, file: rel, repoPath },
|
||||
'⚠️ IncludeExtractor: stat failed during discovery; skipping file',
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
return survivors;
|
||||
}
|
||||
|
||||
// ---------- provider extraction ----------
|
||||
|
||||
private async extractProviders(
|
||||
|
|
@ -293,35 +400,53 @@ export class IncludeExtractor implements ContractExtractor {
|
|||
): Promise<ExtractedContract[]> {
|
||||
// Strategy A: graph-assisted
|
||||
if (dbExecutor) {
|
||||
const graphProviders = await this.extractProvidersGraph(dbExecutor);
|
||||
const graphProviders = await this.extractProvidersGraph(dbExecutor, repoPath);
|
||||
if (graphProviders.length > 0) return graphProviders;
|
||||
}
|
||||
// Strategy B: filesystem fallback
|
||||
return this.extractProvidersFallback(repoPath, allFiles);
|
||||
}
|
||||
|
||||
private async extractProvidersGraph(db: CypherExecutor): Promise<ExtractedContract[]> {
|
||||
private async extractProvidersGraph(
|
||||
db: CypherExecutor,
|
||||
repoPath: string,
|
||||
): Promise<ExtractedContract[]> {
|
||||
try {
|
||||
const rows = await db(
|
||||
`MATCH (f:File)
|
||||
WHERE f.filePath =~ '.*\\\\.(h|hpp|hxx|hh)$'
|
||||
RETURN f.filePath AS filePath, f.id AS fileId`,
|
||||
);
|
||||
return rows
|
||||
.filter((r) => typeof r.filePath === 'string' && r.filePath)
|
||||
.map((r) => {
|
||||
const filePath = (r.filePath as string).replace(/\\/g, '/');
|
||||
return {
|
||||
contractId: `include::${normalizeIncludePath(filePath)}`,
|
||||
type: 'include' as const,
|
||||
role: 'provider' as const,
|
||||
symbolUid: String(r.fileId ?? ''),
|
||||
symbolRef: { filePath, name: path.basename(filePath) },
|
||||
symbolName: path.basename(filePath),
|
||||
confidence: 1.0,
|
||||
meta: { source: 'graph' },
|
||||
};
|
||||
// gitnexus analyze stores absolute paths in the File.filePath column.
|
||||
// Provider contract IDs MUST be repo-relative — otherwise the consumer
|
||||
// emits `include::map/base/view.h` and the provider emits
|
||||
// `include::/abs/path/to/repo/map/base/view.h`, which never match
|
||||
// through runExactMatch and the cross-link silently disappears.
|
||||
// (PR #1156 follow-up review: graph provider absolute-path bug.)
|
||||
const normalizedRepoPath = path.resolve(repoPath);
|
||||
const out: ExtractedContract[] = [];
|
||||
for (const r of rows) {
|
||||
if (typeof r.filePath !== 'string' || !r.filePath) continue;
|
||||
const absolute = r.filePath as string;
|
||||
const rel = path.relative(normalizedRepoPath, absolute);
|
||||
// Skip rows that resolve outside the repo (e.g., system headers
|
||||
// somehow indexed, or stale absolute paths from a different machine).
|
||||
// path.relative returns a `..`-prefixed path or an absolute path
|
||||
// when the target is outside the base — both are wrong for our IDs.
|
||||
if (!rel || rel.startsWith('..') || path.isAbsolute(rel)) continue;
|
||||
const normalizedRel = rel.replace(/\\/g, '/');
|
||||
out.push({
|
||||
contractId: `include::${normalizeIncludePath(normalizedRel)}`,
|
||||
type: 'include' as const,
|
||||
role: 'provider' as const,
|
||||
symbolUid: String(r.fileId ?? ''),
|
||||
symbolRef: { filePath: normalizedRel, name: path.basename(normalizedRel) },
|
||||
symbolName: path.basename(normalizedRel),
|
||||
confidence: 1.0,
|
||||
meta: { source: 'graph' },
|
||||
});
|
||||
}
|
||||
return out;
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
|
|
@ -349,16 +474,9 @@ export class IncludeExtractor implements ContractExtractor {
|
|||
|
||||
private async extractConsumers(
|
||||
repoPath: string,
|
||||
normalizedFiles: string[],
|
||||
allFiles: string[],
|
||||
sourceFiles: string[],
|
||||
suffixIndex: SuffixIndex,
|
||||
): Promise<ExtractedContract[]> {
|
||||
const sourceFiles = await glob(SOURCE_GLOB, {
|
||||
cwd: repoPath,
|
||||
ignore: STANDARD_IGNORES,
|
||||
nodir: true,
|
||||
});
|
||||
|
||||
const parser = new Parser();
|
||||
const out: ExtractedContract[] = [];
|
||||
// Compile the include query once per grammar to avoid re-compilation per file
|
||||
|
|
@ -381,8 +499,11 @@ export class IncludeExtractor implements ContractExtractor {
|
|||
}
|
||||
}
|
||||
|
||||
// Collect raw include paths: tree-sitter first, regex fallback for large files
|
||||
// Collect raw include paths: tree-sitter first, regex fallback for large files.
|
||||
// `extractionSource` is stamped on each emitted consumer contract so
|
||||
// regex-fallback contracts stay auditable post-hoc (PR #1156 review finding #6).
|
||||
let rawIncludes: string[];
|
||||
let extractionSource: 'tree_sitter' | 'regex_fallback';
|
||||
try {
|
||||
parser.setLanguage(lang);
|
||||
const tree = parseSourceSafe(parser, content);
|
||||
|
|
@ -393,6 +514,7 @@ export class IncludeExtractor implements ContractExtractor {
|
|||
matches = [];
|
||||
}
|
||||
rawIncludes = [];
|
||||
extractionSource = 'tree_sitter';
|
||||
for (const match of matches) {
|
||||
const sourceNode = match.captures.find((c) => c.name === 'import.source');
|
||||
if (!sourceNode) continue;
|
||||
|
|
@ -402,11 +524,15 @@ export class IncludeExtractor implements ContractExtractor {
|
|||
if (cleaned && cleaned.length <= 2048) rawIncludes.push(cleaned);
|
||||
}
|
||||
} catch {
|
||||
// tree-sitter failed (e.g. file > 32 KB) — fall back to regex
|
||||
// tree-sitter failed (e.g. file > 32 KB) — fall back to regex.
|
||||
// Strip block comments first so we don't emit a consumer contract
|
||||
// for a commented-out #include (PR #1156 review finding #5).
|
||||
rawIncludes = [];
|
||||
extractionSource = 'regex_fallback';
|
||||
const scanTarget = stripBlockComments(content);
|
||||
INCLUDE_REGEX.lastIndex = 0;
|
||||
let m: RegExpExecArray | null;
|
||||
while ((m = INCLUDE_REGEX.exec(content)) !== null) {
|
||||
while ((m = INCLUDE_REGEX.exec(scanTarget)) !== null) {
|
||||
if (m[1] && m[1].length <= 2048) rawIncludes.push(m[1]);
|
||||
}
|
||||
}
|
||||
|
|
@ -415,10 +541,38 @@ export class IncludeExtractor implements ContractExtractor {
|
|||
// Filter: skip known system headers and system path prefixes
|
||||
if (isSystemHeader(cleaned)) continue;
|
||||
|
||||
// Local resolution: try to resolve against this repo's own files
|
||||
const pathParts = cleaned.split('/').filter(Boolean);
|
||||
const resolved = suffixResolve(pathParts, normalizedFiles, allFiles, suffixIndex);
|
||||
if (resolved !== null) continue; // Local include — not cross-repo
|
||||
// Skip relative-up includes: `#include "../include/foo.h"` is
|
||||
// almost always an intra-repo reference. The suffix index is built
|
||||
// from repo-relative paths, so isLocalInclude can never match
|
||||
// `../foo.h`, and emitting it as a consumer contract just pollutes
|
||||
// the registry with an entry no provider can ever satisfy.
|
||||
// (PR #1156 follow-up review: `../` relative includes produce
|
||||
// spurious consumer contracts.)
|
||||
if (cleaned.startsWith('../') || cleaned.startsWith('..\\')) continue;
|
||||
|
||||
// Skip macro-style includes: `#include PLATFORM_HEADER` parses as an
|
||||
// identifier under tree-sitter's `(_) @import.source` wildcard. The
|
||||
// identifier text passes the strip/clean step unchanged, so without
|
||||
// this guard we would emit `include::platform_header` as a consumer
|
||||
// contract — and no provider in any repo will ever expose a contract
|
||||
// for a macro identifier (no file is named `PLATFORM_HEADER`). The
|
||||
// contract would sit permanently orphaned in the registry. Real
|
||||
// header references always contain a path separator (`/`, `\`) or an
|
||||
// extension dot (`foo.h`), so an absent both is a reliable signal we
|
||||
// are looking at a macro identifier. (PR #1156 follow-up review:
|
||||
// macro includes emit orphaned consumer contracts.)
|
||||
if (!/[./\\]/.test(cleaned)) continue;
|
||||
|
||||
// Local resolution (PR #1156 review finding #4): only accept an
|
||||
// exact-suffix match on the *full* include path. The generic
|
||||
// suffixResolve() iterates all truncated suffixes, which would
|
||||
// silently suppress a cross-repo `#include "map/base/view.h"`
|
||||
// when the local repo has any `internal/view.h` — a realistic
|
||||
// false-negative in large C++ codebases. Here we only resolve
|
||||
// locally if a file path ends with the complete include string
|
||||
// (optionally re-appending one of the C/C++ header extensions
|
||||
// when the include already omits it).
|
||||
if (isLocalInclude(cleaned, suffixIndex)) continue;
|
||||
|
||||
// Unresolved: emit as consumer contract
|
||||
const normalizedRel = rel.replace(/\\/g, '/');
|
||||
|
|
@ -431,7 +585,7 @@ export class IncludeExtractor implements ContractExtractor {
|
|||
symbolName: cleaned,
|
||||
confidence: 0.85,
|
||||
meta: {
|
||||
source: 'tree_sitter',
|
||||
source: extractionSource,
|
||||
includePath: cleaned,
|
||||
},
|
||||
});
|
||||
|
|
|
|||
|
|
@ -32,7 +32,7 @@ import {
|
|||
/** Max depth for compound-receiver chain resolution (`a().b().c().d()`).
|
||||
* Practical code rarely exceeds 3-4 hops; the cap prevents
|
||||
* pathological recursion if the receiver text is malformed. */
|
||||
const COMPOUND_RECEIVER_MAX_DEPTH = 6;
|
||||
const COMPOUND_RECEIVER_MAX_DEPTH = 4;
|
||||
|
||||
const MAP_TUPLE_SENTINEL_RE = /^__MAP_TUPLE_(\d+)__:(.+)$/;
|
||||
|
||||
|
|
|
|||
|
|
@ -941,9 +941,12 @@ describe('Write access tracking (C++)', () => {
|
|||
const accesses = getRelationships(result, 'ACCESSES');
|
||||
const writes = accesses.filter((e) => e.rel.reason === 'write');
|
||||
expect(writes.length).toBe(3);
|
||||
const fieldNames = writes.map((e) => e.target);
|
||||
expect(fieldNames).toContain('name');
|
||||
expect(fieldNames).toContain('address');
|
||||
// Per-field exact counts: both `user.name = ...` and `user.name += ...`
|
||||
// must produce distinct edges (no dedup); single write to `address`.
|
||||
const nameWrites = writes.filter((e) => e.target === 'name');
|
||||
expect(nameWrites.length).toBe(2);
|
||||
const addrWrites = writes.filter((e) => e.target === 'address');
|
||||
expect(addrWrites.length).toBe(1);
|
||||
const sources = writes.map((e) => e.source);
|
||||
expect(sources).toContain('updateUser');
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,7 +1,15 @@
|
|||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
|
||||
import * as fs from 'node:fs';
|
||||
import * as path from 'node:path';
|
||||
import * as os from 'node:os';
|
||||
|
||||
const { parseSourceSafeSpy } = vi.hoisted(() => ({ parseSourceSafeSpy: vi.fn() }));
|
||||
|
||||
vi.mock('../../../src/core/tree-sitter/safe-parse.js', async () => {
|
||||
const { buildSafeParseMock } = await import('../../helpers/parse-source-safe-mock.js');
|
||||
return buildSafeParseMock(parseSourceSafeSpy);
|
||||
});
|
||||
|
||||
import { IncludeExtractor } from '../../../src/core/group/extractors/include-extractor.js';
|
||||
import type { RepoHandle } from '../../../src/core/group/types.js';
|
||||
import { normalizeContractId } from '../../../src/core/group/matching.js';
|
||||
|
|
@ -196,6 +204,132 @@ int main() { return 0; }`,
|
|||
});
|
||||
});
|
||||
|
||||
// ---- Review finding #4: suffixResolve ambiguity ----
|
||||
|
||||
describe('finding #4: suffix-ambiguity does not silently suppress cross-repo include', () => {
|
||||
it('emits a cross-repo contract when the include path does not match any local file (even if a shorter suffix does)', async () => {
|
||||
// local repo has `internal/api.h` but NOT `ext/api.h`
|
||||
writeFile('internal/api.h', '#pragma once');
|
||||
writeFile(
|
||||
'src/main.cpp',
|
||||
`#include "ext/api.h"
|
||||
int main() { return 0; }`,
|
||||
);
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumers = contracts.filter((c) => c.role === 'consumer');
|
||||
|
||||
// Previously suffixResolve would match `api.h` against `internal/api.h`
|
||||
// and drop the cross-repo contract. After finding #4 fix, we only
|
||||
// accept exact full-path matches — so `ext/api.h` must still be
|
||||
// emitted as a consumer contract.
|
||||
expect(consumers).toHaveLength(1);
|
||||
expect(consumers[0].contractId).toBe('include::ext/api.h');
|
||||
});
|
||||
|
||||
it('still suppresses a local include when the FULL path matches', async () => {
|
||||
writeFile('ext/api.h', '#pragma once');
|
||||
writeFile('src/main.cpp', '#include "ext/api.h"\nint main(){return 0;}');
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumers = contracts.filter((c) => c.role === 'consumer');
|
||||
|
||||
expect(consumers).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('resolves locally when include omits extension and a matching .h exists', async () => {
|
||||
writeFile('foo/bar.h', '#pragma once');
|
||||
writeFile('src/main.cpp', '#include "foo/bar"\nint main(){return 0;}');
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumers = contracts.filter((c) => c.role === 'consumer');
|
||||
|
||||
expect(consumers).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
// ---- Review finding #5: regex fallback must strip block comments ----
|
||||
|
||||
describe('finding #5: regex fallback ignores block-commented includes', () => {
|
||||
it('does not emit a contract for an #include inside /* ... */', async () => {
|
||||
// Force regex fallback by producing a file larger than tree-sitter's
|
||||
// 32 KB hard cap. The include we care about lives inside a block
|
||||
// comment that spans the file.
|
||||
const filler = 'int dummy_' + 'x'.repeat(32) + ' = 0;\n'.repeat(1200);
|
||||
const content = `/*
|
||||
* Historical include, kept for reference only:
|
||||
* #include "legacy/old-api.h"
|
||||
*/
|
||||
${filler}
|
||||
#include "real/api.h"
|
||||
int main(){return 0;}`;
|
||||
writeFile('src/huge.cpp', content);
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumers = contracts.filter((c) => c.role === 'consumer');
|
||||
const ids = consumers.map((c) => c.contractId);
|
||||
|
||||
// The live include should appear; the commented-out one must NOT.
|
||||
expect(ids).toContain('include::real/api.h');
|
||||
expect(ids).not.toContain('include::legacy/old-api.h');
|
||||
});
|
||||
});
|
||||
|
||||
// ---- Review finding #6: meta.source must reflect which extraction path ran ----
|
||||
|
||||
describe('finding #6: meta.source reflects extraction path', () => {
|
||||
it('stamps `tree_sitter` on contracts produced via AST walking', async () => {
|
||||
writeFile('src/main.cpp', '#include "app/small.h"\nint main(){return 0;}');
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumers = contracts.filter((c) => c.role === 'consumer');
|
||||
|
||||
expect(consumers).toHaveLength(1);
|
||||
expect((consumers[0].meta as { source?: string } | undefined)?.source).toBe('tree_sitter');
|
||||
});
|
||||
|
||||
it('meta.source is one of the two documented values (tree_sitter | regex_fallback)', async () => {
|
||||
// Regex fallback is a defensive branch that only fires if
|
||||
// parser.setLanguage() or parser.parse() throws. In practice
|
||||
// tree-sitter-c/cpp handles realistic inputs, so we only assert
|
||||
// the meta.source contract: it is always present and always one of
|
||||
// the two documented values. This guards against future regressions
|
||||
// that might hard-code the wrong string.
|
||||
writeFile('src/main.cpp', '#include "ext/whatever.h"\nint main(){return 0;}');
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumer = contracts.find((c) => c.role === 'consumer');
|
||||
expect(consumer).toBeDefined();
|
||||
const src = (consumer?.meta as { source?: string } | undefined)?.source;
|
||||
expect(['tree_sitter', 'regex_fallback']).toContain(src);
|
||||
});
|
||||
});
|
||||
|
||||
// ---- Review finding #3: provider id collision on case-sensitive FS ----
|
||||
|
||||
describe('finding #3: case-folding is documented and deterministic', () => {
|
||||
it('collapses `Foo.h` and `foo.h` onto the same provider contract-id (documented trade-off)', async () => {
|
||||
writeFile('Foo.h', '#pragma once\n// Capital Foo');
|
||||
// On case-insensitive filesystems (macOS default) the second writeFile
|
||||
// will overwrite the first, so we only create this when distinct files
|
||||
// can coexist (case-sensitive FS, e.g. Linux CI).
|
||||
try {
|
||||
fs.writeFileSync(path.join(tmpDir, 'foo.h'), '#pragma once\n// lowercase foo');
|
||||
} catch {
|
||||
// Ignore — some FS won't allow both names to coexist.
|
||||
}
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const providers = contracts.filter((c) => c.role === 'provider');
|
||||
const ids = providers.map((p) => p.contractId);
|
||||
|
||||
// Both files (if they coexist) must normalize to the same id.
|
||||
// dedupe() keeps only one; caller code must be aware of this.
|
||||
expect(ids).toContain('include::foo.h');
|
||||
// Never see a mixed-case contract-id leak out.
|
||||
expect(ids.every((id) => id === id.toLowerCase())).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
// ---- Deduplication ----
|
||||
|
||||
describe('deduplication', () => {
|
||||
|
|
@ -233,4 +367,236 @@ int main() { return 0; }`,
|
|||
expect(normalizeContractId('include::map//base///foo.h')).toBe('include::map/base/foo.h');
|
||||
});
|
||||
});
|
||||
|
||||
// ---- PR #1156 follow-up: `../` relative includes ----
|
||||
|
||||
describe('follow-up: `../` relative includes are skipped', () => {
|
||||
it('does not emit a consumer contract for `#include "../foo.h"`', async () => {
|
||||
// Producer: a header that exists locally but only via parent reference
|
||||
writeFile('include/foo.h', '#pragma once');
|
||||
writeFile(
|
||||
'src/sub/main.cpp',
|
||||
`#include "../../include/foo.h"
|
||||
#include "real/cross_repo.h"
|
||||
int main() { return 0; }`,
|
||||
);
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumers = contracts.filter((c) => c.role === 'consumer');
|
||||
|
||||
// Only `real/cross_repo.h` should remain — the `..`-prefixed include
|
||||
// is intra-repo noise that no provider can ever satisfy.
|
||||
expect(consumers.map((c) => c.contractId)).toEqual(['include::real/cross_repo.h']);
|
||||
});
|
||||
|
||||
it('skips backslash-form `..\\` for completeness', async () => {
|
||||
writeFile(
|
||||
'src/main.cpp',
|
||||
`#include "..\\\\sibling\\\\foo.h"
|
||||
#include "remote/header.h"
|
||||
int main() { return 0; }`,
|
||||
);
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumers = contracts.filter((c) => c.role === 'consumer');
|
||||
|
||||
const ids = consumers.map((c) => c.contractId);
|
||||
expect(ids).toContain('include::remote/header.h');
|
||||
expect(ids.some((id) => id.includes('..'))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// ---- PR #1156 follow-up: macro-style includes ----
|
||||
|
||||
describe('follow-up: macro-style #include emits no consumer contract', () => {
|
||||
it('does not emit a consumer contract for `#include PLATFORM_HEADER` (no separator, no dot)', async () => {
|
||||
// `#include PLATFORM_HEADER` parses under tree-sitter as an identifier
|
||||
// node, slips past the existing system-header / `..` filters, and used
|
||||
// to leak through as a permanently orphaned consumer contract because
|
||||
// no file is ever named `PLATFORM_HEADER`. Verify the macro guard
|
||||
// suppresses it while preserving the real cross-repo include.
|
||||
writeFile(
|
||||
'src/main.cpp',
|
||||
`#include PLATFORM_HEADER
|
||||
#include "real/api.h"
|
||||
int main() { return 0; }`,
|
||||
);
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumers = contracts.filter((c) => c.role === 'consumer');
|
||||
|
||||
expect(consumers.map((c) => c.contractId)).toEqual(['include::real/api.h']);
|
||||
});
|
||||
|
||||
it('skips multiple macro identifiers in the same translation unit', async () => {
|
||||
writeFile(
|
||||
'src/cfg.cpp',
|
||||
`#include CONFIG_HEADER
|
||||
#include PLATFORM_HEADER
|
||||
#include ASSERT_H_
|
||||
int main(){return 0;}`,
|
||||
);
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumers = contracts.filter((c) => c.role === 'consumer');
|
||||
|
||||
expect(consumers).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
// ---- PR #1156 follow-up: graph provider absolute paths ----
|
||||
|
||||
describe('follow-up: extractProvidersGraph strips repo root from absolute paths', () => {
|
||||
it('produces repo-relative contract IDs when the graph returns absolute paths', async () => {
|
||||
writeFile('map/base/view.h', '#pragma once\nclass View {};');
|
||||
writeFile('utils/types.hpp', '#pragma once');
|
||||
|
||||
// Stub the Cypher executor to return absolute paths the way
|
||||
// gitnexus analyze actually persists them.
|
||||
const absolute1 = path.join(tmpDir, 'map/base/view.h');
|
||||
const absolute2 = path.join(tmpDir, 'utils/types.hpp');
|
||||
const stubDb = async () => [
|
||||
{ filePath: absolute1, fileId: 'File:abs:1' },
|
||||
{ filePath: absolute2, fileId: 'File:abs:2' },
|
||||
];
|
||||
|
||||
const contracts = await extractor.extract(stubDb, tmpDir, makeRepo(tmpDir));
|
||||
const providers = contracts.filter((c) => c.role === 'provider');
|
||||
|
||||
const ids = providers.map((p) => p.contractId).sort();
|
||||
expect(ids).toEqual(['include::map/base/view.h', 'include::utils/types.hpp']);
|
||||
expect(providers.every((p) => p.meta?.source === 'graph')).toBe(true);
|
||||
});
|
||||
|
||||
it('drops graph rows whose path resolves outside the repo root', async () => {
|
||||
writeFile('local/header.h', '#pragma once');
|
||||
const absoluteLocal = path.join(tmpDir, 'local/header.h');
|
||||
const stubDb = async () => [
|
||||
{ filePath: absoluteLocal, fileId: 'File:1' },
|
||||
// Stale absolute path from a different machine — must be skipped.
|
||||
{ filePath: '/some/other/repo/foreign.h', fileId: 'File:2' },
|
||||
];
|
||||
|
||||
const contracts = await extractor.extract(stubDb, tmpDir, makeRepo(tmpDir));
|
||||
const providers = contracts.filter((c) => c.role === 'provider');
|
||||
|
||||
expect(providers.map((p) => p.contractId)).toEqual(['include::local/header.h']);
|
||||
});
|
||||
});
|
||||
|
||||
// ---- PR #1156 Codex follow-up: discovery aligned with ingestion ----
|
||||
|
||||
describe('follow-up: file discovery honors createIgnoreFilter and getMaxFileSizeBytes', () => {
|
||||
it('does not emit a provider contract for a header excluded by .gitignore', async () => {
|
||||
writeFile('.gitignore', 'vendor-headers/\n');
|
||||
writeFile('vendor-headers/blocked.h', '#pragma once');
|
||||
writeFile('src/wanted.h', '#pragma once');
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const providerIds = contracts.filter((c) => c.role === 'provider').map((p) => p.contractId);
|
||||
|
||||
expect(providerIds).toContain('include::src/wanted.h');
|
||||
expect(providerIds).not.toContain('include::vendor-headers/blocked.h');
|
||||
});
|
||||
|
||||
it('does not emit a provider contract for a header excluded by .gitnexusignore', async () => {
|
||||
writeFile('.gitnexusignore', 'legacy/\n');
|
||||
writeFile('legacy/old.h', '#pragma once');
|
||||
writeFile('src/current.h', '#pragma once');
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const providerIds = contracts.filter((c) => c.role === 'provider').map((p) => p.contractId);
|
||||
|
||||
expect(providerIds).toContain('include::src/current.h');
|
||||
expect(providerIds).not.toContain('include::legacy/old.h');
|
||||
});
|
||||
|
||||
it('does not parse #include directives in a source file excluded by .gitignore', async () => {
|
||||
// The ignored source file references a header that would otherwise be
|
||||
// a cross-repo consumer. After alignment, the ignored file is invisible
|
||||
// to the consumer scan — no consumer contract should appear.
|
||||
writeFile('.gitignore', 'generated/\n');
|
||||
writeFile(
|
||||
'generated/auto.cpp',
|
||||
`#include "remote/should_not_appear.h"
|
||||
int auto_main() { return 0; }`,
|
||||
);
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumerIds = contracts.filter((c) => c.role === 'consumer').map((c) => c.contractId);
|
||||
|
||||
expect(consumerIds).not.toContain('include::remote/should_not_appear.h');
|
||||
});
|
||||
|
||||
it('skips a provider header whose size exceeds GITNEXUS_MAX_FILE_SIZE', async () => {
|
||||
const previous = process.env.GITNEXUS_MAX_FILE_SIZE;
|
||||
process.env.GITNEXUS_MAX_FILE_SIZE = '1'; // 1 KB cap
|
||||
try {
|
||||
// 4 KB header — comfortably exceeds the cap.
|
||||
const oversized = '#pragma once\n' + 'x'.repeat(4 * 1024);
|
||||
writeFile('huge/big.h', oversized);
|
||||
writeFile('small/tiny.h', '#pragma once');
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const providerIds = contracts.filter((c) => c.role === 'provider').map((p) => p.contractId);
|
||||
|
||||
expect(providerIds).toContain('include::small/tiny.h');
|
||||
expect(providerIds).not.toContain('include::huge/big.h');
|
||||
} finally {
|
||||
if (previous === undefined) delete process.env.GITNEXUS_MAX_FILE_SIZE;
|
||||
else process.env.GITNEXUS_MAX_FILE_SIZE = previous;
|
||||
}
|
||||
});
|
||||
|
||||
it('skips parsing #include directives in source files exceeding GITNEXUS_MAX_FILE_SIZE', async () => {
|
||||
const previous = process.env.GITNEXUS_MAX_FILE_SIZE;
|
||||
process.env.GITNEXUS_MAX_FILE_SIZE = '1';
|
||||
try {
|
||||
const oversized =
|
||||
'#include "remote/should_not_appear.h"\n' +
|
||||
'// padding to push the file past 1 KB\n' +
|
||||
'x'.repeat(4 * 1024);
|
||||
writeFile('big/main.cpp', oversized);
|
||||
|
||||
const contracts = await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
const consumerIds = contracts.filter((c) => c.role === 'consumer').map((c) => c.contractId);
|
||||
|
||||
expect(consumerIds).not.toContain('include::remote/should_not_appear.h');
|
||||
} finally {
|
||||
if (previous === undefined) delete process.env.GITNEXUS_MAX_FILE_SIZE;
|
||||
else process.env.GITNEXUS_MAX_FILE_SIZE = previous;
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('Windows SIGSEGV regression — large input must route through parseSourceSafe', () => {
|
||||
it('routes >32 767-char header file through parseSourceSafe (not direct parser.parse)', async () => {
|
||||
parseSourceSafeSpy.mockClear();
|
||||
|
||||
// Bump the file-size cap so the >40 000-char file isn't filtered before
|
||||
// it ever reaches the parser. Direct parser.parse(content) on a string
|
||||
// this size SIGSEGVs the process on Windows. The spy assertion catches
|
||||
// the regression — a "no throw" assertion alone is satisfied by the
|
||||
// bypass on Linux/macOS where parser.parse(40 000 chars) succeeds.
|
||||
const previousLimit = process.env.GITNEXUS_MAX_FILE_SIZE;
|
||||
process.env.GITNEXUS_MAX_FILE_SIZE = '512';
|
||||
try {
|
||||
const includes = Array.from(
|
||||
{ length: 1500 },
|
||||
(_, i) => `#include "lib/header_${i}.h"\n`,
|
||||
).join('');
|
||||
const largeHeader = `#pragma once\n${includes}\nstruct Big {};\n`;
|
||||
expect(largeHeader.length).toBeGreaterThan(40_000);
|
||||
|
||||
writeFile('big/big.cpp', largeHeader);
|
||||
|
||||
await extractor.extract(null, tmpDir, makeRepo(tmpDir));
|
||||
|
||||
expect(parseSourceSafeSpy).toHaveBeenCalled();
|
||||
} finally {
|
||||
if (previousLimit === undefined) delete process.env.GITNEXUS_MAX_FILE_SIZE;
|
||||
else process.env.GITNEXUS_MAX_FILE_SIZE = previousLimit;
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue