feature: local-usage-stats (1/4) - #1123
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, persistent event storage, aggregation, exports, task lifecycle recording, and extension-host message contracts. It includes comprehensive validation and integration tests for the new statistics pipeline. ChangesUsage statistics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Task
participant UsageRecorder
participant UsageEventStore
participant UsageStatsService
participant UsageAggregator
Task->>UsageRecorder: finalizeUsageEvent(status, context)
UsageRecorder->>UsageEventStore: append(UsageEventV1)
UsageStatsService->>UsageEventStore: readAll()
UsageStatsService->>UsageAggregator: query(events, StatsQuery)
UsageAggregator-->>UsageStatsService: StatsSnapshot
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (23)
packages/types/src/vscode-extension-host.ts (2)
257-260: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake
dataoptional inexportUsageStatsResult.The shape requires
data: stringand also allowserror. On a failed export the producer has no data, so it must send a placeholder such asdata: "". The webview then cannot distinguish an empty export from a failure by shape alone.Model the result as a discriminated union, or mark
dataoptional.♻️ Proposed payload shape
- exportUsageStatsResult?: { format: "json" | "csv"; data: string; error?: string } + exportUsageStatsResult?: + | { success: true; format: "json" | "csv"; data: string } + | { success: false; format: "json" | "csv"; error: string }🤖 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 257 - 260, Update the exportUsageStatsResult type in the usage stats response payloads so failed exports can omit data, preferably by modeling success and failure as a discriminated union; otherwise make data optional while preserving the existing format and error fields.
758-761: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse one exported
ExportFormatunion.The literal union
"json" | "csv"now exists at line 260, at line 761, and asExportFormatinsrc/services/stats/UsageStatsService.ts. Export the union once frompackages/types/src/usage-stats.tsand reference it in all three places. A single source prevents drift when a third format is added.🤖 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, Define and export a shared ExportFormat union in usage-stats.ts, then replace the inline "json" | "csv" declarations in the usage-stats query types and the usage-stats request payload with references to ExportFormat. Update UsageStatsService to import and reuse the same exported type, preserving the current supported formats.src/services/stats/__tests__/UsageEventStore.spec.ts (2)
224-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a
clearthat races an in-flightappend.The
clearsuite always awaits everyappendbefore it callsclear. It never exercises the concurrent case.clearbypasses the promise queue thatappenduses, as noted onsrc/services/stats/UsageEventStore.tslines 309-315.Start several appends without awaiting them, call
clear, await all of them, and then assert thatreadAllreturns a deterministic result.🤖 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 224 - 259, Add a concurrent-operation test within the existing clear suite that starts several append calls without awaiting them, invokes clear before they finish, then awaits all append and clear promises. Assert that readAll returns a deterministic expected result, covering the race caused by clear bypassing the append promise queue while preserving the existing clear tests.
101-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for segment rotation.
The
appendsuite exercises a single segment only. No test drivesappendInternalpastSEGMENT_MAX_BYTES, so the rotation branch has no coverage. That branch currently writes to the wrong file, as noted onsrc/services/stats/UsageEventStore.tslines 430-471.Add a test that pre-fills
events-000001.ndjsonbeyond 5 MiB, appends one event, and asserts that the event lands inevents-000002.ndjsonand thatgetManifest().currentSegmentis2.💚 Proposed test
+ it("should rotate to the next segment past SEGMENT_MAX_BYTES", async () => { + const segment1 = path.join(store._getStatsDir(), "events-000001.ndjson") + // 5 MiB를 넘도록 padding line을 채운다. + await fs.writeFile(segment1, "x".repeat(5 * 1024 * 1024) + "\n") + + const event = makeEvent({ eventId: "evt-rotated", idempotencyKey: "idem-rotated" }) + await store.append(event) + + const manifest = await store.getManifest() + expect(manifest.currentSegment).toBe(2) + + const segment2 = path.join(store._getStatsDir(), "events-000002.ndjson") + const content = await fs.readFile(segment2, "utf-8") + expect(JSON.parse(content.trim()).eventId).toBe("evt-rotated") + })🤖 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 101 - 161, Add a segment-rotation test within the append suite that pre-fills events-000001.ndjson beyond SEGMENT_MAX_BYTES, appends an event through store.append, and verifies the event is written to events-000002.ndjson rather than the original segment. Also assert getManifest().currentSegment equals 2, using the existing store setup and event helpers.src/services/stats/UsageEventStore.ts (5)
91-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe hash implementation does not match the documented contract.
The
QuarantineReportEntry.hashdoc at line 96 specifies a SHA-256 hash truncated to 16 characters.makeQuarantineEntrycomputes a 32-bit rolling hash and formats it as 8 hex characters. The comment at lines 653-655 justifies this by avoiding a dependency, butcryptois a Node built-in andsrc/services/stats/UsageRecorder.tsalready callscrypto.randomUUID().A 32-bit value also collides often. Corrupt lines with different content can share one hash, which weakens the report as a diagnostic.
Use
node:cryptoso the code matches the documented contract.♻️ Proposed implementation
+import { createHash } 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") + // 원문은 복사하지 않고 hash만 기록한다. + const hashHex = createHash("sha256").update(content, "utf-8").digest("hex").slice(0, 16) return { segment, line, hash: hashHex, at: new Date().toISOString(), } }Also applies to: 652-670
🤖 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 91 - 99, Update makeQuarantineEntry to compute the corrupt line hash with node:crypto’s SHA-256 implementation and truncate the resulting hexadecimal digest to 16 characters, matching QuarantineReportEntry.hash. Remove the existing 32-bit rolling-hash logic and its dependency-avoidance rationale, while preserving the remaining quarantine entry fields and behavior.
476-478: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid a full directory size scan on every append.
checkTotalSizecallsfs.readdirand thenfs.statfor each segment file. With the 100 MiB cap and 5 MiB segments that is up to 21 filesystem calls per event, in addition to the lock acquisition and thehandle.sync()at line 461. Every finalized LLM API call pays this cost.Track a running byte total instead. Add the written line length after each append, and re-scan only during
initializeand afterclear.🤖 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 476 - 478, Replace the per-append checkTotalSize call in the append flow with a running byte-total update based on the written line length, then update capped from that total. Initialize the total during initialize and recompute it after clear; keep checkTotalSize out of the normal append path while preserving the existing cap behavior.
583-616: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBound the idempotency rebuild scan.
This method reads every segment of the current generation with
fs.readFileand runsJSON.parseon every line. The hard cap allows 100 MiB across up to 20 segments, soinitializecan read 100 MiB and parse hundreds of thousands of lines.ensureInitializedruns this lazily on the firstappendorreadAll, which places the cost on the first LLM API call after startup.The set only needs to catch recent duplicate finalizations. Scan only the current segment, or read the last N lines, and record the chosen bound in a comment.
🤖 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 583 - 616, Update rebuildIdempotencySet to avoid scanning every historical segment: restrict the rebuild to the current segment, or a documented bounded tail of recent lines within it, while preserving idempotencyKeys population and existing missing-file/error handling. Add a comment near the bound explaining the chosen limit and update the loop/read logic accordingly.
188-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe doc comment does not match the dedupe location.
The comment at line 190 states that the dedupe check runs inside the lock. The check at line 413 runs before
acquireManifestLockat line 420. The in-process queue makes this safe within one process. Two extension hosts that share the same global storage can still write the sameidempotencyKey, because the set is in-memory only.Update the comment to describe the actual guarantee.
Also applies to: 412-415
🤖 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 188 - 195, The append method’s documentation incorrectly claims deduplication occurs inside the lock. Update the doc comment for the append operation to describe the actual in-memory/in-process queue guarantee and acknowledge that cross-process or shared-storage duplicate prevention is not guaranteed; keep the implementation unchanged.
709-721: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests reach the store through a widened public API instead of bracket notation.
UsageEventStoreexposes_-prefixed public methods so the spec can inspect internal state. The prefix is a convention only, so internal state becomes part of the public surface. The coding guidelines direct tests to reach private members with bracket notation.
src/services/stats/UsageEventStore.ts#L709-L721: make_getStatsDirand_getIdempotencyKeyCountprivate, or replace_getStatsDirwith areadonly statsDirproperty._getIdempotencyKeyCounthas no caller and can be removed.src/services/stats/__tests__/UsageEventStore.spec.ts#L276-L280: readstore["statsDir"]and setstore["capped"]with bracket notation, and use the importedStatsStoreErrorto assert theSTATS_STORE/append/003code.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards."🤖 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 709 - 721, In src/services/stats/UsageEventStore.ts lines 709-721, remove the public _getIdempotencyKeyCount method and make _getStatsDir private, or expose statsDir as readonly. In src/services/stats/__tests__/UsageEventStore.spec.ts lines 276-280, access statsDir and capped with bracket notation instead of the widened API, and use the imported StatsStoreError to assert the STATS_STORE/append/003 code; avoid any casts.Source: Coding guidelines
packages/types/src/usage-stats.ts (2)
69-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making
presetandfrom/tomutually exclusive.The schema accepts
presettogether withfromandto. The contract does not state which wins, so each consumer must decide. Add a.superRefinecheck, or document the precedence in a comment.🤖 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 69 - 76, The StatsQuery schema currently permits preset and explicit from/to ranges simultaneously without defining precedence. Update StatsQuery with a superRefine validation that rejects preset when from or to is provided, preserving all existing field validation and defaults.
36-56: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTighten numeric and timestamp validation in
UsageEventV1.
UsageEventStore.readAlluses this schema as the only gate before events reach aggregation.z.string()accepts anyoccurredAtvalue, andz.number()accepts negative, fractional,NaN-adjacent, andInfinity-free-but-huge token counts. A malformed line therefore passes validation and corrupts totals instead of going to quarantine.Add format and range constraints at the contract level.
♻️ Proposed stricter field constraints
+const NonNegativeInt = z.number().int().nonnegative() + export const SourcedNumber = z.object({ - value: z.number(), + value: z.number().finite().nonnegative(), source: UsageValueSource, })eventId: z.string(), idempotencyKey: z.string(), - occurredAt: z.string(), // ISO 8601 UTC - timezoneOffsetMinutes: z.number(), + occurredAt: z.string().datetime(), // ISO 8601 UTC + timezoneOffsetMinutes: z.number().int().min(-1080).max(1080), status: UsageEventStatus, - attempt: z.number(), + attempt: z.number().int().nonnegative(),🤖 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 36 - 56, Update the UsageEventV1 schema’s occurredAt and numeric fields to enforce contract-level validation: require occurredAt to be a valid ISO 8601 UTC timestamp, and ensure timezoneOffsetMinutes, attempt, and all token/cost values represented by SourcedNumber are finite, non-negative, and integral where applicable. Keep malformed records rejected by schema parsing so UsageEventStore.readAll quarantines them before aggregation.src/core/task/__tests__/Task.usage-stats.spec.ts (2)
265-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated construction tests.
The tests at lines 468-482 and 484-494 assert the same behaviour as the test at lines 265-278:
usageRecorderis defined, not null, and an instance ofUsageRecorder. Keep one test.Also applies to: 468-494
🤖 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, Remove the duplicated usageRecorder construction tests near the later test cases, retaining the existing test named “should initialize usageRecorder on Task construction” as the single coverage for defined, non-null UsageRecorder initialization.
281-285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the Task terminal finalize path, and extract the mock store.
Every
UsageRecordertest builds the same inline mock store and casts it withas unknown as UsageEventStore. Extract onemakeMockStore()helper.More importantly, the suite exercises
UsageRecorderdirectly and asserts only thatTaskconstructs a recorder. It never drives theTaskterminal finalize boundary. TherequestKeyvalues in the tests ("task-1:0","abc-123:5") are hand-written, so they cannot detect the key thatTask.tsactually builds. Add a test that runs a task through two sequential API attempts and asserts that two events are appended.Also applies to: 513-552
🤖 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 - 285, Extract the repeated inline UsageEventStore setup into a shared makeMockStore() helper and use it throughout the UsageRecorder tests. Add coverage that executes a Task through two sequential API attempts, reaches the terminal finalize path, and verifies two usage events are appended; derive assertions from the Task-generated request keys rather than hand-written keys.src/services/stats/UsageRecorder.ts (1)
78-83: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider marking the key finalized only after a successful append.
finalizeUsageEventaddsidempotencyKeytofinalizedKeysbefore it callsstore.append. Ifappendrejects with a transient error, the event is dropped permanently and a later call with the samerequestKeyandstatusreturns early.UsageEventStore.appendalready deduplicates byidempotencyKey, so a retry is safe.♻️ Proposed change
const idempotencyKey = `${requestKey}:${status}` if (this.finalizedKeys.has(idempotencyKey)) { return } - this.finalizedKeys.add(idempotencyKey)try { await this.store.append(event) + this.finalizedKeys.add(idempotencyKey) } catch { // store error must not break task // STATS_STORE/append/* 오류는 UsageEventStore 내부에서 분류됨 }Also applies to: 123-128
🤖 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 78 - 83, Update finalizeUsageEvent so finalizedKeys is updated only after UsageEventStore.append completes successfully; keep the existing idempotency check before appending, but move the finalizedKeys.add(idempotencyKey) operation to the success path so rejected appends can be retried safely.src/services/stats/UsageStatsService.ts (2)
405-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the CSV column parameter as the literal union.
extractCsvValuedeclarescolumn: string, so the compiler cannot check theswitchfor exhaustiveness. If someone appends an entry toCSV_COLUMNS, the call falls intodefaultand the export writes an empty cell with no build error and no test failure.Derive the parameter type from the constant. The compiler then reports the missing case.
♻️ Proposed refactor
+type CsvColumn = (typeof CSV_COLUMNS)[number] +- private extractCsvValue(event: UsageEventV1, column: string): string { + private extractCsvValue(event: UsageEventV1, column: CsvColumn): string { switch (column) {🤖 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 405 - 421, Update extractCsvValue in UsageStatsService so its column parameter uses the literal union derived from CSV_COLUMNS rather than string. Ensure CSV_COLUMNS preserves literal element types, allowing the switch cases to be exhaustively checked and requiring a corresponding case whenever a column is added.
215-244: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBackfill writes one event per store round trip.
Each iteration awaits
store.append, and the store serializes every call through its promise queue with its own lock acquisition, idempotency check, rotation check, and file write. A history backfill covering months of tasks turns into thousands of sequential file operations on the extension host.Consider a batched append on
UsageEventStorethat takes an array, acquires the lock once, filters duplicates in memory, and writes the NDJSON lines in one call. Keep the current per-event error isolation by reporting which events were rejected.🤖 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 215 - 244, Replace the per-event store.append calls in UsageStatsService.backfillFromHistory with a batched append API on UsageEventStore that acquires the lock once, filters duplicates in memory, and writes NDJSON once. Preserve provenance and appended-count behavior, while having the batch result identify rejected events so backfill reports each event’s failure without aborting unrelated events; continue wrapping unexpected failures in StatsServiceError.src/services/stats/__tests__/UsageAggregator.spec.ts (2)
242-269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the
sourceaxis test to mixed-source events.Each event here carries a single
costUsdsource, so the test never exercises the branch ingetAxisValuesthat returns more than one source. That branch adds the full event to every source bucket, which double counts tokens, cost, and theeventscounter. See the comment onsrc/services/stats/UsageAggregator.tslines 397-415.Add a case with
inputTokens.source = "provider"andcostUsd.source = "estimated", then assert that the bucket sums equaltotals. Theweekandmonthgroup axes and bucket-levelunknownEventCountalso have no coverage.🤖 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 242 - 269, The source-grouping test in “query - source grouping” only covers single-source events. Add a mixed-source event with inputTokens.source set to provider and costUsd.source set to estimated, then assert the grouped bucket sums match result.totals without double counting tokens, cost, or events; also cover the week and month group axes and bucket-level unknownEventCount as requested.
333-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the clock in the preset tests.
The
today,7d, andalltests build events fromnew Date()and compare against a range thatresolveTimeRangederives from the same real clock. The assertions pass today, but the tests cannot express boundary behavior, which is where preset resolution is most likely to break. A day-boundary or DST-transition regression stays invisible.Use
vi.useFakeTimers()withvi.setSystemTime(...)and fixed event timestamps. Then assert the exact boundary, for example an event atAsia/Seoul00:00:00 and one at 23:59:59.999 on the same day.🤖 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 333 - 379, Pin the clock in the preset tests within the “query - time range filtering” describe block by using vi.useFakeTimers() and vi.setSystemTime(...) with a fixed date, and restore timers after the tests. Replace new Date()-derived timestamps with fixed values that exercise exact preset boundaries, including Asia/Seoul midnight and 23:59:59.999 for “today”, while keeping “7d” and “all” timestamps deterministic and asserting the expected inclusion behavior.src/services/stats/UsageAggregator.ts (5)
184-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
toTimezoneDatereturns the instant it received.The method converts the input to timezone wall-clock, then adds
tzOffset, which is the inverse of that conversion. The result equalsdate. Both call sites also re-format the value in the same timezone, so the conversion has no effect:resolveTimeRangepassestzNowtostartOfDay, andstartOfDaycomputestzDateon line 251 and never reads it.Remove the method and pass
nowdirectly tostartOfDay. This removes one Intl formatter construction per query.🤖 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/UsageAggregator.ts` around lines 184 - 212, Remove the unused toTimezoneDate method and update resolveTimeRange to pass now directly to startOfDay instead of converting it first. Preserve the existing timezone and range behavior while eliminating the redundant Intl.DateTimeFormat construction.
466-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the no-op inclusion branches.
All three branches of each conditional execute the same statement.
cacheReadInInput,cacheWriteInInput, andreasoningInOutputtherefore have no effect on the accumulated token fields; onlyunknownEventCountreacts to them. The comments on lines 467-469 and 476-481 describe deduplication behavior that the code does not perform.If raw accumulation is the intended contract for this layer, reduce the code to three additions and correct the comments. State explicitly that
inputTokensmay already contain cache-read tokens and that consumers must usecacheReadInInputto interpret the totals.♻️ Proposed refactor
- // 토큰 값 누적 - // cacheReadInInput이 "included"면 inputTokens에 이미 cacheRead가 포함되어 있으므로 - // cacheReadTokens를 별도로 더하지 않음 (중복 방지) - // "excluded"면 cacheReadTokens를 별도로 더함 - bucket.inputTokens += inputTokens - bucket.outputTokens += outputTokens - - if (event.semantics.cacheReadInInput === "excluded") { - bucket.cacheReadTokens += cacheReadTokens - } else if (event.semantics.cacheReadInInput === "included") { - // inputTokens에 이미 포함되어 있으므로 별도 추가 없음 - // 하지만 cacheReadTokens 필드에는 기록 (참고용) - bucket.cacheReadTokens += cacheReadTokens - } else { - // unknown: 일단 더하되 unknownEventCount로 표시 - bucket.cacheReadTokens += cacheReadTokens - } - - if (event.semantics.cacheWriteInInput === "excluded") { - bucket.cacheWriteTokens += cacheWriteTokens - } else if (event.semantics.cacheWriteInInput === "included") { - bucket.cacheWriteTokens += cacheWriteTokens - } else { - bucket.cacheWriteTokens += cacheWriteTokens - } - - if (event.semantics.reasoningInOutput === "excluded") { - bucket.reasoningTokens += reasoningTokens - } else if (event.semantics.reasoningInOutput === "included") { - bucket.reasoningTokens += reasoningTokens - } else { - bucket.reasoningTokens += reasoningTokens - } + // 토큰은 provider가 보고한 값 그대로 누적한다. + // inputTokens에 cacheRead/cacheWrite가 포함되었는지는 semantics 필드로만 표현하며, + // 해석은 consumer가 담당한다 (unknown인 경우 unknownEventCount로 표시). + bucket.inputTokens += inputTokens + bucket.outputTokens += outputTokens + bucket.cacheReadTokens += cacheReadTokens + bucket.cacheWriteTokens += cacheWriteTokens + bucket.reasoningTokens += reasoningTokens🤖 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/UsageAggregator.ts` around lines 466 - 498, In the token accumulation logic around the event semantics checks, replace the three equivalent conditional branches for cacheReadInInput, cacheWriteInInput, and reasoningInOutput with direct additions to bucket.cacheReadTokens, bucket.cacheWriteTokens, and bucket.reasoningTokens. Update the surrounding comments to document raw accumulation, explicitly noting that inputTokens may already include cache-read tokens and consumers must use cacheReadInInput to interpret totals.
518-542: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a tiebreaker for mixed time and category axes.
When
groupByis["day", "provider"], the comparator only compares the day key. Buckets that share a day compare equal, so they keep insertion order, which follows event order in the segment files. The order of providers inside a day is then unstable across reads, and the documented "total descending, then name ascending" rule on line 516 does not apply.Fall through to the category comparison after the time key ties.
♻️ Proposed refactor
if (hasTimeAxis) { const timeAxis = groupBy.find((g) => g === "day" || g === "week" || g === "month")! return buckets.sort((a, b) => { const aTime = a.key[timeAxis] ?? "" const bTime = b.key[timeAxis] ?? "" - return aTime.localeCompare(bTime) + const timeDiff = aTime.localeCompare(bTime) + if (timeDiff !== 0) return timeDiff + return this.compareCategory(a, b) }) } - - // category만 있는 경우: known total 내림차순 후 이름 오름차순 - return buckets.sort((a, b) => { - // totalTokens 기준 내림차순 - const diff = b.totalTokens - a.totalTokens - if (diff !== 0) return diff - - // 이름 오름차순 - const aName = Object.values(a.key).join("/") - const bName = Object.values(b.key).join("/") - return aName.localeCompare(bName) - }) + + // category만 있는 경우: known total 내림차순 후 이름 오름차순 + return buckets.sort((a, b) => this.compareCategory(a, b)) + } + + private compareCategory(a: StatsBucket, b: StatsBucket): number { + const diff = b.totalTokens - a.totalTokens + if (diff !== 0) return diff + const aName = Object.values(a.key).join("/") + const bName = Object.values(b.key).join("/") + return aName.localeCompare(bName) }🤖 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/UsageAggregator.ts` around lines 518 - 542, Update sortBuckets so the time-axis comparator falls through to the existing category ordering when the time keys are equal, applying totalTokens descending and joined key name ascending as the tiebreaker for mixed time/category groupings while preserving chronological ordering across different time keys.
549-566: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecide which event set
coveragedescribes.
computeCoverageacceptsallEventsand never reads it.firstEventAtandlastEventAtcome from the filteredvisibleEvents, so they restate the query range rather than the recorded data range. A dashboard cannot use them to show how far the local history reaches, or to detect that the selected range starts before the first recorded event.Either compute
firstEventAtandlastEventAtfromallEvents, or remove the parameter and document that coverage is range-scoped.backfilledEventCounthas the same ambiguity.🤖 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/UsageAggregator.ts` around lines 549 - 566, The computeCoverage method currently accepts allEvents but derives coverage timestamps and backfilledEventCount only from visibleEvents, making the reported range query-scoped. Use allEvents consistently for firstEventAt, lastEventAt, and backfilledEventCount so coverage describes the recorded history, or remove allEvents and explicitly make the coverage contract range-scoped; keep the chosen event-set semantics consistent across all fields.
286-300: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDerive the month bucket from the already-computed day bucket.
The month bucket is consistently
2026-07in the checked runtime, so slicingdayBucketto the first 7 characters keeps the bucket identity consistent and avoids a second locale formatting step.🤖 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/UsageAggregator.ts` around lines 286 - 300, Update the month bucket calculation in the UsageAggregator date-bucketing flow to derive it directly from the existing dayBucket by taking its first seven characters. Remove the separate monthFormatter and locale-formatting step while preserving the YYYY-MM bucket format.
🤖 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/__tests__/usage-stats.spec.ts`:
- Around line 133-138: Rename the UsageEventV1 test to describe accepting a
non-negative/zero attempt value, keeping its existing attempt: 0 assertion. Add
an explicit UsageEventV1.parse assertion using a negative attempt to document
the current V1 contract that negative numbers are also accepted.
In `@src/core/task/__tests__/Task.usage-stats.spec.ts`:
- Around line 275-277: The usage-stats spec should avoid explicit any casts by
accessing the private usageRecorder member with bracket notation and by
introducing a typed makeMockStore() helper for store doubles instead of repeated
unknown-to-UsageEventStore casts. In
src/core/task/__tests__/Task.usage-stats.spec.ts lines 275-277, update the
usageRecorder assertions and mock construction accordingly; in
src/eslint-suppressions.json lines 857-861, reduce or remove the no-explicit-any
suppression after the spec changes, ensuring suppression counts do not increase.
In `@src/core/task/Task.ts`:
- Around line 3216-3246: Update src/core/task/Task.ts lines 3216-3246 and
3360-3390 to include a unique per-request identifier, such as lastApiReqIndex or
the corresponding api_req_started timestamp, in the requestKey construction
alongside taskId and retryAttempt. Use the identical construction in both
terminal finalize paths so each normal and retry attempt remains distinct for
UsageRecorder.finalizeUsageEvent deduplication.
In `@src/services/stats/__tests__/UsageEventStore.spec.ts`:
- Around line 276-280: Update the error-handling test around
UsageEventStore.isCapped to exercise the cap path: set the private capped flag
via bracket notation, invoke the append operation, and assert it throws
StatsStoreError with code STATS_STORE/append/003. Keep the existing
initial-state check separate or rename it to reflect its behavior, and retain
the StatsStoreError import now that it is used.
In `@src/services/stats/UsageAggregator.ts`:
- Around line 148-171: Update the preset range logic in the query
date-resolution method containing the “today”, “7d”, and “30d” cases so day
boundaries are advanced using the query timezone’s calendar, not Date.setDate on
the host-local timezone. Derive the query-timezone year/month/day, add the
required calendar day, and convert each resulting wall-clock midnight back using
getTimezoneOffsetMinutes; preserve the existing range lengths and “all”
behavior.
- Around line 397-415: Update the source-axis aggregation in getAxisValues and
the corresponding accumulation flow so mixed-source events cannot duplicate full
metrics across multiple buckets. Prefer splitting each cost, input-token, and
output-token metric into the bucket matching its own source, while incrementing
the event count only once or otherwise preserving totals consistency; reuse the
existing SourceSeparatedCost design if applicable and extend tests for mixed
sources.
- Around line 76-89: Update the time-range filtering in
UsageAggregator.queryStats to discard events when new
Date(event.occurredAt).getTime() is NaN before applying the from/to comparisons.
Preserve the existing range and cancelled-event filtering behavior for events
with valid timestamps.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 561-575: Update the onCompromised callback in the manifest lock
flow to log the compromise and mark the UsageEventStore as unusable without
throwing the error. Ensure the callback returns normally so the internal update
timer cannot surface an uncaught exception, while preserving the existing lock
configuration and diagnostic logging.
- Around line 494-520: Update loadOrCreateManifest to validate manifestVersion
equals the supported version value, not merely that it is numeric. For
non-ENOENT read or parse failures, stop returning a DEFAULT_MANIFEST fallback
and propagate the original error so appendInternal does not derive a segment
path from reset tracking state; preserve default-manifest creation for missing
or structurally invalid manifests.
- Around line 309-315: Serialize clear through the same in-process queue as
append by extracting append’s deferred-promise logic into a private enqueue<T>
helper. Update append and clear in UsageEventStore to execute their full
mutation bodies through enqueue, while preserving the existing manifest-lock
handling and return behavior.
- Around line 430-471: Recompute segmentPath after the rotation branch
increments manifest.currentSegment and persists the manifest, before the fs.open
append flow. Ensure the write targets the new segment and the existing error
message reports that same segment number.
- Around line 336-371: In the clear flow around writeManifestAtomic, persist the
new manifest before creating oldGenDir and moving segment files. Keep the
existing rename loop and console.warn handling unchanged so later move failures
remain tolerated, while a manifest-write failure leaves the original segments
and manifest intact.
- Around line 264-299: The readAll method reports the same corrupt lines on
every invocation because writeQuarantineReport appends to the report file and
readAll does not track previously reported entries. Maintain an in-memory Set or
Map to track reported segment:line:hash combinations across readAll calls.
Before calling writeQuarantineReport, filter quarantineEntries to exclude
entries already present in the tracking structure, then add the new entries to
the tracking structure after writing. This deduplicates the quarantine report
and prevents unbounded growth from repeated calls to readAll via queryStats and
exportStats.
In `@src/services/stats/UsageStatsService.ts`:
- Around line 259-288: Extract the duplicated query-range, timezone, and
cancelled-event filtering logic into shared exports resolveTimeRange,
getTimezoneOffsetMinutes, startOfDayInTimezone, and filterEvents in
src/services/stats/statsQueryRange.ts. In
src/services/stats/UsageStatsService.ts lines 259-288, replace
filterEventsByQuery with the shared filterEvents call; at lines 293-376, remove
resolvePresetRange, toTimezoneStartOfDay, and getTimezoneOffsetMinutes and
import the shared helpers. In src/services/stats/UsageAggregator.ts lines
250-271, remove startOfDay and getTimezoneOffsetMinutes and use the shared
timezone helpers so both query paths apply identical range resolution and
timezone behavior.
- Around line 484-507: Update the quoting condition in escapeCsvCell to also
detect carriage returns, so values containing a bare \r are wrapped in CSV
quotes while preserving the existing formula-injection prefix and quote-doubling
order.
---
Nitpick comments:
In `@packages/types/src/usage-stats.ts`:
- Around line 69-76: The StatsQuery schema currently permits preset and explicit
from/to ranges simultaneously without defining precedence. Update StatsQuery
with a superRefine validation that rejects preset when from or to is provided,
preserving all existing field validation and defaults.
- Around line 36-56: Update the UsageEventV1 schema’s occurredAt and numeric
fields to enforce contract-level validation: require occurredAt to be a valid
ISO 8601 UTC timestamp, and ensure timezoneOffsetMinutes, attempt, and all
token/cost values represented by SourcedNumber are finite, non-negative, and
integral where applicable. Keep malformed records rejected by schema parsing so
UsageEventStore.readAll quarantines them before aggregation.
In `@packages/types/src/vscode-extension-host.ts`:
- Around line 257-260: Update the exportUsageStatsResult type in the usage stats
response payloads so failed exports can omit data, preferably by modeling
success and failure as a discriminated union; otherwise make data optional while
preserving the existing format and error fields.
- Around line 758-761: Define and export a shared ExportFormat union in
usage-stats.ts, then replace the inline "json" | "csv" declarations in the
usage-stats query types and the usage-stats request payload with references to
ExportFormat. Update UsageStatsService to import and reuse the same exported
type, preserving the current supported formats.
In `@src/core/task/__tests__/Task.usage-stats.spec.ts`:
- Around line 265-278: Remove the duplicated usageRecorder construction tests
near the later test cases, retaining the existing test named “should initialize
usageRecorder on Task construction” as the single coverage for defined, non-null
UsageRecorder initialization.
- Around line 281-285: Extract the repeated inline UsageEventStore setup into a
shared makeMockStore() helper and use it throughout the UsageRecorder tests. Add
coverage that executes a Task through two sequential API attempts, reaches the
terminal finalize path, and verifies two usage events are appended; derive
assertions from the Task-generated request keys rather than hand-written keys.
In `@src/services/stats/__tests__/UsageAggregator.spec.ts`:
- Around line 242-269: The source-grouping test in “query - source grouping”
only covers single-source events. Add a mixed-source event with
inputTokens.source set to provider and costUsd.source set to estimated, then
assert the grouped bucket sums match result.totals without double counting
tokens, cost, or events; also cover the week and month group axes and
bucket-level unknownEventCount as requested.
- Around line 333-379: Pin the clock in the preset tests within the “query -
time range filtering” describe block by using vi.useFakeTimers() and
vi.setSystemTime(...) with a fixed date, and restore timers after the tests.
Replace new Date()-derived timestamps with fixed values that exercise exact
preset boundaries, including Asia/Seoul midnight and 23:59:59.999 for “today”,
while keeping “7d” and “all” timestamps deterministic and asserting the expected
inclusion behavior.
In `@src/services/stats/__tests__/UsageEventStore.spec.ts`:
- Around line 224-259: Add a concurrent-operation test within the existing clear
suite that starts several append calls without awaiting them, invokes clear
before they finish, then awaits all append and clear promises. Assert that
readAll returns a deterministic expected result, covering the race caused by
clear bypassing the append promise queue while preserving the existing clear
tests.
- Around line 101-161: Add a segment-rotation test within the append suite that
pre-fills events-000001.ndjson beyond SEGMENT_MAX_BYTES, appends an event
through store.append, and verifies the event is written to events-000002.ndjson
rather than the original segment. Also assert getManifest().currentSegment
equals 2, using the existing store setup and event helpers.
In `@src/services/stats/UsageAggregator.ts`:
- Around line 184-212: Remove the unused toTimezoneDate method and update
resolveTimeRange to pass now directly to startOfDay instead of converting it
first. Preserve the existing timezone and range behavior while eliminating the
redundant Intl.DateTimeFormat construction.
- Around line 466-498: In the token accumulation logic around the event
semantics checks, replace the three equivalent conditional branches for
cacheReadInInput, cacheWriteInInput, and reasoningInOutput with direct additions
to bucket.cacheReadTokens, bucket.cacheWriteTokens, and bucket.reasoningTokens.
Update the surrounding comments to document raw accumulation, explicitly noting
that inputTokens may already include cache-read tokens and consumers must use
cacheReadInInput to interpret totals.
- Around line 518-542: Update sortBuckets so the time-axis comparator falls
through to the existing category ordering when the time keys are equal, applying
totalTokens descending and joined key name ascending as the tiebreaker for mixed
time/category groupings while preserving chronological ordering across different
time keys.
- Around line 549-566: The computeCoverage method currently accepts allEvents
but derives coverage timestamps and backfilledEventCount only from
visibleEvents, making the reported range query-scoped. Use allEvents
consistently for firstEventAt, lastEventAt, and backfilledEventCount so coverage
describes the recorded history, or remove allEvents and explicitly make the
coverage contract range-scoped; keep the chosen event-set semantics consistent
across all fields.
- Around line 286-300: Update the month bucket calculation in the
UsageAggregator date-bucketing flow to derive it directly from the existing
dayBucket by taking its first seven characters. Remove the separate
monthFormatter and locale-formatting step while preserving the YYYY-MM bucket
format.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 91-99: Update makeQuarantineEntry to compute the corrupt line hash
with node:crypto’s SHA-256 implementation and truncate the resulting hexadecimal
digest to 16 characters, matching QuarantineReportEntry.hash. Remove the
existing 32-bit rolling-hash logic and its dependency-avoidance rationale, while
preserving the remaining quarantine entry fields and behavior.
- Around line 476-478: Replace the per-append checkTotalSize call in the append
flow with a running byte-total update based on the written line length, then
update capped from that total. Initialize the total during initialize and
recompute it after clear; keep checkTotalSize out of the normal append path
while preserving the existing cap behavior.
- Around line 583-616: Update rebuildIdempotencySet to avoid scanning every
historical segment: restrict the rebuild to the current segment, or a documented
bounded tail of recent lines within it, while preserving idempotencyKeys
population and existing missing-file/error handling. Add a comment near the
bound explaining the chosen limit and update the loop/read logic accordingly.
- Around line 188-195: The append method’s documentation incorrectly claims
deduplication occurs inside the lock. Update the doc comment for the append
operation to describe the actual in-memory/in-process queue guarantee and
acknowledge that cross-process or shared-storage duplicate prevention is not
guaranteed; keep the implementation unchanged.
- Around line 709-721: In src/services/stats/UsageEventStore.ts lines 709-721,
remove the public _getIdempotencyKeyCount method and make _getStatsDir private,
or expose statsDir as readonly. In
src/services/stats/__tests__/UsageEventStore.spec.ts lines 276-280, access
statsDir and capped with bracket notation instead of the widened API, and use
the imported StatsStoreError to assert the STATS_STORE/append/003 code; avoid
any casts.
In `@src/services/stats/UsageRecorder.ts`:
- Around line 78-83: Update finalizeUsageEvent so finalizedKeys is updated only
after UsageEventStore.append completes successfully; keep the existing
idempotency check before appending, but move the
finalizedKeys.add(idempotencyKey) operation to the success path so rejected
appends can be retried safely.
In `@src/services/stats/UsageStatsService.ts`:
- Around line 405-421: Update extractCsvValue in UsageStatsService so its column
parameter uses the literal union derived from CSV_COLUMNS rather than string.
Ensure CSV_COLUMNS preserves literal element types, allowing the switch cases to
be exhaustively checked and requiring a corresponding case whenever a column is
added.
- Around line 215-244: Replace the per-event store.append calls in
UsageStatsService.backfillFromHistory with a batched append API on
UsageEventStore that acquires the lock once, filters duplicates in memory, and
writes NDJSON once. Preserve provenance and appended-count behavior, while
having the batch result identify rejected events so backfill reports each
event’s failure without aborting unrelated events; continue wrapping unexpected
failures in StatsServiceError.
🪄 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: c79809c0-d92a-431d-8c7d-b31127845a25
📒 Files selected for processing (14)
packages/types/src/__tests__/usage-stats.spec.tspackages/types/src/index.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/index.ts
| it("should reject negative attempt", () => { | ||
| // z.number() accepts negatives, but attempt should be >= 0 logically | ||
| // This test confirms the schema accepts any number (no min constraint in V1) | ||
| const result = UsageEventV1.parse({ ...validEvent, attempt: 0 }) | ||
| expect(result.attempt).toBe(0) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Rename the test to match its assertion.
The test is named "should reject negative attempt", but it asserts that attempt: 0 parses successfully. It never passes a negative value. The name states the opposite of the behavior under test.
Rename the test, and add an explicit assertion for a negative value so the current contract is documented.
💚 Proposed test correction
- it("should reject negative attempt", () => {
- // z.number() accepts negatives, but attempt should be >= 0 logically
- // This test confirms the schema accepts any number (no min constraint in V1)
+ it("should accept any attempt number (no min constraint in V1)", () => {
const result = UsageEventV1.parse({ ...validEvent, attempt: 0 })
expect(result.attempt).toBe(0)
+ expect(UsageEventV1.parse({ ...validEvent, attempt: -1 }).attempt).toBe(-1)
})📝 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.
| it("should reject negative attempt", () => { | |
| // z.number() accepts negatives, but attempt should be >= 0 logically | |
| // This test confirms the schema accepts any number (no min constraint in V1) | |
| const result = UsageEventV1.parse({ ...validEvent, attempt: 0 }) | |
| expect(result.attempt).toBe(0) | |
| }) | |
| it("should accept any attempt number (no min constraint in V1)", () => { | |
| const result = UsageEventV1.parse({ ...validEvent, attempt: 0 }) | |
| expect(result.attempt).toBe(0) | |
| expect(UsageEventV1.parse({ ...validEvent, attempt: -1 }).attempt).toBe(-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 `@packages/types/src/__tests__/usage-stats.spec.ts` around lines 133 - 138,
Rename the UsageEventV1 test to describe accepting a non-negative/zero attempt
value, keeping its existing attempt: 0 assertion. Add an explicit
UsageEventV1.parse assertion using a negative attempt to document the current V1
contract that negative numbers are also accepted.
| expect((task as any).usageRecorder).toBeDefined() | ||
| expect((task as any).usageRecorder).not.toBeNull() | ||
| expect((task as any).usageRecorder).toBeInstanceOf(UsageRecorder) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The as any casts in the new spec add 26 lint suppressions. The root cause is the untyped access pattern in the spec: (task as any).usageRecorder and the repeated as unknown as UsageEventStore mock casts. Both violate the guideline against as any and force a new suppression entry.
src/core/task/__tests__/Task.usage-stats.spec.ts#L275-L277: replace(task as any).usageRecorderwithtask["usageRecorder"], and extract a typedmakeMockStore()helper for the store doubles.src/eslint-suppressions.json#L857-L861: reduce the@typescript-eslint/no-explicit-anycount, or remove the entry, after the spec casts are fixed.
As per coding guidelines: "Avoid as any; use typed APIs, bracket notation for private members where appropriate" and "Suppression counts in src/eslint-suppressions.json must never increase".
📍 Affects 2 files
src/core/task/__tests__/Task.usage-stats.spec.ts#L275-L277(this comment)src/eslint-suppressions.json#L857-L861
🤖 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 275 - 277, The
usage-stats spec should avoid explicit any casts by accessing the private
usageRecorder member with bracket notation and by introducing a typed
makeMockStore() helper for store doubles instead of repeated
unknown-to-UsageEventStore casts. In
src/core/task/__tests__/Task.usage-stats.spec.ts lines 275-277, update the
usageRecorder assertions and mock construction accordingly; in
src/eslint-suppressions.json lines 857-861, reduce or remove the no-explicit-any
suppression after the spec changes, ensuring suppression counts do not increase.
Source: Coding guidelines
| 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
Non-unique requestKey drops most usage events. Both terminal finalize sites build requestKey as ${this.taskId}:${currentItem.retryAttempt ?? 0}. Normal task turns pushed at lines 3762-3765 and 3827-3832 do not set retryAttempt, so every non-retry attempt of a task shares the key ${taskId}:0. UsageRecorder.finalizeUsageEvent deduplicates on ${requestKey}:${status}, so only the first attempt per status is persisted. Add a per-request identifier such as lastApiReqIndex or the api_req_started message ts.
src/core/task/Task.ts#L3216-L3246: include the per-request identifier in therequestKeyfor the completed/cancelled path.src/core/task/Task.ts#L3360-L3390: use the identicalrequestKeyconstruction for the failed/cancelled path.
📍 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, Update src/core/task/Task.ts
lines 3216-3246 and 3360-3390 to include a unique per-request identifier, such
as lastApiReqIndex or the corresponding api_req_started timestamp, in the
requestKey construction alongside taskId and retryAttempt. Use the identical
construction in both terminal finalize paths so each normal and retry attempt
remains distinct for UsageRecorder.finalizeUsageEvent deduplication.
| describe("error handling", () => { | ||
| it("should throw StatsStoreError with correct code on cap reached", async () => { | ||
| // 이 테스트는 cap을 강제로 설정하기 어려우므로, isCapped() 메서드 동작만 확인 | ||
| expect(store.isCapped()).toBe(false) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test does not cover the cap path, and its name states that it does.
The test is named "should throw StatsStoreError with correct code on cap reached". It only asserts store.isCapped() === false on a fresh store. It never reaches the cap, and it never asserts an error code. StatsStoreError, imported at line 9, has no other use in this file, so the import is currently unused and no-unused-vars reports it under the mandated --max-warnings=0 run.
Either rename the test to describe the initial state, or drive the cap path and assert the STATS_STORE/append/003 code. Reaching the cap does not require 100 MiB of data. Set the private capped flag with bracket notation.
💚 Proposed test
- it("should throw StatsStoreError with correct code on cap reached", async () => {
- // 이 테스트는 cap을 강제로 설정하기 어려우므로, isCapped() 메서드 동작만 확인
- expect(store.isCapped()).toBe(false)
- })
+ it("should report not capped for a fresh store", () => {
+ expect(store.isCapped()).toBe(false)
+ })
+
+ it("should throw StatsStoreError with code STATS_STORE/append/003 when capped", async () => {
+ store["capped"] = true
+ await expect(store.append(makeEvent())).rejects.toThrow(StatsStoreError)
+ await expect(store.append(makeEvent())).rejects.toThrow("STATS_STORE/append/003")
+ })As per coding guidelines: "Avoid as any; use typed APIs, bracket notation for private members where appropriate" and "run ESLint with pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 <relative-file>".
📝 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.
| describe("error handling", () => { | |
| it("should throw StatsStoreError with correct code on cap reached", async () => { | |
| // 이 테스트는 cap을 강제로 설정하기 어려우므로, isCapped() 메서드 동작만 확인 | |
| expect(store.isCapped()).toBe(false) | |
| }) | |
| describe("error handling", () => { | |
| it("should report not capped for a fresh store", () => { | |
| expect(store.isCapped()).toBe(false) | |
| }) | |
| it("should throw StatsStoreError with code STATS_STORE/append/003 when capped", async () => { | |
| store["capped"] = true | |
| await expect(store.append(makeEvent())).rejects.toThrow(StatsStoreError) | |
| await expect(store.append(makeEvent())).rejects.toThrow("STATS_STORE/append/003") | |
| }) |
🤖 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 - 280,
Update the error-handling test around UsageEventStore.isCapped to exercise the
cap path: set the private capped flag via bracket notation, invoke the append
operation, and assert it throws StatsStoreError with code
STATS_STORE/append/003. Keep the existing initial-state check separate or rename
it to reflect its behavior, and retain the StatsStoreError import now that it is
used.
Source: Coding guidelines
| // 1. 시간 범위 필터링 | ||
| const { from, to } = this.resolveTimeRange(query) | ||
| const filtered = events.filter((event) => { | ||
| const eventTime = new Date(event.occurredAt).getTime() | ||
| if (from && eventTime < from.getTime()) return false | ||
| if (to && eventTime >= to.getTime()) return false | ||
| return true | ||
| }) | ||
|
|
||
| // 2. cancelled 이벤트 필터링 | ||
| const includeCancelled = query.includeCancelled ?? false | ||
| const visibleEvents = includeCancelled | ||
| ? filtered | ||
| : filtered.filter((e) => e.status !== "cancelled") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against unparsable occurredAt values.
UsageEventV1.occurredAt is typed as z.string() only, so the store can return an event whose timestamp does not parse. new Date(event.occurredAt).getTime() then returns NaN, and both range comparisons are false, so the event passes the filter. The same event later reaches computeCoverage, where new Date(NaN).toISOString() throws RangeError: Invalid time value and fails the whole queryStats call.
Drop events with an unparsable timestamp during filtering.
🛠️ Proposed fix
const { from, to } = this.resolveTimeRange(query)
const filtered = events.filter((event) => {
const eventTime = new Date(event.occurredAt).getTime()
+ if (Number.isNaN(eventTime)) return false
if (from && eventTime < from.getTime()) return false
if (to && eventTime >= to.getTime()) return false
return true
})📝 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.
| // 1. 시간 범위 필터링 | |
| const { from, to } = this.resolveTimeRange(query) | |
| const filtered = events.filter((event) => { | |
| const eventTime = new Date(event.occurredAt).getTime() | |
| if (from && eventTime < from.getTime()) return false | |
| if (to && eventTime >= to.getTime()) return false | |
| return true | |
| }) | |
| // 2. cancelled 이벤트 필터링 | |
| const includeCancelled = query.includeCancelled ?? false | |
| const visibleEvents = includeCancelled | |
| ? filtered | |
| : filtered.filter((e) => e.status !== "cancelled") | |
| // 1. 시간 범위 필터링 | |
| const { from, to } = this.resolveTimeRange(query) | |
| const filtered = events.filter((event) => { | |
| const eventTime = new Date(event.occurredAt).getTime() | |
| if (Number.isNaN(eventTime)) return false | |
| if (from && eventTime < from.getTime()) return false | |
| if (to && eventTime >= to.getTime()) return false | |
| return true | |
| }) | |
| // 2. cancelled 이벤트 필터링 | |
| const includeCancelled = query.includeCancelled ?? false | |
| const visibleEvents = includeCancelled | |
| ? filtered | |
| : filtered.filter((e) => e.status !== "cancelled") |
🤖 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/UsageAggregator.ts` around lines 76 - 89, Update the
time-range filtering in UsageAggregator.queryStats to discard events when new
Date(event.occurredAt).getTime() is NaN before applying the from/to comparisons.
Preserve the existing range and cancelled-event filtering behavior for events
with valid timestamps.
| const manifest = await this.loadOrCreateManifest() | ||
| const segmentPath = this.getSegmentPath(manifest.currentSegment) | ||
|
|
||
| // segment 파일이 존재하는지 확인하고 크기 체크 | ||
| let segmentSize = 0 | ||
| try { | ||
| const stat = await fs.stat(segmentPath) | ||
| segmentSize = stat.size | ||
| } catch (err) { | ||
| if ((err as NodeJS.ErrnoException).code !== "ENOENT") { | ||
| throw err | ||
| } | ||
| // 파일이 없으면 새로 생성 | ||
| } | ||
|
|
||
| // segment 회전 확인 | ||
| if (segmentSize >= SEGMENT_MAX_BYTES) { | ||
| manifest.currentSegment += 1 | ||
| manifest.updatedAt = new Date().toISOString() | ||
| await this.writeManifestAtomic(manifest) | ||
| } | ||
|
|
||
| // 이벤트를 compact JSON + \n으로 append | ||
| const line = JSON.stringify(event) + "\n" | ||
|
|
||
| try { | ||
| // append mode로 열어서 write | ||
| const handle = await fs.open(segmentPath, "a") | ||
| try { | ||
| await handle.writeFile(line, "utf-8") | ||
| // file handle sync 후 성공으로 반환 | ||
| await handle.sync() | ||
| } finally { | ||
| await handle.close() | ||
| } | ||
| } catch (err) { | ||
| throw new StatsStoreError( | ||
| "STATS_STORE/append/004", | ||
| `Failed to write event to segment ${manifest.currentSegment}`, | ||
| err, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Rotation writes to the old, full segment.
Line 431 computes segmentPath from manifest.currentSegment. Lines 446-450 then increment manifest.currentSegment and persist the new manifest, but they never recompute segmentPath. The fs.open(segmentPath, "a") call at line 457 still appends to the segment that already reached SEGMENT_MAX_BYTES.
Two consequences follow. The rotated segment file is never created by the append that triggered the rotation, and the error message at line 468 reports the new segment number while the code writes the old file.
Recompute the path after rotation.
🐛 Proposed fix
const manifest = await this.loadOrCreateManifest()
- const segmentPath = this.getSegmentPath(manifest.currentSegment)
+ let segmentPath = this.getSegmentPath(manifest.currentSegment)
@@
// segment 회전 확인
if (segmentSize >= SEGMENT_MAX_BYTES) {
manifest.currentSegment += 1
manifest.updatedAt = new Date().toISOString()
await this.writeManifestAtomic(manifest)
+ segmentPath = this.getSegmentPath(manifest.currentSegment)
}📝 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 manifest = await this.loadOrCreateManifest() | |
| const segmentPath = this.getSegmentPath(manifest.currentSegment) | |
| // segment 파일이 존재하는지 확인하고 크기 체크 | |
| let segmentSize = 0 | |
| try { | |
| const stat = await fs.stat(segmentPath) | |
| segmentSize = stat.size | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code !== "ENOENT") { | |
| throw err | |
| } | |
| // 파일이 없으면 새로 생성 | |
| } | |
| // segment 회전 확인 | |
| if (segmentSize >= SEGMENT_MAX_BYTES) { | |
| manifest.currentSegment += 1 | |
| manifest.updatedAt = new Date().toISOString() | |
| await this.writeManifestAtomic(manifest) | |
| } | |
| // 이벤트를 compact JSON + \n으로 append | |
| const line = JSON.stringify(event) + "\n" | |
| try { | |
| // append mode로 열어서 write | |
| const handle = await fs.open(segmentPath, "a") | |
| try { | |
| await handle.writeFile(line, "utf-8") | |
| // file handle sync 후 성공으로 반환 | |
| await handle.sync() | |
| } finally { | |
| await handle.close() | |
| } | |
| } catch (err) { | |
| throw new StatsStoreError( | |
| "STATS_STORE/append/004", | |
| `Failed to write event to segment ${manifest.currentSegment}`, | |
| err, | |
| ) | |
| } | |
| const manifest = await this.loadOrCreateManifest() | |
| let segmentPath = this.getSegmentPath(manifest.currentSegment) | |
| // segment 파일이 존재하는지 확인하고 크기 체크 | |
| let segmentSize = 0 | |
| try { | |
| const stat = await fs.stat(segmentPath) | |
| segmentSize = stat.size | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code !== "ENOENT") { | |
| throw err | |
| } | |
| // 파일이 없으면 새로 생성 | |
| } | |
| // segment 회전 확인 | |
| if (segmentSize >= SEGMENT_MAX_BYTES) { | |
| manifest.currentSegment += 1 | |
| manifest.updatedAt = new Date().toISOString() | |
| await this.writeManifestAtomic(manifest) | |
| segmentPath = this.getSegmentPath(manifest.currentSegment) | |
| } | |
| // 이벤트를 compact JSON + \n으로 append | |
| const line = JSON.stringify(event) + "\n" | |
| try { | |
| // append mode로 열어서 write | |
| const handle = await fs.open(segmentPath, "a") | |
| try { | |
| await handle.writeFile(line, "utf-8") | |
| // file handle sync 후 성공으로 반환 | |
| await handle.sync() | |
| } finally { | |
| await handle.close() | |
| } | |
| } catch (err) { | |
| throw new StatsStoreError( | |
| "STATS_STORE/append/004", | |
| `Failed to write event to segment ${manifest.currentSegment}`, | |
| 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 430 - 471, Recompute
segmentPath after the rotation branch increments manifest.currentSegment and
persists the manifest, before the fs.open append flow. Ensure the write targets
the new segment and the existing error message reports that same segment number.
| private async loadOrCreateManifest(): Promise<UsageStatsManifest> { | ||
| try { | ||
| const content = await fs.readFile(this.manifestPath, "utf-8") | ||
| const parsed = JSON.parse(content) | ||
| // 기본 필드 검증 | ||
| if ( | ||
| typeof parsed.manifestVersion === "number" && | ||
| typeof parsed.generation === "number" && | ||
| typeof parsed.currentSegment === "number" | ||
| ) { | ||
| return parsed as UsageStatsManifest | ||
| } | ||
| // 검증 실패 시 기본값으로 덮어쓰기 | ||
| const defaultManifest = { ...DEFAULT_MANIFEST, updatedAt: new Date().toISOString() } | ||
| await this.writeManifestAtomic(defaultManifest) | ||
| return defaultManifest | ||
| } catch (err) { | ||
| if ((err as NodeJS.ErrnoException).code === "ENOENT") { | ||
| // manifest가 없으면 생성 | ||
| const defaultManifest = { ...DEFAULT_MANIFEST, updatedAt: new Date().toISOString() } | ||
| await this.writeManifestAtomic(defaultManifest) | ||
| return defaultManifest | ||
| } | ||
| // 다른 오류는 기본값 반환 | ||
| console.warn(`[UsageEventStore] failed to load manifest, using default:`, err) | ||
| return { ...DEFAULT_MANIFEST, updatedAt: new Date().toISOString() } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A transient manifest read error silently resets generation and segment tracking.
Lines 517-519 catch every non-ENOENT failure and return DEFAULT_MANIFEST, which has generation: 1 and currentSegment: 1. The method does not persist that fallback, so the on-disk manifest keeps its real values.
appendInternal calls this method inside the lock at line 430 and then derives segmentPath from currentSegment. If the manifest is temporarily unreadable, for example on EACCES or a partially written file, the append writes into events-000001.ndjson even when the real current segment is a later one. That mixes new events into an already rotated segment and can also push the segment past SEGMENT_MAX_BYTES.
Two separate concerns exist in this method. Line 499 checks the type of manifestVersion but never checks that the value is 1, so a future v2 manifest is read as v1.
Propagate the read failure for the write path instead of guessing, and validate the version value.
🛡️ Proposed handling
if (
- typeof parsed.manifestVersion === "number" &&
+ parsed.manifestVersion === 1 &&
typeof parsed.generation === "number" &&
typeof parsed.currentSegment === "number"
) {
return parsed as UsageStatsManifest
}
@@
- // 다른 오류는 기본값 반환
- console.warn(`[UsageEventStore] failed to load manifest, using default:`, err)
- return { ...DEFAULT_MANIFEST, updatedAt: new Date().toISOString() }
+ // 기본값으로 대체하면 generation/currentSegment가 되돌아가 기존 segment를 오염시킨다.
+ throw new StatsStoreError("STATS_STORE/append/005", "Failed to load manifest", err)
}append already isolates StatsStoreError from the calling task, so a thrown error suspends recording rather than corrupting a segment.
📝 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.
| private async loadOrCreateManifest(): Promise<UsageStatsManifest> { | |
| try { | |
| const content = await fs.readFile(this.manifestPath, "utf-8") | |
| const parsed = JSON.parse(content) | |
| // 기본 필드 검증 | |
| if ( | |
| typeof parsed.manifestVersion === "number" && | |
| typeof parsed.generation === "number" && | |
| typeof parsed.currentSegment === "number" | |
| ) { | |
| return parsed as UsageStatsManifest | |
| } | |
| // 검증 실패 시 기본값으로 덮어쓰기 | |
| const defaultManifest = { ...DEFAULT_MANIFEST, updatedAt: new Date().toISOString() } | |
| await this.writeManifestAtomic(defaultManifest) | |
| return defaultManifest | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code === "ENOENT") { | |
| // manifest가 없으면 생성 | |
| const defaultManifest = { ...DEFAULT_MANIFEST, updatedAt: new Date().toISOString() } | |
| await this.writeManifestAtomic(defaultManifest) | |
| return defaultManifest | |
| } | |
| // 다른 오류는 기본값 반환 | |
| console.warn(`[UsageEventStore] failed to load manifest, using default:`, err) | |
| return { ...DEFAULT_MANIFEST, updatedAt: new Date().toISOString() } | |
| } | |
| private async loadOrCreateManifest(): Promise<UsageStatsManifest> { | |
| try { | |
| const content = await fs.readFile(this.manifestPath, "utf-8") | |
| const parsed = JSON.parse(content) | |
| // 기본 필드 검증 | |
| if ( | |
| parsed.manifestVersion === 1 && | |
| typeof parsed.generation === "number" && | |
| typeof parsed.currentSegment === "number" | |
| ) { | |
| return parsed as UsageStatsManifest | |
| } | |
| // 검증 실패 시 기본값으로 덮어쓰기 | |
| const defaultManifest = { ...DEFAULT_MANIFEST, updatedAt: new Date().toISOString() } | |
| await this.writeManifestAtomic(defaultManifest) | |
| return defaultManifest | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code === "ENOENT") { | |
| // manifest가 없으면 생성 | |
| const defaultManifest = { ...DEFAULT_MANIFEST, updatedAt: new Date().toISOString() } | |
| await this.writeManifestAtomic(defaultManifest) | |
| return defaultManifest | |
| } | |
| // 기본값으로 대체하면 generation/currentSegment가 되돌아가 기존 segment를 오염시킨다. | |
| throw new StatsStoreError("STATS_STORE/append/005", "Failed to load manifest", err) | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 495-495: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(this.manifestPath, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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 494 - 520, Update
loadOrCreateManifest to validate manifestVersion equals the supported version
value, not merely that it is numeric. For non-ENOENT read or parse failures,
stop returning a DEFAULT_MANIFEST fallback and propagate the original error so
appendInternal does not derive a segment path from reset tracking state;
preserve default-manifest creation for missing or structurally invalid
manifests.
| return lockfile.lock(this.manifestPath, { | ||
| stale: 31000, | ||
| update: 10000, | ||
| realpath: false, | ||
| retries: { | ||
| retries: 5, | ||
| factor: 2, | ||
| minTimeout: 100, | ||
| maxTimeout: 1000, | ||
| }, | ||
| onCompromised: (err) => { | ||
| console.error(`[UsageEventStore] manifest lock was compromised:`, err) | ||
| throw err | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not throw from onCompromised.
proper-lockfile invokes onCompromised from its internal update timer, not from the lock() promise chain. The throw err at line 573 therefore surfaces as an uncaught exception rather than a rejected promise. No caller can catch it. In the VS Code extension host that produces an unhandled error, and the design goal states that storage failures must not break the LLM task.
Log the compromise and mark the store as unusable. Do not re-throw.
🛡️ Proposed handling
onCompromised: (err) => {
+ // 이 콜백은 lockfile 내부 타이머에서 호출된다. throw하면 uncaught exception이 된다.
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 561 - 575, Update the
onCompromised callback in the manifest lock flow to log the compromise and mark
the UsageEventStore as unusable without throwing the error. Ensure the callback
returns normally so the internal update timer cannot surface an uncaught
exception, while preserving the existing lock configuration and diagnostic
logging.
| private filterEventsByQuery(events: UsageEventV1[], query: StatsQuery): UsageEventV1[] { | ||
| // 시간 범위 | ||
| let from: Date | undefined | ||
| let to: Date | undefined | ||
|
|
||
| if (query.preset) { | ||
| const now = new Date() | ||
| const range = this.resolvePresetRange(query.preset, query.timezone, now) | ||
| from = range.from | ||
| to = range.to | ||
| } else { | ||
| from = query.from ? new Date(query.from) : undefined | ||
| to = query.to ? new Date(query.to) : undefined | ||
| } | ||
|
|
||
| let filtered = events.filter((event) => { | ||
| const eventTime = new Date(event.occurredAt).getTime() | ||
| if (from && eventTime < from.getTime()) return false | ||
| if (to && eventTime >= to.getTime()) return false | ||
| return true | ||
| }) | ||
|
|
||
| // cancelled 필터링 | ||
| const includeCancelled = query.includeCancelled ?? false | ||
| if (!includeCancelled) { | ||
| filtered = filtered.filter((e) => e.status !== "cancelled") | ||
| } | ||
|
|
||
| return filtered | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Query-range resolution and event filtering are implemented twice. UsageStatsService.filterEventsByQuery and UsageAggregator.query each resolve the preset range, convert timezone midnights, and drop cancelled events with their own copy of the logic. The two copies can drift, so exportStats and queryStats can return different event sets for the same StatsQuery. The timezone defect reported on src/services/stats/UsageAggregator.ts lines 148-171 also has to be fixed in both places.
Extract the shared logic into one module, for example src/services/stats/statsQueryRange.ts, exporting resolveTimeRange(query), getTimezoneOffsetMinutes(date, timezone), startOfDayInTimezone(date, timezone), and filterEvents(events, query). Then have both classes call it.
src/services/stats/UsageStatsService.ts#L259-L288: replace the body offilterEventsByQuerywith a call to the sharedfilterEvents.src/services/stats/UsageStatsService.ts#L293-L376: deleteresolvePresetRange,toTimezoneStartOfDay, andgetTimezoneOffsetMinutes, and import the shared helpers.src/services/stats/UsageAggregator.ts#L250-L271: deletestartOfDayandgetTimezoneOffsetMinutes, and import the shared helpers so both paths share one timezone implementation.
📍 Affects 2 files
src/services/stats/UsageStatsService.ts#L259-L288(this comment)src/services/stats/UsageStatsService.ts#L293-L376src/services/stats/UsageAggregator.ts#L250-L271
🤖 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 259 - 288, Extract the
duplicated query-range, timezone, and cancelled-event filtering logic into
shared exports resolveTimeRange, getTimezoneOffsetMinutes, startOfDayInTimezone,
and filterEvents in src/services/stats/statsQueryRange.ts. In
src/services/stats/UsageStatsService.ts lines 259-288, replace
filterEventsByQuery with the shared filterEvents call; at lines 293-376, remove
resolvePresetRange, toTimezoneStartOfDay, and getTimezoneOffsetMinutes and
import the shared helpers. In src/services/stats/UsageAggregator.ts lines
250-271, remove startOfDay and getTimezoneOffsetMinutes and use the shared
timezone helpers so both query paths apply identical range resolution and
timezone behavior.
| /** | ||
| * CSV cell을 escape한다. | ||
| * - spreadsheet formula injection 방지: `=`, `+`, `-`, `@`로 시작하면 `'`를 붙임 | ||
| * - 값에 `,`, `"`, `\n`이 포함되면 `"..."`로 감싸고 내부 `"`는 `""`로 escape | ||
| */ | ||
| private escapeCsvCell(value: string): string { | ||
| // 빈 값은 빈 cell | ||
| if (value === "") { | ||
| return "" | ||
| } | ||
|
|
||
| // formula injection 방지 | ||
| let escaped = value | ||
| if (/^[=+\-@]/.test(escaped)) { | ||
| escaped = `'${escaped}` | ||
| } | ||
|
|
||
| // quoting 필요 여부 | ||
| if (/[",\n]/.test(escaped)) { | ||
| escaped = `"${escaped.replace(/"/g, '""')}"` | ||
| } | ||
|
|
||
| return escaped | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add \r to the CSV quoting character class.
escapeCsvCell quotes a value when it contains ,, ", or \n, but not when it contains a bare \r. RFC 4180 parsers treat \r as part of the record terminator. A model, mode, or taskId value that carries a carriage return therefore splits the record, and every column after it shifts into a new row.
The formula-injection prefix and the quote doubling are correct, including their order.
🛠️ Proposed fix
// quoting 필요 여부
- if (/[",\n]/.test(escaped)) {
+ if (/[",\r\n]/.test(escaped)) {
escaped = `"${escaped.replace(/"/g, '""')}"`
}📝 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.
| /** | |
| * CSV cell을 escape한다. | |
| * - spreadsheet formula injection 방지: `=`, `+`, `-`, `@`로 시작하면 `'`를 붙임 | |
| * - 값에 `,`, `"`, `\n`이 포함되면 `"..."`로 감싸고 내부 `"`는 `""`로 escape | |
| */ | |
| private escapeCsvCell(value: string): string { | |
| // 빈 값은 빈 cell | |
| if (value === "") { | |
| return "" | |
| } | |
| // formula injection 방지 | |
| let escaped = value | |
| if (/^[=+\-@]/.test(escaped)) { | |
| escaped = `'${escaped}` | |
| } | |
| // quoting 필요 여부 | |
| if (/[",\n]/.test(escaped)) { | |
| escaped = `"${escaped.replace(/"/g, '""')}"` | |
| } | |
| return escaped | |
| } | |
| /** | |
| * CSV cell을 escape한다. | |
| * - spreadsheet formula injection 방지: `=`, `+`, `-`, `@`로 시작하면 `'`를 붙임 | |
| * - 값에 `,`, `"`, `\n`이 포함되면 `"..."`로 감싸고 내부 `"`는 `""`로 escape | |
| */ | |
| private escapeCsvCell(value: string): string { | |
| // 빈 값은 빈 cell | |
| if (value === "") { | |
| return "" | |
| } | |
| // formula injection 방지 | |
| let escaped = value | |
| if (/^[=+\-@]/.test(escaped)) { | |
| escaped = `'${escaped}` | |
| } | |
| // quoting 필요 여부 | |
| if (/[",\r\n]/.test(escaped)) { | |
| escaped = `"${escaped.replace(/"/g, '""')}"` | |
| } | |
| return escaped | |
| } |
🤖 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 484 - 507, Update the
quoting condition in escapeCsvCell to also detect carriage returns, so values
containing a bare \r are wrapped in CSV quotes while preserving the existing
formula-injection prefix and quote-doubling order.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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 minimal-field usage event/query contracts, append-only segmented NDJSON store, lock/queue, idempotency, rotation/hard cap, and corrupt-line quarantine. Does not include aggregation, capture, or UI.
Included Files
packages/types/src/usage-stats.tssrc/services/stats/UsageEventStore.tssrc/services/stats/index.tspackages/types/src/__tests__/usage-stats.spec.tssrc/services/stats/__tests__/UsageEventStore.spec.tsExclusion Scope
Summary by CodeRabbit