feature: local-usage-stats (1/4) - #1123
Conversation
…cit-any Add new test file to eslint-suppressions.json with count of 26 no-explicit-any suppressions. These are standard test patterns (mock objects, private property access via 'as any') consistent with other test files in the suppressions list. Fixes CI lint failure in PR #25 compile (lint) job.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUsage statistics now have validated contracts, task-level recording, persistent NDJSON storage, aggregation, exports, extension-host messages, and comprehensive tests. The change also adds CI session documentation and generated coverage data. ChangesUsage statistics
Repository and CI records
Estimated code review effort: 5 (Critical) | ~120 minutes 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
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
d16c665 to
4ced785
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 20
♻️ Duplicate comments (7)
src/services/stats/__tests__/UsageEventStore.spec.ts (1)
296-300: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename or remove this test; the name does not match the assertion.
The test name states that
appendthrows on cap reached. The body only assertsstore.isCapped() === falseon a fresh store. The cap path is already covered at lines 337-348. Rename this test to "should report not capped for a fresh store", or delete it.💚 Proposed change
- 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) + })🤖 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 296 - 300, Rename the test in the “error handling” describe block to reflect that it verifies a fresh store reports not capped, such as “should report not capped for a fresh store”; do not leave a name implying append throws on cap reached.src/services/stats/UsageEventStore.ts (6)
430-471: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRotation still writes to the old, full segment.
The code is unchanged from the previous review. Line 431 computes
segmentPathfrommanifest.currentSegment. Lines 446-450 incrementmanifest.currentSegmentand persist the manifest, but they never recomputesegmentPath. Thefs.open(segmentPath, "a")call at Line 457 appends to the segment that already reachedSEGMENT_MAX_BYTES. The error message at Line 468 also reports the new segment number while the write targets the old file.Change
segmentPathtoletand reassign it after the rotation branch.🤖 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, Update the append flow to declare segmentPath as mutable and recompute it from manifest.currentSegment after incrementing and persisting the manifest in the segment rotation branch, so fs.open and the write error context use the new segment.
336-360: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
clearstill moves the segments before it writes the new manifest.The code is unchanged from the previous review. If
writeManifestAtomicat Line 360 fails,clearthrowsSTATS_STORE/clear/002while the segments are already inold-generation-Nand the manifest still reports the previous generation.Call
writeManifestAtomic(newManifest)beforefs.mkdir(oldGenDir). A later rename failure is already tolerated by theconsole.warnpath at Line 355.🤖 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 336 - 360, Update the clear flow around writeManifestAtomic and oldGenDir so writeManifestAtomic(newManifest) executes before creating the backup directory or moving segment files. Preserve the existing rename loop and its console.warn handling for later move failures.
494-520: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA transient manifest read error still resets generation and segment tracking.
The code is unchanged from the previous review. Lines 517-519 catch every non-
ENOENTfailure and returnDEFAULT_MANIFESTwithgeneration: 1andcurrentSegment: 1, without persisting it.appendInternalderivessegmentPathfrom that value at Line 431, so anEACCESor a partially written manifest sends new events intoevents-000001.ndjsonand mixes them into an already rotated segment.Line 500 also checks only the type of
manifestVersion, not the value, so a future v2 manifest is read as v1.Throw for non-
ENOENTfailures, and compareparsed.manifestVersion === 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/UsageEventStore.ts` around lines 494 - 520, Update loadOrCreateManifest to accept a parsed manifest only when parsed.manifestVersion === 1, while retaining the existing numeric checks for generation and currentSegment. In its catch block, keep ENOENT creation behavior, but rethrow every other read or parse error instead of returning an in-memory DEFAULT_MANIFEST fallback.
561-575: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
onCompromisedstill throws.The code is unchanged from the previous review.
proper-lockfileinvokesonCompromisedfrom its internal update timer, not from thelock()promise chain. Thethrow errat Line 573 becomes an uncaught exception in the extension host. No caller can catch it, and the stated design goal is that storage failures must not break the LLM task.Log the compromise and mark the store as unusable. Do not re-throw.
🤖 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 setup to log the compromise and mark the UsageEventStore as unusable, removing the throw so the internal lock timer cannot produce an uncaught exception. Preserve the existing error logging and use the store’s established unusable-state mechanism.
309-315: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
clearstill bypasses the in-process queue used byappend.The code is unchanged from the previous review.
appendserializes throughthis.queue, andcleardoes not. Both mutate the same segment files and the sameidempotencyKeysset. Theproper-lockfilelock is acquired per critical section, so a queuedappendcan createevents-000001.ndjsonbetween thereaddirat Line 343 and the rename loop at Line 348. That event is then moved intoold-generation-Nand disappears fromreadAll.Extract the deferred-promise logic from
appendinto a privateenqueue<T>helper. Run bothappendandclearthrough it.🤖 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 309 - 315, Update UsageEventStore.clear and append to share the same in-process serialization by extracting append’s deferred-promise queue logic into a private enqueue<T> helper. Wrap each operation’s full mutation flow, including manifest-lock acquisition and release, through this helper so clear cannot interleave with queued appends; preserve existing append and clear behavior otherwise.
264-299: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
readAllstill re-quarantines the same corrupt lines on every call.The code is unchanged from the previous review.
writeQuarantineReportopens the report in append mode, and no state marks a line as already reported. EveryreadAllcall appends one entry per corrupt line that is still present. The report file grows without bound.Track reported
segment:line:hashkeys in aSetand filterquarantineEntriesbefore the write.🤖 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 264 - 299, Update readAll to deduplicate quarantine entries across calls by tracking reported segment:line:hash keys in a Set, using each entry’s segment, line, and content hash to form the key. Filter quarantineEntries against this state before invoking writeQuarantineReport, and mark newly written entries as reported only when they are actually included.
🧹 Nitpick comments (4)
src/services/stats/__tests__/UsageStatsService.spec.ts (1)
610-611: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests reach private members through
as unknown asdouble assertions. The shared root cause is the cast pattern used to set private state on the store. The coding guidelines require bracket notation for private members and permit double assertions only as a last resort with an explanatory comment. Bracket notation keeps each member typed and removes every cast.
src/services/stats/__tests__/UsageStatsService.spec.ts#L610-L611: replace(cappedService as unknown as { store: { capped: boolean } }).storewithcappedService["store"], and apply the same change to thestore.appendmonkeypatch at lines 629-638.src/services/stats/__tests__/UsageEventStore.spec.ts#L337-L348: replacestore as unknown as { capped: boolean }with directstore["capped"]assignments for both the set and the reset.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards. Use double assertions only as a last resort and explain them with 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/__tests__/UsageStatsService.spec.ts` around lines 610 - 611, Replace the double-assertion private-member access in src/services/stats/__tests__/UsageStatsService.spec.ts lines 610-611 with bracket notation on cappedService, and make the same change for the store.append monkeypatch at lines 629-638. In src/services/stats/__tests__/UsageEventStore.spec.ts lines 337-348, access the private capped member directly with bracket notation for both setting and resetting it; remove the unnecessary casts.Source: Coding guidelines
scripts/task_b14.ts (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDo not commit a production source snapshot under
scripts/.This file is a full copy of the
Taskclass fromsrc/core/task/. The relative imports (./AskIgnoredError,./RateLimitClock,../../shared/package,../../api) only resolve fromsrc/core/task/. Fromscripts/they are unresolvable, so this module cannot compile or run.The copy also drifts from the live implementation. Example: line 3217 builds
requestKeyas${taskId}:${retryAttempt}, whilescripts/task_b15.tsdocuments that same form as a defect. Keeping the snapshot creates a second, silently stale source of truth.Remove the file from the PR. If the snapshot is needed for a migration record, keep it outside the compiled tree (for example an attachment on the PR or a
.patchartifact).🤖 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 `@scripts/task_b14.ts` around lines 1 - 9, Remove the scripts/task_b14.ts snapshot from the PR; do not retain a duplicate Task implementation under scripts/. If historical migration evidence is required, move it outside the compiled source tree as a patch or PR attachment rather than maintaining this unresolvable, stale copy.src/services/stats/UsageEventStore.ts (2)
476-477: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
checkTotalSizeruns a full directory scan on everyappend.
checkTotalSizecallsreaddirand then onestatper segment.appendInternalcalls it after every event, inside the cross-process lock. At the 100 MiB cap with 5 MiB segments that is up to 21 syscalls per recorded event, and the lock is held for all of them.Track the total size in a field. Add the byte length of the written line after each append, and re-scan only when the tracked value crosses the cap or when
initializeruns.🤖 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 - 477, Replace the per-append checkTotalSize scan in appendInternal with a tracked total-size field: increment it by the written line’s byte length after each successful append, and rescan only when the tracked value crosses the cap. Initialize or refresh this field during initialize, while preserving capped-state behavior and the existing lock flow.
155-186: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMemoize the initialization promise.
initializesetsthis.initializedonly after all async work completes.ensureInitializedruns outside the queue inreadAll,clear, andgetManifest. Two concurrent callers can therefore both pass the guard at Line 156 and run the body twice. The secondrebuildIdempotencySetcall clearsidempotencyKeysat Line 584, so a key added by an in-flightappendInternalcan be dropped and the same event can be written twice.Store the in-flight promise and return it to later callers.
♻️ Proposed memoization
/** 초기화 완료 여부 */ private initialized = false + + /** 진행 중인 초기화 promise */ + private initPromise: Promise<void> | undefinedasync initialize(): Promise<void> { if (this.initialized) { return } + if (!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 to memoize its in-flight initialization promise: the first caller should run the existing directory, manifest, idempotency, and cap setup, while concurrent or later callers return the same promise instead of entering the body again. Preserve the initialized guard and ensure the stored promise is cleared or finalized appropriately after completion so initialization remains safe for subsequent calls.
🤖 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 `@b15_task_diff.patch`:
- Around line 1-4: Delete the generated b15_task_diff.patch artifact from the
repository, and update .gitignore to exclude *.patch files so future development
artifacts are not committed.
In `@cherry-codecov.ps1`:
- Around line 28-48: Stop processing each branch whenever a Git operation fails
by checking $LASTEXITCODE immediately after every relevant command. In
cherry-codecov.ps1 lines 28-48, validate checkout, cherry-pick or fallback
checkout/commit, and push; in clean-docs.ps1 lines 16-38, clean-docs2.ps1 lines
13-36, clean-docs3.ps1 lines 15-39, clean-docs4.ps1 lines 15-40, and
clean-docs5.ps1 lines 14-39, validate each listed checkout/reset, removal
commit, and push before continuing, aborting or returning on failure so no
incorrect branch is pushed.
In `@clean-docs.ps1`:
- Line 28: Update the file-removal command in the cleanup loop to use git rm -f
instead of git rm --cached, ensuring each documentation file is removed from
both the index and worktree before the subsequent checkout.
In `@coverage-output.txt`:
- Around line 1-2: Remove the tracked generated file coverage-output.txt from
the repository, and ensure future coverage terminal output is excluded from
version control. Leave coverage generation and CI artifact handling to the
existing workflow if coverage results are required.
In `@docs/260804_0002_session_ci-fix-compile/161600_vp-handoff.md`:
- Around line 55-71: Fix the command examples in the handoff document: mark both
fenced blocks as powershell, and replace the invalid loop with a PowerShell
foreach over a defined $prs collection containing all listed PR numbers,
invoking gh pr checks for each value.
In `@docs/260804_pr_audit/hands-off-document.md`:
- Line 45: Update the dependency graph code fence in hands-off-document.md to
specify the text language identifier, preserving the fenced content unchanged.
In `@docs/260805_0001_session_ci-all-green/hands-off-document.md`:
- Line 175: Add language identifiers to both code fences in the document: use
text for the dependency graph fence and markdown for the PR-description snippet,
including the corresponding fence at the additional referenced location, to
satisfy markdownlint MD040.
- Around line 130-132: Remove the stale Option A coverage-bypass path: in
docs/260805_0001_session_ci-all-green/hands-off-document.md lines 130-132,
replace the recommendation with the approved Option B plan; at lines 250-257,
direct the next session to add tests without presenting Option A; in
docs/260805_0001_session_ci-all-green/new-session-prompt.md lines 23-26,
instruct implementation of Option B; and delete the obsolete bypass automation
from fix-codecov-b05.ps1 lines 15-27 and fix-codecov-missing.ps1 lines 17-35.
In `@fix-codecov-b05.ps1`:
- Around line 27-28: Check $LASTEXITCODE immediately after the git push in
fix-codecov-b05.ps1 lines 27-28 and fix-codecov-missing.ps1 lines 33-35 before
printing “Pushed”; on failure, output the captured $pushResult details and stop
processing, while preserving the success message only for successful pushes.
In `@packages/types/coverage-json/coverage-final.json`:
- Around line 2-3: Remove the tracked generated coverage artifact
coverage-final.json containing local workspace paths, or reconfigure the
coverage generation so committed entries use project-relative paths instead.
Ensure no sensitive absolute paths remain in coverage metadata.
In `@restore-codecov.ps1`:
- Line 19: Update the branch checkout flow in restore-codecov.ps1 immediately
after git checkout $branch to inspect $LASTEXITCODE and terminate on failure
before any subsequent mutation, commit, or push. Apply the same failure guard to
the later checkout operation near the push flow, ensuring a failed checkout
cannot continue to commit or force-push the wrong branch state.
In `@scripts/merge_b15_task_v2.py`:
- Around line 78-106: Make the merge fail instead of writing a partial Task.ts
when any usage block lacks its preceding-line anchor. Update the insertion loop
around usage_blocks to track failed insertions, validate that every required
block was inserted and the generated result is valid, and return a nonzero exit
status before the write step when validation fails; only write the file and
report success after all checks pass.
In `@scripts/merge_b15_task.py`:
- Around line 19-73: Extend the merge logic beyond the imports and endpoint
helpers to also apply the complete UsageRecorder lifecycle changes from B15 to
Task.ts, including recorder construction and terminal event finalization. Locate
and merge the corresponding Task class or task-execution methods and preserve
their event-recording behavior so the generated file both uses the imported
UsageRecorder symbols and records usage through completion or failure.
In `@scripts/squash-final.ps1`:
- Around line 47-58: Remove the blanket conflict-resolution loop in the squash
script that runs git checkout --theirs for every unmerged file. Update the
merge-conflict handling around $mergeOutput to stop and require deliberate
conflict resolution, preserving both base and source changes; only continue
after conflicts are explicitly resolved and staged, with appropriate validation
before completing the squash.
In `@scripts/squash-push-17prs.ps1`:
- Around line 95-103: Update the Step 4 push in the squash-push loop to refresh
the relevant myk1yt/pr/* refs before pushing, then replace --force with
--force-with-lease on the git push command. Preserve the existing failure
handling and stop the loop when the lease rejects the push.
- Around line 8-10: Update the setup before the squash loop in the script to
fetch the required myk1yt/pr/* fork refs, then validate that those refs are
available before processing any PRs. Keep the existing upstream/main reset
behavior and ensure the loop’s merge inputs use freshly fetched, non-stale fork
references.
- Around line 7-10: Update the setup flow before git reset in
squash-push-17prs.ps1 to inspect git status --porcelain and abort when the
workspace has staged or unstaged changes, before executing git reset --hard
upstream/main. Preserve the existing checkout and fetch behavior for clean
workspaces.
In `@scripts/task_b14.ts`:
- Around line 1-9: Remove the dead duplicate files scripts/task_b14.ts and
scripts/task_base.ts. Remove scripts/task_b15.ts after first porting its
resolveEndpoint logic into src/core/task/Task.ts if that behavior is required;
in the real Task implementation, also replace the undocumented double assertion
and properly await or handle the postMessageToWebview promise. Apply these
changes respectively to scripts/task_b14.ts (lines 1-9), scripts/task_b15.ts
(lines 629-645), and scripts/task_base.ts (lines 1-9).
In `@src/services/stats/__tests__/UsageStatsService.spec.ts`:
- Around line 771-781: Remove the ineffective fallback test around
service.issueClearNonce, including the unused originalRequire and moduleCache
declarations, since it neither stubs crypto.randomUUID nor exercises the
fallback and may reference require in ESM scope.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 96-97: Update makeQuarantineEntry to use node:crypto
createHash("sha256") for the corruption hash, returning the first 16 hexadecimal
characters to match the hash field contract. Remove the rolling 32-bit hash
implementation and its dependency-minimization justification, while preserving
the existing quarantine entry and deduplication flow.
---
Duplicate comments:
In `@src/services/stats/__tests__/UsageEventStore.spec.ts`:
- Around line 296-300: Rename the test in the “error handling” describe block to
reflect that it verifies a fresh store reports not capped, such as “should
report not capped for a fresh store”; do not leave a name implying append throws
on cap reached.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 430-471: Update the append flow to declare segmentPath as mutable
and recompute it from manifest.currentSegment after incrementing and persisting
the manifest in the segment rotation branch, so fs.open and the write error
context use the new segment.
- Around line 336-360: Update the clear flow around writeManifestAtomic and
oldGenDir so writeManifestAtomic(newManifest) executes before creating the
backup directory or moving segment files. Preserve the existing rename loop and
its console.warn handling for later move failures.
- Around line 494-520: Update loadOrCreateManifest to accept a parsed manifest
only when parsed.manifestVersion === 1, while retaining the existing numeric
checks for generation and currentSegment. In its catch block, keep ENOENT
creation behavior, but rethrow every other read or parse error instead of
returning an in-memory DEFAULT_MANIFEST fallback.
- Around line 561-575: Update the onCompromised callback in the manifest lock
setup to log the compromise and mark the UsageEventStore as unusable, removing
the throw so the internal lock timer cannot produce an uncaught exception.
Preserve the existing error logging and use the store’s established
unusable-state mechanism.
- Around line 309-315: Update UsageEventStore.clear and append to share the same
in-process serialization by extracting append’s deferred-promise queue logic
into a private enqueue<T> helper. Wrap each operation’s full mutation flow,
including manifest-lock acquisition and release, through this helper so clear
cannot interleave with queued appends; preserve existing append and clear
behavior otherwise.
- Around line 264-299: Update readAll to deduplicate quarantine entries across
calls by tracking reported segment:line:hash keys in a Set, using each entry’s
segment, line, and content hash to form the key. Filter quarantineEntries
against this state before invoking writeQuarantineReport, and mark newly written
entries as reported only when they are actually included.
---
Nitpick comments:
In `@scripts/task_b14.ts`:
- Around line 1-9: Remove the scripts/task_b14.ts snapshot from the PR; do not
retain a duplicate Task implementation under scripts/. If historical migration
evidence is required, move it outside the compiled source tree as a patch or PR
attachment rather than maintaining this unresolvable, stale copy.
In `@src/services/stats/__tests__/UsageStatsService.spec.ts`:
- Around line 610-611: Replace the double-assertion private-member access in
src/services/stats/__tests__/UsageStatsService.spec.ts lines 610-611 with
bracket notation on cappedService, and make the same change for the store.append
monkeypatch at lines 629-638. In
src/services/stats/__tests__/UsageEventStore.spec.ts lines 337-348, access the
private capped member directly with bracket notation for both setting and
resetting it; remove the unnecessary casts.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 476-477: Replace the per-append checkTotalSize scan in
appendInternal with a tracked total-size field: increment it by the written
line’s byte length after each successful append, and rescan only when the
tracked value crosses the cap. Initialize or refresh this field during
initialize, while preserving capped-state behavior and the existing lock flow.
- Around line 155-186: Update UsageEventStore.initialize to memoize its
in-flight initialization promise: the first caller should run the existing
directory, manifest, idempotency, and cap setup, while concurrent or later
callers return the same promise instead of entering the body again. Preserve the
initialized guard and ensure the stored promise is cleared or finalized
appropriately after completion so initialization remains safe for subsequent
calls.
🪄 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: 3cb1e77e-2bd5-40dc-afe9-4425b832dca2
📒 Files selected for processing (53)
b15_task_diff.patchcherry-codecov.ps1clean-docs.ps1clean-docs2.ps1clean-docs3.ps1clean-docs4.ps1clean-docs5.ps1coverage-output.txtdocs/260804_0002_session_ci-fix-compile/013100_debug-report.mddocs/260804_0002_session_ci-fix-compile/161500_debug-report.mddocs/260804_0002_session_ci-fix-compile/161600_vp-handoff.mddocs/260804_0002_session_ci-fix-compile/180500_debug-report.mddocs/260804_0002_session_ci-fix-compile/194800_debug-report.mddocs/260804_0002_session_ci-fix-compile/205100_debug-report.mddocs/260804_0003_session_merge-conflict-resolution/033900_debug-report.mddocs/260804_0003_session_merge-conflict-resolution/113400_debug-report.mddocs/260804_pr_audit/hands-off-document.mddocs/260805_0001_session_ci-all-green/052917_debug-coverage-b13.mddocs/260805_0001_session_ci-all-green/decisions.mddocs/260805_0001_session_ci-all-green/hands-off-document.mddocs/260805_0001_session_ci-all-green/new-session-prompt.mdfix-codecov-b05.ps1fix-codecov-missing.ps1packages/types/coverage-json/coverage-final.jsonpackages/types/src/__tests__/usage-stats.spec.tspackages/types/src/index.tspackages/types/src/usage-stats.tspackages/types/src/vscode-extension-host.tsrestore-codecov.ps1scripts/create-upstream-prs.ps1scripts/merge_b15_task.pyscripts/merge_b15_task_v2.pyscripts/pr-creation-results.jsonscripts/pr-metadata.jsonscripts/squash-continue.ps1scripts/squash-final.ps1scripts/squash-push-17prs.ps1scripts/squash-results.jsonscripts/task_b14.tsscripts/task_b15.tsscripts/task_base.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.usage-stats.spec.tssrc/coverage-json/coverage-final.jsonsrc/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/index.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/services/stats/index.ts
- packages/types/src/index.ts
- packages/types/src/tests/usage-stats.spec.ts
- src/eslint-suppressions.json
- src/core/task/tests/Task.usage-stats.spec.ts
- packages/types/src/vscode-extension-host.ts
- packages/types/src/usage-stats.ts
- src/services/stats/UsageRecorder.ts
- src/core/task/Task.ts
- src/services/stats/UsageAggregator.ts
- src/services/stats/UsageStatsService.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 20
♻️ Duplicate comments (7)
src/services/stats/__tests__/UsageEventStore.spec.ts (1)
296-300: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename or remove this test; the name does not match the assertion.
The test name states that
appendthrows on cap reached. The body only assertsstore.isCapped() === falseon a fresh store. The cap path is already covered at lines 337-348. Rename this test to "should report not capped for a fresh store", or delete it.💚 Proposed change
- 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) + })🤖 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 296 - 300, Rename the test in the “error handling” describe block to reflect that it verifies a fresh store reports not capped, such as “should report not capped for a fresh store”; do not leave a name implying append throws on cap reached.src/services/stats/UsageEventStore.ts (6)
430-471: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRotation still writes to the old, full segment.
The code is unchanged from the previous review. Line 431 computes
segmentPathfrommanifest.currentSegment. Lines 446-450 incrementmanifest.currentSegmentand persist the manifest, but they never recomputesegmentPath. Thefs.open(segmentPath, "a")call at Line 457 appends to the segment that already reachedSEGMENT_MAX_BYTES. The error message at Line 468 also reports the new segment number while the write targets the old file.Change
segmentPathtoletand reassign it after the rotation branch.🤖 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, Update the append flow to declare segmentPath as mutable and recompute it from manifest.currentSegment after incrementing and persisting the manifest in the segment rotation branch, so fs.open and the write error context use the new segment.
336-360: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
clearstill moves the segments before it writes the new manifest.The code is unchanged from the previous review. If
writeManifestAtomicat Line 360 fails,clearthrowsSTATS_STORE/clear/002while the segments are already inold-generation-Nand the manifest still reports the previous generation.Call
writeManifestAtomic(newManifest)beforefs.mkdir(oldGenDir). A later rename failure is already tolerated by theconsole.warnpath at Line 355.🤖 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 336 - 360, Update the clear flow around writeManifestAtomic and oldGenDir so writeManifestAtomic(newManifest) executes before creating the backup directory or moving segment files. Preserve the existing rename loop and its console.warn handling for later move failures.
494-520: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA transient manifest read error still resets generation and segment tracking.
The code is unchanged from the previous review. Lines 517-519 catch every non-
ENOENTfailure and returnDEFAULT_MANIFESTwithgeneration: 1andcurrentSegment: 1, without persisting it.appendInternalderivessegmentPathfrom that value at Line 431, so anEACCESor a partially written manifest sends new events intoevents-000001.ndjsonand mixes them into an already rotated segment.Line 500 also checks only the type of
manifestVersion, not the value, so a future v2 manifest is read as v1.Throw for non-
ENOENTfailures, and compareparsed.manifestVersion === 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/UsageEventStore.ts` around lines 494 - 520, Update loadOrCreateManifest to accept a parsed manifest only when parsed.manifestVersion === 1, while retaining the existing numeric checks for generation and currentSegment. In its catch block, keep ENOENT creation behavior, but rethrow every other read or parse error instead of returning an in-memory DEFAULT_MANIFEST fallback.
561-575: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
onCompromisedstill throws.The code is unchanged from the previous review.
proper-lockfileinvokesonCompromisedfrom its internal update timer, not from thelock()promise chain. Thethrow errat Line 573 becomes an uncaught exception in the extension host. No caller can catch it, and the stated design goal is that storage failures must not break the LLM task.Log the compromise and mark the store as unusable. Do not re-throw.
🤖 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 setup to log the compromise and mark the UsageEventStore as unusable, removing the throw so the internal lock timer cannot produce an uncaught exception. Preserve the existing error logging and use the store’s established unusable-state mechanism.
309-315: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
clearstill bypasses the in-process queue used byappend.The code is unchanged from the previous review.
appendserializes throughthis.queue, andcleardoes not. Both mutate the same segment files and the sameidempotencyKeysset. Theproper-lockfilelock is acquired per critical section, so a queuedappendcan createevents-000001.ndjsonbetween thereaddirat Line 343 and the rename loop at Line 348. That event is then moved intoold-generation-Nand disappears fromreadAll.Extract the deferred-promise logic from
appendinto a privateenqueue<T>helper. Run bothappendandclearthrough it.🤖 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 309 - 315, Update UsageEventStore.clear and append to share the same in-process serialization by extracting append’s deferred-promise queue logic into a private enqueue<T> helper. Wrap each operation’s full mutation flow, including manifest-lock acquisition and release, through this helper so clear cannot interleave with queued appends; preserve existing append and clear behavior otherwise.
264-299: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
readAllstill re-quarantines the same corrupt lines on every call.The code is unchanged from the previous review.
writeQuarantineReportopens the report in append mode, and no state marks a line as already reported. EveryreadAllcall appends one entry per corrupt line that is still present. The report file grows without bound.Track reported
segment:line:hashkeys in aSetand filterquarantineEntriesbefore the write.🤖 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 264 - 299, Update readAll to deduplicate quarantine entries across calls by tracking reported segment:line:hash keys in a Set, using each entry’s segment, line, and content hash to form the key. Filter quarantineEntries against this state before invoking writeQuarantineReport, and mark newly written entries as reported only when they are actually included.
🧹 Nitpick comments (4)
src/services/stats/__tests__/UsageStatsService.spec.ts (1)
610-611: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests reach private members through
as unknown asdouble assertions. The shared root cause is the cast pattern used to set private state on the store. The coding guidelines require bracket notation for private members and permit double assertions only as a last resort with an explanatory comment. Bracket notation keeps each member typed and removes every cast.
src/services/stats/__tests__/UsageStatsService.spec.ts#L610-L611: replace(cappedService as unknown as { store: { capped: boolean } }).storewithcappedService["store"], and apply the same change to thestore.appendmonkeypatch at lines 629-638.src/services/stats/__tests__/UsageEventStore.spec.ts#L337-L348: replacestore as unknown as { capped: boolean }with directstore["capped"]assignments for both the set and the reset.As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles andunknownwith type guards. Use double assertions only as a last resort and explain them with 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/__tests__/UsageStatsService.spec.ts` around lines 610 - 611, Replace the double-assertion private-member access in src/services/stats/__tests__/UsageStatsService.spec.ts lines 610-611 with bracket notation on cappedService, and make the same change for the store.append monkeypatch at lines 629-638. In src/services/stats/__tests__/UsageEventStore.spec.ts lines 337-348, access the private capped member directly with bracket notation for both setting and resetting it; remove the unnecessary casts.Source: Coding guidelines
scripts/task_b14.ts (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDo not commit a production source snapshot under
scripts/.This file is a full copy of the
Taskclass fromsrc/core/task/. The relative imports (./AskIgnoredError,./RateLimitClock,../../shared/package,../../api) only resolve fromsrc/core/task/. Fromscripts/they are unresolvable, so this module cannot compile or run.The copy also drifts from the live implementation. Example: line 3217 builds
requestKeyas${taskId}:${retryAttempt}, whilescripts/task_b15.tsdocuments that same form as a defect. Keeping the snapshot creates a second, silently stale source of truth.Remove the file from the PR. If the snapshot is needed for a migration record, keep it outside the compiled tree (for example an attachment on the PR or a
.patchartifact).🤖 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 `@scripts/task_b14.ts` around lines 1 - 9, Remove the scripts/task_b14.ts snapshot from the PR; do not retain a duplicate Task implementation under scripts/. If historical migration evidence is required, move it outside the compiled source tree as a patch or PR attachment rather than maintaining this unresolvable, stale copy.src/services/stats/UsageEventStore.ts (2)
476-477: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
checkTotalSizeruns a full directory scan on everyappend.
checkTotalSizecallsreaddirand then onestatper segment.appendInternalcalls it after every event, inside the cross-process lock. At the 100 MiB cap with 5 MiB segments that is up to 21 syscalls per recorded event, and the lock is held for all of them.Track the total size in a field. Add the byte length of the written line after each append, and re-scan only when the tracked value crosses the cap or when
initializeruns.🤖 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 - 477, Replace the per-append checkTotalSize scan in appendInternal with a tracked total-size field: increment it by the written line’s byte length after each successful append, and rescan only when the tracked value crosses the cap. Initialize or refresh this field during initialize, while preserving capped-state behavior and the existing lock flow.
155-186: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMemoize the initialization promise.
initializesetsthis.initializedonly after all async work completes.ensureInitializedruns outside the queue inreadAll,clear, andgetManifest. Two concurrent callers can therefore both pass the guard at Line 156 and run the body twice. The secondrebuildIdempotencySetcall clearsidempotencyKeysat Line 584, so a key added by an in-flightappendInternalcan be dropped and the same event can be written twice.Store the in-flight promise and return it to later callers.
♻️ Proposed memoization
/** 초기화 완료 여부 */ private initialized = false + + /** 진행 중인 초기화 promise */ + private initPromise: Promise<void> | undefinedasync initialize(): Promise<void> { if (this.initialized) { return } + if (!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 to memoize its in-flight initialization promise: the first caller should run the existing directory, manifest, idempotency, and cap setup, while concurrent or later callers return the same promise instead of entering the body again. Preserve the initialized guard and ensure the stored promise is cleared or finalized appropriately after completion so initialization remains safe for subsequent calls.
🤖 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 `@b15_task_diff.patch`:
- Around line 1-4: Delete the generated b15_task_diff.patch artifact from the
repository, and update .gitignore to exclude *.patch files so future development
artifacts are not committed.
In `@cherry-codecov.ps1`:
- Around line 28-48: Stop processing each branch whenever a Git operation fails
by checking $LASTEXITCODE immediately after every relevant command. In
cherry-codecov.ps1 lines 28-48, validate checkout, cherry-pick or fallback
checkout/commit, and push; in clean-docs.ps1 lines 16-38, clean-docs2.ps1 lines
13-36, clean-docs3.ps1 lines 15-39, clean-docs4.ps1 lines 15-40, and
clean-docs5.ps1 lines 14-39, validate each listed checkout/reset, removal
commit, and push before continuing, aborting or returning on failure so no
incorrect branch is pushed.
In `@clean-docs.ps1`:
- Line 28: Update the file-removal command in the cleanup loop to use git rm -f
instead of git rm --cached, ensuring each documentation file is removed from
both the index and worktree before the subsequent checkout.
In `@coverage-output.txt`:
- Around line 1-2: Remove the tracked generated file coverage-output.txt from
the repository, and ensure future coverage terminal output is excluded from
version control. Leave coverage generation and CI artifact handling to the
existing workflow if coverage results are required.
In `@docs/260804_0002_session_ci-fix-compile/161600_vp-handoff.md`:
- Around line 55-71: Fix the command examples in the handoff document: mark both
fenced blocks as powershell, and replace the invalid loop with a PowerShell
foreach over a defined $prs collection containing all listed PR numbers,
invoking gh pr checks for each value.
In `@docs/260804_pr_audit/hands-off-document.md`:
- Line 45: Update the dependency graph code fence in hands-off-document.md to
specify the text language identifier, preserving the fenced content unchanged.
In `@docs/260805_0001_session_ci-all-green/hands-off-document.md`:
- Line 175: Add language identifiers to both code fences in the document: use
text for the dependency graph fence and markdown for the PR-description snippet,
including the corresponding fence at the additional referenced location, to
satisfy markdownlint MD040.
- Around line 130-132: Remove the stale Option A coverage-bypass path: in
docs/260805_0001_session_ci-all-green/hands-off-document.md lines 130-132,
replace the recommendation with the approved Option B plan; at lines 250-257,
direct the next session to add tests without presenting Option A; in
docs/260805_0001_session_ci-all-green/new-session-prompt.md lines 23-26,
instruct implementation of Option B; and delete the obsolete bypass automation
from fix-codecov-b05.ps1 lines 15-27 and fix-codecov-missing.ps1 lines 17-35.
In `@fix-codecov-b05.ps1`:
- Around line 27-28: Check $LASTEXITCODE immediately after the git push in
fix-codecov-b05.ps1 lines 27-28 and fix-codecov-missing.ps1 lines 33-35 before
printing “Pushed”; on failure, output the captured $pushResult details and stop
processing, while preserving the success message only for successful pushes.
In `@packages/types/coverage-json/coverage-final.json`:
- Around line 2-3: Remove the tracked generated coverage artifact
coverage-final.json containing local workspace paths, or reconfigure the
coverage generation so committed entries use project-relative paths instead.
Ensure no sensitive absolute paths remain in coverage metadata.
In `@restore-codecov.ps1`:
- Line 19: Update the branch checkout flow in restore-codecov.ps1 immediately
after git checkout $branch to inspect $LASTEXITCODE and terminate on failure
before any subsequent mutation, commit, or push. Apply the same failure guard to
the later checkout operation near the push flow, ensuring a failed checkout
cannot continue to commit or force-push the wrong branch state.
In `@scripts/merge_b15_task_v2.py`:
- Around line 78-106: Make the merge fail instead of writing a partial Task.ts
when any usage block lacks its preceding-line anchor. Update the insertion loop
around usage_blocks to track failed insertions, validate that every required
block was inserted and the generated result is valid, and return a nonzero exit
status before the write step when validation fails; only write the file and
report success after all checks pass.
In `@scripts/merge_b15_task.py`:
- Around line 19-73: Extend the merge logic beyond the imports and endpoint
helpers to also apply the complete UsageRecorder lifecycle changes from B15 to
Task.ts, including recorder construction and terminal event finalization. Locate
and merge the corresponding Task class or task-execution methods and preserve
their event-recording behavior so the generated file both uses the imported
UsageRecorder symbols and records usage through completion or failure.
In `@scripts/squash-final.ps1`:
- Around line 47-58: Remove the blanket conflict-resolution loop in the squash
script that runs git checkout --theirs for every unmerged file. Update the
merge-conflict handling around $mergeOutput to stop and require deliberate
conflict resolution, preserving both base and source changes; only continue
after conflicts are explicitly resolved and staged, with appropriate validation
before completing the squash.
In `@scripts/squash-push-17prs.ps1`:
- Around line 95-103: Update the Step 4 push in the squash-push loop to refresh
the relevant myk1yt/pr/* refs before pushing, then replace --force with
--force-with-lease on the git push command. Preserve the existing failure
handling and stop the loop when the lease rejects the push.
- Around line 8-10: Update the setup before the squash loop in the script to
fetch the required myk1yt/pr/* fork refs, then validate that those refs are
available before processing any PRs. Keep the existing upstream/main reset
behavior and ensure the loop’s merge inputs use freshly fetched, non-stale fork
references.
- Around line 7-10: Update the setup flow before git reset in
squash-push-17prs.ps1 to inspect git status --porcelain and abort when the
workspace has staged or unstaged changes, before executing git reset --hard
upstream/main. Preserve the existing checkout and fetch behavior for clean
workspaces.
In `@scripts/task_b14.ts`:
- Around line 1-9: Remove the dead duplicate files scripts/task_b14.ts and
scripts/task_base.ts. Remove scripts/task_b15.ts after first porting its
resolveEndpoint logic into src/core/task/Task.ts if that behavior is required;
in the real Task implementation, also replace the undocumented double assertion
and properly await or handle the postMessageToWebview promise. Apply these
changes respectively to scripts/task_b14.ts (lines 1-9), scripts/task_b15.ts
(lines 629-645), and scripts/task_base.ts (lines 1-9).
In `@src/services/stats/__tests__/UsageStatsService.spec.ts`:
- Around line 771-781: Remove the ineffective fallback test around
service.issueClearNonce, including the unused originalRequire and moduleCache
declarations, since it neither stubs crypto.randomUUID nor exercises the
fallback and may reference require in ESM scope.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 96-97: Update makeQuarantineEntry to use node:crypto
createHash("sha256") for the corruption hash, returning the first 16 hexadecimal
characters to match the hash field contract. Remove the rolling 32-bit hash
implementation and its dependency-minimization justification, while preserving
the existing quarantine entry and deduplication flow.
---
Duplicate comments:
In `@src/services/stats/__tests__/UsageEventStore.spec.ts`:
- Around line 296-300: Rename the test in the “error handling” describe block to
reflect that it verifies a fresh store reports not capped, such as “should
report not capped for a fresh store”; do not leave a name implying append throws
on cap reached.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 430-471: Update the append flow to declare segmentPath as mutable
and recompute it from manifest.currentSegment after incrementing and persisting
the manifest in the segment rotation branch, so fs.open and the write error
context use the new segment.
- Around line 336-360: Update the clear flow around writeManifestAtomic and
oldGenDir so writeManifestAtomic(newManifest) executes before creating the
backup directory or moving segment files. Preserve the existing rename loop and
its console.warn handling for later move failures.
- Around line 494-520: Update loadOrCreateManifest to accept a parsed manifest
only when parsed.manifestVersion === 1, while retaining the existing numeric
checks for generation and currentSegment. In its catch block, keep ENOENT
creation behavior, but rethrow every other read or parse error instead of
returning an in-memory DEFAULT_MANIFEST fallback.
- Around line 561-575: Update the onCompromised callback in the manifest lock
setup to log the compromise and mark the UsageEventStore as unusable, removing
the throw so the internal lock timer cannot produce an uncaught exception.
Preserve the existing error logging and use the store’s established
unusable-state mechanism.
- Around line 309-315: Update UsageEventStore.clear and append to share the same
in-process serialization by extracting append’s deferred-promise queue logic
into a private enqueue<T> helper. Wrap each operation’s full mutation flow,
including manifest-lock acquisition and release, through this helper so clear
cannot interleave with queued appends; preserve existing append and clear
behavior otherwise.
- Around line 264-299: Update readAll to deduplicate quarantine entries across
calls by tracking reported segment:line:hash keys in a Set, using each entry’s
segment, line, and content hash to form the key. Filter quarantineEntries
against this state before invoking writeQuarantineReport, and mark newly written
entries as reported only when they are actually included.
---
Nitpick comments:
In `@scripts/task_b14.ts`:
- Around line 1-9: Remove the scripts/task_b14.ts snapshot from the PR; do not
retain a duplicate Task implementation under scripts/. If historical migration
evidence is required, move it outside the compiled source tree as a patch or PR
attachment rather than maintaining this unresolvable, stale copy.
In `@src/services/stats/__tests__/UsageStatsService.spec.ts`:
- Around line 610-611: Replace the double-assertion private-member access in
src/services/stats/__tests__/UsageStatsService.spec.ts lines 610-611 with
bracket notation on cappedService, and make the same change for the store.append
monkeypatch at lines 629-638. In
src/services/stats/__tests__/UsageEventStore.spec.ts lines 337-348, access the
private capped member directly with bracket notation for both setting and
resetting it; remove the unnecessary casts.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 476-477: Replace the per-append checkTotalSize scan in
appendInternal with a tracked total-size field: increment it by the written
line’s byte length after each successful append, and rescan only when the
tracked value crosses the cap. Initialize or refresh this field during
initialize, while preserving capped-state behavior and the existing lock flow.
- Around line 155-186: Update UsageEventStore.initialize to memoize its
in-flight initialization promise: the first caller should run the existing
directory, manifest, idempotency, and cap setup, while concurrent or later
callers return the same promise instead of entering the body again. Preserve the
initialized guard and ensure the stored promise is cleared or finalized
appropriately after completion so initialization remains safe for subsequent
calls.
🪄 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: 3cb1e77e-2bd5-40dc-afe9-4425b832dca2
📒 Files selected for processing (53)
b15_task_diff.patchcherry-codecov.ps1clean-docs.ps1clean-docs2.ps1clean-docs3.ps1clean-docs4.ps1clean-docs5.ps1coverage-output.txtdocs/260804_0002_session_ci-fix-compile/013100_debug-report.mddocs/260804_0002_session_ci-fix-compile/161500_debug-report.mddocs/260804_0002_session_ci-fix-compile/161600_vp-handoff.mddocs/260804_0002_session_ci-fix-compile/180500_debug-report.mddocs/260804_0002_session_ci-fix-compile/194800_debug-report.mddocs/260804_0002_session_ci-fix-compile/205100_debug-report.mddocs/260804_0003_session_merge-conflict-resolution/033900_debug-report.mddocs/260804_0003_session_merge-conflict-resolution/113400_debug-report.mddocs/260804_pr_audit/hands-off-document.mddocs/260805_0001_session_ci-all-green/052917_debug-coverage-b13.mddocs/260805_0001_session_ci-all-green/decisions.mddocs/260805_0001_session_ci-all-green/hands-off-document.mddocs/260805_0001_session_ci-all-green/new-session-prompt.mdfix-codecov-b05.ps1fix-codecov-missing.ps1packages/types/coverage-json/coverage-final.jsonpackages/types/src/__tests__/usage-stats.spec.tspackages/types/src/index.tspackages/types/src/usage-stats.tspackages/types/src/vscode-extension-host.tsrestore-codecov.ps1scripts/create-upstream-prs.ps1scripts/merge_b15_task.pyscripts/merge_b15_task_v2.pyscripts/pr-creation-results.jsonscripts/pr-metadata.jsonscripts/squash-continue.ps1scripts/squash-final.ps1scripts/squash-push-17prs.ps1scripts/squash-results.jsonscripts/task_b14.tsscripts/task_b15.tsscripts/task_base.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.usage-stats.spec.tssrc/coverage-json/coverage-final.jsonsrc/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/index.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/services/stats/index.ts
- packages/types/src/index.ts
- packages/types/src/tests/usage-stats.spec.ts
- src/eslint-suppressions.json
- src/core/task/tests/Task.usage-stats.spec.ts
- packages/types/src/vscode-extension-host.ts
- packages/types/src/usage-stats.ts
- src/services/stats/UsageRecorder.ts
- src/core/task/Task.ts
- src/services/stats/UsageAggregator.ts
- src/services/stats/UsageStatsService.ts
🛑 Comments failed to post (4)
b15_task_diff.patch (1)
1-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove this patch file from the repository.
b15_task_diff.patchis a generatedgit diffartifact. It duplicates changes that already exist insrc/core/task/Task.ts, so the two copies can drift.The file is also UTF-16 encoded with a BOM. Every character is separated by a NUL byte. Git treats it as binary-adjacent text, and
grep,rg,git apply, and diff viewers all render it incorrectly, as visible in this review.Delete the file and add
*.patchto.gitignoreif these artifacts are produced during development.🤖 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 `@b15_task_diff.patch` around lines 1 - 4, Delete the generated b15_task_diff.patch artifact from the repository, and update .gitignore to exclude *.patch files so future development artifacts are not committed.coverage-output.txt (1)
1-2: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== repo file list candidate ==" git ls-files | rg '(^|/)(coverage-output\.txt|\.gitignore|.*coverage.*|.*test.*|.*ci|.*workflow.*|.*github.*)$' || true echo "== coverage-output.txt stats ==" stat -c '%a %n' coverage-output.txt 2>/dev/null || true wc -l coverage-output.txt 2>/dev/null || true echo "== coverage-output.txt first bytes/text ==" python3 - <<'PY' from pathlib import Path p=Path('coverage-output.txt') if p.exists(): raw=p.read_bytes() print(f"size={len(raw)}") txt=raw.decode('utf-16-le', errors='replace') lines=txt.splitlines() for i,line in enumerate(lines[:5],1): print(f"--- line {i} ---") print(repr(line[:300])) print("contains_local_path=", 'C:\\Users\\k1yt\\OneDrive\\Projects\\ZooCode\\src' in txt) PY echo "== gitignore references ==" for f in $(git ls-files | rg '(^|/)\.gitignore$|(^|/)gitignore$'); do echo "--- $f ---" rg -n 'coverage|coverage-output|\.txt$|/\*' "$f" || true done echo "== workflow references to coverage-output or upload-artifact ==" rg -n 'coverage-output|upload-artifact|coverage' . --glob '!coverage-output.txt' --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50377
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' p = 'coverage-output.txt' from pathlib import Path raw = Path(p).read_bytes() print(raw[:2000]) print('bytes_len', len(raw)) PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 5391
Information Disclosure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: Internal
Remove the checked-in coverage terminal output.
coverage-output.txtis generated UTF-16 output that contains local workspace and user paths, includingC:\Users\k1yt\OneDrive\Projects\ZooCode\src. Keep generated coverage output out of the repository; generate coverage in CI and upload it as a workflow artifact if coverage results are needed.🤖 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 `@coverage-output.txt` around lines 1 - 2, Remove the tracked generated file coverage-output.txt from the repository, and ensure future coverage terminal output is excluded from version control. Leave coverage generation and CI artifact handling to the existing workflow if coverage results are required.packages/types/coverage-json/coverage-final.json (1)
2-3: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -eu echo "== repo files around coverage artifact ==" git ls-files | rg '(^|/)packages/types/coverage-json/coverage-final.json$|(^|/)\.gitignore$|(^|/)package\.json$|(^|/)pnpm-lock.yaml$|(^|/)yarn.lock$|(^|/)coverage|coverage-json' || true echo echo "== artifact first entries if present ==" if [ -f packages/types/coverage-json/coverage-final.json ]; then head -c 2000 packages/types/coverage-json/coverage-final.json echo echo "== path count/profile ==" python3 - <<'PY' import json, os p='packages/types/coverage-json/coverage-final.json' with open(p, encoding='utf-8') as f: data=json.load(f) for key in sorted(data)[:10]: v=data[key] print(key) for k in ('path','tsconfigBaseName'): if k in v: print(' ', k, v[k]) print('total keys:', len(data)) abs_paths=[k for k in data if not (k.startswith('./') or k.startswith('.\\'))] print('absolute-looking keys:', len(abs_paths)) PY fi echo echo "== global ignores for coverage/" python3 - <<'PY' import pathlib, re for f in pathlib.Path('.').glob('**/.gitignore'): if f.is_file(): txt=f.read_text(errors='replace') if re.search(r'(^|\n)\s*coverage(-json)?(/|$)|coverage', txt): print(f) for i, line in enumerate(txt.splitlines(), 1): if re.search(r'(^|\n)\s*coverage(-json)?(/|$)|coverage', line): print(f' {i}: {line}') PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4701
🏁 Script executed:
#!/bin/bash set -eu echo "== repo files around coverage artifact ==" git ls-files | rg '(^|/)packages/types/coverage-json/coverage-final.json$|(^|/)\.gitignore$|(^|/)package\.json$|(^|/)pnpm-lock.yaml$|(^|/)yarn.lock$|(^|/)coverage|coverage-json' || true echo echo "== artifact first entries if present ==" if [ -f packages/types/coverage-json/coverage-final.json ]; then head -c 2000 packages/types/coverage-json/coverage-final.json echo echo "== path count/profile ==" python3 - <<'PY' import json, os p='packages/types/coverage-json/coverage-final.json' with open(p, encoding='utf-8') as f: data=json.load(f) for key in sorted(data)[:10]: v=data[key] print(key) for k in ('path','tsconfigBaseName'): if k in v: print(' ', k, v[k]) print('total keys:', len(data)) abs_paths=[k for k in data if not (k.startswith('./') or k.startswith('.\\') or k.startswith('/') or k.startswith('/'))] print('absolute-looking keys:', len(abs_paths)) PY fi echo echo "== global ignores for coverage/ ==" python3 - <<'PY' import pathlib, re for f in pathlib.Path('.').glob('**/.gitignore'): if f.is_file(): txt=f.read_text(errors='replace') if re.search(r'(^|\n)(\*\/)?coverage(-json)?(/|$)|coverage', txt): print(f) for i, line in enumerate(txt.splitlines(), 1): if re.search(r'(^|\n)(\*\/)?coverage(-json)?(/|$)|coverage', line): print(f' {i}: {line}') PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4704
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: Internal
Remove local workspace paths from the committed coverage metadata.
packages/types/coverage-json/coverage-final.jsonis tracked and all entries encodeC:\Users\k1yt\OneDrive\Projects\ZooCode\.... Delete this generated artifact from version control, or configure coverage output to use non-sensitive project-relative paths before committing coverage results.🤖 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/coverage-json/coverage-final.json` around lines 2 - 3, Remove the tracked generated coverage artifact coverage-final.json containing local workspace paths, or reconfigure the coverage generation so committed entries use project-relative paths instead. Ensure no sensitive absolute paths remain in coverage metadata.scripts/task_b14.ts (1)
1-9: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Three full copies of
src/core/task/Task.tswere committed underscripts/. Each file is roughly 5000 lines of production orchestration code whose relative imports (./AskIgnoredError,../../shared/package,../../api) resolve only fromsrc/core/task/. Fromscripts/they cannot compile, so all three are dead, unmaintained duplicates that will drift from the live implementation.
scripts/task_b14.ts#L1-L9: delete the file; it duplicates the liveTaskclass and already carries a stalerequestKeyform at line 3217.scripts/task_b15.ts#L629-L645: delete the file; first portresolveEndpoint(lines 207-240) intosrc/core/task/Task.tsif that endpoint logic is wanted, and fix the undocumented double assertion at line 637 and the floatingpostMessageToWebviewpromise at line 638 in the real source.scripts/task_base.ts#L1-L9: delete the file; it is byte-identical toscripts/task_b14.ts.📍 Affects 3 files
scripts/task_b14.ts#L1-L9(this comment)scripts/task_b15.ts#L629-L645scripts/task_base.ts#L1-L9🤖 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 `@scripts/task_b14.ts` around lines 1 - 9, Remove the dead duplicate files scripts/task_b14.ts and scripts/task_base.ts. Remove scripts/task_b15.ts after first porting its resolveEndpoint logic into src/core/task/Task.ts if that behavior is required; in the real Task implementation, also replace the undocumented double assertion and properly await or handle the postMessageToWebview promise. Apply these changes respectively to scripts/task_b14.ts (lines 1-9), scripts/task_b15.ts (lines 629-645), and scripts/task_base.ts (lines 1-9).Source: Coding guidelines
00cfd91 to
b1a03e3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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 `@cherry-codecov.ps1`:
- Around line 24-48: Fetch each selected remote branch immediately before
checking it out in cherry-codecov.ps1 (lines 24-48), clean-docs3.ps1 (lines
11-39), and clean-docs5.ps1 (lines 10-39), then replace the force-pushes in all
three scripts with lease-protected pushes using the freshly fetched remote
state.
In `@docs/260804_0002_session_ci-fix-compile/161600_vp-handoff.md`:
- Around line 112-119: Add the `text` language identifier to the fenced
dependency-order block surrounding the dependency list, while preserving the
existing entries and ordering.
In `@docs/260804_pr_audit/hands-off-document.md`:
- Around line 15-20: Update the Repository documentation entry for Local
workspace to remove the contributor-specific absolute filesystem path, replacing
it with the literal <repo-root> placeholder or removing that field while
preserving the surrounding repository information.
- Around line 21-41: Align the PR inventory in
docs/260804_pr_audit/hands-off-document.md with the session report identifiers,
replacing the `#22`–#38 targets with their correct `#1122`–#1136 mappings and
preserving each branch, base, feature, and commit association. In
cherry-codecov.ps1 at line 13, verify and correct the b11-mimo-capability
target’s exact remote branch and PR mapping before it is used for merging.
In `@fix-codecov-b05.ps1`:
- Around line 12-13: Update the checkout flow after `git checkout -b
"temp/pr/$branch" ...` to inspect `$LASTEXITCODE`; when checkout fails, emit the
captured command output and terminate before any branch modification or
force-push occurs. Preserve the existing successful checkout path.
In `@fix-codecov-missing.ps1`:
- Around line 13-15: Update the checkout flow after the git checkout command to
inspect $LASTEXITCODE and terminate the script immediately when creating the
temporary branch from refs/remotes/myk1yt/pr/$branch fails. Ensure subsequent
code, including codecov.yml processing and cherry-picking or force-pushing, only
runs after a successful checkout.
In `@scripts/merge_b15_task_v2.py`:
- Around line 12-17: Update both git show subprocess calls in the script to use
check=True, validate that each command returns non-empty stdout before assigning
b14_lines or b15_lines, and terminate without writing when either revision or
path is unavailable.
In `@scripts/merge_b15_task.py`:
- Around line 11-12: Update the subprocess handling before `b15_lines` is
assigned: check `result.returncode` after `subprocess.run` and, when the Git
command fails, stop execution while reporting `result.stderr`; only split and
process `result.stdout` on success so `Task.ts` is not modified with incomplete
data.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 603-610: Update the idempotency-key scan in UsageEventStore to
validate each parsed record with UsageEventV1Schema.safeParse() before adding
its idempotencyKey. Only add the key when schema validation succeeds, preserving
readAll()’s handling of malformed records and preventing incomplete JSON objects
from suppressing valid events.
- Around line 348-356: The clear flow in UsageEventStore must not continue or
write a new manifest after fs.rename fails in the segmentFiles move loop.
Propagate the failure and abort clear so callers do not observe success after a
partial move; ensure storage remains generation-scoped or uses an atomic
directory switch so readAll cannot expose unmoved old segments.
- Around line 175-180: The initialization path around
UsageEventStore.rebuildIdempotencySet must not continue recording after recovery
fails. Replace the catch-and-continue behavior with failure propagation using
the existing StatsStoreError mechanism, or mark the store as suspended so append
operations are rejected; ensure later appends cannot use an incomplete
idempotencyKeys set.
- Around line 412-420: Update the append flow in UsageEventStore to recheck the
event’s idempotencyKey against persisted state after acquireManifestLock
succeeds and before appending; return false and release the lock when the key
already exists. Add a regression test creating two UsageEventStore instances
before either append, then verify only one append succeeds for the same key.
- Around line 404-410: Update the append flow in UsageEventStore around the
this.capped check to calculate the encoded event-line size and current total
while holding the lock, then reject the append before opening or writing a
segment whenever the projected total exceeds TOTAL_MAX_BYTES. Preserve the
existing StatsStoreError behavior for capped writes and ensure the size
calculation includes the complete encoded line.
🪄 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: a26b0a41-145a-4292-a8dd-23e9a75b5d02
📒 Files selected for processing (53)
b15_task_diff.patchcherry-codecov.ps1clean-docs.ps1clean-docs2.ps1clean-docs3.ps1clean-docs4.ps1clean-docs5.ps1coverage-output.txtdocs/260804_0002_session_ci-fix-compile/013100_debug-report.mddocs/260804_0002_session_ci-fix-compile/161500_debug-report.mddocs/260804_0002_session_ci-fix-compile/161600_vp-handoff.mddocs/260804_0002_session_ci-fix-compile/180500_debug-report.mddocs/260804_0002_session_ci-fix-compile/194800_debug-report.mddocs/260804_0002_session_ci-fix-compile/205100_debug-report.mddocs/260804_0003_session_merge-conflict-resolution/033900_debug-report.mddocs/260804_0003_session_merge-conflict-resolution/113400_debug-report.mddocs/260804_pr_audit/hands-off-document.mddocs/260805_0001_session_ci-all-green/052917_debug-coverage-b13.mddocs/260805_0001_session_ci-all-green/decisions.mddocs/260805_0001_session_ci-all-green/hands-off-document.mddocs/260805_0001_session_ci-all-green/new-session-prompt.mdfix-codecov-b05.ps1fix-codecov-missing.ps1packages/types/coverage-json/coverage-final.jsonpackages/types/src/__tests__/usage-stats.spec.tspackages/types/src/index.tspackages/types/src/usage-stats.tspackages/types/src/vscode-extension-host.tsrestore-codecov.ps1scripts/create-upstream-prs.ps1scripts/merge_b15_task.pyscripts/merge_b15_task_v2.pyscripts/pr-creation-results.jsonscripts/pr-metadata.jsonscripts/squash-continue.ps1scripts/squash-final.ps1scripts/squash-push-17prs.ps1scripts/squash-results.jsonscripts/task_b14.tsscripts/task_b15.tsscripts/task_base.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.usage-stats.spec.tssrc/coverage-json/coverage-final.jsonsrc/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/index.ts
🚧 Files skipped from review as they are similar to previous changes (34)
- src/eslint-suppressions.json
- scripts/pr-creation-results.json
- scripts/create-upstream-prs.ps1
- clean-docs2.ps1
- clean-docs4.ps1
- docs/260804_0002_session_ci-fix-compile/180500_debug-report.md
- docs/260805_0001_session_ci-all-green/decisions.md
- packages/types/src/index.ts
- src/services/stats/index.ts
- packages/types/src/usage-stats.ts
- packages/types/src/tests/usage-stats.spec.ts
- scripts/pr-metadata.json
- packages/types/coverage-json/coverage-final.json
- scripts/squash-continue.ps1
- docs/260804_0002_session_ci-fix-compile/205100_debug-report.md
- scripts/squash-results.json
- packages/types/src/vscode-extension-host.ts
- clean-docs.ps1
- docs/260804_0002_session_ci-fix-compile/194800_debug-report.md
- docs/260805_0001_session_ci-all-green/052917_debug-coverage-b13.md
- docs/260804_0002_session_ci-fix-compile/013100_debug-report.md
- docs/260804_0002_session_ci-fix-compile/161500_debug-report.md
- restore-codecov.ps1
- src/core/task/Task.ts
- src/services/stats/UsageRecorder.ts
- src/services/stats/tests/UsageAggregator.spec.ts
- src/services/stats/UsageStatsService.ts
- src/services/stats/tests/UsageStatsService.spec.ts
- scripts/task_b15.ts
- src/core/task/tests/Task.usage-stats.spec.ts
- scripts/squash-push-17prs.ps1
- scripts/task_b14.ts
- src/services/stats/UsageAggregator.ts
- scripts/task_base.ts
| ``` | ||
| b01 → b02 → b03 (error contracts) | ||
| b04 → b05 → b05a → b06 → b07 (shell integration) | ||
| b05a → b12 (mimo enforcement) | ||
| b08 → b09 → b10 (task org) | ||
| b13 → b14 → b15 → b16 (usage stats) | ||
| b17 (provider cost) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the dependency-order fence.
The fence at Line 112 contains plain dependency text. Use text.
Proposed fix
-```
+```text📝 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.
| ``` | |
| b01 → b02 → b03 (error contracts) | |
| b04 → b05 → b05a → b06 → b07 (shell integration) | |
| b05a → b12 (mimo enforcement) | |
| b08 → b09 → b10 (task org) | |
| b13 → b14 → b15 → b16 (usage stats) | |
| b17 (provider cost) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 112-112: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/260804_0002_session_ci-fix-compile/161600_vp-handoff.md` around lines
112 - 119, Add the `text` language identifier to the fenced dependency-order
block surrounding the dependency list, while preserving the existing entries and
ordering.
Source: Linters/SAST tools
| - Use `--no-verify` for both commit and push (pre-commit/pre-push hooks will block) | ||
| - Use `--force` for push (branches already have squashed commits) | ||
| - The `error: failed to delete '.git/worktrees/-wt-shell-fix': Permission denied` is non-fatal | ||
| - PowerShell treats git stderr output as errors (exit code 1) but the push succeeds | ||
|
|
||
| ## Environment Issues | ||
|
|
||
| - **PowerShell stderr**: Git writes progress messages to stderr, causing PowerShell to report exit code 1 even when the command succeeds. Check the actual output for success indicators like `SHA..SHA HEAD -> branch`. | ||
| - **git worktree permission**: `error: failed to delete '.git/worktrees/-wt-shell-fix'` appears on every commit but is non-fatal. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching target =="
fd -a '161600_vp-handoff.md' . || true
echo "== target excerpt =="
target="$(fd '161600_vp-handoff.md' . | head -n1)"
if [ -n "${target:-}" ]; then
nl -ba "$target" | sed -n '118,142p'
fi
echo "== PowerShell availability =="
if command -v pwsh >/dev/null 2>&1; then
pwsh -NoProfile -Command 'Write-Output $PSVersionTable.PSVersion; Write-Output "cmd /c echo diagnostic 1>&2"; cmd /c "echo diagnostic 1>&2"; Write-Output "LASTEXITCODE=$LASTEXITCODE"; throw "error"' 2>&1 || true
else
echo "pwsh not available"
fi
echo "== generic command substitution check =="
if command -v bash >/dev/null 2>&1 && command -v pwsh >/dev/null 2>&1; then
pwsh -NoProfile -Command '$exitcode=8?6;"cmd /c echo diagnostic 1>&2",$exitcode | Out-Null; Write-Output "LASTEXITCODE=$LASTEXITCODE"' 2>&1
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target excerpt =="
target='/home/jailuser/git/docs/260804_0002_session_ci-fix-compile/161600_vp-handoff.md'
python3 - "
import sys
with open(sys.argv[1], encoding='utf-8') as f:
lines = f.readlines()
for i in range(127, 137):
line = lines[i] if i < len(lines) else ''
print(f'{i+1}: {line}', end='')
" "$target"
echo "== PowerShell availability =="
if command -v pwsh >/dev/null 2>&1; then
pwsh -Version -NoProfile -NoLogo 2>&1 | head -n 5
pwsh -NoProfile -Command 'Write-Warning "native stderr diagnostic 1>&2"; Write-Host "diagnostic 1>&2" -NoNewline; cmd /c "echo diagnostic 1>&2"; Write-Host "LASTEXITCODE=$LASTEXITCODE"' 2>&1 || true
pwsh -NoProfile -Command 'exit 8' | cat -n; pwsh -NoProfile -NoLogo -Command '$exitcode=8?6;$LASTEXITCODE' 2>&1 || true
else
echo "pwsh: command not found"
fi
echo "== repository PowerShell references around target related paths =="
rg -n "PowerShell|git stderr|LASTEXITCODE|exit code|stderr|worktrees/-wt-shell-fix" docs github workflows .git 2>/dev/null | head -n 120 || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target excerpt =="
python3 - <<'PY'
from pathlib import Path
p = Path('/home/jailuser/git/docs/260804_0002_session_ci-fix-compile/161600_vp-handoff.md')
lines = p.read_text(encoding='utf-8').splitlines()
for i in range(127, 137):
if i <= len(lines):
print(f'{i+1}: {lines[i]}')
PY
echo "== PowerShell availability =="
if command -v pwsh >/dev/null 2>&1; then
pwsh -Version -NoProfile -NoLogo 2>&1 | head -n 5 || true
echo "== native cmd stderr path =="
pwsh -NoProfile -NoLogo -Command 'cmd /c "echo diagnostic 1>&2"; Write-Output "LASTEXITCODE=$LASTEXITCODE"' 2>&1 || true
echo "== command substitution without PowerShell native stderr =="
pwsh -NoProfile -NoLogo -Command '$code=8; ($code * 6) | Out-Host; cmd /c "exit $code"; Write-Output "LASTEXITCODE=$LASTEXITCODE"' 2>&1 || true
echo "== PowerShell native stderr throw/errorrecord =="
pwsh -NoProfile -NoLogo -Command 'Write-Warning "message"; $LASTEXITCODE' 2>&1 || true
else
echo "pwsh: command not found"
fi
echo "== repository references =="
rg -n "PowerShell|git stderr|LASTEXITCODE|exit code|stderr|worktrees/-wt-shell-fix|PushAsync|push" docs .github workflows 2>/dev/null | head -n 200 || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 11901
Correct the PowerShell exit-status guidance.
PowerShell writes stderr output as warnings/errors, but stderr alone does not make native commands return exit code 1. Use the command/process actual exit code separately, such as $LASTEXITCODE for native Git, and do not tell operators to skip exit codes in favor of only checking stdout text.
🧰 Tools
🪛 LanguageTool
[style] ~129-~129: To make your writing flow more naturally, try moving the adverb ‘already’ closer to the verb ‘squashed’.
Context: ...ock) - Use --force for push (branches already have squashed commits) - The `error: failed to delete...
(PERF_TENS_ADV_PLACEMENT)
| try { | ||
| await this.rebuildIdempotencySet(manifest) | ||
| } catch (err) { | ||
| // scan 실패는 치명적이지 않음: dedupe가 느슨해질 뿐 | ||
| console.warn(`[UsageEventStore] idempotency scan failed, continuing with empty set:`, err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not append after idempotency recovery fails.
If one segment cannot be read during initialization, this code continues with an incomplete idempotencyKeys set. A later append can duplicate an existing key when filesystem access recovers.
Fail initialization or suspend recording after this failure. The caller can handle StatsStoreError without failing the task.
🤖 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 175 - 180, The
initialization path around UsageEventStore.rebuildIdempotencySet must not
continue recording after recovery fails. Replace the catch-and-continue behavior
with failure propagation using the existing StatsStoreError mechanism, or mark
the store as suspended so append operations are rejected; ensure later appends
cannot use an incomplete idempotencyKeys set.
| 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) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not complete clear() after a segment move fails.
readAll() reads every root events-*.ndjson file and does not filter by manifest generation. If fs.rename() fails here, the old segment remains in the root directory. The new manifest can still be written, and cleared events remain visible.
Use generation-scoped storage or an atomic directory switch. Do not report a successful clear after a partial move.
🤖 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 348 - 356, The clear flow
in UsageEventStore must not continue or write a new manifest after fs.rename
fails in the segmentFiles move loop. Propagate the failure and abort clear so
callers do not observe success after a partial move; ensure storage remains
generation-scoped or uses an atomic directory switch so readAll cannot expose
unmoved old segments.
| // hard cap 확인 | ||
| if (this.capped) { | ||
| throw new StatsStoreError( | ||
| "STATS_STORE/append/003", | ||
| "Storage hard cap (100 MiB) reached, new events suspended", | ||
| ) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce the total byte cap before writing the event.
this.capped reflects the total before the current append. An event that crosses TOTAL_MAX_BYTES is still written, so the documented hard cap can be exceeded by the full event size.
Calculate the encoded line size and current total inside the lock. Reject the append before opening the segment when it would exceed the cap.
🤖 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 404 - 410, Update the
append flow in UsageEventStore around the this.capped check to calculate the
encoded event-line size and current total while holding the lock, then reject
the append before opening or writing a segment whenever the projected total
exceeds TOTAL_MAX_BYTES. Preserve the existing StatsStoreError behavior for
capped writes and ensure the size calculation includes the complete encoded
line.
| // idempotency 확인 | ||
| if (this.idempotencyKeys.has(event.idempotencyKey)) { | ||
| return false | ||
| } | ||
|
|
||
| let releaseLock: (() => Promise<void>) = async () => {} | ||
|
|
||
| try { | ||
| releaseLock = await this.acquireManifestLock() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Recheck idempotency after acquiring the manifest lock.
Two UsageEventStore instances can initialize with empty sets. Instance A can append an event and release the lock. Instance B can then acquire the lock and append the same idempotencyKey, because Lines 412-415 run before the lock and no on-disk check follows.
Check the persisted key while holding the lock, or maintain a lock-protected persistent index. Add a regression test with two stores initialized before either append.
🤖 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 412 - 420, Update the
append flow in UsageEventStore to recheck the event’s idempotencyKey against
persisted state after acquireManifestLock succeeds and before appending; return
false and release the lock when the key already exists. Add a regression test
creating two UsageEventStore instances before either append, then verify only
one append succeeds for the same key.
| const lines = content.split("\n") | ||
| for (const line of lines) { | ||
| if (!line.trim()) continue | ||
| try { | ||
| const parsed = JSON.parse(line) | ||
| if (parsed && typeof parsed.idempotencyKey === "string") { | ||
| this.idempotencyKeys.add(parsed.idempotencyKey) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate scanned records before restoring idempotency keys.
A malformed but valid JSON line such as {"idempotencyKey":"key-1"} adds key-1 to the set. A later valid event with that key is then deduplicated and lost, although readAll() correctly treats the malformed line as corrupt.
Use UsageEventV1Schema.safeParse() in this scan before adding the key.
🤖 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 603 - 610, Update the
idempotency-key scan in UsageEventStore to validate each parsed record with
UsageEventV1Schema.safeParse() before adding its idempotencyKey. Only add the
key when schema validation succeeds, preserving readAll()’s handling of
malformed records and preventing incomplete JSON objects from suppressing valid
events.
80c120f to
244e074
Compare
github-actions
Bot
added
awaiting-review
2 participants
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
New Features
Tests