feat(telemetry): aggregate task completion telemetry with delta installments - #1071
feat(telemetry): aggregate task completion telemetry with delta installments#1071edelauna wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThe changes add incremental task telemetry. Tasks track message counts, report tool and message deltas during completion, idle periods, and shutdown, and preserve retry symmetry. Completion telemetry is separate from acceptance-dependent public completion events. ChangesTask telemetry lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/core/tools/__tests__/attemptCompletionTool.spec.ts (1)
88-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting on
flushTelemetryInstallmentinstead of the payload this mock synthesizes.The mock forwards
mockTask.toolUsageandmockTask.messageCountsas-is, so thetoolsUsedandmessageCountarguments asserted throughout this file come from the mock, not fromTask#flushTelemetryInstallment. Delta semantics are covered inTask.spec.ts. What this file needs to verify is that the tool calls the flush and passes"attempt_completion". Assertingexpect(task.flushTelemetryInstallment).toHaveBeenCalledWith("attempt_completion")states that intent directly and stays correct if the payload shape changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/__tests__/attemptCompletionTool.spec.ts` around lines 88 - 90, Update the attempt-completion tests to assert directly on task.flushTelemetryInstallment, verifying it is called with "attempt_completion". Remove assertions on synthesized toolsUsed and messageCount payloads from mockCaptureTaskCompleted, while preserving coverage of the tool’s completion behavior.src/core/task/Task.ts (1)
4777-4793: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe idle check re-fires every 5 minutes after the first idle flush, and
lastTelemetryFlushAtnever suppresses it.
lastMessageTsonly advances on new activity.flushTelemetryInstallmentupdateslastTelemetryFlushAt, notlastMessageTs. After the first idle flush,idleForMsstays aboveIDLE_TELEMETRY_THRESHOLD_MS, so the interval callsflushTelemetryInstallment("idle")again every 5 minutes for the rest of the task's life. Each repeat no-ops on the empty-delta check, so no duplicate events are sent, but the comment at lines 4779-4783 states that this check "avoids waking up to do that check needlessly" — the condition never becomes false.
lastTelemetryFlushAtis also read only in the??fallback, so it has no effect wheneverlastMessageTsis set. Compare against the later of the two timestamps so a completed flush actually resets the window.♻️ Proposed fix
private startIdleTelemetryCheck(): void { this.idleTelemetryCheckInterval = setInterval(() => { - // lastMessageTs only moves forward on activity, so comparing it against the - // last flush tells us whether anything happened since that flush -- if the - // task has been quiet since well before the last flush, there's nothing new - // to report and flushTelemetryInstallment's own empty-check would no-op anyway, - // but skipping here avoids waking up to do that check needlessly. - const idleForMs = Date.now() - (this.lastMessageTs ?? this.lastTelemetryFlushAt) + // Measure idleness from the later of the last activity and the last flush. + // Using lastMessageTs alone would keep this condition true forever after the + // first idle flush, re-running the empty-delta check on every interval tick. + const lastEventAt = Math.max(this.lastMessageTs ?? 0, this.lastTelemetryFlushAt) + const idleForMs = Date.now() - lastEventAt if (idleForMs >= Task.IDLE_TELEMETRY_THRESHOLD_MS) { this.flushTelemetryInstallment("idle") } }, Task.IDLE_TELEMETRY_CHECK_INTERVAL_MS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/Task.ts` around lines 4777 - 4793, Update startIdleTelemetryCheck to calculate idleForMs from the later of lastMessageTs and lastTelemetryFlushAt, while preserving the fallback when neither timestamp is set. Ensure a completed flush resets the idle threshold so the interval does not repeatedly invoke flushTelemetryInstallment("idle") until new activity occurs.src/core/tools/AttemptCompletionTool.ts (1)
126-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
emitFinalTokenUsageUpdate()is already called insideflushTelemetryInstallment.
Task#flushTelemetryInstallmentcallsthis.emitFinalTokenUsageUpdate()atTask.tsline 4769 before it captures the event. The explicit call here is redundant. The same duplication exists at lines 183-184. One difference: the internal call runs only when a delta exists, so removing the external calls also removes the forced token emit for a no-delta completion attempt. Decide which layer owns the responsibility and keep one call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/tools/AttemptCompletionTool.ts` around lines 126 - 127, Remove the redundant emitFinalTokenUsageUpdate call from the completion flow in AttemptCompletionTool, including the duplicate occurrence near the other reported location, and retain flushTelemetryInstallment("attempt_completion") as the single ownership point. Verify the resulting behavior still matches the intended no-delta token usage handling.src/core/task/__tests__/Task.spec.ts (1)
3121-3124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDispose each created
TaskinafterEachso the 5-minute interval does not outlive its test.
createTask()starts a realsetIntervalin theTaskconstructor. Only the threedisposetests clear it. The other seven tests leave a live interval that holds theTaskinstance and can invoke the sharedcaptureTaskCompletedSpywhile a later test in this file is running. That makes the suite order-dependent.♻️ Proposed fix
+ const createdTasks: Task[] = [] + afterEach(() => { + for (const task of createdTasks) { + task.dispose() + } + createdTasks.length = 0 vi.useRealTimers() captureTaskCompletedSpy.mockRestore() }) function createTask() { - return new Task({ + const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "test task", startTask: false, }) + createdTasks.push(task) + return task }
dispose()is idempotent for the interval, so the threedisposetests remain correct.As per path instructions,
**/*.{test,spec}.{ts,tsx,js}: "Use package-local unit tests for pure logic, parsing, state transitions, ...".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/Task.spec.ts` around lines 3121 - 3124, Update the Task test cleanup in afterEach to dispose every Task instance created by createTask before restoring timers and mocks. Track created tasks within the test setup and call each task’s idempotent dispose method during teardown, preserving the existing dispose-test behavior.Source: Path instructions
src/core/task/__tests__/messageCounting.retrySymmetry.spec.ts (1)
28-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests re-implement the Task.ts counter steps, so they cannot catch a regression in Task.ts.
simulatePopOnEmptyResponseandsimulateDeclineRetryhard-codemessageCounts.user--andmessageCounts.user++. Only the add decision comes from production code. If someone removesthis.messageCounts.user--atTask.tsline 3701, or thethis.messageCounts.user++at line 3769, these tests still pass. Consider driving the empty-response retry cycle throughTask#recursivelyMakeClineRequestswith a stubbed stream, or extract the counter mutations intomessageCounting.tsso both sides share one implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/messageCounting.retrySymmetry.spec.ts` around lines 28 - 41, Replace the hard-coded counter mutations in simulatePopOnEmptyResponse and simulateDeclineRetry with tests that exercise the production Task#recursivelyMakeClineRequests retry flow using a stubbed stream, or move the shared counter mutations into messageCounting.ts and test that implementation directly. Ensure the tests fail if Task removes either the user decrement for an empty response or the user increment during declined retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/Task.ts`:
- Around line 3695-3701: Guard the decrement in the retry cleanup around
apiConversationHistory.pop() so messageCounts.user is reduced only when this
method previously incremented it for the removed message, preserving the
existing restoration behavior otherwise. Also update flushTelemetryInstallment
to clamp both user and assistant message-count deltas to zero or greater before
emitting telemetry.
---
Nitpick comments:
In `@src/core/task/__tests__/messageCounting.retrySymmetry.spec.ts`:
- Around line 28-41: Replace the hard-coded counter mutations in
simulatePopOnEmptyResponse and simulateDeclineRetry with tests that exercise the
production Task#recursivelyMakeClineRequests retry flow using a stubbed stream,
or move the shared counter mutations into messageCounting.ts and test that
implementation directly. Ensure the tests fail if Task removes either the user
decrement for an empty response or the user increment during declined retry.
In `@src/core/task/__tests__/Task.spec.ts`:
- Around line 3121-3124: Update the Task test cleanup in afterEach to dispose
every Task instance created by createTask before restoring timers and mocks.
Track created tasks within the test setup and call each task’s idempotent
dispose method during teardown, preserving the existing dispose-test behavior.
In `@src/core/task/Task.ts`:
- Around line 4777-4793: Update startIdleTelemetryCheck to calculate idleForMs
from the later of lastMessageTs and lastTelemetryFlushAt, while preserving the
fallback when neither timestamp is set. Ensure a completed flush resets the idle
threshold so the interval does not repeatedly invoke
flushTelemetryInstallment("idle") until new activity occurs.
In `@src/core/tools/__tests__/attemptCompletionTool.spec.ts`:
- Around line 88-90: Update the attempt-completion tests to assert directly on
task.flushTelemetryInstallment, verifying it is called with
"attempt_completion". Remove assertions on synthesized toolsUsed and
messageCount payloads from mockCaptureTaskCompleted, while preserving coverage
of the tool’s completion behavior.
In `@src/core/tools/AttemptCompletionTool.ts`:
- Around line 126-127: Remove the redundant emitFinalTokenUsageUpdate call from
the completion flow in AttemptCompletionTool, including the duplicate occurrence
near the other reported location, and retain
flushTelemetryInstallment("attempt_completion") as the single ownership point.
Verify the resulting behavior still matches the intended no-delta token usage
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 610a96d6-9c0b-4461-b893-f1da90e47d3b
📒 Files selected for processing (9)
packages/telemetry/src/TelemetryService.tspackages/telemetry/src/__tests__/TelemetryService.task-completed.test.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/task/__tests__/messageCounting.retrySymmetry.spec.tssrc/core/task/__tests__/messageCounting.spec.tssrc/core/task/messageCounting.tssrc/core/tools/AttemptCompletionTool.tssrc/core/tools/__tests__/attemptCompletionTool.spec.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/Task.ts`:
- Around line 3700-3707: Update the error-recovery flow around the history pop
and retry handling to track whether this iteration removed its own user message.
Only pop or restore the message, set the corresponding removal flag, and
increment messageCounts.user when shouldAddUserMessage indicates this iteration
added it; preserve prior persisted history for empty continuations such as
resumeAfterDelegation().
- Line 624: Move idle telemetry initialization from the current launch path into
a shared idempotent task-launch helper, preserving a single active
idleTelemetryCheckInterval. In src/core/task/Task.ts lines 624-624, update the
existing startup logic to use that helper; in src/core/task/Task.ts lines
1902-1902, invoke the same helper from start(), run(), and Task.create() before
startTask() or resumeTaskFromHistory().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b25a08e7-08dd-41f8-9bc5-920ac305f382
📒 Files selected for processing (5)
src/__tests__/history-resume-delegation.spec.tssrc/__tests__/nested-delegation-resume.spec.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/task/__tests__/messageCounting.retrySymmetry.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/task/tests/Task.spec.ts
- src/core/task/tests/messageCounting.retrySymmetry.spec.ts
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
removed
the
awaiting-review
edelauna
changed the title
github-actions
Bot
added
the
awaiting-review
1 participant
Related GitHub Issue
Refs: #830 (task completion telemetry aggregation — split from #835; independent of #1069 and #1070)
Description
Replaces per-turn
Conversation MessageandTool Usedevents with a singleTask Completedinstallment per model-initiatedattempt_completioncall.Delta invariants. Each installment carries only the delta in
toolsUsedandmessageCountsince the prior installment for that task. Summing installments for ataskIdreconstructs full-task totals without double-counting. An unchanged task produces no empty installment.Completion reasons.
completionReasonis constrained to three values:"attempt_completion"— the model called that tool (fires regardless of whether the user accepts, declines, or gives feedback)"idle"— the task has been quiet for 30 minutes"shutdown"— the task was disposed (panel closed, task switched, extension deactivated)Idle and shutdown flushes. A 5-minute check interval fires an idle installment after 30 minutes of quiet.
dispose()flushes any unreported activity as a shutdown installment. Neither path mutatestask.toolUsageortask.messageCounts— those stay running totals for the publicRooCodeEventName.TaskCompletedAPI event and the UI.Stale replay suppression. When a user revisits a completed subtask from history, the handler runs again on a fresh
Taskinstance with a zero baseline. TheisStaleHistoryReplayflag (set whenhistoryItem.status === "completed") prevents that replay from emitting a duplicate installment or a duplicate publicTaskCompletedevent.Public API preserved.
RooCodeEventName.TaskCompletedstill represents genuine task completion or acceptance. The PostHog telemetry flush is independent of it and fires earlier (on every model-initiatedattempt_completion).Retry symmetry.
messageCounts.userdecrements whenTask.tspops a message on an empty assistant response, and restores exactly once on the next successful attempt viauserMessageWasRemoved. The manual-retry branch now also setsuserMessageWasRemoved: true(previously it did not, causing the message count to stay at zero on a user-approved retry). The decline-to-retry branch increments bothmessageCounts.userandmessageCounts.assistantto match the messages it re-adds directly.Scope
Included:
Taskmessage and tool-usage counters for completion telemetryTask Completedpayload additions:completionReason,toolsUsed,messageCountattempt_completionExcluded (per scope):
Test Procedure
pnpm check-typespasses repo-widepnpm lintpasses with no suppression count increasespackages/telemetry/src/__tests__/TelemetryService.task-completed.test.ts—captureTaskCompletedsignature (default reason, with summaries, idle reason)src/core/task/__tests__/messageCounting.spec.ts—shouldAddUserMessageToHistorypredicate (7 cases)src/core/task/__tests__/messageCounting.retrySymmetry.spec.ts— increment/decrement/re-increment symmetry across retry cyclessrc/core/task/__tests__/Task.spec.ts—flushTelemetryInstallment(first/second/no-op installments, failure deltas, idle timer, dispose flush, disposed-task timer stop)src/core/tools/__tests__/attemptCompletionTool.spec.ts— attempt completion before acceptance, feedback-instead-of-acceptance, stale replay suppression, no duplicate publicTaskCompletedon replayPre-Submission Checklist