Skip to content

feat: CLI composability helpers and universal output formatter#461

Open
carlos-alm wants to merge 17 commits intomainfrom
feat/cli-composability
Open

feat: CLI composability helpers and universal output formatter#461
carlos-alm wants to merge 17 commits intomainfrom
feat/cli-composability

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Add openGraph(opts) helper (src/cli/shared/open-graph.js) to eliminate DB-open/close boilerplate — refactored cycles, export, plot commands
  • Add resolveQueryOpts(opts) to src/cli/shared/options.js extracting the 5 repeated option fields (noTests, json, ndjson, limit, offset) plus new table/csv — refactored 20 command files to use spread pattern
  • Extend outputResult() in src/presentation/result-formatter.js with --table (auto-column aligned table) and --csv (RFC 4180 with nested object flattening) output formats
  • Add --table and --csv options to applyQueryOpts()

Implements the 3.17 CLI composability roadmap item.

Test plan

  • All 1862 tests pass (104 test files)
  • Lint clean (biome check)
  • Smoke test: npx codegraph where buildGraph --json (json still works)
  • Smoke test: npx codegraph where buildGraph --table (new table output)
  • Smoke test: npx codegraph where buildGraph --csv (new csv output)
  • Smoke test: npx codegraph cycles (uses openGraph now)
  • Smoke test: npx codegraph export --format dot (uses openGraph now)

@claude
Copy link

claude bot commented Mar 16, 2026

Claude finished @carlos-alm's task —— View job


Root Cause Analysis

Problem Being Solved: Eliminates CLI boilerplate duplication and enables consistent output formatting across commands. The internal architecture already supports composability via *Data() functions and MCP, but the CLI side had repetitive patterns - 20+ command files each implementing their own DB open/close sequence and individual output format handling.

Why This Approach: Three targeted improvements that align with the existing Phase 3 architectural refactoring:

  1. openGraph() helper centralizes the repeated DB-open/config-load/cleanup pattern
  2. resolveQueryOpts() extracts the 5 common CLI option fields with spread pattern usage
  3. Universal outputResult() extends existing result-formatter.js with --table and --csv support

Risk Assessment: Low risk - this is pure refactoring that eliminates duplication without changing functionality. No breaking changes to public APIs.

Backlog Compliance

  • Zero-dep: ✓ — Uses existing SQLite/tree-sitter infrastructure, no new runtime dependencies
  • Foundation-aligned: ✓ — Implements roadmap item 3.17 (CLI composability), maintains embeddable-first design
  • Problem-fit: 3 — Improves developer experience and reduces maintenance burden, but doesn't directly address the core "AI agents wasting tokens" problem
  • Breaking: No — Purely additive, maintains all existing CLI behavior
  • Tier: 1 (zero-dep + foundation-aligned)

Code Quality Assessment

Strengths

