GitNexus/gitnexus/test/integration/cli-limit-e2e.test.ts
Malik 859e4b75a4
Some checks are pending
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Scorecard / Scorecard analysis (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
fix(cli): --limit i18n, 0/negative guard, and correct truncation paths (#2310)
* fix: add --limit i18n, negative guard, correct property paths, and zh-CN translations

- Add i18n keys for context/impact/cypher/detect-changes --limit options
- Add zh-CN translations for all 4 --limit option descriptions
- Add Math.max(0, parseInt()) guard to prevent negative --limit
- Fix ALL property path mismatches discovered by audit:
  - context: callers/callees → incoming.calls/outgoing.calls+accesses
  - impact: upstream/downstream → affected_processes/affected_modules/byDepth
  - cypher: rows → row_count cap (rows embedded in markdown string)
  - detect-changes: affected_flows → affected_processes
- Change query command from required to optional positional arg with -q alias
- Update @ladybugdb/core from ^0.16.1 to ^0.17.1
- Update typescript from ^5.4.5 to ^5.9.3

* test: add E2E tests for --limit flag across all 5 CLI commands

Tests context, impact, cypher, detect-changes, and query with
--limit 1, baseline comparison, and --limit 0 (falsy/no-op).

detect-changes output is formatted text (not JSON), so those
tests count symbol lines matching 'Type name -> filePath' pattern.

14 tests, all passing. No regressions in 6455 existing tests.

* fix: address Copilot review feedback on --limit guards

- Add Math.max(0, ...) guard to queryCommand limit parsing
- Change if(limit) to if(limit !== undefined) in all 5 commands
  (prevents --limit 0 from being treated as falsy/no-op)
- Make queryText parameter optional (Commander may pass undefined)
- Fix usage error strings: --search to -q, --query (en + zh-CN)

* chore(autofix): apply prettier + eslint fixes via /autofix command

* fix(cli): centralize --limit parsing, slice cypher markdown, fix usage text

Address PR review feedback on --limit handling:

- Add a shared parseLimit() helper (Number.isInteger(n) && n > 0), used by all
  5 tool commands. Non-numeric / 0 / negative --limit now means "no limit"
  instead of the `options.limit ? Math.max(0, parseInt(...)) : undefined` path,
  where a string like "abc" is truthy and yields NaN -> slice(0, NaN) -> the
  guardrail commands (impact/context/detect-changes) silently emptied results
  with exit 0.
- cypher: slice the markdown table to --limit data rows so the reported
  row_count matches what is actually printed (was capping row_count while
  printing every row).
- Fix query usage string: [search_query] (optional positional) and
  `--query <text>` invocation form, not the option-definition
  `-q, --query <search_query>` syntax (en + zh-CN).
- Add an E2E regression test for non-numeric --limit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(cli): escape newlines in cypher markdown cells

A multi-line cell value (e.g. a symbol's `content`) was rendered with raw
newlines via String(v), so one logical row spanned multiple physical lines.
That corrupts the markdown table and breaks `cypher --limit`'s line-based
slice (it kept the wrong number of rows, often zero, while row_count
over-claimed). Collapse newlines in formatCypherAsMarkdown so one physical
line == one row; the existing CLI slice is now correct and the pre-existing
un---limited corruption is fixed too. (#2310 review)

* test(cli): de-vacuum the --limit truncation tests

The truncation it()s used the repo-banned vacuous-pass pattern (early-return
on status===null, assertions guarded by if(Array.isArray), bounds-only
toBeLessThanOrEqual — DoD.md:82) against `validateInput`, which has only 1
caller, so context/impact/query --limit 1 compared 1>=1 and stayed green even
if the slice were deleted. Rewrite with unconditional, exact assertions and
target `logMessage` (2 callers, 4 processes) so the no-limit baseline truly
exceeds the limit; detect-changes now mutates two real function bodies (two
changed symbols). Adds a multi-line-cell cypher --limit regression. (#2310)

* test(ci): run cli-limit-e2e in the cross-platform matrix

The --limit E2E suite spawns the real CLI (child_process) but was not in
SPAWN_CLI, so it ran only on Ubuntu — the cross-platform check only fails on
listed-but-missing files, not the reverse (TESTING.md §Cross-platform). Register
it so the --limit regression guard also runs on Windows/macOS, where path
separators, CRLF and the formatted-output arrow differ. (#2310)

* fix(cli): document impact --limit affected-list cap, drop dead byDepth re-slice

`impact --limit` also caps affected_processes/modules, but the help only
mentioned the per-depth cap — so JSON consumers reading the affected lists got
a silently-truncated array. Update en + zh-CN + the command description to say
so. Also remove the client-side byDepth re-slice: the backend already
paginates byDepth to the same limit (paginationLimit = clamp(limit,1,10000),
offset applied backend-side), so the client slice was a guaranteed no-op. (#2310)

* fix(cli): reconcile detect-changes --limit summary, list, and overflow

formatDetectChangesResult computed the "... and N more" overflow from the
already---limit-sliced array length, so under `--limit` the header (true
summary total), the listed rows, and the marker disagreed — e.g. "2 symbols"
in the header but a list of 1 with no marker. Base the overflow on the true
summary.changed_count / affected_count instead, and add the same marker to the
affected-processes list, so header + list + marker stay consistent. (#2310)

* feat(cli): add -l shorthand to impact --limit

The PR added the -l alias to context/cypher/detect-changes but left impact on
the long --limit only, so `impact -l 5` errored while `context -l 5` worked.
Add -l for parity and update the help-i18n OPTION_DESCRIPTION_KEYS key to the
new `-l, --limit <n>` flag string so the description still resolves. (#2310)

* fix(cli): bound all context --limit array categories

context --limit sliced only incoming.calls / outgoing.calls / outgoing.accesses
/ processes, leaving the other relType buckets unbounded — notably
incoming.accesses (bounded on outgoing but not incoming) plus imports/extends/
uses/… and typed_properties. Replace the hardcoded slices with a generic loop
over every array-valued bucket under incoming/outgoing, plus typed_properties
and processes, so --limit caps the whole context payload. (#2310)

* refactor(cli): parse --offset with a parseLimit-style helper

impactCommand parsed --offset with the legacy parseInt/Number.isFinite idiom
while --limit had moved to parseLimit, leaving two parsing styles side by side.
Add a sibling parseOffset helper (non-negative — offset 0 is valid) and use it,
so both options share one idiom; as a bonus it now rejects negative/fractional
offsets instead of forwarding them to the backend. (#2310)

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Gergo Magyar <gergomagyar@icloud.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-01 19:16:36 +01:00

384 lines
14 KiB
TypeScript

/**
* P1 Integration Tests: CLI --limit flag E2E
*
* Verifies that the --limit flag correctly truncates results for all 5
* tool commands: context, impact, cypher, detect-changes, query.
*
* Uses the same subprocess spawn pattern as cli-e2e.test.ts.
* Copies mini-repo fixture to a temp dir, runs analyze, then tests
* --limit truncation against each command.
*
* Assertions are exact (per DoD.md §"Assertions are meaningful") and
* unconditional — no `if (status === null) return` / `if (Array.isArray)`
* guards that would let a broken --limit slice pass vacuously. Targets are
* chosen so the no-limit baseline genuinely exceeds the limit (e.g. `logMessage`
* has 2 callers and 4 processes), so a no-op slice turns the test red.
*
* @see src/cli/tool.ts — limit application logic
*/
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
import { spawnSync } from 'child_process';
import path from 'path';
import fs from 'fs';
import os from 'os';
import { fileURLToPath, pathToFileURL } from 'url';
import { createRequire } from 'module';
import { cleanupTempDirSync } from '../helpers/test-db.js';
const testDir = path.dirname(fileURLToPath(import.meta.url));
const repoRoot = path.resolve(testDir, '../..');
const cliEntry = path.join(repoRoot, 'src/cli/index.ts');
const FIXTURE_SRC = path.resolve(testDir, '..', 'fixtures', 'mini-repo');
let MINI_REPO: string;
let tmpParent: string;
let suiteGitnexusHome: string;
const _require = createRequire(import.meta.url);
const tsxPkgDir = path.dirname(_require.resolve('tsx/package.json'));
const tsxImportUrl = pathToFileURL(path.join(tsxPkgDir, 'dist', 'loader.mjs')).href;
function cliEnv(extraEnv: Record<string, string> = {}) {
return {
...process.env,
GITNEXUS_HOME: suiteGitnexusHome,
NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(),
...extraEnv,
};
}
function runCliRaw(extraArgs: string[], cwd: string, timeoutMs = 30000) {
return spawnSync(process.execPath, ['--import', tsxImportUrl, cliEntry, ...extraArgs], {
cwd,
encoding: 'utf8',
timeout: timeoutMs,
stdio: ['pipe', 'pipe', 'pipe'],
env: cliEnv(),
});
}
/**
* Parse stdout as JSON, returning null on failure (e.g., text output).
*/
function parseStdout(result: ReturnType<typeof runCliRaw>): unknown {
try {
return JSON.parse(result.stdout.trim());
} catch {
return null;
}
}
// ─── Typed result shapes (avoid `any`; just the fields these tests read) ──────
type CallBuckets = { calls?: unknown[]; accesses?: unknown[] };
type ContextResult = { incoming?: CallBuckets; outgoing?: CallBuckets; processes?: unknown[] };
type ImpactResult = { affected_processes?: unknown[]; affected_modules?: unknown[] };
type CypherTabular = { markdown?: string; row_count?: number };
type QueryResult = { processes?: unknown[] };
/** Run a JSON tool command, asserting it exited 0 and produced parseable JSON. */
function runJson<T>(args: string[]): T {
const r = runCliRaw(args, MINI_REPO);
expect(r.status, `exit nonzero — stderr: ${r.stderr}`).toBe(0);
const data = parseStdout(r);
expect(data, `stdout not JSON: ${r.stdout.slice(0, 200)}`).toBeTruthy();
return data as T;
}
/** Run a text-output tool command, asserting it exited 0. */
function runText(args: string[]): string {
const r = runCliRaw(args, MINI_REPO);
expect(r.status, `exit nonzero — stderr: ${r.stderr}`).toBe(0);
return r.stdout;
}
/** detect-changes lists symbols as " Symbol name → file"; count those lines. */
function countChangedSymbolLines(stdout: string): number {
return stdout.split('\n').filter((line) => /^\s+\w+\s+\w+\s+→/.test(line)).length;
}
const len = (a?: unknown[]): number => (Array.isArray(a) ? a.length : 0);
// ─── Setup ───────────────────────────────────────────────────────────────────
beforeAll(() => {
tmpParent = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-cli-limit-'));
suiteGitnexusHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-cli-limit-home-'));
MINI_REPO = path.join(tmpParent, 'mini-repo');
fs.cpSync(FIXTURE_SRC, MINI_REPO, { recursive: true });
// Initialize as git repo
spawnSync('git', ['init'], { cwd: MINI_REPO, stdio: 'pipe' });
spawnSync('git', ['add', '-A'], { cwd: MINI_REPO, stdio: 'pipe' });
spawnSync('git', ['commit', '-m', 'initial commit'], {
cwd: MINI_REPO,
stdio: 'pipe',
env: {
...process.env,
GIT_AUTHOR_NAME: 'test',
GIT_AUTHOR_EMAIL: 'test@test',
GIT_COMMITTER_NAME: 'test',
GIT_COMMITTER_EMAIL: 'test@test',
},
});
// Run analyze to populate .gitnexus/ index (required for all tool commands)
const analyzeResult = runCliRaw(['analyze', '--force'], MINI_REPO, 60000);
if (analyzeResult.status !== 0) {
throw new Error(
`Analyze failed (status ${analyzeResult.status}):\nstdout: ${analyzeResult.stdout}\nstderr: ${analyzeResult.stderr}`,
);
}
});
afterAll(() => {
if (tmpParent) cleanupTempDirSync(tmpParent);
if (suiteGitnexusHome) cleanupTempDirSync(suiteGitnexusHome);
});
// ─── Tests ───────────────────────────────────────────────────────────────────
describe('CLI --limit flag E2E', () => {
// `logMessage` has 2 callers (processRequest, errorMiddleware) and participates
// in 4 processes — so its baseline genuinely exceeds `--limit 1`, making the
// truncation assertions non-vacuous.
// ─── context ────────────────────────────────────────────────────────────
describe('context --limit', () => {
it('truncates incoming/outgoing calls and processes to --limit 1', () => {
const limited = runJson<ContextResult>([
'context',
'logMessage',
'--limit',
'1',
'--repo',
'mini-repo',
]);
expect(len(limited.incoming?.calls)).toBe(1);
expect(len(limited.outgoing?.calls)).toBe(1);
expect(len(limited.processes)).toBe(1);
});
it('returns the full set without --limit (baseline exceeds the limit)', () => {
const base = runJson<ContextResult>(['context', 'logMessage', '--repo', 'mini-repo']);
expect(len(base.incoming?.calls)).toBe(2);
expect(len(base.outgoing?.calls)).toBe(2);
expect(len(base.processes)).toBe(4);
});
it('treats --limit 0 as no limit (resolves to undefined)', () => {
const zero = runJson<ContextResult>([
'context',
'logMessage',
'--limit',
'0',
'--repo',
'mini-repo',
]);
const base = runJson<ContextResult>(['context', 'logMessage', '--repo', 'mini-repo']);
expect(len(zero.processes)).toBe(len(base.processes));
expect(len(zero.incoming?.calls)).toBe(len(base.incoming?.calls));
});
it('treats a non-numeric --limit as no limit (no silent empty)', () => {
// Regression for the headline bug: `--limit abc` used to parse to NaN →
// slice(0, NaN) === [] → results silently emptied with exit 0. parseLimit()
// now rejects non-numeric input, so it must behave exactly like no --limit.
const invalid = runJson<ContextResult>([
'context',
'logMessage',
'--limit',
'abc',
'--repo',
'mini-repo',
]);
const base = runJson<ContextResult>(['context', 'logMessage', '--repo', 'mini-repo']);
const total = (d: ContextResult) =>
len(d.incoming?.calls) +
len(d.outgoing?.calls) +
len(d.outgoing?.accesses) +
len(d.processes);
expect(total(invalid)).toBe(total(base));
expect(total(invalid)).toBeGreaterThan(0); // not the old silent-empty
});
});
// ─── impact ─────────────────────────────────────────────────────────────
describe('impact --limit', () => {
it('truncates affected_processes/modules to --limit 1', () => {
const limited = runJson<ImpactResult>([
'impact',
'logMessage',
'--direction',
'upstream',
'--limit',
'1',
'--repo',
'mini-repo',
]);
expect(len(limited.affected_processes)).toBe(1);
expect(len(limited.affected_modules)).toBe(1);
});
it('returns the full affected set without --limit (baseline exceeds the limit)', () => {
const base = runJson<ImpactResult>([
'impact',
'logMessage',
'--direction',
'upstream',
'--repo',
'mini-repo',
]);
expect(len(base.affected_processes)).toBe(2);
expect(len(base.affected_modules)).toBe(2);
});
it('treats --limit 0 as no limit', () => {
const zero = runJson<ImpactResult>([
'impact',
'logMessage',
'--direction',
'upstream',
'--limit',
'0',
'--repo',
'mini-repo',
]);
const base = runJson<ImpactResult>([
'impact',
'logMessage',
'--direction',
'upstream',
'--repo',
'mini-repo',
]);
expect(len(zero.affected_processes)).toBe(len(base.affected_processes));
expect(len(zero.affected_modules)).toBe(len(base.affected_modules));
});
});
// ─── cypher ───────────────────────────────────────────────────────────────
describe('cypher --limit', () => {
it('truncates tabular result rows to --limit and keeps row_count honest', () => {
const limited = runJson<CypherTabular>([
'cypher',
'MATCH (n:Function) RETURN n.name AS name LIMIT 100',
'--limit',
'2',
'--repo',
'mini-repo',
]);
expect(limited.row_count).toBe(2);
// header + separator + exactly 2 data rows
expect((limited.markdown ?? '').split('\n')).toHaveLength(4);
});
it('slices multi-line-cell rows by logical row, not physical line (#2310)', () => {
// n.content holds multi-line source; the markdown table must still slice to
// exactly `--limit` complete rows (regression for the corruption fix).
const limited = runJson<CypherTabular>([
'cypher',
'MATCH (n:Function) RETURN n.name AS name, n.content AS content LIMIT 8',
'--limit',
'3',
'--repo',
'mini-repo',
]);
expect(limited.row_count).toBe(3);
const lines = (limited.markdown ?? '').split('\n');
expect(lines).toHaveLength(5); // header + separator + 3 rows, no row spanning lines
expect(limited.markdown ?? '').not.toMatch(/\n[^|]/);
});
it('returns more rows without --limit (baseline exceeds the limit)', () => {
const base = runJson<CypherTabular>([
'cypher',
'MATCH (n:Function) RETURN n.name AS name LIMIT 100',
'--repo',
'mini-repo',
]);
expect(base.row_count).toBeGreaterThan(2);
});
});
// ─── detect-changes ───────────────────────────────────────────────────────
describe('detect-changes --limit', () => {
// Modify two exported functions in two files → two changed symbols, so
// `--limit 1` truncates the listed symbols from 2 to 1. Idempotent: re-runs
// don't change the symbol set. (Edits land in the temp copy only.)
function makeTwoSymbolChange() {
const edits: Array<[string, RegExp, string]> = [
['src/logger.ts', /export function logMessage\([^)]*\)[^{]*\{/, '\n const _touchLog = 1;'],
[
'src/middleware.ts',
/export function processRequest\([^)]*\)[^{]*\{/,
'\n const _touchMw = 1;',
],
];
for (const [rel, re, insert] of edits) {
const p = path.join(MINI_REPO, rel);
const src = fs.readFileSync(p, 'utf8');
if (src.includes(insert.trim())) continue; // idempotent
fs.writeFileSync(
p,
src.replace(re, (m) => m + insert),
);
}
}
it('truncates changed_symbols to --limit 1', () => {
makeTwoSymbolChange();
const stdout = runText(['detect-changes', '--limit', '1', '--repo', 'mini-repo']);
expect(countChangedSymbolLines(stdout)).toBe(1);
});
it('lists both changed symbols without --limit (baseline exceeds the limit)', () => {
makeTwoSymbolChange();
const stdout = runText(['detect-changes', '--repo', 'mini-repo']);
expect(countChangedSymbolLines(stdout)).toBe(2);
});
it('treats --limit 0 as no limit', () => {
makeTwoSymbolChange();
const zero = runText(['detect-changes', '--limit', '0', '--repo', 'mini-repo']);
const base = runText(['detect-changes', '--repo', 'mini-repo']);
expect(countChangedSymbolLines(zero)).toBe(countChangedSymbolLines(base));
});
it('header total, listed count, and overflow marker stay consistent under --limit', () => {
// Header keeps the TRUE total (2 symbols), the list is capped to 1, and the
// overflow marker reports the real remainder (1) — not the sliced length.
makeTwoSymbolChange();
const stdout = runText(['detect-changes', '--limit', '1', '--repo', 'mini-repo']);
expect(countChangedSymbolLines(stdout)).toBe(1);
expect(stdout).toMatch(/2 symbols/);
expect(stdout).toMatch(/and 1 more/);
});
});
// ─── query ──────────────────────────────────────────────────────────────
describe('query --limit', () => {
it('truncates processes to --limit 1', () => {
// "message" matches logMessage / createLogEntry / formatLogEntry → 4 processes
const limited = runJson<QueryResult>([
'query',
'message',
'--limit',
'1',
'--repo',
'mini-repo',
]);
expect(len(limited.processes)).toBe(1);
});
it('returns more processes without --limit (baseline exceeds the limit)', () => {
const base = runJson<QueryResult>(['query', 'message', '--repo', 'mini-repo']);
expect(len(base.processes)).toBeGreaterThan(1);
});
});
});