fix: mark consolidated events even when later sessions error

[nvandessel]

Summary

• When runner.Run() returned an error (e.g., one session's LLM call timed out), both CLI and MCP handlers returned early without calling MarkConsolidated
• This left ALL events unmarked, causing the pipeline to re-process the same 27,402 events on every invocation, burning rate limit on redundant work
• Now MarkConsolidated runs before the error check, preserving progress from successful sessions

Test plan

• Existing tests pass: CGO_ENABLED=0 go test ./internal/consolidation/ ./internal/events/ -count=1
• Run consolidation with events that trigger partial failure — verify successfully-processed sessions get marked

🤖 Generated with Claude Code

[codecov-commenter]

Codecov Report

❌ Patch coverage is 28.57143% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.78%. Comparing base (80fe0c5) to head (1da3818).

Files with missing lines	Patch %	Lines
cmd/floop/cmd_consolidate.go	28.57%	3 Missing and 2 partials ⚠️
internal/mcp/handler_consolidate.go	28.57%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #265   +/-   ##
=======================================
  Coverage   78.77%   78.78%           
=======================================
  Files         186      186           
  Lines       18685    18687    +2     
=======================================
+ Hits        14720    14723    +3     
+ Misses       2742     2741    -1     
  Partials     1223     1223           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[greptile-apps]

Greptile Summary

This PR fixes a significant pipeline-efficiency bug: when runner.Run returned a partial error (e.g. one session's LLM call timed out), both the CLI and MCP handlers returned early before calling MarkConsolidated, leaving all successfully-processed events unmarked and causing them to be reprocessed on every subsequent invocation.

The fix reorders the MarkConsolidated call to execute before the error check in both callers, so progress from successful sessions is always preserved. As a side effect, MarkConsolidated failures are demoted from hard errors to logged warnings in both code paths (matching the now-explicit rationale that a failed mark is recoverable — the worst case is a re-run on those events).

Key changes:

• cmd/floop/cmd_consolidate.go and internal/mcp/handler_consolidate.go: MarkConsolidated now runs unconditionally (guarded by !dryRun, result != nil, and len > 0) before runErr is inspected. A result != nil guard is added, though runner.Run always returns a non-nil *RunResult (initialized as aggregated := &RunResult{} at runner.go:78), so this is defensive rather than strictly required.
• internal/consolidation/runner_test.go: Two new tests added — a unit test confirming result.SourceEventIDs is populated from the successful session on partial failure, and an integration test using a real in-memory SQLite store to verify the end-to-end mark behavior.

Confidence Score: 5/5

Safe to merge; the logic change is minimal, well-reasoned, and the prior logger-inconsistency concern was already addressed.

All remaining findings are P2 style/test-design observations. The core reordering is correct: runner.Run always returns a non-nil result with SourceEventIDs only from fully-processed sessions, so calling MarkConsolidated before the error check is sound. No production behavior regressions identified.

runner_test.go — integration test calls MarkConsolidated under DryRun:true, which is a cosmetic mismatch with the !dryRun guard used by actual callers.

Important Files Changed

Filename	Overview
cmd/floop/cmd_consolidate.go	Reorders MarkConsolidated before the runErr check, adds result != nil guard, and demotes MarkConsolidated failures from hard errors to stderr warnings — correctly preserving progress from successful sessions on partial pipeline failure.
internal/mcp/handler_consolidate.go	Same reordering as the CLI handler: MarkConsolidated now runs before runErr is checked, and MarkConsolidated failure is a logged warning rather than a hard error returned to the MCP caller.
internal/consolidation/runner_test.go	Adds two new tests covering partial-failure semantics: a unit test verifying SourceEventIDs are populated from the successful session and an integration test using a real SQLite store. Integration test calls MarkConsolidated under DryRun:true, which doesn't match the production !dryRun guard.

Sequence Diagram

sequenceDiagram
    participant Caller as CLI / MCP Handler
    participant Runner as consolidation.Runner
    participant ES as EventStore

    Caller->>Runner: Run(ctx, evts, graphStore, opts)
    loop per session
        Runner->>Runner: runSession(sess-ok) → SourceEventIDs=[e1,e2]
        Runner->>Runner: runSession(sess-fail) → error
        Runner-->>Caller: result{SourceEventIDs:[e1,e2]}, runErr
    end

    Note over Caller,ES: Fixed order (this PR)
    alt !dryRun && result != nil && len(SourceEventIDs) > 0
        Caller->>ES: MarkConsolidated([e1, e2])
        ES-->>Caller: ok (or warn on failure)
    end
    alt runErr != nil
        Caller-->>Caller: return error (e3 remains unconsolidated)
    end

Loading

_{Reviews (3): Last reviewed commit: "test: add coverage for mark-consolidated..." | Re-trigger Greptile}