feat(types): migrate CLI layer to TypeScript (48 files)#580
feat(types): migrate CLI layer to TypeScript (48 files)#580carlos-alm merged 11 commits intomainfrom
Conversation
Migrate cli.js, cli/index.js, cli/shared/*, and all 43 cli/commands/* from JavaScript to TypeScript as part of Phase 5.5. - Add CommandDefinition, CliContext, and CommandOpts types (cli/types.ts) - Update discoverCommands() to discover both .ts and .js files with dedup - Use bracket notation for Commander opts() per noPropertyAccessFromIndexSignature - Cast typed arrays (SymbolKind[], Role[]) to readonly string[] for .includes()
|
Claude finished @carlos-alm's task —— View job Reviewing TypeScript migration PRI've conducted a comprehensive review of this TypeScript migration PR following strict review criteria. Root Cause AnalysisProblem Being Solved: Migrating the CLI layer (48 files) from JavaScript to TypeScript to improve type safety and developer experience. Critical FindingsPhase 2 - Configuration Changes: ✅ CLEAN
Phase 3 - Test Integrity: ✅ ACCEPTABLECannot verify test execution (permission restrictions), but PR claims 107/110 tests pass with 3 pre-existing failures Phase 4 - Code Quality Issues:
|
Greptile SummaryThis PR migrates all 48 CLI-layer files from JavaScript to TypeScript, adding
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["cli.ts\nentry point"] --> B["cli/index.ts\nrun()"]
B --> C["discoverCommands()\nscans commands/\nprefers .ts over .js"]
C --> D["registerCommand()\nattaches Commander options\nand action handler"]
D --> E["CliContext\n{ config, resolveNoTests,\nresolveQueryOpts, program, … }"]
E --> F["CommandDefinition\n{ name, options, validate, execute }"]
F --> G["cli/commands/*.ts\n43 command modules"]
F --> H["cli/commands/registry.ts\ncli/commands/snapshot.ts\nsubcommands"]
G --> I["cli/shared/options.ts\napplyQueryOpts()"]
G --> J["cli/shared/open-graph.ts\nopenGraph()"]
G --> K["cli/shared/output.ts\noutputResult()"]
style A fill:#d0e8ff
style C fill:#ffe0b0
style E fill:#d4edda
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/cli/commands/build.ts
Outdated
| incremental: opts.incremental, | ||
| ast: opts.ast, | ||
| complexity: opts.complexity, | ||
| engine, | ||
| dataflow: opts.dataflow, | ||
| cfg: opts.cfg, | ||
| }); | ||
| } as any); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
--no-complexity and --no-cfg flags silently dropped
The original build.js passed complexity: opts.complexity and cfg: opts.cfg to buildGraph, but the TypeScript port omits both. As a result, codegraph build --no-complexity and codegraph build --no-cfg are accepted by Commander but have no effect — the flags are silently ignored at the feature level.
The original JS:
await buildGraph(root, {
incremental: opts.incremental,
ast: opts.ast,
complexity: opts.complexity,
engine,
dataflow: opts.dataflow,
cfg: opts.cfg,
});Suggested fix:
| incremental: opts.incremental, | |
| ast: opts.ast, | |
| complexity: opts.complexity, | |
| engine, | |
| dataflow: opts.dataflow, | |
| cfg: opts.cfg, | |
| }); | |
| } as any); | |
| }, | |
| }; | |
| await buildGraph(root, { | |
| incremental: opts.incremental, | |
| ast: opts.ast, | |
| complexity: opts.complexity, | |
| engine, | |
| dataflow: opts.dataflow, | |
| cfg: opts.cfg, | |
| } as any); |
There was a problem hiding this comment.
Fixed — restored complexity: opts.complexity and cfg: opts.cfg in the buildGraph call. These were present in the original JS but dropped during the TypeScript port.
| throw new ConfigError('Invalid --weights JSON', { cause: err as Error }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
any instead of defined types
runHotspots uses opts: any and ctx: any even though CommandOpts and CliContext are already available via the file's imports (through CommandDefinition). The same pattern appears in check.ts for runManifesto(opts: any, qOpts: any). These defeat the purpose of the TypeScript migration — the existing types cover these parameters exactly.
For triage.ts:
| async function runHotspots(opts: CommandOpts, ctx: CliContext): Promise<void> { |
For check.ts line 13:
async function runManifesto(opts: CommandOpts, qOpts: CommandOpts): Promise<void> {You'll also need to add the missing CommandOpts and CliContext imports in each file.
Same issue also appears at src/cli/commands/check.ts:13.
There was a problem hiding this comment.
Fixed — replaced \ and \ with \ and \ in \ (triage.ts), \ (check.ts), \ (triage.ts), and \ (check.ts). Also fixed the options spread cast in index.ts to use \ so TypeScript preserves the third tuple element.
…#580) The TypeScript port omitted complexity and cfg option pass-throughs to buildGraph, making the flags silently ineffective.
Use the defined type aliases in internal helpers (runHotspots, runManifesto, validateFilters, validateKind) and fix the options spread cast to preserve the third tuple element for TypeScript.
|
Addressed Claude's review feedback:
Note: The remaining |
|
Claude finished @carlos-alm's task —— View job Reviewing TypeScript Migration FixesAnalyzed the fixes you implemented based on previous review feedback. Todo List:
Verification Results✅ P1 Regression Fixed - build.ts:21-24 complexity: opts.complexity,
cfg: opts.cfg,This restores the functionality of ✅ Type Improvements Applied
✅ Options Spread Cast Fixed - index.ts:73 cmd.option(...(opt as Parameters<Command['option']>));This preserves TypeScript's ability to type-check all tuple elements, including third arguments like default values and collector functions. Assessment SummaryExcellent work! All the critical issues identified in the previous reviews have been properly addressed:
The fixes are clean, targeted, and maintain the migration's overall quality. Your note about remaining Updated Recommendation
This PR successfully migrates the CLI layer to TypeScript with meaningful type safety improvements while maintaining full functionality. |
src/cli/index.ts
Outdated
| import { outputResult } from './shared/output.js'; | ||
| import type { CliContext, CommandDefinition, CommandOpts } from './types.js'; | ||
|
|
||
| const __cliDir = path.dirname(new URL(import.meta.url).pathname.replace(/^\/([A-Z]:)/i, '$1')); |
There was a problem hiding this comment.
fileURLToPath instead of manual regex for Windows path fix
new URL(import.meta.url).pathname returns a URL-encoded string. The manual .replace(/^\/([A-Z]:)/i, '$1') only strips the leading slash on Windows drive paths, but it does not decode percent-encoded characters. If the repo is checked out inside a path with a space or any other URL-special character (e.g. /home/user/my project/...), .pathname yields /home/user/my%20project/..., and the subsequent fs.readdirSync(commandsDir) call will fail with ENOENT because the OS sees %20 as literal characters.
fileURLToPath (already available in node:url) handles both the Windows drive letter stripping and percent-decoding correctly.
| const __cliDir = path.dirname(new URL(import.meta.url).pathname.replace(/^\/([A-Z]:)/i, '$1')); | |
| const __cliDir = path.dirname(fileURLToPath(import.meta.url)); |
Then update the import line to also pull in fileURLToPath:
import { fileURLToPath, pathToFileURL } from 'node:url';There was a problem hiding this comment.
Fixed — replaced manual regex with fileURLToPath(import.meta.url) which handles both Windows drive letters and percent-encoded characters correctly.
| .filter((f) => !f.endsWith('.d.ts')) | ||
| .sort(); | ||
|
|
||
| // Deduplicate: prefer .ts over .js when both exist |
There was a problem hiding this comment.
Array.includes inside a loop — convert to a Set for O(n) deduplication
files.includes(tsVariant) scans the entire sorted array on every iteration. With 43 command files this is harmless today, but the idiom is needlessly quadratic and will regress if the command count ever grows or the check is called more frequently.
Convert files to a Set<string> once before the loop:
| .filter((f) => !f.endsWith('.d.ts')) | |
| .sort(); | |
| // Deduplicate: prefer .ts over .js when both exist | |
| const fileSet = new Set(files); | |
| const seen = new Set<string>(); | |
| const deduped: string[] = []; | |
| for (const file of files) { | |
| const base = file.replace(/\.[jt]s$/, ''); | |
| if (!seen.has(base)) { | |
| seen.add(base); | |
| const tsVariant = `${base}.ts`; | |
| deduped.push(fileSet.has(tsVariant) ? tsVariant : file); | |
| } | |
| } |
There was a problem hiding this comment.
Fixed — converted to a Set-based deduplication approach, eliminating the O(n²) Array.includes scan.
src/cli/commands/build.ts
Outdated
| incremental: opts.incremental, | ||
| ast: opts.ast, | ||
| complexity: opts.complexity, | ||
| engine, | ||
| dataflow: opts.dataflow, | ||
| cfg: opts.cfg, | ||
| }); | ||
| } as any); | ||
| }, |
There was a problem hiding this comment.
as any cast on buildGraph options nullifies the type safety of the corrected fields
The options object now correctly includes complexity and cfg (restored from the JS original), but the trailing as any cast silently suppresses any future type mismatch for the whole object. If buildGraph's option signature ever changes (new required field, renamed key, etc.), TypeScript won't catch it here.
Consider typing the options against buildGraph's parameter type, or at minimum remove the cast and let the compiler validate the shape:
| incremental: opts.incremental, | |
| ast: opts.ast, | |
| complexity: opts.complexity, | |
| engine, | |
| dataflow: opts.dataflow, | |
| cfg: opts.cfg, | |
| }); | |
| } as any); | |
| }, | |
| await buildGraph(root, { | |
| incremental: opts.incremental as boolean, | |
| ast: opts.ast as boolean, | |
| complexity: opts.complexity as boolean, | |
| engine: engine as string, | |
| dataflow: opts.dataflow as boolean, | |
| cfg: opts.cfg as boolean, | |
| }); |
There was a problem hiding this comment.
Fixed — removed the \ cast and applied individual field casts so TypeScript can validate the shape against buildGraph's signature.
| const { startMCPServer } = await import('../../mcp/index.js'); | ||
| const mcpOpts = {}; | ||
| const mcpOpts: any = {}; | ||
| mcpOpts.multiRepo = opts.multiRepo || !!opts.repos; | ||
| if (opts.repos) { | ||
| mcpOpts.allowedRepos = opts.repos.split(',').map((s) => s.trim()); | ||
| mcpOpts.allowedRepos = (opts.repos as string).split(',').map((s) => s.trim()); | ||
| } | ||
| await startMCPServer(opts.db, mcpOpts); | ||
| await startMCPServer(opts.db as string | undefined, mcpOpts); |
There was a problem hiding this comment.
mcpOpts: any — use an inline typed object instead of a mutable any bag
Building options by mutating a bare any object hides the shape from the reader and from TypeScript. Since startMCPServer's second parameter type is presumably importable, constructing an inline object literal would be cleaner and avoid the need for the any annotation entirely:
| const { startMCPServer } = await import('../../mcp/index.js'); | |
| const mcpOpts = {}; | |
| const mcpOpts: any = {}; | |
| mcpOpts.multiRepo = opts.multiRepo || !!opts.repos; | |
| if (opts.repos) { | |
| mcpOpts.allowedRepos = opts.repos.split(',').map((s) => s.trim()); | |
| mcpOpts.allowedRepos = (opts.repos as string).split(',').map((s) => s.trim()); | |
| } | |
| await startMCPServer(opts.db, mcpOpts); | |
| await startMCPServer(opts.db as string | undefined, mcpOpts); | |
| const mcpOpts = { | |
| multiRepo: opts.multiRepo || !!opts.repos, | |
| allowedRepos: opts.repos ? (opts.repos as string).split(',').map((s) => s.trim()) : undefined, | |
| }; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — replaced mutable any bag with a typed inline object literal.
- Use fileURLToPath instead of manual regex for Windows path decoding - Convert Array.includes to Set lookup in command deduplication - Remove as any cast on buildGraph options, use individual field casts - Replace mutable any bag with typed inline object in MCP command
src/cli/commands/search.ts
Outdated
| limit: parseInt(opts.limit as string, 10), | ||
| offset: opts.offset ? parseInt(opts.offset as string, 10) : undefined, | ||
| noTests: ctx.resolveNoTests(opts), | ||
| minScore: parseFloat(opts.minScore as string), | ||
| model: opts.model as string | undefined, | ||
| kind: opts.kind as string | undefined, | ||
| filePattern, | ||
| rrfK: parseInt(opts.rrfK as string, 10), | ||
| mode: opts.mode as 'hybrid' | 'semantic' | 'keyword' | undefined, | ||
| json: opts.json as boolean | undefined, | ||
| ndjson: opts.ndjson as boolean | undefined, |
There was a problem hiding this comment.
as any cast on search options suppresses type checking
The trailing } as any, cast on the entire options object passed to search() silently bypasses TypeScript validation on this call site — the same anti-pattern that was corrected in build.ts during this migration. If search()'s option signature ever grows a new required field or renames an existing one, this cast will prevent TypeScript from surfacing the mismatch.
Consider removing the cast and instead applying individual field casts for the properties that genuinely require them (e.g. mode and json), similar to how build.ts was fixed:
| limit: parseInt(opts.limit as string, 10), | |
| offset: opts.offset ? parseInt(opts.offset as string, 10) : undefined, | |
| noTests: ctx.resolveNoTests(opts), | |
| minScore: parseFloat(opts.minScore as string), | |
| model: opts.model as string | undefined, | |
| kind: opts.kind as string | undefined, | |
| filePattern, | |
| rrfK: parseInt(opts.rrfK as string, 10), | |
| mode: opts.mode as 'hybrid' | 'semantic' | 'keyword' | undefined, | |
| json: opts.json as boolean | undefined, | |
| ndjson: opts.ndjson as boolean | undefined, | |
| await search( | |
| query!, | |
| opts.db as string | undefined, | |
| { | |
| limit: parseInt(opts.limit as string, 10), | |
| offset: opts.offset ? parseInt(opts.offset as string, 10) : undefined, | |
| noTests: ctx.resolveNoTests(opts), | |
| minScore: parseFloat(opts.minScore as string), | |
| model: opts.model as string | undefined, | |
| kind: opts.kind as string | undefined, | |
| filePattern, | |
| rrfK: parseInt(opts.rrfK as string, 10), | |
| mode: opts.mode as 'hybrid' | 'semantic' | 'keyword' | undefined, | |
| json: opts.json as boolean | undefined, | |
| ndjson: opts.ndjson as boolean | undefined, | |
| }, | |
| ); |
There was a problem hiding this comment.
Fixed — removed the trailing as any cast and dropped the dead offset and ndjson properties (neither field exists in SearchOpts and they were silently ignored). Each remaining property already has an individual field cast, matching the pattern used in build.ts.
|
|
||
| const batchOpts = { | ||
| depth: opts.depth ? parseInt(opts.depth, 10) : undefined, | ||
| depth: opts.depth ? parseInt(opts.depth as string, 10) : undefined, | ||
| file: opts.file, | ||
| kind: opts.kind, |
There was a problem hiding this comment.
isMulti check and as any casts weaken multi-batch handling
The isMulti discriminator relies on (targets[0] as any).command — an unguarded property access on an unknown[] element. If the format of multi-batch input ever changes (e.g. the discriminating key is renamed), TypeScript will not catch the mismatch here.
Similarly, multiBatchData(targets as any, ...) erases the element type that was already narrowed (or should have been) by the isMulti check.
A minimal improvement would be to define a local discriminated interface and use a proper type guard:
interface MultiBatchItem { command: string; targets: string[] }
function isMultiBatch(arr: unknown[]): arr is MultiBatchItem[] {
return arr.length > 0 && typeof (arr[0] as any).command === 'string';
}
if (isMultiBatch(targets)) {
const data = multiBatchData(targets, opts.db, batchOpts);
...
} else {
batch(command, targets as string[], opts.db, batchOpts);
}This makes the intent explicit and lets TypeScript validate both branches.
There was a problem hiding this comment.
Fixed — defined a local MultiBatchItem interface and isMultiBatch type guard function to replace the duck-typed (targets[0] as any).command check and targets as any cast.
| let plotCfg; | ||
| let html; | ||
| let plotCfg: any; | ||
| let html: string; |
There was a problem hiding this comment.
plotCfg: any allows silent field name mistakes
let plotCfg: any accumulates config from loadPlotConfig() and CLI flag overrides (plotCfg.clusterBy, plotCfg.colorBy, plotCfg.seedCount, etc.). Because the variable is fully untyped, any misspelling of a field name (e.g. clusterby instead of clusterBy) compiles cleanly but silently produces no effect at runtime.
If loadPlotConfig returns an exportable type, declaring plotCfg with that type would catch these mistakes at compile time.
There was a problem hiding this comment.
Fixed — defined a local PlotConfig interface with all known fields from DEFAULT_CONFIG (clusterBy, colorBy, sizeBy, seedStrategy, seedCount, overlays, etc.) and used it instead of any. Also removed the intermediate overlays as any cast.
#580) The engine field was cast as string instead of EngineMode, causing a TS2322 compile error. Also adds complexity and cfg to BuildGraphOpts since the pipeline uses them via dynamic access in detect-changes.ts.
Remove the trailing `as any` cast on the search options object and drop dead `offset` and `ndjson` fields that are not part of SearchOpts. Each property is already individually cast to its expected type. Impact: 1 functions changed, 0 affected
Define a MultiBatchItem interface and isMultiBatch type guard function to replace the ad-hoc `(targets[0] as any).command` duck-type check and `targets as any` cast in the multi-batch code path. Impact: 5 functions changed, 1 affected
Define a local PlotConfig interface matching the known config fields from DEFAULT_CONFIG (clusterBy, colorBy, sizeBy, seedStrategy, etc.) and use it instead of `any` for plotCfg. Also removes the intermediate `overlays as any` cast. Impact: 15 functions changed, 0 affected
src/cli/commands/co-change.ts
Outdated
| if (opts.analyze) { | ||
| const result = analyzeCoChanges(opts.db, { | ||
| since: opts.since || ctx.config.coChange?.since, | ||
| const coChangeConfig = (ctx.config as any).coChange; |
There was a problem hiding this comment.
as any casts on ctx.config
ctx.config is already typed as CommandOpts, which is defined as any. So (ctx.config as any).coChange is a no-op cast — the property access works identically without it. The same pattern appears on line 50 of this file and in embed.ts (line 28) and models.ts (line 8).
Additionally, const result: any = analyzeCoChanges(...) on line 37 explicitly suppresses TypeScript's return-type inference from the JS source. If analyzeCoChanges ever gains a JSDoc @returns annotation or a .d.ts, this explicit annotation would prevent the benefit from flowing through.
All affected locations:
src/cli/commands/co-change.ts:29—(ctx.config as any).coChangesrc/cli/commands/co-change.ts:37—const result: any = analyzeCoChanges(...)src/cli/commands/co-change.ts:50—(ctx.config as any).coChangesrc/cli/commands/embed.ts:28—(ctx.config as any).embeddingssrc/cli/commands/models.ts:8—(ctx.config as any).embeddings
Suggested simplification (co-change, line 29):
| const coChangeConfig = (ctx.config as any).coChange; | |
| const coChangeConfig = ctx.config.coChange; |
There was a problem hiding this comment.
Fixed — removed all redundant \ casts in co-change.ts, embed.ts, and models.ts since \ is already \ (which is ). Also replaced \ with a narrower type using \ guard for the error path.
src/cli/commands/models.ts
Outdated
| const modelCfg = cfg as any; | ||
| const ctxWindow = modelCfg.contextWindow ? `${modelCfg.contextWindow} ctx` : ''; | ||
| console.log( | ||
| ` ${key.padEnd(12)} ${String(cfg.dim).padStart(4)}d ${ctxWindow.padEnd(9)} ${cfg.desc}${def}`, | ||
| ` ${key.padEnd(12)} ${String(modelCfg.dim).padStart(4)}d ${ctxWindow.padEnd(9)} ${modelCfg.desc}${def}`, | ||
| ); |
There was a problem hiding this comment.
MODELS entry cast to any — consider a local interface
cfg as any is used to access contextWindow, dim, and desc fields from a MODELS entry. Since MODELS is a plain JS object with no exported type, this is pragmatically necessary today, but it means any rename or restructure of these fields compiles silently.
A lightweight local interface would make intent explicit at zero cost:
interface ModelEntry {
dim: number;
desc: string;
contextWindow?: number;
}
const modelCfg = cfg as ModelEntry;Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — replaced cfg as any with a local ModelEntry interface (dim, desc, contextWindow?) so field renames or restructures surface at compile time.
| * opaque alias keeps the intent clear while allowing dot access. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export type CommandOpts = any; |
There was a problem hiding this comment.
CommandOpts = any makes downstream as any casts redundant
The well-reasoned comment above explains why CommandOpts is typed as any (Commander's dynamic option shapes, noPropertyAccessFromIndexSignature ergonomics). That design decision is sound. A side effect is that all code that holds a CommandOpts value already has an implicit any — so the explicit (ctx.config as any) casts in co-change.ts, embed.ts, and models.ts add visual noise without providing any additional escape. Since ctx.config: CommandOpts, direct property access (ctx.config.coChange) already compiles and behaves identically to the cast form.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Agreed — removed all the redundant as any casts on ctx.config across co-change.ts, embed.ts, and models.ts. Direct property access works identically since CommandOpts = any.
…ility The cli.js → cli.ts rename lives in PR #580. This PR must reference cli.js so CI passes independently without the CLI migration.
1 participant
Summary
cli.js,cli/index.js,cli/shared/*(3 files), and all 43cli/commands/*handlersCommandDefinition,CliContext, andCommandOptstype definitions (cli/types.ts)discoverCommands()to discover both.tsand.jsfiles with deduplication (prefers.ts)Test plan
tsc --noEmit— zero errorsnpm run lint— zero errors (warnings are pre-existing)npm test— 107/110 pass (3 failures are pre-existing native engine parity tests)codegraph --helplists all 43 commands,codegraph stats --jsonreturns valid output