feat(build): report collectMs/detectMs as separate phases (#993)

* feat(build): report collectMs/detectMs as separate phases (#977)

Split collect and detect-change work out of setupMs in BuildResult.phases.
These two phases have very different cost profiles (file-walk + ignore
matching vs hash/mtime checks), and collapsing them hid where time was
actually being spent during incremental-build perf investigations.

- types.ts: add collectMs/detectMs to BuildResult.phases
- context.ts: add collectMs/detectMs to ctx.timing
- collect-files.ts, detect-changes.ts: wrap exported entries with
  performance.now() (try/finally so early returns still record timing)
- pipeline.ts: formatTimingResult and formatNativeTimingResult surface
  the two fields separately instead of folding native-side values back
  into setupMs

Rust PipelineTiming already tracks collect_ms and detect_ms, so no new
instrumentation on the native side.

docs check acknowledged — observability-only change, no README/CLAUDE/
ROADMAP impact.

Impact: 6 functions changed, 7 affected

* fix(types): align PipelineContext.timing interface with class definition (#993)

Impact: 1 functions changed, 0 affected

* refactor(build): separate collectMs from detectMs on scoped path (#993)

Addresses Greptile P2 feedback on #993:

- collectMs no longer includes change-detection work on the scoped-rebuild
  path. Only filesystem-walk work (existence checks + file list) is timed
  under collectMs; parseChanges/removed/isFullBuild assignments are timed
  under detectMs for semantic consistency with the non-scoped path.
- detectChanges now accumulates detectMs additively to respect the partial
  contribution from collectFiles on the scoped path.
- Pipeline test asserts collectMs and detectMs are >= 0, not just 'number',
  so a silent regression where timing is never recorded would be caught.

Impact: 2 functions changed, 5 affected

Author: carlos-alm

src/domain/graph/builder/context.ts

@@ -87,6 +87,8 @@ export class PipelineContext {
   // ── Phase timing ───────────────────────────────────────────────────
   timing: {
     setupMs?: number;
+    collectMs?: number;
+    detectMs?: number;
     parseMs?: number;
     insertMs?: number;
     resolveMs?: number;

src/domain/graph/builder/pipeline.ts

@@ -184,6 +184,8 @@ function formatTimingResult(ctx: PipelineContext): BuildResult {
   return {
     phases: {
       setupMs: +(t.setupMs ?? 0).toFixed(1),
+      collectMs: +(t.collectMs ?? 0).toFixed(1),
+      detectMs: +(t.detectMs ?? 0).toFixed(1),
       parseMs: +(t.parseMs ?? 0).toFixed(1),
       insertMs: +(t.insertMs ?? 0).toFixed(1),
       resolveMs: +(t.resolveMs ?? 0).toFixed(1),
@@ -558,7 +560,9 @@ function formatNativeTimingResult(
 ): BuildResult {
   return {
     phases: {
-      setupMs: +((p.setupMs ?? 0) + (p.collectMs ?? 0) + (p.detectMs ?? 0)).toFixed(1),
+      setupMs: +(p.setupMs ?? 0).toFixed(1),
+      collectMs: +(p.collectMs ?? 0).toFixed(1),
+      detectMs: +(p.detectMs ?? 0).toFixed(1),
       parseMs: +(p.parseMs ?? 0).toFixed(1),
       insertMs: +(p.insertMs ?? 0).toFixed(1),
       resolveMs: +(p.resolveMs ?? 0).toFixed(1),

src/domain/graph/builder/stages/collect-files.ts

@@ -7,6 +7,7 @@
  */
 import fs from 'node:fs';
 import path from 'node:path';
+import { performance } from 'node:perf_hooks';
 import { debug, info } from '../../../../infrastructure/logger.js';
 import { normalizePath } from '../../../../shared/constants.js';
 import { readJournal } from '../../journal.js';
@@ -89,42 +90,60 @@ export async function collectFiles(ctx: PipelineContext): Promise<void> {
   const { rootDir, config, opts } = ctx;
 
   if (opts.scope) {
-    // Scoped rebuild: rebuild only specified files
+    // Scoped rebuild: rebuild only specified files.
+    //
+    // Timer only wraps the filesystem-walk portion (existence checks + file
+    // list construction). Change-detection outputs (parseChanges, removed,
+    // isFullBuild) are attributed to detectMs for semantic consistency with
+    // the non-scoped path, even though this stage computes them.
+    const start = performance.now();
     const scopedFiles = opts.scope.map((f: string) => normalizePath(f));
     const existing: Array<{ file: string; relPath: string }> = [];
     const missing: string[] = [];
-    for (const rel of scopedFiles) {
-      const abs = path.join(rootDir, rel);
-      if (fs.existsSync(abs)) {
-        existing.push({ file: abs, relPath: rel });
-      } else {
-        missing.push(rel);
+    try {
+      for (const rel of scopedFiles) {
+        const abs = path.join(rootDir, rel);
+        if (fs.existsSync(abs)) {
+          existing.push({ file: abs, relPath: rel });
+        } else {
+          missing.push(rel);
+        }
       }
+      ctx.allFiles = existing.map((e) => e.file);
+      ctx.discoveredDirs = new Set(existing.map((e) => path.dirname(e.file)));
+    } finally {
+      ctx.timing.collectMs = performance.now() - start;
     }
-    ctx.allFiles = existing.map((e) => e.file);
-    ctx.discoveredDirs = new Set(existing.map((e) => path.dirname(e.file)));
+    // Change-detection outputs — timed under detectMs for semantic parity.
+    const detectStart = performance.now();
     ctx.parseChanges = existing;
     ctx.metadataUpdates = [];
     ctx.removed = missing;
     ctx.isFullBuild = false;
+    ctx.timing.detectMs = (ctx.timing.detectMs ?? 0) + (performance.now() - detectStart);
     info(`Scoped rebuild: ${existing.length} files to rebuild, ${missing.length} to purge`);
     return;
   }
 
-  // Incremental fast path: reconstruct file list from DB + journal deltas
-  // instead of full recursive filesystem scan (~8ms savings on 473 files).
-  if (ctx.incremental && !ctx.forceFullRebuild) {
-    const fast = tryFastCollect(ctx);
-    if (fast) {
-      ctx.allFiles = fast.files;
-      ctx.discoveredDirs = fast.directories;
-      info(`Found ${ctx.allFiles.length} files (cached)`);
-      return;
+  const start = performance.now();
+  try {
+    // Incremental fast path: reconstruct file list from DB + journal deltas
+    // instead of full recursive filesystem scan (~8ms savings on 473 files).
+    if (ctx.incremental && !ctx.forceFullRebuild) {
+      const fast = tryFastCollect(ctx);
+      if (fast) {
+        ctx.allFiles = fast.files;
+        ctx.discoveredDirs = fast.directories;
+        info(`Found ${ctx.allFiles.length} files (cached)`);
+        return;
+      }
     }
-  }
 
-  const collected = collectFilesUtil(rootDir, [], config, new Set<string>());
-  ctx.allFiles = collected.files;
-  ctx.discoveredDirs = collected.directories;
-  info(`Found ${ctx.allFiles.length} files to parse`);
+    const collected = collectFilesUtil(rootDir, [], config, new Set<string>());
+    ctx.allFiles = collected.files;
+    ctx.discoveredDirs = collected.directories;
+    info(`Found ${ctx.allFiles.length} files to parse`);
+  } finally {
+    ctx.timing.collectMs = performance.now() - start;
+  }
 }

src/domain/graph/builder/stages/detect-changes.ts

@@ -7,6 +7,7 @@
  */
 import fs from 'node:fs';
 import path from 'node:path';
+import { performance } from 'node:perf_hooks';
 import { closeDb } from '../../../../db/index.js';
 import { debug, info } from '../../../../infrastructure/logger.js';
 import { normalizePath } from '../../../../shared/constants.js';
@@ -512,59 +513,66 @@ function handleIncrementalBuild(ctx: PipelineContext): void {
 }
 
 export async function detectChanges(ctx: PipelineContext): Promise<void> {
-  const { db, allFiles, rootDir, incremental, forceFullRebuild, opts } = ctx;
-  if ((opts as Record<string, unknown>).scope) {
-    handleScopedBuild(ctx);
-    return;
-  }
-  const increResult =
-    incremental && !forceFullRebuild
-      ? getChangedFiles(db, allFiles, rootDir)
-      : {
-          changed: allFiles.map((f): ChangedFile => ({ file: f })),
-          removed: [] as string[],
-          isFullBuild: true,
-        };
-  ctx.removed = increResult.removed;
-  ctx.isFullBuild = increResult.isFullBuild;
-  ctx.parseChanges = increResult.changed
-    .filter((c) => !c.metadataOnly)
-    .map((c) => ({
-      file: c.file,
-      relPath: c.relPath,
-      content: c.content,
-      hash: c.hash,
-      stat: c.stat ? { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size } : undefined,
-      _reverseDepOnly: c._reverseDepOnly,
-    }));
-  ctx.metadataUpdates = increResult.changed
-    .filter(
-      (c): c is ChangedFile & { relPath: string; hash: string; stat: FileStat } =>
-        !!c.metadataOnly && !!c.relPath && !!c.hash && !!c.stat,
-    )
-    .map((c) => ({
-      relPath: c.relPath,
-      hash: c.hash,
-      stat: { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size },
-    }));
-  if (!ctx.isFullBuild && ctx.parseChanges.length === 0 && ctx.removed.length === 0) {
-    const ranAnalysis = await runPendingAnalysis(ctx);
-    if (ranAnalysis) {
+  const start = performance.now();
+  try {
+    const { db, allFiles, rootDir, incremental, forceFullRebuild, opts } = ctx;
+    if ((opts as Record<string, unknown>).scope) {
+      handleScopedBuild(ctx);
+      return;
+    }
+    const increResult =
+      incremental && !forceFullRebuild
+        ? getChangedFiles(db, allFiles, rootDir)
+        : {
+            changed: allFiles.map((f): ChangedFile => ({ file: f })),
+            removed: [] as string[],
+            isFullBuild: true,
+          };
+    ctx.removed = increResult.removed;
+    ctx.isFullBuild = increResult.isFullBuild;
+    ctx.parseChanges = increResult.changed
+      .filter((c) => !c.metadataOnly)
+      .map((c) => ({
+        file: c.file,
+        relPath: c.relPath,
+        content: c.content,
+        hash: c.hash,
+        stat: c.stat ? { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size } : undefined,
+        _reverseDepOnly: c._reverseDepOnly,
+      }));
+    ctx.metadataUpdates = increResult.changed
+      .filter(
+        (c): c is ChangedFile & { relPath: string; hash: string; stat: FileStat } =>
+          !!c.metadataOnly && !!c.relPath && !!c.hash && !!c.stat,
+      )
+      .map((c) => ({
+        relPath: c.relPath,
+        hash: c.hash,
+        stat: { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size },
+      }));
+    if (!ctx.isFullBuild && ctx.parseChanges.length === 0 && ctx.removed.length === 0) {
+      const ranAnalysis = await runPendingAnalysis(ctx);
+      if (ranAnalysis) {
+        closeDb(db);
+        writeJournalHeader(rootDir, Date.now());
+        ctx.earlyExit = true;
+        return;
+      }
+      healMetadata(ctx);
+      info('No changes detected. Graph is up to date.');
       closeDb(db);
       writeJournalHeader(rootDir, Date.now());
       ctx.earlyExit = true;
       return;
     }
-    healMetadata(ctx);
-    info('No changes detected. Graph is up to date.');
-    closeDb(db);
-    writeJournalHeader(rootDir, Date.now());
-    ctx.earlyExit = true;
-    return;
-  }
-  if (ctx.isFullBuild) {
-    handleFullBuild(ctx);
-  } else {
-    handleIncrementalBuild(ctx);
+    if (ctx.isFullBuild) {
+      handleFullBuild(ctx);
+    } else {
+      handleIncrementalBuild(ctx);
+    }
+  } finally {
+    // Additive to respect any partial detectMs contribution from collectFiles
+    // (scoped-rebuild path splits change-detection outputs across both stages).
+    ctx.timing.detectMs = (ctx.timing.detectMs ?? 0) + (performance.now() - start);
   }
 }

src/types.ts

@@ -1033,7 +1033,23 @@ export interface PipelineContext {
   lineCountMap: Map<string, number>;
 
   // Phase timing
-  timing: Record<string, number>;
+  timing: {
+    setupMs?: number;
+    collectMs?: number;
+    detectMs?: number;
+    parseMs?: number;
+    insertMs?: number;
+    resolveMs?: number;
+    edgesMs?: number;
+    structureMs?: number;
+    rolesMs?: number;
+    astMs?: number;
+    complexityMs?: number;
+    cfgMs?: number;
+    dataflowMs?: number;
+    finalizeMs?: number;
+    [key: string]: number | undefined;
+  };
   buildStart: number;
 }
 
@@ -1053,6 +1069,8 @@ export interface BuildGraphOpts {
 export interface BuildResult {
   phases: {
     setupMs: number;
+    collectMs: number;
+    detectMs: number;
     parseMs: number;
     insertMs: number;
     resolveMs: number;

tests/builder/pipeline.test.ts

@@ -31,6 +31,10 @@ describe('buildGraph pipeline', () => {
     expect(result).toBeDefined();
     expect(result.phases).toBeDefined();
     expect(typeof result.phases.setupMs).toBe('number');
+    expect(typeof result.phases.collectMs).toBe('number');
+    expect(result.phases.collectMs).toBeGreaterThanOrEqual(0);
+    expect(typeof result.phases.detectMs).toBe('number');
+    expect(result.phases.detectMs).toBeGreaterThanOrEqual(0);
     expect(typeof result.phases.parseMs).toBe('number');
     expect(typeof result.phases.insertMs).toBe('number');
     expect(typeof result.phases.resolveMs).toBe('number');