mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-12 23:02:45 +00:00
Two HIGH findings from the Codex adversarial review on
feat/group-include-extractor:
1. Default-on extraction silently changes existing groups (BLOCKER)
DEFAULT_DETECT.includes was true, so any pre-existing group.yaml
that omits the new field would gain a wave of include::* contracts
on the next sync after upgrade. Flipped to false (opt-in). The
integration test already declares includes: true explicitly so it
survives unchanged; the unit extractor tests bypass parseGroupConfig
entirely; the sync test uses extractorOverride. Only config-parser
needed regression tests covering omitted/explicit/false variants.
2. IncludeExtractor scans outside the indexed file universe (BLOCKER)
The extractor was running glob('**/*', { ignore: STANDARD_IGNORES })
twice with a hand-rolled 9-pattern list, no .gitignore/.gitnexusignore
honoring, and no max-file-size cap. That meant File:<path> contracts
could appear for files ingestion would never index, producing
cross-links group impact cannot fan out to (silent false-negatives).
Refactored to a single discoverIndexableFiles() helper that mirrors
walkRepositoryPaths exactly: createIgnoreFilter + getMaxFileSizeBytes,
one discovery pass shared by provider and consumer paths. Dropped
STANDARD_IGNORES and SOURCE_GLOB entirely.
third_party and 3rdparty (the C/C++ vendored-deps conventions) were
in the local ignore list but not in the canonical DEFAULT_IGNORE_LIST
used by ingestion. Folded both into the canonical set rather than
keep a parallel list — the whole point of the Codex finding is that
two file-discovery implementations drift. Single source of truth.
Tests: 5 new regression tests for the discovery alignment (.gitignore,
.gitnexusignore, max-file-size on both provider and consumer paths)
plus 4 for the opt-in default. All 30 include-extractor tests + the
494-test group suite + ignore-service tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
217 lines
5.5 KiB
TypeScript
217 lines
5.5 KiB
TypeScript
import { describe, it, expect } from 'vitest';
|
|
import * as fs from 'node:fs/promises';
|
|
import * as os from 'node:os';
|
|
import * as path from 'node:path';
|
|
import { loadGroupConfig, parseGroupConfig } from '../../../src/core/group/config-parser.js';
|
|
|
|
const VALID_YAML = `
|
|
version: 1
|
|
name: company
|
|
description: "All company microservices"
|
|
repos:
|
|
hr/hiring/backend: hr-hiring-backend
|
|
hr/hiring/ui: hr-hiring-ui
|
|
links:
|
|
- from: hr/hiring/backend
|
|
to: hr/hiring/ui
|
|
type: http
|
|
contract: "/api/users"
|
|
role: provider
|
|
packages:
|
|
hr/common:
|
|
npm: "@hr/common"
|
|
detect:
|
|
http: true
|
|
grpc: false
|
|
topics: false
|
|
shared_libs: true
|
|
embedding_fallback: false
|
|
matching:
|
|
bm25_threshold: 0.7
|
|
embedding_threshold: 0.65
|
|
max_candidates_per_step: 3
|
|
`;
|
|
|
|
describe('parseGroupConfig', () => {
|
|
it('parses valid group.yaml', () => {
|
|
const config = parseGroupConfig(VALID_YAML);
|
|
expect(config.name).toBe('company');
|
|
expect(config.version).toBe(1);
|
|
expect(Object.keys(config.repos)).toHaveLength(2);
|
|
expect(config.repos['hr/hiring/backend']).toBe('hr-hiring-backend');
|
|
expect(config.links).toHaveLength(1);
|
|
expect(config.links[0].type).toBe('http');
|
|
expect(config.links[0].role).toBe('provider');
|
|
expect(config.packages['hr/common'].npm).toBe('@hr/common');
|
|
expect(config.detect.http).toBe(true);
|
|
expect(config.detect.grpc).toBe(false);
|
|
});
|
|
|
|
it('applies defaults for missing optional fields', () => {
|
|
const minimal = `
|
|
version: 1
|
|
name: test
|
|
repos:
|
|
app: my-app
|
|
`;
|
|
const config = parseGroupConfig(minimal);
|
|
expect(config.description).toBe('');
|
|
expect(config.links).toEqual([]);
|
|
expect(config.packages).toEqual({});
|
|
expect(config.detect.http).toBe(true);
|
|
expect(config.matching.bm25_threshold).toBe(0.7);
|
|
expect(config.matching.exclude_links_paths).toEqual([]);
|
|
expect(config.matching.exclude_links_param_only_paths).toBe(false);
|
|
});
|
|
|
|
it('defaults thrift detection to true', () => {
|
|
const minimal = `
|
|
version: 1
|
|
name: test
|
|
repos:
|
|
app: my-app
|
|
`;
|
|
const config = parseGroupConfig(minimal);
|
|
expect(config.detect.thrift).toBe(true);
|
|
});
|
|
|
|
// PR #1156 Codex follow-up: include extraction is opt-in. Existing
|
|
// group.yaml files that do not declare `detect.includes` must not gain
|
|
// a wave of new include::* contracts on the next sync after upgrade.
|
|
describe('detect.includes opt-in default', () => {
|
|
it('defaults includes detection to false when detect block omits it', () => {
|
|
const minimal = `
|
|
version: 1
|
|
name: test
|
|
repos:
|
|
app: my-app
|
|
`;
|
|
const config = parseGroupConfig(minimal);
|
|
expect(config.detect.includes).toBe(false);
|
|
});
|
|
|
|
it('defaults includes detection to false when detect block is present but omits the key', () => {
|
|
const yaml = `
|
|
version: 1
|
|
name: test
|
|
repos:
|
|
app: my-app
|
|
detect:
|
|
http: true
|
|
grpc: false
|
|
`;
|
|
const config = parseGroupConfig(yaml);
|
|
expect(config.detect.includes).toBe(false);
|
|
});
|
|
|
|
it('honors explicit detect.includes: true (opt-in works)', () => {
|
|
const yaml = `
|
|
version: 1
|
|
name: test
|
|
repos:
|
|
app: my-app
|
|
detect:
|
|
includes: true
|
|
`;
|
|
const config = parseGroupConfig(yaml);
|
|
expect(config.detect.includes).toBe(true);
|
|
});
|
|
|
|
it('honors explicit detect.includes: false', () => {
|
|
const yaml = `
|
|
version: 1
|
|
name: test
|
|
repos:
|
|
app: my-app
|
|
detect:
|
|
includes: false
|
|
`;
|
|
const config = parseGroupConfig(yaml);
|
|
expect(config.detect.includes).toBe(false);
|
|
});
|
|
});
|
|
|
|
it('parses thrift manifest links', () => {
|
|
const yaml = `
|
|
version: 1
|
|
name: test
|
|
repos:
|
|
gateway: gateway-repo
|
|
orders: orders-repo
|
|
links:
|
|
- from: gateway
|
|
to: orders
|
|
type: thrift
|
|
contract: billing.v1.OrderService/PlaceOrder
|
|
role: consumer
|
|
`;
|
|
const config = parseGroupConfig(yaml);
|
|
expect(config.links[0].type).toBe('thrift');
|
|
expect(config.links[0].contract).toBe('billing.v1.OrderService/PlaceOrder');
|
|
});
|
|
|
|
it('throws on missing required fields', () => {
|
|
expect(() => parseGroupConfig('version: 1')).toThrow(/name.*required/i);
|
|
expect(() => parseGroupConfig('name: test')).toThrow(/version.*required/i);
|
|
expect(() => parseGroupConfig('version: 1\nname: test')).toThrow(/repos.*required/i);
|
|
});
|
|
|
|
it('allows empty repos object (fresh group before first add)', () => {
|
|
const yaml = `version: 1
|
|
name: new-group
|
|
repos: {}
|
|
`;
|
|
const config = parseGroupConfig(yaml);
|
|
expect(Object.keys(config.repos)).toHaveLength(0);
|
|
});
|
|
|
|
it('loadGroupConfig reads group.yaml from disk', async () => {
|
|
const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-group-load-'));
|
|
const yaml = `version: 1
|
|
name: disk-test
|
|
repos:
|
|
a: repo-a
|
|
`;
|
|
await fs.writeFile(path.join(dir, 'group.yaml'), yaml, 'utf-8');
|
|
const config = await loadGroupConfig(dir);
|
|
expect(config.name).toBe('disk-test');
|
|
expect(config.repos.a).toBe('repo-a');
|
|
});
|
|
|
|
it('throws on invalid version', () => {
|
|
expect(() => parseGroupConfig('version: 2\nname: test\nrepos:\n a: b')).toThrow(/version/i);
|
|
});
|
|
|
|
it('throws on invalid link role', () => {
|
|
const yaml = `
|
|
version: 1
|
|
name: test
|
|
repos:
|
|
a: repo-a
|
|
b: repo-b
|
|
links:
|
|
- from: a
|
|
to: b
|
|
type: http
|
|
contract: "/api"
|
|
role: invalid
|
|
`;
|
|
expect(() => parseGroupConfig(yaml)).toThrow(/role/i);
|
|
});
|
|
|
|
it('throws when link references non-existent repo path', () => {
|
|
const yaml = `
|
|
version: 1
|
|
name: test
|
|
repos:
|
|
a: repo-a
|
|
links:
|
|
- from: a
|
|
to: nonexistent
|
|
type: http
|
|
contract: "/api"
|
|
role: provider
|
|
`;
|
|
expect(() => parseGroupConfig(yaml)).toThrow(/nonexistent/i);
|
|
});
|
|
});
|