feature: local-usage-stats (2/4) - #1131
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThis PR adds usage-statistics schemas and extension messages, records task API usage, stores events durably, aggregates statistics with timezone and cost handling, and exposes querying, exports, clearing, backfill, and change notifications. ChangesUsage statistics
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Task
participant UsageRecorder
participant UsageEventStore
participant UsageStatsService
participant UsageAggregator
Task->>UsageRecorder: finalize terminal API usage
UsageRecorder->>UsageEventStore: append UsageEventV1
UsageStatsService->>UsageEventStore: readAll stored events
UsageStatsService->>UsageAggregator: query events and StatsQuery
UsageAggregator-->>UsageStatsService: StatsSnapshot
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
packages/types/src/vscode-extension-host.ts-758-761 (1)
758-761: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the pre-parse query type for the webview payload.
StatsQuerymakesincludeCancelledrequired because Zod adds its default, but valid raw queries can omit it. ChangeusageStatsQueryto the parsed input type and parse it before passing the resultingStatsQueryto aggregation.🤖 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 `@packages/types/src/vscode-extension-host.ts` around lines 758 - 761, Change usageStatsQuery in the usage stats request payload definitions to use the pre-parse input type so callers may omit includeCancelled. Before aggregation, parse the payload query with the existing StatsQuery schema and pass the resulting StatsQuery object to the aggregation flow.src/services/stats/UsageRecorder.ts-113-113 (1)
113-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA cost of exactly 0 is dropped.
ctx.totalCost ? { ... } : undefinedomitscostUsdwhentotalCostis0. A zero cost is meaningful for local and free-tier models: it means "known to be free", not "unknown". The aggregator cannot distinguish the two cases, and provider-pricing recalculation may then substitute a derived cost for a request that was genuinely free.Test for
undefinedinstead of truthiness.🐛 Proposed fix
- costUsd: ctx.totalCost ? { value: ctx.totalCost, source: ctx.costSource } : undefined, + costUsd: ctx.totalCost !== undefined ? { value: ctx.totalCost, source: ctx.costSource } : undefined,🤖 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/services/stats/UsageRecorder.ts` at line 113, Update the costUsd assignment in UsageRecorder to check ctx.totalCost explicitly against undefined rather than using a truthiness check, preserving a cost value of exactly 0 while still omitting the field when no cost is known.src/services/stats/UsageEventStore.ts-675-688 (1)
675-688: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe quarantine report grows without bound.
readAll()re-scans every segment on each call and appends a quarantine entry for each corrupt line it finds. A corrupt line in the middle of a segment is never removed or rewritten, so everyreadAll()call appends a new entry for the same line. Repeated dashboard queries makecorrupt-lines.jsonlgrow without limit, and the store applies its 100 MiB cap only toevents-*.ndjsonfiles.Deduplicate by
segment:line:hashbefore writing, and bound the report size.🐛 Proposed fix: skip entries already reported in this session
+ /** 이미 보고한 corrupt line 식별자 (segment:line:hash) */ + private reportedQuarantineKeys: Set<string> = new Set() + private async writeQuarantineReport(entries: QuarantineReportEntry[]): Promise<void> { try { - const lines = entries.map((e) => JSON.stringify(e)).join("\n") + "\n" + const fresh = entries.filter((e) => { + const key = `${e.segment}:${e.line}:${e.hash}` + if (this.reportedQuarantineKeys.has(key)) { + return false + } + this.reportedQuarantineKeys.add(key) + return true + }) + if (fresh.length === 0) { + return + } + const lines = fresh.map((e) => JSON.stringify(e)).join("\n") + "\n" const handle = await fs.open(this.quarantineReportPath, "a")🤖 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/services/stats/UsageEventStore.ts` around lines 675 - 688, Update the quarantine reporting flow around writeQuarantineReport and readAll to deduplicate entries using segment, line, and hash before appending, including entries already written during the current session. Also enforce a size limit for corrupt-lines.jsonl, retaining the existing event-file cap or an established equivalent rather than allowing the quarantine report to grow without bound.src/services/stats/UsageEventStore.ts-431-471 (1)
431-471: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSegment rotation increments the manifest but writes to the old segment.
Line 431 computes
segmentPathfrommanifest.currentSegmentbefore the rotation check. Line 447 incrementsmanifest.currentSegmentand persists the manifest, but Line 457 still opens the stalesegmentPath. The event that triggers rotation is therefore appended to the segment that already reachedSEGMENT_MAX_BYTES.Recompute the path after the increment.
🐛 Proposed fix: recompute the segment path after rotation
const manifest = await this.loadOrCreateManifest() - const segmentPath = this.getSegmentPath(manifest.currentSegment) + let segmentPath = this.getSegmentPath(manifest.currentSegment) // segment 파일이 존재하는지 확인하고 크기 체크 let segmentSize = 0 @@ // segment 회전 확인 if (segmentSize >= SEGMENT_MAX_BYTES) { manifest.currentSegment += 1 manifest.updatedAt = new Date().toISOString() await this.writeManifestAtomic(manifest) + segmentPath = this.getSegmentPath(manifest.currentSegment) }🤖 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/services/stats/UsageEventStore.ts` around lines 431 - 471, Update the segment rotation flow in the event append method so that after incrementing and persisting manifest.currentSegment, segmentPath is recomputed with getSegmentPath(manifest.currentSegment) before opening the file. Keep the existing size check and append behavior unchanged for non-rotated segments.src/services/stats/UsageEventStore.ts-155-186 (1)
155-186: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
initialize()against concurrent callers.
ensureInitialized()checksthis.initializedand awaitsinitialize().this.initializedis set only at the end ofinitialize(). IfreadAll()andappend()run without an intervening await, both enterinitialize()and runrebuildIdempotencySet()concurrently.rebuildIdempotencySet()starts withthis.idempotencyKeys.clear()(Line 584), so a racing rebuild can erase a key that the other path already added, and the store then writes a duplicate event.Cache the in-flight initialization promise so concurrent callers share one run.
🔒 Proposed fix: memoize the initialization promise
/** 초기화 완료 여부 */ private initialized = false + + /** 진행 중인 초기화 promise (동시 호출 직렬화용) */ + private initPromise: Promise<void> | undefinedasync initialize(): Promise<void> { if (this.initialized) { return } + if (this.initPromise) { + return this.initPromise + } + this.initPromise = this.initializeInternal().finally(() => { + this.initPromise = undefined + }) + return this.initPromise + } + private async initializeInternal(): Promise<void> { try {🤖 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/services/stats/UsageEventStore.ts` around lines 155 - 186, Update UsageEventStore.initialize and its initialization flow to memoize the in-flight initialization promise, so concurrent callers share one execution instead of entering rebuildIdempotencySet multiple times. Preserve the existing initialized fast path and ensure the cached promise is cleared after completion or failure, allowing later retries when initialization fails.src/services/stats/__tests__/UsageAggregator.spec.ts-640-657 (1)
640-657: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the exact ISO week assignments.
This test only checks the key format. It passes if every event receives an incorrect week.
Assert the expected bucket keys and event counts. The comments also assign different ISO weeks to July 13 and July 15, 2026, although both dates are in the same Monday-based ISO week.
🤖 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/services/stats/__tests__/UsageAggregator.spec.ts` around lines 640 - 657, Strengthen the “should group events by ISO week bucket” test to assert the exact bucket keys and event counts rather than only the key format. Correct the expected ISO-week comments and expectations so July 13, July 15, and July 20, 2026 are assigned to their actual Monday-based ISO weeks, with the first two events sharing a bucket and the third in the following week.src/services/stats/__tests__/UsageStatsService.spec.ts-846-854 (1)
846-854: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the nonce fallback branch.
This test calls the normal
crypto.randomUUID()path. It does not makerequire("crypto")fail, so the catch branch remains untested.Force the crypto path to fail and call the public
issueClearNonce()API. Avoid the double assertion used for private access.As per coding guidelines, “Use bracket notation for private members where appropriate” and “Use double assertions only as a last resort and explain them with a comment.”
<coding_guidelines>🤖 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/services/stats/__tests__/UsageStatsService.spec.ts` around lines 846 - 854, Update the “generateNonce fallback” test to force the crypto dependency used by generateNonce to fail, then exercise the fallback through the public issueClearNonce() API. Remove the private-method access and its double assertion, while preserving assertions that the returned nonce is a non-empty string.Source: Coding guidelines
src/services/stats/__tests__/UsageStatsService.spec.ts-729-741 (1)
729-741: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrigger a real
StatsStoreErrorin this test.This test performs deduplication only.
UsageEventStore.append()returnsfalse; it does not throwStatsStoreError.Use a precise store test double that rejects one append with
StatsStoreError. Then verify that processing continues with the next event.🤖 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/services/stats/__tests__/UsageStatsService.spec.ts` around lines 729 - 741, The test named "should swallow StatsStoreError and continue processing remaining events" currently tests deduplication (where UsageEventStore.append returns false) rather than actual error handling. Update the test to configure the store test double to throw a StatsStoreError on the append call for one of the three events (for example, the second event), while the others append successfully. Then verify that the count reflects only the successfully processed events, demonstrating that backfillFromHistory continues processing after swallowing the StatsStoreError.
🧹 Nitpick comments (6)
src/services/stats/UsageEventStore.ts (2)
526-546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
writeManifestAtomicalways reports theappenderror code.
writeManifestAtomicthrowsSTATS_STORE/append/005on every failure.clear()also reaches this method throughloadOrCreateManifest()andwriteManifestAtomic(newManifest). A manifest write failure during a clear is therefore reported with anappendcode, and the declaredSTATS_STORE/clear/002code never describes it.Pass the code from the caller so diagnostics match the operation.
♻️ Proposed refactor: parameterize the error code
- private async writeManifestAtomic(manifest: UsageStatsManifest): Promise<void> { + private async writeManifestAtomic( + manifest: UsageStatsManifest, + errorCode: StatsStoreErrorCode = "STATS_STORE/append/005", + ): Promise<void> { const tempPath = `${this.manifestPath}.tmp.${Date.now()}.${Math.random().toString(36).slice(2)}` @@ throw new StatsStoreError( - "STATS_STORE/append/005", + errorCode, "Failed to write manifest atomically", err, )Then call
await this.writeManifestAtomic(newManifest, "STATS_STORE/clear/002")inclear().🤖 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/services/stats/UsageEventStore.ts` around lines 526 - 546, Update writeManifestAtomic to accept the caller’s operation-specific error code and use it when constructing StatsStoreError instead of hardcoding STATS_STORE/append/005. Pass STATS_STORE/clear/002 from clear() when writing the cleared manifest, while preserving the append code at append() call sites.
652-670: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe hash does not match its documented format.
QuarantineReportEntry.hashis documented on Lines 96-97 as the first 16 characters of a SHA-256 hash.makeQuarantineEntryproduces an 8-character 32-bit hash instead. The comment on Lines 653-655 justifies this by dependency minimization, butnode:cryptois a built-in module andUsageRecorder.tsalready imports it.Use
crypto.createHash("sha256")so the value matches the documented contract and collisions become negligible.♻️ Proposed refactor: use SHA-256
+import * as crypto from "crypto"private makeQuarantineEntry(segment: string, line: number, content: string): QuarantineReportEntry { - // 간단한 hash (crypto 없이, content 기반) - // 실제 환경에서는 crypto.createHash를 사용할 수 있으나, - // 여기서는 의존성 최소화를 위해 간단한 hash를 사용한다. - let hash = 0 - for (let i = 0; i < content.length; i++) { - const char = content.charCodeAt(i) - hash = (hash << 5) - hash + char - hash = hash & hash // 32bit 정수로 유지 - } - const hashHex = (hash >>> 0).toString(16).padStart(8, "0") + // 원문은 저장하지 않고 SHA-256 앞 16자만 기록한다. + const hashHex = crypto.createHash("sha256").update(content, "utf-8").digest("hex").slice(0, 16) return { segment, line, hash: hashHex, at: new Date().toISOString(), } }🤖 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/services/stats/UsageEventStore.ts` around lines 652 - 670, Update makeQuarantineEntry to generate the hash with the existing built-in crypto dependency using SHA-256, then retain only the first 16 hexadecimal characters to match QuarantineReportEntry.hash’s documented contract. Remove the manual 32-bit hash implementation and its dependency-minimization comments.src/services/stats/__tests__/UsageEventStore.spec.ts (1)
276-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe cap test does not test the cap.
The test is named "should throw StatsStoreError with correct code on cap reached" but only asserts
store.isCapped() === false. It never reaches the cap and never asserts an error code.StatsStoreErroris imported on Line 9 and stays unused as a result. Rename the test to describe what it checks, or drive the cap by stubbingcheckTotalSize.Segment rotation is also untested. A rotation test would cover the path where
appendInternalincrementsmanifest.currentSegment.💚 Proposed change: assert the real behavior and add rotation coverage
- it("should throw StatsStoreError with correct code on cap reached", async () => { - // 이 테스트는 cap을 강제로 설정하기 어려우므로, isCapped() 메서드 동작만 확인 - expect(store.isCapped()).toBe(false) - }) + it("should report isCapped() as false for an empty store", () => { + expect(store.isCapped()).toBe(false) + }) + + it("should throw StatsStoreError with append/003 when the cap is reached", async () => { + // checkTotalSize를 stub하여 hard cap 도달 상태를 강제한다. + const internal = store as unknown as { capped: boolean } + internal.capped = true + + await expect(store.append(makeEvent())).rejects.toBeInstanceOf(StatsStoreError) + }) + + it("should rotate to the next segment when the current segment is full", async () => { + const segmentPath = path.join(store._getStatsDir(), "events-000001.ndjson") + // SEGMENT_MAX_BYTES(5 MiB)를 초과하도록 채운다. + await fs.writeFile(segmentPath, "x".repeat(5 * 1024 * 1024 + 1)) + + await store.append(makeEvent({ idempotencyKey: "idem-rotate" })) + + const manifest = await store.getManifest() + expect(manifest.currentSegment).toBe(2) + + // 회전 후의 이벤트는 새 segment에 기록되어야 한다. + const rotated = await fs.readFile(path.join(store._getStatsDir(), "events-000002.ndjson"), "utf-8") + expect(rotated.trim().split("\n")).toHaveLength(1) + })🤖 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/services/stats/__tests__/UsageEventStore.spec.ts` around lines 276 - 289, Replace the misleading cap-reached test around store.isCapped() with either a test name that accurately describes the uncapped-state assertion or a real cap scenario by stubbing checkTotalSize and asserting the thrown StatsStoreError code. Also add coverage for segment rotation by driving appendInternal until the manifest currentSegment increments, using the existing store and event helpers.src/core/task/Task.ts (1)
3363-3385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
UsageRecordingContextconstruction.The completed path (Lines 3218-3241) and this failed/cancelled path build the same 14-field
UsageRecordingContextwith identical provider, model, mode, semantics, and source values. Only the token values andattemptdiffer. The two copies must stay in sync whenever the context type changes.Extract a private helper on
Taskand call it from both sites.♻️ Proposed refactor: one context builder
/** * terminal finalize에서 사용할 UsageRecordingContext를 만든다. * provider/model/mode/semantics는 두 terminal path에서 동일하다. */ private buildUsageRecordingContext( attempt: number, tokens: { input: number; output: number; cacheWrite: number; cacheRead: number; total?: number }, ): UsageRecordingContext { const apiProvider = this.apiConfiguration.apiProvider return { taskId: this.taskId, parentTaskId: this.parentTaskId, provider: apiProvider && !isRetiredProvider(apiProvider) ? apiProvider : "unknown", model: getModelId(this.apiConfiguration) || "unknown", mode: this._taskMode || defaultModeSlug, attempt, inputTokens: tokens.input, outputTokens: tokens.output, cacheWriteTokens: tokens.cacheWrite, cacheReadTokens: tokens.cacheRead, totalCost: tokens.total, // V1 semantics: provider-reported values, inclusion unknown cacheReadInInput: "unknown", cacheWriteInInput: "unknown", reasoningInOutput: "unknown", costSource: "provider", tokenSource: "provider", } }🤖 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 3363 - 3385, Extract the duplicated UsageRecordingContext construction into a private Task helper, such as buildUsageRecordingContext, centralizing the shared task, provider, model, mode, semantic, and source fields. Accept attempt and token values as parameters, then update both the completed path and the failed/cancelled path to call the helper while preserving their distinct values.src/core/task/__tests__/Task.usage-stats.spec.ts (2)
281-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated mock store and call-inspection boilerplate.
The four-line
mockStoreliteral is repeated in eleven tests. The expression(mockStore.append as unknown as ReturnType<typeof vi.fn>).mock.calls[0][0]is repeated about twelve times. Two small helpers remove both repetitions and make each assertion readable.♻️ Proposed refactor: helpers for the mock store and recorded events
function makeMockStore(appendImpl?: () => Promise<boolean>) { const append = appendImpl ? vi.fn().mockImplementation(appendImpl) : vi.fn().mockResolvedValue(true) const store = { append, initialize: vi.fn().mockResolvedValue(undefined), } as unknown as UsageEventStore return { store, append } } /** append에 전달된 n번째 이벤트를 반환한다. */ function recordedEvent(append: ReturnType<typeof vi.fn>, index = 0): UsageEventV1 { return append.mock.calls[index][0] as UsageEventV1 }Each test then reads:
const { store, append } = makeMockStore() const recorder = new UsageRecorder(store) await recorder.finalizeUsageEvent("task-1:0", "completed", makeRecordingContext()) expect(append).toHaveBeenCalledTimes(1) expect(recordedEvent(append).status).toBe("completed")🤖 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.usage-stats.spec.ts` around lines 281 - 291, Extract shared makeMockStore and recordedEvent helpers in Task.usage-stats.spec.ts, then replace the repeated UsageEventStore mock literals and append mock call-inspection expressions across the tests. Keep support for custom append implementations, return the append spy alongside the store, and use recordedEvent for indexed event access while preserving existing assertions.
265-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test covers the Task terminal-finalize integration.
The file header states the goals: record only at terminal finalize, distinguish completed, failed, and cancelled partial usage, and isolate store errors from task results. Every behavioral test calls
recorder.finalizeUsageEventdirectly with a mock store. No test drivesTaskand asserts that the recorder is called fromcaptureUsageDataor from the streaming-failurecatchblock.Four tests (Lines 265-278, 468-482, 484-494, 496-508) assert the same fact:
usageRecorderis a non-nullUsageRecorder. Together they cover construction only.The untested integration boundary is where the
requestKeyis built. A test that runs two API turns in one task and asserts two distinctidempotencyKeyvalues onstore.appendwould catch the collision reported onTask.tsLines 3216-3246.Do you want me to draft a Task-level integration test that injects a stub recorder and asserts one event per API turn?
🤖 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.usage-stats.spec.ts` around lines 265 - 278, Add a Task-level integration test that injects a stub UsageRecorder/store and drives two API turns through Task, exercising captureUsageData and terminal finalization. Assert that store.append receives exactly one event per turn and that each event has a distinct idempotencyKey derived from the requestKey. Include the streaming-failure path if needed to verify failed or cancelled turns are finalized without affecting task results.
🤖 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 `@packages/types/src/usage-stats.ts`:
- Around line 20-23: Update SourcedNumber and the UsageEventV1 usage schema so
token fields use a non-negative integer validator, while cost fields retain
numeric precision but reject negative values. Ensure the distinct token and cost
schemas are applied to the corresponding fields instead of reusing SourcedNumber
for both domains.
- Around line 39-40: Update the usage-stat schema definitions for occurredAt,
from, and to to validate ISO date-time values rather than arbitrary strings,
preserving the UTC-only contract for occurredAt. Add schema rejection tests
covering malformed timestamps, invalid range values, and non-UTC occurredAt
values.
- Line 42: In packages/types/src/usage-stats.ts at line 42, update the `attempt`
field schema from unconstrained `z.number()` to `z.number().int().nonnegative()`
to enforce that only non-negative integers are accepted. In
packages/types/src/__tests__/usage-stats.spec.ts at lines 133-138, replace or
extend the existing test to separately validate that invalid negative values
(attempt: -1) throw an error and that valid zero values (attempt: 0) pass
validation, removing any intermediate test cases that only check zero.
In `@src/core/task/Task.ts`:
- Around line 3216-3246: Make request keys unique per API request in both
finalize sites: src/core/task/Task.ts lines 3216-3246 and 3360-3390. Update the
requestKey construction in captureUsageData to include the in-scope
lastApiReqIndex alongside taskId and retryAttempt, using the same format for
successful, failed, and cancelled turns.
- Around line 553-562: The UsageEventStore must be shared across tasks instead
of being constructed inside each Task. Add or reuse an extension-host-scoped
usage-stats service that owns one UsageEventStore for the shared
globalStoragePath, inject that service into Task instances, and update the
UsageRecorder initialization in Task to use the injected shared store while
preserving the existing best-effort failure behavior.
In `@src/services/stats/__tests__/UsageStatsService.spec.ts`:
- Around line 78-85: Update the afterEach cleanup to call service.dispose()
before removing tempDir, ensuring the FileSystemWatcher is released before
filesystem cleanup and preventing leaked handles or cross-test callbacks.
In `@src/services/stats/costRecalculation.ts`:
- Around line 110-117: The fallback matching loop in the model-ID resolution
logic should stop using unrestricted lowerModel.includes matching. Update the
check around knownIds, sortedIds, and registry so it accepts only an exact known
ID or a documented version suffix beginning with “${knownId}-”, while preserving
longest-ID-first ordering and avoiding fabricated matches from embedded custom
model names.
- Around line 162-169: Update the cost calculation flow around
calculateApiCostAnthropic and calculateApiCostOpenAI to normalize inputTokens,
outputTokens, cacheWriteTokens, and cacheReadTokens using the inclusion
semantics recorded in event.semantics before invoking either helper. Stop
selecting token interpretation solely from event.provider, while preserving the
provider-specific pricing helper selection.
In `@src/services/stats/UsageAggregator.ts`:
- Around line 394-420: The source-grouped aggregation currently assigns the full
event to every reported source, duplicating values across buckets. Update the
source handling in UsageAggregator to associate each metric only with its own
source, or consistently select one documented event-level source, then update
the source-group assertions in
src/services/stats/__tests__/UsageAggregator.spec.ts lines 867-887 to verify
bucket values and event counts; both affected sites require changes.
- Around line 247-267: Both UsageAggregator.ts (lines 247-267) and
UsageStatsService.ts (lines 386-468) have separate DST-unsafe implementations
that use the offset from the supplied instant. Fix the root cause by updating
the startOfDay method in UsageAggregator.ts to resolve the timezone offset at
local midnight itself rather than at the supplied input date, then calculate the
UTC time correctly using that resolved offset. After fixing startOfDay to be
DST-safe, refactor UsageStatsService.ts (lines 386-468) to reuse the corrected
startOfDay helper instead of duplicating the boundary logic, ensuring both
queries and exports use the same calculation and cannot diverge.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 340-364: Update clear()’s segment-file rename loop to track
whether any fs.rename operation fails, and reject or throw after the loop when a
failure occurred instead of writing the new manifest and reporting success.
Preserve the existing warning log, but ensure the failure propagates to the
caller so clear() does not claim completion while unreadable old segments remain
in statsDir.
- Around line 222-302: Update UsageEventStore.readAll() to process each segment
with a streaming line reader instead of fs.readFile(), splitting the full file,
and preserve existing JSON/schema validation, crash-tail handling, and
quarantine reporting. Add a cache keyed by each segment’s size and mtime so
unchanged segments reuse parsed events while changed or new segments are
streamed and reparsed. Ensure cache invalidation handles removed segments and
readAll() returns the combined current event set.
- Around line 571-574: Remove the throw statement from the onCompromised
callback in UsageEventStore to prevent uncaught exceptions during lock renewal.
Keep the console.error log for visibility, and instead mark the store as
unusable by setting an internal flag or state variable (such as an existing
property used to track store health) to indicate the manifest lock was
compromised. This allows the compromise to be handled gracefully as a storage
error rather than breaking the promise chain.
In `@src/services/stats/UsageRecorder.ts`:
- Around line 79-83: Update finalizeUsageEvent in UsageRecorder so finalizedKeys
is updated only after append completes successfully, allowing failed writes to
be retried; preserve the existing duplicate check. In the append failure catch
block, log the error with sufficient context instead of silently swallowing it.
In `@src/services/stats/UsageStatsService.ts`:
- Around line 125-128: Update UsageStatsService.initialize and setupFileWatcher
so repeated initialization does not create multiple active watchers. Guard
watcher creation when an existing watcher is active, or dispose the existing
watcher before replacing it, while preserving the idempotent store
initialization.
- Around line 337-339: Update the file-watcher subscription setup around the
existing onDidChange and onDidCreate calls to also register onDidDelete(notify),
ensuring deletion events trigger the same cross-window refresh callback before
the surrounding try block completes.
---
Minor comments:
In `@packages/types/src/vscode-extension-host.ts`:
- Around line 758-761: Change usageStatsQuery in the usage stats request payload
definitions to use the pre-parse input type so callers may omit
includeCancelled. Before aggregation, parse the payload query with the existing
StatsQuery schema and pass the resulting StatsQuery object to the aggregation
flow.
In `@src/services/stats/__tests__/UsageAggregator.spec.ts`:
- Around line 640-657: Strengthen the “should group events by ISO week bucket”
test to assert the exact bucket keys and event counts rather than only the key
format. Correct the expected ISO-week comments and expectations so July 13, July
15, and July 20, 2026 are assigned to their actual Monday-based ISO weeks, with
the first two events sharing a bucket and the third in the following week.
In `@src/services/stats/__tests__/UsageStatsService.spec.ts`:
- Around line 846-854: Update the “generateNonce fallback” test to force the
crypto dependency used by generateNonce to fail, then exercise the fallback
through the public issueClearNonce() API. Remove the private-method access and
its double assertion, while preserving assertions that the returned nonce is a
non-empty string.
- Around line 729-741: The test named "should swallow StatsStoreError and
continue processing remaining events" currently tests deduplication (where
UsageEventStore.append returns false) rather than actual error handling. Update
the test to configure the store test double to throw a StatsStoreError on the
append call for one of the three events (for example, the second event), while
the others append successfully. Then verify that the count reflects only the
successfully processed events, demonstrating that backfillFromHistory continues
processing after swallowing the StatsStoreError.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 675-688: Update the quarantine reporting flow around
writeQuarantineReport and readAll to deduplicate entries using segment, line,
and hash before appending, including entries already written during the current
session. Also enforce a size limit for corrupt-lines.jsonl, retaining the
existing event-file cap or an established equivalent rather than allowing the
quarantine report to grow without bound.
- Around line 431-471: Update the segment rotation flow in the event append
method so that after incrementing and persisting manifest.currentSegment,
segmentPath is recomputed with getSegmentPath(manifest.currentSegment) before
opening the file. Keep the existing size check and append behavior unchanged for
non-rotated segments.
- Around line 155-186: Update UsageEventStore.initialize and its initialization
flow to memoize the in-flight initialization promise, so concurrent callers
share one execution instead of entering rebuildIdempotencySet multiple times.
Preserve the existing initialized fast path and ensure the cached promise is
cleared after completion or failure, allowing later retries when initialization
fails.
In `@src/services/stats/UsageRecorder.ts`:
- Line 113: Update the costUsd assignment in UsageRecorder to check
ctx.totalCost explicitly against undefined rather than using a truthiness check,
preserving a cost value of exactly 0 while still omitting the field when no cost
is known.
---
Nitpick comments:
In `@src/core/task/__tests__/Task.usage-stats.spec.ts`:
- Around line 281-291: Extract shared makeMockStore and recordedEvent helpers in
Task.usage-stats.spec.ts, then replace the repeated UsageEventStore mock
literals and append mock call-inspection expressions across the tests. Keep
support for custom append implementations, return the append spy alongside the
store, and use recordedEvent for indexed event access while preserving existing
assertions.
- Around line 265-278: Add a Task-level integration test that injects a stub
UsageRecorder/store and drives two API turns through Task, exercising
captureUsageData and terminal finalization. Assert that store.append receives
exactly one event per turn and that each event has a distinct idempotencyKey
derived from the requestKey. Include the streaming-failure path if needed to
verify failed or cancelled turns are finalized without affecting task results.
In `@src/core/task/Task.ts`:
- Around line 3363-3385: Extract the duplicated UsageRecordingContext
construction into a private Task helper, such as buildUsageRecordingContext,
centralizing the shared task, provider, model, mode, semantic, and source
fields. Accept attempt and token values as parameters, then update both the
completed path and the failed/cancelled path to call the helper while preserving
their distinct values.
In `@src/services/stats/__tests__/UsageEventStore.spec.ts`:
- Around line 276-289: Replace the misleading cap-reached test around
store.isCapped() with either a test name that accurately describes the
uncapped-state assertion or a real cap scenario by stubbing checkTotalSize and
asserting the thrown StatsStoreError code. Also add coverage for segment
rotation by driving appendInternal until the manifest currentSegment increments,
using the existing store and event helpers.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 526-546: Update writeManifestAtomic to accept the caller’s
operation-specific error code and use it when constructing StatsStoreError
instead of hardcoding STATS_STORE/append/005. Pass STATS_STORE/clear/002 from
clear() when writing the cleared manifest, while preserving the append code at
append() call sites.
- Around line 652-670: Update makeQuarantineEntry to generate the hash with the
existing built-in crypto dependency using SHA-256, then retain only the first 16
hexadecimal characters to match QuarantineReportEntry.hash’s documented
contract. Remove the manual 32-bit hash implementation and its
dependency-minimization comments.
🪄 Autofix
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: 22255d96-2d62-4db6-a6a2-6b46d931c5a6
📒 Files selected for processing (18)
packages/types/src/__tests__/usage-stats.spec.tspackages/types/src/index.tspackages/types/src/providers/qwen-code.tspackages/types/src/usage-stats.tspackages/types/src/vscode-extension-host.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.usage-stats.spec.tssrc/eslint-suppressions.jsonsrc/services/stats/UsageAggregator.tssrc/services/stats/UsageEventStore.tssrc/services/stats/UsageRecorder.tssrc/services/stats/UsageStatsService.tssrc/services/stats/__tests__/UsageAggregator.spec.tssrc/services/stats/__tests__/UsageEventStore.spec.tssrc/services/stats/__tests__/UsageStatsService.spec.tssrc/services/stats/__tests__/costRecalculation.spec.tssrc/services/stats/costRecalculation.tssrc/services/stats/index.ts
| export const SourcedNumber = z.object({ | ||
| value: z.number(), | ||
| source: UsageValueSource, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching usage-stats.ts:"
fd -a 'usage-stats\.ts$' . || true
file="$(fd 'usage-stats\.ts$' . | head -n 1 || true)"
if [ -n "$file" ]; then
echo "--- outline ---"
ast-grep outline "$file" || true
echo "--- file excerpt ---"
cat -n "$file"
fi
echo "--- usages of SourcedNumber and UsageEventV1 ---"
rg -n "SourcedNumber|UsageEventV1|usage:" packages || true
echo "--- package/types path if exists ---"
git ls-files 'packages/types/src/*' | sed -n '1,120p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 16584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Zod version references ---"
rg -n '"zod"\s*[:=]|zod @|^zod|node_modules/|pnpm-lock.yaml|package.json' -S . \
--glob 'package.json' --glob 'pnpm-lock.yaml' --glob 'bun.lockb' --glob '*.lock' || true
echo "--- relevant source files that manipulate usage aggregation/totals ---"
rg -n "UsageAggregator|inputTokens|outputTokens|cacheReadTokens|cacheWriteTokens|reasoningTokens|totalTokens|costUsd|SourcedNumber|UsageEventV1" packages \
--iglob '!**/node_modules/**' --iglob '!**/dist/**' --iglob '!**/build/**' || true
echo "--- files likely containing usage aggregation ---"
fd -e ts . packages | rg 'usage|stats|aggreg|total' || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 11828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- files containing UsageAggregator / query usage stats aggregation ---"
rg -n "class UsageAggregator|UsageAggregator|query\\(\\)|occurredAt|InputTokens|outputTokens|cacheReadTokens|cacheWriteTokens|reasoningTokens|totalTokens|costUsd" packages -S || true
echo "--- inspect candidate files with usage-stat aggregation ---"
while IFS= read -r f; do
[ -z "$f" ] && continue
echo "### $f"
wc -l "$f"
ast-grep outline "$f" || true
done < <(rg -l "UsageAggregator|UsageStatistics|usage stats|UsageEventV1" packages -S 2>/dev/null || true)
echo "--- test imports/expectations for negative/fractional usage events if present ---"
sed -n '1,180p' packages/types/src/__tests__/usage-stats.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 9877
Restrict usage values by their domain.
SourcedNumber accepts negative and fractional values, and UsageEventV1.usage reuses it for token fields while keeping costs as SourcedNumber. Use a non-negative integer schema for token fields and a non-negative numeric schema for cost fields.
🤖 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 `@packages/types/src/usage-stats.ts` around lines 20 - 23, Update SourcedNumber
and the UsageEventV1 usage schema so token fields use a non-negative integer
validator, while cost fields retain numeric precision but reject negative
values. Ensure the distinct token and cost schemas are applied to the
corresponding fields instead of reusing SourcedNumber for both domains.
| occurredAt: z.string(), // ISO 8601 UTC | ||
| timezoneOffsetMinutes: z.number(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)packages/types/src/usage-stats\.ts$|usage-stats' || true
echo
echo "usage-stats outline:"
ast-grep outline packages/types/src/usage-stats.ts --view expanded || true
echo
echo "Relevant usage-stats.ts:"
wc -l packages/types/src/usage-stats.ts
sed -n '1,140p' packages/types/src/usage-stats.ts
echo
echo "Search query methods/usages:"
rg -n "query\\(|occurredAt|timezoneOffsetMinutes|UsageAggregator|from.*to|Date\\(" packages/types/src packages -g '!node_modules' -g '!dist' -g '!build' || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 26296
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find files with query methods and usage-stats usages:"
rg -n "class .*UsageAggregator|UsageAggregator|function query|query\\(" src packages -g '!node_modules' -g '!dist' -g '!build' || true
echo
echo "List matching source files:"
git ls-files 'src/**/*' 'packages/**/*' | grep -i 'usage' | head -80 || true
echo
echo "Search by filename/type near usage stats:"
rg -n "StatsQuery|UsageEventV1|from: z\\.string|to: z\\.string|occurredAt" . -g '!node_modules' -g '!dist' -g '!build' -g '!coverage' || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 27876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "UsageAggregator relevant sections:"
sed -n '1,110p' src/services/stats/UsageAggregator.ts
echo "---"
sed -n '120,165p' src/services/stats/UsageAggregator.ts
echo "---"
sed -n '430,590p' src/services/stats/UsageAggregator.ts
echo
echo "UsageStatsService relevant sections:"
sed -n '340,390p' src/services/stats/UsageStatsService.ts
sed -n '150,180p' src/services/stats/UsageStatsService.ts
echo
echo "Parse and runtime behavior probe:"
node - <<'JS'
const bad = [
"not-a-time",
"2026-01-01",
"2026-01-01T10:00:00",
"2026-01-01T10:00:00+00:00",
"2026-01-01T10:00:00.000Z",
];
for (const value of bad) {
const date = new Date(value);
const ms = date.getTime();
const compared = [
["ms >= from", nanOk(ms, NaN)],
["ms < to", nanOk(ms, NaN)],
["ms > from", nanOk(ms, NaN)],
["ms <= to", nanOk(ms, NaN)],
].map(([name, ok]) => `${name}=${ok}`).join(", ");
console.log(`${name(value)} -> JS:time=${ms} comparison=${compared}`);
}
console.log("zod type? zod is not available in this read-only probe.");
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 13714
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const values = [
"not-a-time",
"2026-01-01",
"2026-01-01T10:00:00",
"2026-01-01T10:00:00+00:00",
"2026-01-01T10:00:00.000Z",
];
for (const value of values) {
const date = new Date(value);
const ms = date.getTime();
const from = new Date("2026-01-01T00:00:00.000Z");
const to = new Date("2026-01-14T00:00:00.000Z");
const checks = {
"eventTime < from.getTime()": ms < from.getTime(),
"eventTime >= to.getTime()": ms >= to.getTime(),
"passes simple from/to filter": (ms >= from.getTime()) && (ms < to.getTime()),
};
console.log(JSON.stringify({ value, date: Number.isFinite(ms) ? date.toISOString() : "Invalid Date", ms, checks }));
}
JS
echo
echo "package zod dependency:"
if [ -f package.json ]; then jq '.devDependencies.zod // .dependencies.zod' package.json; fi
if [ -f packages/types/package.json ]; then jq '.devDependencies.zod // .dependencies.zod' packages/types/package.json; fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 1177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant tests around StatsQuery validation:"
sed -n '143,186p' packages/types/src/__tests__/usage-stats.spec.ts
echo
echo "Current suppression count for packages/types/src/usage-stats.ts:"
node - <<'JS'
const fs = require('fs');
const p = 'packages/types/src/usage-stats.ts';
const text = fs.readFileSync(p, 'utf8');
const count = (text.match(/\/\/ eslint-disable-next-line/g) || []).length + (text.match(/eslint-disable/g) || []).length;
console.log(`${p}: ${count}`);
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 1506
Reject malformed timestamps at the schema boundary.
occurredAt, from, and to currently accept arbitrary strings. UsageAggregator.query() and UsageStatsService.filterEventsByQuery() parse occurredAt/ranges with new Date(), so date-only strings enter aggregation and invalid date strings pass the current simple range checks. Validate these fields as ISO date-time values and reject non-UTC occurredAt if that remains the event contract. Add rejection tests for invalid values.
🤖 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 `@packages/types/src/usage-stats.ts` around lines 39 - 40, Update the
usage-stat schema definitions for occurredAt, from, and to to validate ISO
date-time values rather than arbitrary strings, preserving the UTC-only contract
for occurredAt. Add schema rejection tests covering malformed timestamps,
invalid range values, and non-UTC occurredAt values.
| occurredAt: z.string(), // ISO 8601 UTC | ||
| timezoneOffsetMinutes: z.number(), | ||
| status: UsageEventStatus, | ||
| attempt: z.number(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files 'packages/types/src/usage-stats.ts' 'packages/types/src/__tests__/usage-stats.spec.ts'
echo
echo "usage-stats.ts excerpt:"
sed -n '1,90p' packages/types/src/usage-stats.ts
echo
echo "usage-stats spec excerpt:"
sed -n '1,180p' packages/types/src/__tests__/usage-stats.spec.ts
echo
echo "Search for attempts in usage stats:"
rg -n "attempt|UsageEventV1|usage-stats" packages/types/src packages/types/src/__tests__Repository: Zoo-Code-Org/Zoo-Code
Length of output: 16033
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check installed Zod/runtime availability:"
node - <<'JS'
console.log(process.versions.node)
try {
const z = require("zod")
console.log("zod", require("zod/package.json").version)
const schema = z.number()
const accepted = []
const rejected = []
for (const value of [-1, -0.5, 0, 0.5, 1, NaN]) {
try {
schema.parse(value)
accepted.push(value)
} catch (e) {
rejected.push(value)
}
}
console.log("accepted", JSON.stringify(accepted))
console.log("rejected", JSON.stringify(rejected))
try {
z.number().int().nonnegative().parse(-1)
} catch (e) {
console.log("negative rejected by z.number().int().nonnegative():", e.message?.split("\n")[0] || e.toString())
}
} catch (e) {
console.log("zod not loadable:", e.message)
}
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 296
🌐 Web query:
Zod z.number accepts NaN documentation z.number() integers constraints
💡 Result:
In Zod, z.number does not accept NaN by default; it is designed to only validate finite numbers [1][2][3]. Attempting to parse NaN using a standard z.number schema will result in a validation error [1][4][3]. If you explicitly need to validate NaN, you should use the dedicated z.nan schema [1][2]. Regarding integer constraints, Zod provides specific helpers for integer validation: - z.int: Restricts numbers to safe integer ranges [5][6]. - z.int32: Restricts numbers to the int32 range [5][6]. These helpers are used to ensure the numeric input meets specific integer criteria, building upon the base number validation functionality [5][6].
Citations:
- 1: https://github.com/colinhacks/zod/blob/v4.4.3/packages/docs/content/api.mdx
- 2: https://zod.dev/api
- 3: https://mintlify.wiki/colinhacks/zod/api/primitives/number
- 4: https://github.com/colinhacks/zod/blob/c7805073/packages/zod/src/v4/classic/tests/number.test.ts
- 5: https://github.com/colinhacks/zod/blob/c7805073/packages/docs/content/api.mdx
- 6: https://github.com/colinhacks/zod/blob/912f0f51/packages/docs/content/api.mdx
Restrict and test the attempt retry counter.
attempt currently uses unconstrained z.number(), so negative and fractional retry values are accepted while the test coverage only checks 0. Set the contract in the schema, then test invalid negatives/fractions separately from a valid zero value.
packages/types/src/usage-stats.ts#L42-L42: usez.number().int().nonnegative()forattempt.packages/types/src/__tests__/usage-stats.spec.ts#L133-L138: assert thatattempt: -1throws. Keep a separate test forattempt: 0if zero is valid.
📍 Affects 2 files
packages/types/src/usage-stats.ts#L42-L42(this comment)packages/types/src/__tests__/usage-stats.spec.ts#L133-L138
🤖 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 `@packages/types/src/usage-stats.ts` at line 42, In
packages/types/src/usage-stats.ts at line 42, update the `attempt` field schema
from unconstrained `z.number()` to `z.number().int().nonnegative()` to enforce
that only non-negative integers are accepted. In
packages/types/src/__tests__/usage-stats.spec.ts at lines 133-138, replace or
extend the existing test to separately validate that invalid negative values
(attempt: -1) throw an error and that valid zero values (attempt: 0) pass
validation, removing any intermediate test cases that only check zero.
| // Initialize usage recorder (best-effort: failure results in null recorder) | ||
| // Store initialization is deferred to first append; here we only construct the recorder. | ||
| // If the store fails at runtime, UsageRecorder catches errors internally. | ||
| try { | ||
| const store = new UsageEventStore(this.globalStoragePath) | ||
| this.usageRecorder = new UsageRecorder(store) | ||
| } catch (err) { | ||
| console.warn(`[Task#${this.taskId}] Failed to initialize UsageRecorder, stats will be skipped:`, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find existing owners of a shared usage-stats service to inject into Task.
set -euo pipefail
rg -n -C 4 'new UsageEventStore\(|new UsageRecorder\(|UsageStatsService' src || true
fd -t f 'ClineProvider.ts' src --exec ast-grep outline {} --match 'Usage|Stats' \;Repository: Zoo-Code-Org/Zoo-Code
Length of output: 18899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Task usageRecorder construction"
sed -n '520,575p' src/core/task/Task.ts
echo
echo "## Stats imports/usages"
rg -n 'UsageEventStore|UsageRecorder|UsageStatsService|usageRecorder|recordUsage|finalizeUsageEvent|append\(' src --glob '!**/__tests__/**' | head -n 200
echo
echo "## UsageEventStore relevant implementation"
ast-grep outline src/services/stats/UsageEventStore.ts --view expanded || true
sed -n '1,260p' src/services/stats/UsageEventStore.ts
echo
echo "## UsageRecorder implementation"
ast-grep outline src/services/stats/UsageRecorder.ts --view expanded || true
sed -n '1,260p' src/services/stats/UsageRecorder.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 22482
Move the usage event store behind a shared service.
Task creates a new UsageEventStore for this.globalStoragePath, so each task has its own in-memory dedupe set, append queue, manifest lock state, and lazy segment scan for the same usage-stats directory. Concurrent tasks can therefore contend and lose retry room on manifest lock acquisition. Inject one usage-stats service for the extension host and share the same UsageEventStore across tasks.
🤖 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 553 - 562, The UsageEventStore must be
shared across tasks instead of being constructed inside each Task. Add or reuse
an extension-host-scoped usage-stats service that owns one UsageEventStore for
the shared globalStoragePath, inject that service into Task instances, and
update the UsageRecorder initialization in Task to use the injected shared store
while preserving the existing best-effort failure behavior.
| if (this.usageRecorder) { | ||
| const requestKey = `${this.taskId}:${currentItem.retryAttempt ?? 0}` | ||
| const ctx: UsageRecordingContext = { | ||
| taskId: this.taskId, | ||
| parentTaskId: this.parentTaskId, | ||
| provider: String( | ||
| this.apiConfiguration.apiProvider && !isRetiredProvider(this.apiConfiguration.apiProvider) | ||
| ? this.apiConfiguration.apiProvider | ||
| : "unknown", | ||
| ), | ||
| model: getModelId(this.apiConfiguration) || "unknown", | ||
| mode: this._taskMode || defaultModeSlug, | ||
| attempt: currentItem.retryAttempt ?? 0, | ||
| inputTokens: tokens.input, | ||
| outputTokens: tokens.output, | ||
| cacheWriteTokens: tokens.cacheWrite, | ||
| cacheReadTokens: tokens.cacheRead, | ||
| totalCost: tokens.total, | ||
| // V1 semantics: provider-reported values, inclusion unknown | ||
| // (aggregator handles double-counting via inclusion metadata) | ||
| cacheReadInInput: "unknown", | ||
| cacheWriteInInput: "unknown", | ||
| reasoningInOutput: "unknown", | ||
| costSource: "provider", | ||
| tokenSource: "provider", | ||
| } | ||
| // Fire-and-forget: store error must not block task | ||
| this.usageRecorder | ||
| .finalizeUsageEvent(requestKey, status, ctx) | ||
| .catch(() => {}) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
requestKey is not unique per API request, so both terminal paths record only the first turn of a task. Both finalize sites build requestKey as ${this.taskId}:${currentItem.retryAttempt ?? 0}. The agentic loop pushes non-retry turns without retryAttempt, so every turn resolves to 0. UsageRecorder.finalizedKeys and UsageEventStore.idempotencyKeys then discard every turn after the first for a given status.
src/core/task/Task.ts#L3216-L3246: add a per-request identifier torequestKey, for example${this.taskId}:${lastApiReqIndex}:${currentItem.retryAttempt ?? 0}, which is already in scope incaptureUsageData.src/core/task/Task.ts#L3360-L3390: buildrequestKeywith the same per-request identifier so failed and cancelled turns are recorded independently.
📍 Affects 1 file
src/core/task/Task.ts#L3216-L3246(this comment)src/core/task/Task.ts#L3360-L3390
🤖 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 3216 - 3246, Make request keys unique per
API request in both finalize sites: src/core/task/Task.ts lines 3216-3246 and
3360-3390. Update the requestKey construction in captureUsageData to include the
in-scope lastApiReqIndex alongside taskId and retryAttempt, using the same
format for successful, failed, and cancelled turns.
| const oldGenDir = path.join(this.statsDir, `old-generation-${manifest.generation}`) | ||
| await fs.mkdir(oldGenDir, { recursive: true }) | ||
|
|
||
| const allFiles = await fs.readdir(this.statsDir) | ||
| const segmentFiles = allFiles.filter( | ||
| (f) => f.startsWith(SEGMENT_PREFIX) && f.endsWith(SEGMENT_EXT), | ||
| ) | ||
|
|
||
| for (const file of segmentFiles) { | ||
| const oldPath = path.join(this.statsDir, file) | ||
| const newPath = path.join(oldGenDir, file) | ||
| try { | ||
| await fs.rename(oldPath, newPath) | ||
| } catch (err) { | ||
| // 이동 실패는 로그만 남기고 계속 | ||
| console.warn(`[UsageEventStore] failed to move old segment ${file}:`, err) | ||
| } | ||
| } | ||
|
|
||
| // 새 manifest 저장 (safeWriteJson 패턴: temp → rename) | ||
| await this.writeManifestAtomic(newManifest) | ||
|
|
||
| // idempotency set 초기화 | ||
| this.idempotencyKeys.clear() | ||
| this.capped = false |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
clear() reports success even when segment files were not moved.
readAll() does not filter by generation. It reads every events-*.ndjson file in statsDir. So the only operation that actually removes data in clear() is the fs.rename loop; the incremented generation has no effect on reads.
The loop catches each rename failure, logs a warning, and continues (Lines 351-356). clear() then writes the new manifest and resolves successfully. If a rename fails, for example with EPERM or EBUSY on Windows when a file is open, the user receives a successful "statistics cleared" result while every event stays readable through readAll().
Track rename failures and fail the operation, so the caller can report the real outcome. Filtering reads by generation would also make the guarantee independent of rename success.
🐛 Proposed fix: fail `clear()` when a segment cannot be moved
+ const failedMoves: string[] = []
for (const file of segmentFiles) {
const oldPath = path.join(this.statsDir, file)
const newPath = path.join(oldGenDir, file)
try {
await fs.rename(oldPath, newPath)
} catch (err) {
- // 이동 실패는 로그만 남기고 계속
console.warn(`[UsageEventStore] failed to move old segment ${file}:`, err)
+ failedMoves.push(file)
}
}
+ // 이동에 실패한 segment는 readAll()에서 계속 읽히므로 삭제가 완료되지 않았다.
+ if (failedMoves.length > 0) {
+ throw new Error(`Failed to move segments: ${failedMoves.join(", ")}`)
+ }
+
// 새 manifest 저장 (safeWriteJson 패턴: temp → rename)
await this.writeManifestAtomic(newManifest)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const oldGenDir = path.join(this.statsDir, `old-generation-${manifest.generation}`) | |
| await fs.mkdir(oldGenDir, { recursive: true }) | |
| const allFiles = await fs.readdir(this.statsDir) | |
| const segmentFiles = allFiles.filter( | |
| (f) => f.startsWith(SEGMENT_PREFIX) && f.endsWith(SEGMENT_EXT), | |
| ) | |
| for (const file of segmentFiles) { | |
| const oldPath = path.join(this.statsDir, file) | |
| const newPath = path.join(oldGenDir, file) | |
| try { | |
| await fs.rename(oldPath, newPath) | |
| } catch (err) { | |
| // 이동 실패는 로그만 남기고 계속 | |
| console.warn(`[UsageEventStore] failed to move old segment ${file}:`, err) | |
| } | |
| } | |
| // 새 manifest 저장 (safeWriteJson 패턴: temp → rename) | |
| await this.writeManifestAtomic(newManifest) | |
| // idempotency set 초기화 | |
| this.idempotencyKeys.clear() | |
| this.capped = false | |
| const oldGenDir = path.join(this.statsDir, `old-generation-${manifest.generation}`) | |
| await fs.mkdir(oldGenDir, { recursive: true }) | |
| const allFiles = await fs.readdir(this.statsDir) | |
| const segmentFiles = allFiles.filter( | |
| (f) => f.startsWith(SEGMENT_PREFIX) && f.endsWith(SEGMENT_EXT), | |
| ) | |
| const failedMoves: string[] = [] | |
| for (const file of segmentFiles) { | |
| const oldPath = path.join(this.statsDir, file) | |
| const newPath = path.join(oldGenDir, file) | |
| try { | |
| await fs.rename(oldPath, newPath) | |
| } catch (err) { | |
| console.warn(`[UsageEventStore] failed to move old segment ${file}:`, err) | |
| failedMoves.push(file) | |
| } | |
| } | |
| // 이동에 실패한 segment는 readAll()에서 계속 읽히므로 삭제가 완료되지 않았다. | |
| if (failedMoves.length > 0) { | |
| throw new Error(`Failed to move segments: ${failedMoves.join(", ")}`) | |
| } | |
| // 새 manifest 저장 (safeWriteJson 패턴: temp → rename) | |
| await this.writeManifestAtomic(newManifest) | |
| // idempotency set 초기화 | |
| this.idempotencyKeys.clear() | |
| this.capped = false |
🤖 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/services/stats/UsageEventStore.ts` around lines 340 - 364, Update
clear()’s segment-file rename loop to track whether any fs.rename operation
fails, and reject or throw after the loop when a failure occurred instead of
writing the new manifest and reporting success. Preserve the existing warning
log, but ensure the failure propagates to the caller so clear() does not claim
completion while unreadable old segments remain in statsDir.
| onCompromised: (err) => { | ||
| console.error(`[UsageEventStore] manifest lock was compromised:`, err) | ||
| throw err | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
proper-lockfile onCompromised option default behavior throws uncaught exception
💡 Result:
Yes, the proper-lockfile library's onCompromised option has a default behavior that throws an uncaught exception, which typically causes the Node.js process to exit [1][2][3]. The onCompromised option is a callback function executed when the lock is determined to be compromised—such as when the lock has become stale because the process failed to update the lockfile's modification time (mtime) within the expected threshold [3][4]. By default, this option is implemented as a function that simply throws the provided error [1][5]. Because this occurs within an asynchronous timer callback (used for lock renewal), the exception cannot be caught by a standard try-catch block surrounding the initial lock acquisition, leading to an unhandled exception that crashes the process [4][5]. To prevent this, it is recommended to provide a custom implementation for the onCompromised callback that handles the error gracefully (e.g., by logging the error, performing cleanup, or setting a flag) instead of throwing [3][6].
Citations:
- 1: https://github.com/moxystudio/node-proper-lockfile/blob/master/README.md
- 2: https://www.npmjs.com/package/proper-lockfile
- 3: Compromised locks moxystudio/node-proper-lockfile#11
- 4: proper-lockfile ECOMPROMISED throws inside timer callback, crashing the process cline/kanban#128
- 5: Can't use proper-lockfile while debugging memory leaks moxystudio/node-proper-lockfile#71
- 6: https://context7.com/moxystudio/node-proper-lockfile/llms.txt
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching UsageEventStore.ts:"
fd -a 'UsageEventStore\.ts$' . || true
file="$(fd 'UsageEventStore\.ts$' . | head -n 1 || true)"
if [ -z "$file" ]; then
echo "No UsageEventStore.ts file found"
exit 0
fi
echo
echo "File: $file"
wc -l "$file"
echo
echo "Relevant sections around initialize/appending/clearing/lock:"
sed -n '1,160p' "$file" | cat -n
echo "----"
sed -n '520,630p' "$file" | cat -n
echo "----"
sed -n '780,920p' "$file" | cat -n
echo "----"
sed -n '920,1040p' "$file" | cat -n
echo
echo "Search for append/clear handlers and try/catch usage:"
rg -n "function |async |public |private |append|clear|try|catch|process\.on|onCompromised|proper-lockfile|storage|errors" "$file"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 14063
Do not throw from onCompromised.
proper-lockfile calls onCompromised during lock renewal, outside the append()/clear() promise chain. Rethrowing turns a storage error into an uncaught exception instead of returning as StatsStoreError, so it can break the LLM task. Log the compromise and mark the store unusable without rethrowing.
🔒 Proposed fix: log without rethrowing
onCompromised: (err) => {
console.error(`[UsageEventStore] manifest lock was compromised:`, err)
- throw err
},🤖 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/services/stats/UsageEventStore.ts` around lines 571 - 574, Remove the
throw statement from the onCompromised callback in UsageEventStore to prevent
uncaught exceptions during lock renewal. Keep the console.error log for
visibility, and instead mark the store as unusable by setting an internal flag
or state variable (such as an existing property used to track store health) to
indicate the manifest lock was compromised. This allows the compromise to be
handled gracefully as a storage error rather than breaking the promise chain.
| const idempotencyKey = `${requestKey}:${status}` | ||
| if (this.finalizedKeys.has(idempotencyKey)) { | ||
| return | ||
| } | ||
| this.finalizedKeys.add(idempotencyKey) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The idempotency key is recorded before the append succeeds.
finalizeUsageEvent adds idempotencyKey to finalizedKeys on Line 83, then appends on Line 124. If append throws, for example on STATS_STORE/append/003 when the hard cap is reached or on STATS_STORE/append/004 on a write failure, the key stays in the set. A later call for the same request returns early on Line 81, so the event is never retried and is lost permanently.
The catch on Lines 125-128 also swallows the error with no log, so a persistent storage failure produces empty statistics and no diagnostic signal.
Record the key only after a successful append, and log the failure.
🐛 Proposed fix: mark the key after a successful append and log failures
// terminal finalize: idempotency check
const idempotencyKey = `${requestKey}:${status}`
if (this.finalizedKeys.has(idempotencyKey)) {
return
}
- this.finalizedKeys.add(idempotencyKey) try {
await this.store.append(event)
- } catch {
+ // append 성공 후에만 기록하여 실패한 이벤트가 재시도 가능하도록 한다.
+ this.finalizedKeys.add(idempotencyKey)
+ } catch (err) {
// store error must not break task
// STATS_STORE/append/* 오류는 UsageEventStore 내부에서 분류됨
+ console.warn(`[UsageRecorder] failed to append usage event ${idempotencyKey}:`, err)
}
}Note: UsageEventStore deduplicates on idempotencyKey as well, so a retry after a transient failure cannot create a duplicate record.
🤖 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/services/stats/UsageRecorder.ts` around lines 79 - 83, Update
finalizeUsageEvent in UsageRecorder so finalizedKeys is updated only after
append completes successfully, allowing failed writes to be retried; preserve
the existing duplicate check. In the append failure catch block, log the error
with sufficient context instead of silently swallowing it.
| async initialize(): Promise<void> { | ||
| await this.store.initialize() | ||
| this.setupFileWatcher() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make repeated initialization watcher-safe.
UsageEventStore.initialize() is idempotent, but UsageStatsService.initialize() creates a new watcher on every call. The assignment loses the previous watcher without disposing it.
Guard watcher creation or dispose the existing watcher before replacement.
🤖 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/services/stats/UsageStatsService.ts` around lines 125 - 128, Update
UsageStatsService.initialize and setupFileWatcher so repeated initialization
does not create multiple active watchers. Guard watcher creation when an
existing watcher is active, or dispose the existing watcher before replacing it,
while preserving the idempotent store initialization.
| this.watcher.onDidChange(notify) | ||
| this.watcher.onDidCreate(notify) | ||
| } catch { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Subscribe to file deletion events.
clearStats() can delete statistics segments. Another VS Code window will not receive a notification because the watcher handles only create and change events.
Register onDidDelete(notify) so cross-window dashboards refresh after a clear operation.
🤖 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/services/stats/UsageStatsService.ts` around lines 337 - 339, Update the
file-watcher subscription setup around the existing onDidChange and onDidCreate
calls to also register onDidDelete(notify), ensuring deletion events trigger the
same cross-window refresh callback before the surrounding try block completes.
github-actions
Bot
added
the
awaiting-review
Patch coverage checks were blocking 10+ PRs with 80%/70% thresholds. Changed to informational: true so patch coverage is reported but not a required status check.
1 participant
Stack Position
feature/local-usage-statsDescription
https://www.youtube.com/shorts/UHnnOCM1_f0
Full Feature Description
feature/local-usage-statsusage-stats.ts,src/services/stats, the provider/task capture pathsTask.ts, the stats IPCusageStatsMessageHandler.ts, and the UIDashboardView.tsxanduseDashboardStatsStream.ts.Why Split Into 17 PRs
Instead of submitting this feature as a single unified PR, it was split into individual PRs because as code size grows, safely reviewing a PR becomes very difficult. The feature was broken into mutually exclusive individual PRs so that each can be reviewed independently.
What This PR Specifically Changes
Adds date/provider/model/mode grouping, totals, cache ratio, unknown event handling, and provider-aware cost recalculation. Does not include live capture, database, IPC, or UI.
Included Files
src/services/stats/UsageAggregator.tssrc/services/stats/UsageStatsService.tssrc/services/stats/costRecalculation.tsExclusion Scope
Summary by CodeRabbit
New Features
Bug Fixes
Tests