refactor: split cli.js into self-contained command modules (ROADMAP 3.6)#427
refactor: split cli.js into self-contained command modules (ROADMAP 3.6)#427carlos-alm merged 8 commits intomainfrom
Conversation
… (ROADMAP 3.6)
Move 1,525 lines of inline Commander chains from src/cli.js into
src/cli/ with auto-discovery of command modules:
- src/cli/index.js: Commander setup + registerCommand() + discoverCommands()
- src/cli/commands/: 40 independently testable command modules
- src/cli/shared/: options.js (query opts, resolveNoTests) + output.js
- src/cli.js: thin 5-line wrapper calling src/cli/index.js
Each command exports { name, description, options?, queryOpts?, validate?, execute }
and is auto-registered at startup. Subcommands (registry, snapshot) nest naturally.
Impact: 68 functions changed, 4 affected
|
Claude finished @carlos-alm's task —— View job PR Review: CLI Refactor AnalysisCompleted comprehensive analysis of this major CLI refactor that splits 1,525 lines into 40 modular commands. Root Cause AnalysisProblem Being Solved: The monolithic Why This Approach: Auto-discovery pattern with standardized command interfaces enables:
Risk Assessment: LOW RISK - This is a pure architectural refactor with no behavioral changes. Critical Concerns: NONE FOUND ✅✅ No configuration relaxations - biome.json, vitest.config.js unchanged Architecture AnalysisStrong Design Patterns:
Implementation Quality:
Test Coverage Analysis:
Code Quality ReviewStrengths:
Minor Observations:
Security & Performance:
Final RecommendationThis PR represents exceptional refactoring work that significantly improves codebase maintainability while preserving all existing functionality. |
Greptile SummaryThis PR completes ROADMAP 3.6 by splitting the 1,525-line monolithic Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["src/cli.js\n(5-line wrapper)"] --> B["run() — src/cli/index.js"]
B --> C["discoverCommands()\nreaddirSync(commands/).sort()"]
C --> D["import(pathToFileURL(file))"]
D --> E{mod.command\nor mod.commands?}
E -->|"mod.command"| F["registerCommand(program, def)"]
E -->|"mod.commands[]"| F
F --> G{def.queryOpts?}
G -->|yes| H["applyQueryOpts(cmd)\nsrc/cli/shared/options.js"]
G -->|no| I["cmd.option(...) for\neach def.options entry"]
H --> I
I --> J{def.execute?}
J -->|yes| K["cmd.action()\nargCount = positional arg count\nargs = actionArgs.slice(0, argCount)\nopts = actionArgs[argCount]"]
J -->|no| L["(no action — subcommands only)"]
K --> M["def.validate?(args, opts, ctx)\n→ error string or falsy"]
M -->|error| N["console.error + process.exit(1)"]
M -->|ok| O["def.execute(args, opts, ctx)"]
L --> P{def.subcommands?}
J -->|no| P
P -->|yes| Q["registerCommand(cmd, sub)\nfor each subcommand"]
B --> R["program.parse()"]
O --> S["ctx = \{ config, resolveNoTests,\nformatSize, outputResult, program \}"]
Last reviewed commit: 3fa03bd |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| kind: opts.kind, | ||
| filePattern: opts.file, | ||
| rrfK: parseInt(opts.rrfK, 10), | ||
| mode: opts.mode, | ||
| json: opts.json, | ||
| }); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
offset and ndjson options declared but never forwarded
Both --offset and --ndjson are listed in the command's options array (lines 21–22) but are not included in the object passed to search(). This means any user relying on these flags in the original monolithic cli.js will silently get no pagination or newline-delimited output.
| kind: opts.kind, | |
| filePattern: opts.file, | |
| rrfK: parseInt(opts.rrfK, 10), | |
| mode: opts.mode, | |
| json: opts.json, | |
| }); | |
| }, | |
| }; | |
| await search(query, opts.db, { | |
| limit: parseInt(opts.limit, 10), | |
| offset: opts.offset ? parseInt(opts.offset, 10) : undefined, | |
| noTests: ctx.resolveNoTests(opts), | |
| minScore: parseFloat(opts.minScore), | |
| model: opts.model, | |
| kind: opts.kind, | |
| filePattern: opts.file, | |
| rrfK: parseInt(opts.rrfK, 10), | |
| mode: opts.mode, | |
| json: opts.json, | |
| ndjson: opts.ndjson, | |
| }); |
There was a problem hiding this comment.
Fixed in e0a0af0 — offset and ndjson are now forwarded to search().
| } | ||
|
|
||
| /** | ||
| * Auto-discover and register all command modules from src/cli/commands/. | ||
| * Each module must export a `command` (single definition) or `commands` (array). | ||
| */ | ||
| async function discoverCommands() { | ||
| const commandsDir = path.join(__cliDir, 'commands'); |
There was a problem hiding this comment.
readdirSync result is not sorted — command order is filesystem-dependent
fs.readdirSync returns entries in filesystem-defined order, which is creation order on Linux ext4 but alphabetical on macOS HFS+ and Windows NTFS. The original monolithic cli.js had a fixed, deterministic registration order. Without an explicit .sort() here the --help command listing may differ between developer machines and CI, making output order non-deterministic in a multi-platform repo.
| } | |
| /** | |
| * Auto-discover and register all command modules from src/cli/commands/. | |
| * Each module must export a `command` (single definition) or `commands` (array). | |
| */ | |
| async function discoverCommands() { | |
| const commandsDir = path.join(__cliDir, 'commands'); | |
| const files = fs.readdirSync(commandsDir).filter((f) => f.endsWith('.js')).sort(); |
There was a problem hiding this comment.
Fixed in e0a0af0 — .sort() added to readdirSync result for deterministic command order.
- Pass offset and ndjson options through to search() so the CLI flags actually take effect - Sort readdirSync result in discoverCommands() for deterministic command ordering across platforms Impact: 2 functions changed, 37 affected
Impact: 1 functions changed, 1 affected
|
Fixed both Greptile findings in cbfb3fe:
|
src/cli/commands/embed.js
Outdated
| }, | ||
| async execute([dir], opts, ctx) { | ||
| const root = path.resolve(dir || '.'); | ||
| const model = opts.model || ctx.config.embeddings?.model; |
There was a problem hiding this comment.
DEFAULT_MODEL fallback silently removed
The original cli.js resolved the model with an explicit three-part fallback:
const model = opts.model || config.embeddings?.model || DEFAULT_MODEL;The new code drops the || DEFAULT_MODEL guard, so when neither --model is passed nor embeddings.model is set in config, model becomes undefined. buildEmbeddings still works correctly because getModelConfig(undefined) falls back to DEFAULT_MODEL internally — but this now relies on an undocumented implementation detail of embedder.js. Consider restoring the explicit fallback to keep the call site self-documenting and resilient to future changes in buildEmbeddings's internal defaults:
| const model = opts.model || ctx.config.embeddings?.model; | |
| const model = opts.model || ctx.config.embeddings?.model || DEFAULT_MODEL; |
(and re-add DEFAULT_MODEL to the import on line 2: import { buildEmbeddings, DEFAULT_MODEL, EMBEDDING_STRATEGIES } from '../../embedder.js';)
There was a problem hiding this comment.
Fixed in 672c261 — restored explicit DEFAULT_MODEL fallback and re-added the import.
Restores the three-part model resolution (--model || config || DEFAULT_MODEL) that was present in the original cli.js, so the call site doesn't silently depend on getModelConfig's internal default. Impact: 1 functions changed, 0 affected
|
Fixed in 672c261: restored explicit |
src/cli/commands/dataflow.js
Outdated
| limit: opts.limit ? parseInt(opts.limit, 10) : undefined, | ||
| offset: opts.offset ? parseInt(opts.offset, 10) : undefined, | ||
| impact: opts.impact, | ||
| depth: opts.depth, |
There was a problem hiding this comment.
depth forwarded as a string instead of a number
Every other command that exposes --depth converts it in the CLI layer with parseInt(opts.depth, 10) before passing it down (e.g. fn-impact.js:20, sequence.js:21, query.js:38). Here the raw Commander string ('5') is passed instead.
This is functionally safe today because commands/dataflow.js → dataflowImpact() explicitly handles the conversion with Number(opts.depth), and dataflowData() doesn't use depth at all. However, it's an inconsistency that could silently break if dataflowImpactData is ever refactored to consume depth directly (it currently does const maxDepth = opts.depth || 5, which would then receive a string).
| depth: opts.depth, | |
| depth: parseInt(opts.depth, 10), |
There was a problem hiding this comment.
Fixed in e6518de — depth is now parsed with parseInt(opts.depth, 10) for consistency.
Impact: 1 functions changed, 0 affected
|
Fixed in e6518de: |
src/cli.js
Outdated
| }); | ||
|
|
||
| program.parse(); | ||
| run(); |
There was a problem hiding this comment.
Unhandled promise rejection on startup failure
run() is called without .catch(). If discoverCommands() fails — for example because a new command file has a syntax error or a top-level import throws — the error propagates as an unhandled promise rejection. In Node.js 20+ this terminates the process, but the message the user sees is a raw UnhandledPromiseRejection stack dump rather than a clean codegraph error. The original synchronous code would have surfaced a clean module-loading error.
| run(); | |
| run().catch((err) => { | |
| console.error(`codegraph: fatal error — ${err.message || err}`); | |
| process.exit(1); | |
| }); |
There was a problem hiding this comment.
Fixed in 3fa03bd — run() now has a .catch() handler that prints a clean error message and exits with code 1.
1 participant
Summary
src/cli.jsintosrc/cli/with auto-discovery of command modulessrc/cli/commands/, each exporting{ name, description, options?, queryOpts?, validate?, execute }src/cli/shared/(query options, output formatting)src/cli/index.jsprovidesregisterCommand()+discoverCommands()— new commands are added by dropping a file intocommands/src/cli.jsreduced to a 5-line thin wrapperTest plan
codegraph --helplists all 40 commands correctlycodegraph --versionworks