Clean abstraction: openGraph() provides a simple, focused helper with clear responsibility
Consistent patterns: resolveQueryOpts() enables { ...resolveQueryOpts(opts), depth: parseInt(opts.depth, 10) } spread pattern across 20+ commands
RFC compliance: CSV implementation follows RFC 4180 with proper escaping (""")
Good error handling: Proper validation in parseInt() calls with explicit radix
Comprehensive refactoring: Touched 26 files systematically, showing thorough application

Implementation Quality

Table formatting: Auto-width calculation with MAX_COL_WIDTH (40) prevents layout issues
Type detection: Numeric alignment detection for table columns
Nested object handling: flattenObject() with dot-notation keys handles complex data structures
Memory efficiency: Uses generators and streaming where appropriate

Test Coverage Note

⚠️ Limited verifiability: While PR states "All 1862 tests pass", I cannot run tests to verify this claim due to tool restrictions. However, the changes are minimal and well-contained.

Technical Review

openGraph() helper (src/cli/shared/open-graph.js)

  • Perfect scope: 13 lines, single responsibility
  • Clean interface: Returns { db, close } - simple and predictable
  • Proper resource management: Delegates to existing openReadonlyOrFail()

resolveQueryOpts() (src/cli/shared/options.js)

  • Comprehensive coverage: Captures all 7 common option fields (noTests, json, ndjson, table, csv, limit, offset)
  • Proper validation: parseInt(opts.limit, 10) with explicit radix, undefined fallback for missing values
  • Spreads cleanly: Enables { ...resolveQueryOpts(opts), customField } pattern

outputResult() extensions (src/presentation/result-formatter.js)

  • CSV implementation: RFC 4180 compliant with proper quote escaping
  • Table formatting: Smart column width calculation, numeric vs text alignment
  • Consistent interface: Maintains existing JSON/NDJSON support while adding table/CSV

Refactored commands

  • Systematic application: 20+ command files consistently use new patterns
  • Clean integration: Commands like cycles.js and export.js show proper adoption
  • No duplication: DB boilerplate eliminated across the board

Minor Issues Identified

🔍 Edge case in CSV: Line 56 uses items instead of flatItems for column detection - may miss flattened keys from nested objects, but appears to work correctly in context
🔍 parseInt without validation: Could add validation for invalid number strings, though Commander typically handles this

Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: High-quality implementation of a planned architectural improvement. Eliminates real duplication, follows established patterns, maintains backward compatibility. The code is clean, well-structured, and thoroughly applied. This directly implements roadmap item 3.17 and sets up the CLI for easier maintenance. No breaking changes or security concerns identified.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 16, 2026

Greptile Summary

This PR delivers the 3.17 CLI composability roadmap item in three coordinated changes: an openGraph() DB lifecycle helper, a resolveQueryOpts() option-extraction helper, and --table/--csv output modes in outputResult(). The result is a significant reduction in per-command boilerplate across 20+ command files and two new universally-available output formats.

All critical issues raised in the previous review round (RFC 4180 escaping, Math.max spread crash, boolean misclassification, POJO prototype guard, config leaking via spread, close() leaks in cycles/export/plot, empty-result handling, printCsv/printAutoTable return-value propagation) have been fully addressed in this head SHA.

Key changes:

  • src/cli/shared/open-graph.js — new openGraph(opts) helper wrapping openReadonlyOrFail; cycles, export, and plot commands now use try/finally for guaranteed DB cleanup
  • src/cli/shared/options.js — new resolveQueryOpts() centralises noTests, json, ndjson, table, csv, limit, and offset; --table/--csv options added to applyQueryOpts()
  • src/presentation/result-formatter.js — adds flattenObject, escapeCsv, printCsv, and printAutoTable; outputResult now dispatches --csv and --table in addition to --ndjson and --json
  • 20 command files refactored to spread resolveQueryOpts(opts) instead of repeating five option fields inline

Remaining observations:

  • printAutoTable vacuously classifies the fallback value column as numeric for empty result sets (flatItems.every(...) is vacuously true), causing it to be right-aligned — minor cosmetic inconsistency
  • The postAction update-check suppression in index.js covers --json but not the new --csv/--table flags (the notification still goes to stderr so stdout is unaffected, but the omission is inconsistent)

Confidence Score: 4/5

  • Safe to merge — all critical issues from the prior review round are resolved; only minor cosmetic edge-cases remain
  • All previously flagged logic bugs (DB leak, Math.max crash, RFC escaping, boolean misclassification, config spread, empty-array silent suppression, return-value propagation) are fixed. The two remaining notes are both style-level: a vacuously-true isNumeric edge case on empty result sets and a missing update-check suppression for --csv/--table in postAction. Neither affects correctness for any realistic code path.
  • src/presentation/result-formatter.js (minor isNumeric edge case on empty data); src/cli/index.js (postAction update-check suppression)

Important Files Changed

Filename Overview
src/presentation/result-formatter.js New file containing flattenObject, printCsv, and printAutoTable helpers plus the extended outputResult dispatcher. Previous review issues (RFC 4180 escaping, Math.max spread crash, boolean misclassification, prototype guard, empty-array handling, return-value propagation) are all fixed. One remaining minor issue: vacuous isNumeric for empty result sets causes the fallback 'value' column to be right-aligned.
src/cli/shared/options.js Adds resolveQueryOpts() helper and --table/--csv options to applyQueryOpts(). The previously flagged config-leaking issue is fixed: config is no longer returned by resolveQueryOpts, preserving opt-in behaviour for commands that need it.
src/cli/shared/open-graph.js New thin wrapper over openReadonlyOrFail that returns a { db, close } pair, eliminating per-command open/close boilerplate. Straightforward and correct.
src/cli/commands/cycles.js Refactored to use openGraph(). Previously flagged close() leak on findCycles throw is fixed via try/finally. Logic is correct.
src/cli/commands/export.js Refactored to use openGraph() with try/finally. Previously flagged issues (early return inside try leaking the DB, neo4j output === undefined invariant) are resolved. The if (output === undefined) return guard correctly covers the neo4j+output case without ambiguity.
src/cli/commands/plot.js Refactored to use openGraph() with try/finally. The config-parse early-return path correctly triggers the outer finally (close() is called), and html is undefined in that path but the code using it is never reached. DB lifecycle is safe.
src/cli/commands/check.js Uses resolveQueryOpts() and explicitly passes config from the module-level import rather than via spread. qOpts.table and qOpts.csv are undefined for this command (no --table/--csv options defined), so they flow through manifesto as harmless undefined values.
src/cli/commands/diff-impact.js Removed manual config: ctx.config. The downstream domain function has an opts.config
src/cli/index.js Adds resolveQueryOpts to the shared ctx object. Minor: the postAction hook's update-check suppression covers --json but not the new --csv/--table flags (or pre-existing --ndjson). Notification still goes to stderr so stdout is not corrupted, but the inconsistency is worth tidying.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    CMD["CLI Command\n(execute)"] --> RQO["ctx.resolveQueryOpts(opts)\noptions.js"]
    RQO --> SPREAD["...qOpts spread\ninto domain fn opts"]

    CMD --> OG["openGraph(opts)\nopen-graph.js"]
    OG --> DB["db: Database\nclose: () => db.close()"]
    DB --> TRY["try { domain fn(db, ...) }\nfinally { close() }"]

    CMD --> OR["ctx.outputResult(data, field, opts)\nresult-formatter.js"]
    OR --> NDJSON{"opts.ndjson?"}
    NDJSON -->|yes| PNJ["printNdjson()"]
    NDJSON -->|no| JSON_{"opts.json?"}
    JSON_ -->|yes| PJ["JSON.stringify → stdout"]
    JSON_ -->|no| CSV_{"opts.csv?"}
    CSV_ -->|yes| PC["printCsv()\nflattenObject → escapeCsv → stdout"]
    CSV_ -->|no| TABLE_{"opts.table?"}
    TABLE_ -->|yes| PAT["printAutoTable()\nflattenObject → formatTable → stdout"]
    TABLE_ -->|no| FALSE["return false\n(caller handles output)"]

    PC --> RET_C["return true/false"]
    PAT --> RET_T["return true/false"]
Loading

Comments Outside Diff (1)

  1. src/cli/index.js, line 34 (link)

    Update-check suppression not extended to --csv / --table

    The postAction hook suppresses the update-check notification when --json is used. --ndjson was already missing the same guard before this PR, and this PR adds --csv and --table in the same category of machine-readable outputs. printUpdateNotification already writes to process.stderr (not stdout), so the notification won't corrupt piped CSV/table output — but for consistency (and to avoid the notification noise when a user is scripting with --csv), it's worth extending the guard:

Last reviewed commit: bf6f551

Comment on lines +36 to +42
function escapeCsv(val) {
const str = val == null ? '' : String(val);
if (str.includes(',') || str.includes('"') || str.includes('\n')) {
return `"${str.replace(/"/g, '""')}"`;
}
return str;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

escapeCsv missing \r check for RFC 4180
The JSDoc says "RFC 4180 CSV output", but RFC 4180 §2.6 requires quoting fields that contain CRLF (\r\n). The current check handles \n but not \r, so a bare \r in a value would pass through unquoted. Unlikely with codegraph data, but worth fixing for correctness since the doc promises RFC compliance.

Suggested change
function escapeCsv(val) {
const str = val == null ? '' : String(val);
if (str.includes(',') || str.includes('"') || str.includes('\n')) {
return `"${str.replace(/"/g, '""')}"`;
}
return str;
}
function escapeCsv(val) {
const str = val == null ? '' : String(val);
if (str.includes(',') || str.includes('"') || str.includes('\n') || str.includes('\r')) {
return `"${str.replace(/"/g, '""')}"`;
}
return str;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a28cf6f — added str.includes('\r') to the escapeCsv condition so fields containing CR are properly quoted per RFC 4180 §2.6.

const flatItems = items.map((item) =>
typeof item === 'object' && item !== null ? flattenObject(item) : { value: item },
);
const columns = autoColumns(items.filter((i) => typeof i === 'object' && i !== null));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Column detection uses unflattened items
Both printCsv (here) and printAutoTable (line 79) derive columns from autoColumns(items.filter(...)) — the unflattened original items filtered to objects only. autoColumns internally calls flattenObject, so the flattened keys are correct. However, if the array contains a mix of primitives and objects, the primitive items are mapped to { value: item } in flatItems but the value key won't appear in columns (since autoColumns only sees the objects). Those rows silently produce empty values for every column.

This won't happen with current codegraph data (always homogeneous object arrays), but if you want the helper to be truly general-purpose, consider deriving columns from flatItems instead:

const columns = autoColumns(flatItems);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a28cf6f — both printCsv and printAutoTable now derive columns from autoColumns(flatItems) instead of autoColumns(items.filter(...)), so primitive-only and mixed arrays correctly produce the value column.

…pts, universal output formatter

Add openGraph(opts) helper to eliminate DB-open/close boilerplate in
cycles, export, and plot commands.

Add resolveQueryOpts(opts) to extract the 5 repeated option fields
(noTests, json, ndjson, limit, offset) plus new table/csv into one
call. Refactor 20 command files to use the spread pattern.

Extend outputResult() with --table (auto-column aligned table) and
--csv (RFC 4180 with nested object flattening) output formats. Add
--table and --csv options to applyQueryOpts().

Impact: 31 functions changed, 71 affected
…d items

Impact: 3 functions changed, 2 affected
@carlos-alm carlos-alm force-pushed the feat/cli-composability branch from 3546120 to a28cf6f Compare March 16, 2026 10:08
@carlos-alm
Copy link
Contributor Author

@greptileai

if (columns.length === 0) columns.push('value');

const colDefs = columns.map((col) => {
const maxLen = Math.max(col.length, ...flatItems.map((item) => String(item[col] ?? '').length));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Math.max(...spread) crashes on large result sets

Math.max(col.length, ...flatItems.map(...)) passes every row's string length as a separate function argument. V8 limits the argument count to roughly 65k; beyond that this throws RangeError: Maximum call stack size exceeded. On a large codebase that returns, say, 100k symbols the --table flag would hard-crash instead of printing.

A reduce-based approach has no argument-count ceiling:

Suggested change
const maxLen = Math.max(col.length, ...flatItems.map((item) => String(item[col] ?? '').length));
const maxLen = flatItems.reduce(
(max, item) => Math.max(max, String(item[col] ?? '').length),
col.length,
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — replaced Math.max(...spread) with a reduce-based approach that has no argument-count ceiling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed in 323e704 — replaced Math.max(...spread) with a reduce-based approach that has no argument-count ceiling.

…rflow on large sets

Impact: 1 functions changed, 1 affected
@carlos-alm
Copy link
Contributor Author

@greptileai

Comment on lines +59 to +62
console.log(columns.map(escapeCsv).join(','));
for (const row of flatItems) {
console.log(columns.map((col) => escapeCsv(row[col])).join(','));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC 4180 CRLF line endings not honored

The JSDoc for printCsv says "RFC 4180 CSV output", but RFC 4180 §2 mandates CRLF (\r\n) as the record delimiter. console.log always appends a plain \n. In practice most CSV consumers tolerate LF-only files, but the JSDoc claim of RFC compliance isn't fully met.

Consider using process.stdout.write with explicit \r\n endings, or tone down the JSDoc to say "CSV output (LF line endings)":

Suggested change
console.log(columns.map(escapeCsv).join(','));
for (const row of flatItems) {
console.log(columns.map((col) => escapeCsv(row[col])).join(','));
}
const CRLF = '\r\n';
process.stdout.write(columns.map(escapeCsv).join(',') + CRLF);
for (const row of flatItems) {
process.stdout.write(columns.map((col) => escapeCsv(row[col])).join(',') + CRLF);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — toned down the JSDoc to say 'CSV output (LF line endings)' instead of claiming full RFC 4180 compliance, since console.log uses LF and changing to CRLF would break terminal output.

const flatItems = items.map((item) =>
typeof item === 'object' && item !== null ? flattenObject(item) : { value: item },
);
const columns = autoColumns(flatItems);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant double-flattening in autoColumns

flatItems are already flat (all values are primitives or JSON strings after the flattenObject call above), yet autoColumns(flatItems) passes each one through flattenObject a second time internally. The second call is a no-op on flat objects — every value is a primitive, so no recursion happens — but it does iterate every key of every row twice for large result sets.

A small, safe optimization is to derive columns directly from the already-flat items instead:

Suggested change
const columns = autoColumns(flatItems);
const columns = (() => {
const keys = new Set();
for (const item of flatItems) for (const key of Object.keys(item)) keys.add(key);
return [...keys];
})();

The same double-flattening appears on line 79 in printAutoTable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — replaced autoColumns(flatItems) with inline key extraction in both printCsv and printAutoTable, eliminating the redundant second pass through flattenObject.

@carlos-alm
Copy link
Contributor Author

@greptileai

Comment on lines +8 to +20
function flattenObject(obj, prefix = '') {
const result = {};
for (const [key, value] of Object.entries(obj)) {
const fullKey = prefix ? `${prefix}.${key}` : key;
if (value != null && typeof value === 'object' && !Array.isArray(value)) {
Object.assign(result, flattenObject(value, fullKey));
} else if (Array.isArray(value)) {
result[fullKey] = JSON.stringify(value);
} else {
result[fullKey] = value;
}
}
return result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Date (and other special objects) silently dropped by flattenObject

Date, RegExp, Map, Set, and similar built-in objects all satisfy typeof value === 'object' && !Array.isArray(value), so they are recursed into. But Object.entries(new Date()) returns [], meaning the date value is silently dropped from the flattened output — no key is emitted at all, rather than even a fallback string like "2024-01-01T00:00:00.000Z".

For codegraph's current SQLite-backed domain objects this is unlikely to matter, but since flattenObject is now a general-purpose utility it's worth guarding against it with an explicit prototype check:

function flattenObject(obj, prefix = '') {
  const result = {};
  for (const [key, value] of Object.entries(obj)) {
    const fullKey = prefix ? `${prefix}.${key}` : key;
    if (value !== null && typeof value === 'object'
        && !Array.isArray(value)
        && Object.getPrototypeOf(value) === Object.prototype) {
      Object.assign(result, flattenObject(value, fullKey));
    } else if (Array.isArray(value)) {
      result[fullKey] = JSON.stringify(value);
    } else {
      result[fullKey] = value;
    }
  }
  return result;
}

This ensures Date, RegExp, etc. fall through to the else branch and are stored as their string representation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added a plain-object prototype check so only POJOs are recursed into. Date, RegExp, Map, Set etc. now fall through to the else branch and are stored as their string representation.

@carlos-alm
Copy link
Contributor Author

@greptileai

Comment on lines +88 to +91
const isNumeric = flatItems.every((item) => {
const v = item[col];
return v == null || v === '' || Number.isFinite(Number(v));
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Booleans misclassified as numeric columns

Number.isFinite(Number(true)) evaluates to true (since Number(true) is 1), so boolean-valued columns — such as exported, isTest, or hasBody that are common in codegraph domain objects — are classified as numeric and rendered right-aligned. This makes the table visually misleading since true/false reads more naturally left-aligned.

Suggested change
const isNumeric = flatItems.every((item) => {
const v = item[col];
return v == null || v === '' || Number.isFinite(Number(v));
});
const isNumeric = flatItems.every((item) => {
const v = item[col];
return v == null || v === '' || (typeof v !== 'boolean' && Number.isFinite(Number(v)));
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 026d923. Added the \ guard before \ so booleans are no longer misclassified as numeric columns.

Comment on lines +15 to +20
const { db, close } = openGraph(opts);
const cycles = findCycles(db, {
fileLevel: !opts.functions,
noTests: ctx.resolveNoTests(opts),
});
db.close();
close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close() not called if findCycles throws

If findCycles raises an unhandled exception, the close() call on line 20 is never reached and the database connection is leaked for the lifetime of the process. better-sqlite3 will eventually reclaim it via GC, but it's inconsistent with the explicit-cleanup pattern this PR establishes via openGraph.

Note: the old code had the same risk with db.close(), so this is not a regression — but if the intent of openGraph is to make lifecycle management uniform across commands, wrapping the body in a try/finally (as is done in the plot.js error path for the config read) would make cycles.js consistent:

const { db, close } = openGraph(opts);
let cycles;
try {
  cycles = findCycles(db, {
    fileLevel: !opts.functions,
    noTests: ctx.resolveNoTests(opts),
  });
} finally {
  close();
}

The same pattern applies to export.js if any export function throws before the close() at line 66.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 026d923. Wrapped findCycles in try/finally so close() is always called. Also applied the same fix to export.js (the switch block could throw before reaching close()) and plot.js (same pattern — generatePlotHTML could throw, leaking the DB handle).

…ly for close()

- result-formatter: skip boolean values in numeric column heuristic
  (Number(true) === 1 was misclassifying boolean columns as numeric)
- cycles/export/plot commands: wrap DB operations in try/finally to
  ensure close() is called even if an export function throws

Impact: 4 functions changed, 1 affected
…degraph into feat/cli-composability

Impact: 11 functions changed, 72 affected
@carlos-alm
Copy link
Contributor Author

@greptileai

Comment on lines +46 to +48
const flatItems = items.map((item) =>
typeof item === 'object' && item !== null ? flattenObject(item) : { value: item },
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top-level array items routed to flattenObject instead of JSON-stringified

Both printCsv and printAutoTable use the same guard:

typeof item === 'object' && item !== null ? flattenObject(item) : { value: item }

Since typeof [] === 'object' is true, an items entry that is itself an array (e.g. a query that returns [[1, 2], [3, 4]]) passes the branch and goes into flattenObject. Object.entries([1, 2]) yields [["0", 1], ["1", 2]], so the flattened object gets numeric string keys — the CSV/table ends up with columns named 0, 1, etc. rather than a single value column containing the serialised array.

This is internally inconsistent: array values nested inside an object are correctly JSON-stringified (the else if (Array.isArray(value)) branch inside flattenObject), but top-level array items are not.

The fix is the same !Array.isArray(item) guard used inside flattenObject:

Suggested change
const flatItems = items.map((item) =>
typeof item === 'object' && item !== null ? flattenObject(item) : { value: item },
);
const flatItems = items.map((item) =>
typeof item === 'object' && item !== null && !Array.isArray(item)
? flattenObject(item)
: { value: item },
);

The same change is needed on the identical line inside printAutoTable (line 73).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added !Array.isArray(item) guard in both printCsv and printAutoTable so top-level array items are treated as primitives (JSON-stringified via the value branch) instead of being flattened into numeric-key columns.

Comment on lines +42 to +59
function printCsv(data, field) {
const items = field ? data[field] : data;
if (!Array.isArray(items) || items.length === 0) return;

const flatItems = items.map((item) =>
typeof item === 'object' && item !== null ? flattenObject(item) : { value: item },
);
const columns = (() => {
const keys = new Set();
for (const item of flatItems) for (const key of Object.keys(item)) keys.add(key);
return [...keys];
})();
if (columns.length === 0) columns.push('value');

console.log(columns.map(escapeCsv).join(','));
for (const row of flatItems) {
console.log(columns.map((col) => escapeCsv(row[col])).join(','));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

printCsv silently emits nothing (not even a header) for empty result sets

When items.length === 0 the function returns before printing anything — no header row, no rows. outputResult still returns true, so the caller's fallback (e.g. formatStructure(data) or a "no results" message) is suppressed. The user sees no output at all.

This is different from --json, which always prints the full data object (even {}). It is also surprising for downstream CSV consumers: an empty CSV file carries no schema information, while a header-only file does.

Consider emitting the header even when there are no rows:

function printCsv(data, field) {
  const items = field ? data[field] : data;
  if (!Array.isArray(items)) return;

  const flatItems = items.map((item) =>
    typeof item === 'object' && item !== null && !Array.isArray(item)
      ? flattenObject(item)
      : { value: item },
  );
  const columns = (() => {
    const keys = new Set();
    for (const item of flatItems) for (const key of Object.keys(item)) keys.add(key);
    return [...keys];
  })();
  if (columns.length === 0) columns.push('value');

  console.log(columns.map(escapeCsv).join(','));
  for (const row of flatItems) {
    console.log(columns.map((col) => escapeCsv(row[col])).join(','));
  }
}

If the intent is truly "nothing for empty", then outputResult should return false so the caller can present its own empty-state message instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — removed the items.length === 0 early return so an empty result set still emits a header-only CSV row, consistent with schema-aware CSV consumers.

…empty sets

Impact: 2 functions changed, 1 affected
@carlos-alm
Copy link
Contributor Author

@greptileai

Comment on lines +71 to +73
function printAutoTable(data, field) {
const items = field ? data[field] : data;
if (!Array.isArray(items) || items.length === 0) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--table on empty results suppresses all output silently

printAutoTable returns early when items.length === 0 without printing anything, but outputResult still returns true — telling every caller that output was "handled". This means any fallback path (e.g. console.log(formatStructure(data)) in structure.js, or the "no results" message in other commands) is unconditionally suppressed. Running codegraph where nonexistent --table produces zero output with no indication of why.

This is inconsistent with the --csv path: after the fix in this PR, printCsv no longer has the items.length === 0 guard and always emits at least a header row.

Consider printing at least the column headers (or a "No results." message) so the caller's return true is meaningful:

Suggested change
function printAutoTable(data, field) {
const items = field ? data[field] : data;
if (!Array.isArray(items) || items.length === 0) return;
function printAutoTable(data, field) {
const items = field ? data[field] : data;
if (!Array.isArray(items)) return;
const flatItems = items.map((item) =>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — removed the items.length === 0 early return in printAutoTable, consistent with the CSV path. Empty result sets now emit a header-only table instead of suppressing all output.

@carlos-alm
Copy link
Contributor Author

@greptileai

Comment on lines +127 to +134
if (opts.csv) {
printCsv(data, field);
return true;
}
if (opts.table) {
printAutoTable(data, field);
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outputResult swallows fallback when items is not an array

Both printCsv (line 44) and printAutoTable (line 73) contain an early-return guard:

if (!Array.isArray(items)) return;

When items is not an array (e.g. data[field] is undefined or a non-array object), these functions return without printing anything. However, outputResult always returns true after calling them, telling every caller that output was handled. This suppresses the fallback path (e.g. console.log(formatStructure(data))) silently — the user sees nothing.

A minimal fix is to propagate the return value:

function printCsv(data, field) {
  const items = field ? data[field] : data;
  if (!Array.isArray(items)) return false;   // <-- signal failure
  // ...
  return true;
}

// in outputResult:
if (opts.csv) {
  return printCsv(data, field) !== false;
}
if (opts.table) {
  return printAutoTable(data, field) !== false;
}

The same change is needed in printAutoTable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in bf6f551. printCsv and printAutoTable now return false when items is not an array (and true on success). outputResult checks the return value with !== false so callers see the fallback correctly.

Comment on lines +41 to +52
export function resolveQueryOpts(opts) {
return {
noTests: resolveNoTests(opts),
json: opts.json,
ndjson: opts.ndjson,
table: opts.table,
csv: opts.csv,
limit: opts.limit ? parseInt(opts.limit, 10) : undefined,
offset: opts.offset ? parseInt(opts.offset, 10) : undefined,
config,
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config now injected into every domain call via spread

resolveQueryOpts unconditionally returns config. Because many command files now use ...ctx.resolveQueryOpts(opts) as a full spread, config is injected into every domain function's options object — including functions (fileDeps, impactAnalysis, where, fileExports, etc.) that previously did not receive it.

This is a silent behavioural change: if any of those domain functions inspect opts.config (or newly added helper functions later do), they will now receive the global config even when no explicit config argument was intended by the caller. Functions that previously opted in to config (like audit, check, diffImpact) will continue to work correctly, but the coupling is now implicit for all others.

Consider removing config from resolveQueryOpts and having the handful of commands that need it pass it explicitly (as they did before), to preserve the opt-in behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in bf6f551. Removed config from the object returned by resolveQueryOpts, so it no longer leaks into every domain call via spread. The two commands that need it (audit, check) now import config directly from ../shared/options.js and pass it explicitly. diff-impact spreads resolveQueryOpts but the downstream domain/analysis/impact.js already has a opts.config || loadConfig(repoRoot) fallback, so removing config from the spread is safe there.

Comment on lines +49 to +57
case 'neo4j': {
const csv = exportNeo4jCSV(db, exportOpts);
if (opts.output) {
const base = opts.output.replace(/\.[^.]+$/, '') || opts.output;
fs.writeFileSync(`${base}-nodes.csv`, csv.nodes, 'utf-8');
fs.writeFileSync(`${base}-relationships.csv`, csv.relationships, 'utf-8');
console.log(`Exported to ${base}-nodes.csv and ${base}-relationships.csv`);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close() called before neo4j file-write completes — then return bypasses output check

When opts.format === 'neo4j' and opts.output is set, the code calls return inside the try block (line 56). finally { close(); } correctly fires before the function exits. However, output remains undefined after the try/finally block, and the code below:

if (opts.output) {
  fs.writeFileSync(opts.output, output, 'utf-8');

…would write undefined to disk if the return on line 56 were ever removed or the control flow changed. While this is currently safe due to the return, the invariant that output is always a string after the try block is silently broken for this branch.

A defensive approach is to move the return outside the try/finally entirely or add an explicit output !== undefined guard after the block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in bf6f551. Removed the early return inside the neo4j try block. The neo4j+output case now writes files and leaves output as undefined, while the neo4j+stdout case sets output normally. After the try/finally (which always calls close()), an explicit if (output === undefined) return guard skips the output block when it was already handled inside the switch. This makes the control flow unambiguous — close() always runs via finally, and the post-block output logic only runs when output was actually set.

- Make printCsv/printAutoTable return false when items is not an array,
  and propagate that in outputResult so callers see the failure
- Remove config from resolveQueryOpts spread; audit and check now import
  config directly, other consumers use their existing loadConfig fallback
- Restructure export neo4j branch to avoid return inside try/finally;
  use output===undefined guard after the block instead
@carlos-alm
Copy link
Contributor Author

@greptileai

Comment on lines +93 to +96
const isNumeric = flatItems.every((item) => {
const v = item[col];
return v == null || v === '' || (typeof v !== 'boolean' && Number.isFinite(Number(v)));
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vacuous isNumeric right-aligns fallback value column on empty result sets

When flatItems is empty, flatItems.every(predicate) returns true vacuously — making isNumeric = true and producing a right-aligned header for the synthetic value column. Every non-empty string-result query correctly renders its columns left-aligned, so codegraph where nonexistent --table would produce a conspicuously right-padded value heading that is inconsistent with the actual column layout users would see on any successful query.

A minimal guard:

Suggested change
const isNumeric = flatItems.every((item) => {
const v = item[col];
return v == null || v === '' || (typeof v !== 'boolean' && Number.isFinite(Number(v)));
});
const isNumeric = flatItems.length > 0 && flatItems.every((item) => {
const v = item[col];
return v == null || v === '' || (typeof v !== 'boolean' && Number.isFinite(Number(v)));
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant