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

[carlos-alm]

Summary

• Splits collect and detect-change work out of setupMs in BuildResult.phases so incremental-build perf investigations can see the two phases separately (file-walk + ignore-matching vs hash/mtime checks).
• JS path instruments collectFiles and detectChanges with performance.now() wrapped in try/finally so early returns still record timing.
• Native path stops folding collectMs/detectMs back into setupMs — the Rust PipelineTiming already tracks them, so the formatter just surfaces them.

Closes #977.

Files touched

• src/types.ts — adds collectMs/detectMs to BuildResult.phases
• src/domain/graph/builder/context.ts — adds collectMs/detectMs to ctx.timing
• src/domain/graph/builder/stages/collect-files.ts — wraps collectFiles with performance.now()
• src/domain/graph/builder/stages/detect-changes.ts — wraps detectChanges with performance.now()
• src/domain/graph/builder/pipeline.ts — formatTimingResult and formatNativeTimingResult surface the two fields
• tests/builder/pipeline.test.ts — asserts the new fields

No README/CLAUDE/ROADMAP updates — observability-only change.

Test plan

• npx tsc --noEmit
• npx vitest run tests/builder (14/14 pass)
• npx vitest run tests/integration/incremental-parity.test.ts (pass)
• CI runs the full suite

[claude]

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

---

PR Review: feat(build): report collectMs/detectMs as separate phases

Reviewing this PR according to the established review criteria. Progress:

• Phase 1: Root cause analysis - determine problem and approach justification
• Phase 2: Configuration change detection - check for high-risk changes
• Phase 3: Test integrity - look for red flags in test modifications
• Phase 4: Code quality - review implementation details
• Phase 5: Backlog compliance - rate against BACKLOG.md criteria
• Final assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: Issue #977 requests splitting setupMs timing into separate collectMs and detectMs phases to better understand incremental build performance bottlenecks.

Why This Approach: This is a legitimate observability improvement. The PR splits timing for two distinct operations:

• File collection (filesystem walk + ignore matching)
• Change detection (hash/mtime checks)

Risk Assessment: Low risk - purely additive timing instrumentation with no behavioral changes.

Code Quality Review ✓

Implementation is solid:

Timing instrumentation:

• collect-files.ts:134: Proper try/finally pattern ensures timing recorded even on early returns
• detect-changes.ts:574: Same pattern in detectChanges stage
• Both use performance.now() consistently with existing code

Type safety:

• types.ts:1056-1057: New fields added to BuildResult.phases interface
• context.ts:90-91: Added to PipelineContext timing structure
• All timing fields properly typed as optional numbers

Consistency:

• Both functions follow established pipeline patterns
• Native path in pipeline.ts:564-565 correctly surfaces the new timings
• Native orchestrator result formatting preserves the split

Testing:

• pipeline.test.ts:34-35: Adds assertions for new timing fields
• Test coverage appropriate for observability feature

Backlog Compliance

• Zero-dep: ✓ - No new runtime dependencies
• Foundation-aligned: ✓ - Pure observability improvement, no principle violations
• Problem-fit: 3 - Useful but tangential to core AI agent problems
• Breaking: No - purely additive fields to existing interface
• Tier: 1

Final Recommendation

• Rating: ⭐⭐⭐⭐ (4/5)
• Action: APPROVE
• Reasoning: Clean, well-implemented observability improvement that addresses a legitimate performance investigation need. Code follows established patterns, includes appropriate tests, and is purely additive with no behavioral changes.

The PR correctly identifies that file collection and change detection are distinct performance bottlenecks worth measuring separately. The implementation properly instruments both JS and native code paths while maintaining backward compatibility.

[greptile-apps]

Greptile Summary

This PR splits collectMs and detectMs out of setupMs in BuildResult.phases to expose file-walk and hash/mtime-check timing as separate telemetry fields. The JS path wraps collectFiles and detectChanges with performance.now() / try-finally, and the native path stops folding those two Rust-tracked fields back into setupMs.

Confidence Score: 5/5

Safe to merge — no correctness issues found; timing instrumentation is properly guarded with try/finally and the interface drift from the prior review is resolved.

All findings are P2 or lower. The try/finally pattern is used correctly on all code paths (including early returns). The additive detectMs accumulation is intentional and well-documented. The PipelineContext.timing interface now accurately reflects the concrete class. Tests cover the new fields.

No files require special attention.

Important Files Changed

Filename	Overview
src/types.ts	Expands PipelineContext.timing from Record<string,number> to a fully-typed optional-field object with index signature, and adds required collectMs/detectMs to BuildResult.phases; fixes the interface drift flagged in prior review.
src/domain/graph/builder/stages/collect-files.ts	Adds performance.now() timers wrapped in try/finally for both the scoped and non-scoped code paths; scoped path also records a near-zero detectMs contribution for the change-detection assignments, additive with detectChanges's own measurement.
src/domain/graph/builder/stages/detect-changes.ts	Entire function body wrapped in try/finally; detectMs uses additive accumulation (?? 0 + elapsed) to correctly combine any partial contribution already written by collectFiles on the scoped path.
src/domain/graph/builder/pipeline.ts	Both formatTimingResult and formatNativeTimingResult now surface collectMs/detectMs as separate fields; native path stops summing them into setupMs, which reduces setupMs for existing metric consumers.
src/domain/graph/builder/context.ts	Adds collectMs? and detectMs? optional fields to the concrete PipelineContext.timing object, aligning with the updated interface in types.ts.
tests/builder/pipeline.test.ts	Adds type and >= 0 value assertions for the two new phase fields; straightforward coverage of the happy path.

Sequence Diagram

sequenceDiagram
    participant P as pipeline.ts
    participant C as collectFiles
    participant D as detectChanges
    participant T as ctx.timing

    P->>C: collectFiles(ctx)
    activate C
    Note over C: start = performance.now()
    C->>T: timing.collectMs = elapsed
    alt scoped path
        C->>T: timing.detectMs += ~0ms (assignments only)
    end
    deactivate C

    P->>D: detectChanges(ctx)
    activate D
    Note over D: start = performance.now()
    alt scoped path
        D->>D: handleScopedBuild(ctx)
    else incremental / full
        D->>D: getChangedFiles / fullBuild logic
    end
    D->>T: timing.detectMs = (timing.detectMs ?? 0) + elapsed
    deactivate D

    P->>P: formatTimingResult(ctx)
    Note over P: setupMs, collectMs, detectMs, parseMs…

Loading

_{Reviews (4): Last reviewed commit: "refactor(build): separate collectMs from..." | Re-trigger Greptile}

[github-actions]

Codegraph Impact Analysis

7 functions changed → 7 callers affected across 5 files

• PipelineContext in src/domain/graph/builder/context.ts:21 (6 transitive callers)
• formatTimingResult in src/domain/graph/builder/pipeline.ts:182 (6 transitive callers)
• formatNativeTimingResult in src/domain/graph/builder/pipeline.ts:556 (4 transitive callers)
• collectFiles in src/domain/graph/builder/stages/collect-files.ts:89 (5 transitive callers)
• detectChanges in src/domain/graph/builder/stages/detect-changes.ts:515 (5 transitive callers)
• PipelineContext.timing in src/types.ts:1036 (0 transitive callers)
• BuildResult.phases in src/types.ts:1070 (0 transitive callers)

[carlos-alm]

@greptileai Thanks — addressed the PipelineContext timing interface drift in bf3c3d3. Replaced timing: Record<string, number> in src/types.ts with the full typed object shape (optional setupMs/collectMs/detectMs/... fields + [key: string]: number | undefined index signature) so the interface in types.ts now mirrors the concrete class in context.ts. Consumers typed against the interface will now see the new fields and tolerate undefined timing values.

• npx tsc --noEmit: clean
• npx vitest run tests/builder: 14/14 pass
• biome check on touched files: clean

[carlos-alm]

@greptileai

[carlos-alm]

Addressed Greptile's P2 feedback in 87ce95d:

• Issue 1 — scoped-path collectMs/detectMs overlap: collectFiles now times only the filesystem-walk portion (existence checks + file list construction) under collectMs. Change-detection outputs (parseChanges/removed/isFullBuild) are attributed to detectMs via an additive accumulator, matching the semantics of the non-scoped path. detectChanges also accumulates detectMs additively so the contribution from collectFiles isn't clobbered.
• Issue 2 — positive-value assertions: tests/builder/pipeline.test.ts now asserts toBeGreaterThanOrEqual(0) on collectMs and detectMs in addition to the typeof check, so a silent regression where the finally block never runs would be caught.

Verified:

• npx tsc --noEmit: clean
• npx vitest run tests/builder: 14/14 pass
• npx vitest run tests/integration/incremental-parity.test.ts: 12/12 pass
• biome check on touched files: clean

[carlos-alm]

@greptileai