feature: local-usage-stats (3/4) - #1133
Open
myk1yt wants to merge 31 commits into
Open
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.
…cing - Remove UTF-8 BOM (U+FEFF) from costRecalculation.ts and costRecalculation.spec.ts - Fix qwenCodeModels pricing: qwen3-coder-plus inputPrice 0->1.0, outputPrice 0->5.0 - Fix qwenCodeModels pricing: qwen3-coder-flash inputPrice 0->0.3, outputPrice 0->1.5 Fixes invisible-chars CI check and 3 failing costRecalculation tests
…exactly-once recorder - UsageRecorder: per-task exactly-once usage event recording with endpoint domain extraction - costRecalculation: compute effective cost from token deltas and model pricing - Provider usage deltas: moonshot, openai, openai-codex, vscode-lm yield cumulative usage; Task diffs and records - Task finalization: flush pending usage events on abort/complete - ClineProvider: initialize UsageStatsService, expose getUsageStatsService, forward usageStatsChanged to webview - types: add usage-stats schemas and usageStatsChanged ExtensionMessage type
…proper types, fix run->start renames, add UsageEventStore import
The B15 usage-capture cherry-pick was authored against an older base and reverted newer upstream/base behavior in several files, causing e2e-mock subtask timeouts (7 tests) and unit-test failures. Restore clobbered base behavior while keeping B15's genuine usage/cost capture additions: - Task.ts: restore run() + _runPromise/_isHistoryTask, safeEnsureModelFetched (def + 3 call sites), abort-aware ask wait, resume_completed_task via initialStatus, and t() i18n in sayAndCreateMissingParamError. - ClineProvider.ts: scheduler gates on task.run() (completion promise) instead of fire-and-forget task.start(). This is the root cause of the subtask/resume e2e timeouts. - openai-codex.ts: restore service-tier feature alongside cost capture. - moonshot.ts, vscode-lm.ts, vscode-lm-format.ts, eslint-suppressions.json: revert to base (pure clobber, no genuine B15 content). - task-run-dispatch.spec.ts: bind run() (not start()). - openai-usage-tracking.spec.ts: assert totalCost from cost capture.
myk1yt
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
August 4, 2026 11:47
Contributor
|
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:
📝 WalkthroughWalkthroughThis change adds usage-statistics schemas, durable event storage, aggregation, exports, task recording, webview messaging, provider cost calculation, and validation and integration tests. ChangesUsage statistics
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
myk1yt
force-pushed
the
pr/b15-usage-capture-v2
branch
2 times, most recently
from
August 4, 2026 20:41
3667bc0 to
a1f9879
Compare
This was referenced Aug 5, 2026
myk1yt
force-pushed
the
pr/b15-usage-capture-v2
branch
from
August 6, 2026 20:02
8cb5125 to
0b02cbe
Compare
github-actions
Bot
removed
the
awaiting-review
Contributor
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (4)
src/services/stats/UsageEventStore.ts (2)
652-670: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
node:cryptoinstead of a hand-rolled 32-bit hash; the doc and the code disagree.The
QuarantineReportEntry.hashdoc at line 96 states "SHA-256 hash (앞 16자)". This implementation produces an 8-character 32-bit value from a djb2-style loop. The comment at lines 654-655 justifies this by dependency minimization, butcryptois a Node built-in and adds no dependency. A 32-bit space also collides often, which weakens the report when many corrupt lines are triaged.♻️ Proposed refactor to use SHA-256
+import { createHash } from "node: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") - return { segment, line, - hash: hashHex, + hash: createHash("sha256").update(content, "utf-8").digest("hex").slice(0, 16), at: new Date().toISOString(), } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/stats/UsageEventStore.ts` around lines 652 - 670, Update makeQuarantineEntry to use the Node built-in node:crypto SHA-256 implementation for content hashing, and return the first 16 hexadecimal characters to match the QuarantineReportEntry.hash contract. Remove the hand-rolled 32-bit hash logic and its dependency-minimization comments while preserving the existing report fields and timestamp behavior.
242-301: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
readAllmaterializes every segment and every event on each call.The loop reads each segment fully with
fs.readFileat line 247, splits it into a string array at line 256, and accumulates all parsed events intoevents.TOTAL_MAX_BYTESallows 100 MiB of segments, so one call can hold the file text, the split line array, and the parsed event objects at the same time. Peak memory is several times the on-disk size.If
readAllruns on each stats query, this cost repeats per query. Consider streaming lines withreadlineand applying the query filter during the scan, or caching parsed events keyed by segment size and mtime.🤖 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 242 - 301, Update readAll to avoid materializing complete segment contents and all parsed events at once: stream each segment line-by-line with readline, process JSON and UsageEventV1Schema validation incrementally, and apply the query filter during scanning where supported. Preserve quarantine handling and reporting, while keeping memory usage bounded instead of accumulating full file text and split-line arrays.src/services/stats/__tests__/UsageEventStore.spec.ts (1)
137-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a segment rotation test.
This suite covers append, dedupe, corrupt lines, crash tails, clear, and restart recovery. It does not cover segment rotation at
SEGMENT_MAX_BYTES. Rotation currently has an active defect:appendInternalincrementsmanifest.currentSegmentbut writes to the previously resolvedsegmentPath. A test at this layer would have caught it.Export
SEGMENT_MAX_BYTESor accept it as a constructor option, then assert that after crossing the threshold the manifest reportscurrentSegment === 2and thatevents-000002.ndjsoncontains the new event.As per path instructions: "For regressions, add the test at the lowest layer that would have failed; add an e2e test only when lower-level tests cannot represent the failure mode."
🤖 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 137 - 148, Add a segment-rotation regression test alongside the existing append persistence test, using the store’s lowest-level API. Expose SEGMENT_MAX_BYTES or provide a constructor override, append enough data to cross the threshold, then assert the manifest currentSegment is 2 and events-000002.ndjson contains the newly appended event.Source: Path instructions
scripts/fix_mock_cast.py (1)
1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese one-shot rewrite scripts should not be committed. Each script hardcodes a source path, mutates that file in place, and is not idempotent or re-runnable. Three of them supersede each other in sequence, which shows they record a local editing session rather than a maintained tool. They add permanent maintenance surface and mislead future readers into re-running a mutation that no longer applies. Remove them from the PR, or move them under a clearly scoped tooling directory with argument parsing, exit codes, and a README that states when each script applies.
scripts/fix_mock_cast.py#L1-L7: delete this script; it writes an invalid intermediate type thatscripts/fix_mock_cast3.pyimmediately replaces.scripts/fix_mock_cast2.py#L1-L2: delete this script; its search string never matches, so it performs no work.scripts/fix_mock_cast3.py#L1-L7: delete this script, or keep only this final step with the target path passed as an argument.scripts/fix_b15_types8.py#L1-L2: delete this script; apply the type fixes directly insrc/api/transform/__tests__/vscode-lm-format.spec.ts.scripts/insert_b04_tests.py#L40-L43: delete this script; the test insertion it performed is already committed in the spec file.scripts/resolve_b05_test_conflicts.py#L4-L7: delete this script; the merge conflict it resolved no longer exists on this 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 `@scripts/fix_mock_cast.py` around lines 1 - 7, Remove the one-shot scripts/fix_mock_cast.py (lines 1-7), scripts/fix_mock_cast2.py (lines 1-2), scripts/insert_b04_tests.py (lines 40-43), and scripts/resolve_b05_test_conflicts.py (lines 4-7); their work is obsolete or already applied. Remove scripts/fix_b15_types8.py (lines 1-2) and apply its type fixes directly in src/api/transform/__tests__/vscode-lm-format.spec.ts. Remove scripts/fix_mock_cast3.py (lines 1-7), or retain only its final transformation with the target path supplied as an argument.
🤖 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 `@docs/260801_0001_session_fork-pr-rebase-ci/002500_code-report.md`:
- Around line 68-69: Reconcile the inherited lint results in both report
sections: docs/260801_0001_session_fork-pr-rebase-ci/002500_code-report.md lines
68-69 and docs/260801_0001_session_fork-pr-rebase-ci/095600_code-report.md lines
37-38. Make the error counts and affected file lists consistent, or document the
exact lint command and base revision that explains the differing results.
- Around line 45-52: Update the verification report to explicitly state whether
TerminalLifecycle.spec.ts and CommandScheduler.spec.ts were run as part of B06
verification. Clarify the tested suite scope before claiming complete
verification, distinguishing executed tests from affected files that were not
run.
In `@docs/260801_0001_session_fork-pr-rebase-ci/093300_code-report.md`:
- Around line 34-36: Fix the Markdown-lint formatting across all listed sites:
in docs/260801_0001_session_fork-pr-rebase-ci/093300_code-report.md lines 34-36,
label the command fence as text or shell; in
docs/260801_0001_session_fork-pr-rebase-ci/094700_code-report.md lines 32-37 and
40-44, docs/260801_0001_session_fork-pr-rebase-ci/095600_code-report.md lines
34-39 and 42-47,
docs/260801_0001_session_fork-pr-rebase-ci/103000_code-report.md lines 25-30,
and docs/260801_0001_session_fork-pr-rebase-ci/111500_code-report.md lines
48-53, add blank lines before and after each table; in
docs/260801_0001_session_fork-pr-rebase-ci/095600_code-report.md lines 53-55,
label the push-output fence as text.
In `@docs/260801_0001_session_fork-pr-rebase-ci/094700_code-report.md`:
- Around line 22-29: Update the surgical-edit count in the report to match the
seven numbered changes currently listed, or regroup those changes into exactly
five numbered edits while preserving their details.
In `@docs/260801_0001_session_fork-pr-rebase-ci/101100_code-report.md`:
- Line 43: Update the reported pre-push hook result in the session report to
distinguish the workspace package count from the Turbo task count: state 14
packages and 11 tasks separately, and avoid wording that combines them into a
single count.
In `@scripts/fix_b15_types2.py`:
- Around line 10-29: Update the utility’s replacement logic to perform the
intended type fixes and persist the modified contents of vscode-lm.ts and
vscode-lm-format.spec.ts, or explicitly convert the script into a
diagnostic-only utility with an accurate name and behavior. Remove the no-op
replacement and ensure any advertised edits are actually applied and written.
In `@scripts/fix_b15_types5.py`:
- Around line 27-40: The .run() to .start() migration is too broad and changes
non-Task receivers. In scripts/fix_b15_types5.py, replace the global regex with
receiver- or syntax-aware matching that updates only intended Task calls,
including the Task target in ClineProvider.ts; in scripts/fix_b15_types6.py,
preserve runnable-helper calls such as obj.run() in task-run-dispatch.spec.ts
and do not modify them.
In `@scripts/fix_mock_cast.py`:
- Around line 3-5: Update the replacement in the script around the old and new
cast strings so the final cast uses ReturnType<typeof vi.fn> directly, rather
than vi.Mock. Ensure the generated code does not contain the invalid
intermediate vi.Mock type.
In `@scripts/fix_mock_cast2.py`:
- Around line 4-8: Fix the replacement logic in the script by correcting old_str
to match the intended vitest Mock text exactly, then track the number of matches
before replacing and fail instead of reporting success when no match is found.
Update the final message to report the actual replacement count rather than
c.count(new_str), and only write the file after a valid match is confirmed.
In `@scripts/resolve_b05_conflicts.py`:
- Around line 123-127: Fix the Ruff E741 error in the conflict-marker handling
by renaming the ambiguous `l` comprehension variable and loop variable to `line`
in both expressions, while preserving the existing filtering and warning output.
In `@src/services/stats/__tests__/UsageAggregator.spec.ts`:
- Around line 640-658: Strengthen the assertions in the “should group events by
ISO week bucket” test around aggregator.query so the returned bucket keys are
exactly 2026-W29 and 2026-W30, with the expected event grouping/counts for each
week. Replace the format-only weekKeys check while preserving the existing
ISO-week scenario and ordering-independent validation.
- Around line 409-423: Update the “should filter events by preset 'today'” test
to use a fixed fake clock for both event timestamp creation and aggregator.query
execution, preventing timezone-boundary flakiness. Ensure real timers are
restored in a finally block after the assertions.
In `@src/services/stats/__tests__/UsageEventStore.spec.ts`:
- Around line 276-280: Update the cap-reached test around UsageEventStore so it
actually reaches the configured cap, using an injectable or exported threshold,
then invokes the append path and asserts that it throws StatsStoreError with the
expected STATS_STORE/append/003 code. Remove the placeholder isCapped()
assertion and ensure both the true cap state and documented error behavior are
covered.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 309-322: The clear method currently acquires the manifest lock
outside the process queue, allowing same-process append and clear operations to
contend. Extract append’s this.queue wrapper into a reusable helper, then route
clear through that helper while preserving its existing initialization, lock
handling, and StatsStoreError behavior.
- Around line 155-186: Make UsageEventStore.initialize concurrency-safe by
memoizing its in-flight initialization promise. Ensure concurrent callers reuse
and await the same promise, while preserving the existing initialized fast path
and setting initialization state only after the full setup completes; clear the
memoized promise on failure so later calls can retry.
---
Nitpick comments:
In `@scripts/fix_mock_cast.py`:
- Around line 1-7: Remove the one-shot scripts/fix_mock_cast.py (lines 1-7),
scripts/fix_mock_cast2.py (lines 1-2), scripts/insert_b04_tests.py (lines
40-43), and scripts/resolve_b05_test_conflicts.py (lines 4-7); their work is
obsolete or already applied. Remove scripts/fix_b15_types8.py (lines 1-2) and
apply its type fixes directly in
src/api/transform/__tests__/vscode-lm-format.spec.ts. Remove
scripts/fix_mock_cast3.py (lines 1-7), or retain only its final transformation
with the target path supplied as an argument.
In `@src/services/stats/__tests__/UsageEventStore.spec.ts`:
- Around line 137-148: Add a segment-rotation regression test alongside the
existing append persistence test, using the store’s lowest-level API. Expose
SEGMENT_MAX_BYTES or provide a constructor override, append enough data to cross
the threshold, then assert the manifest currentSegment is 2 and
events-000002.ndjson contains the newly appended event.
In `@src/services/stats/UsageEventStore.ts`:
- Around line 652-670: Update makeQuarantineEntry to use the Node built-in
node:crypto SHA-256 implementation for content hashing, and return the first 16
hexadecimal characters to match the QuarantineReportEntry.hash contract. Remove
the hand-rolled 32-bit hash logic and its dependency-minimization comments while
preserving the existing report fields and timestamp behavior.
- Around line 242-301: Update readAll to avoid materializing complete segment
contents and all parsed events at once: stream each segment line-by-line with
readline, process JSON and UsageEventV1Schema validation incrementally, and
apply the query filter during scanning where supported. Preserve quarantine
handling and reporting, while keeping memory usage bounded instead of
accumulating full file text and split-line arrays.
🪄 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: df7ba832-59ed-48ba-b63e-01a6283f039d
📒 Files selected for processing (49)
docs/260801_0001_session_fork-pr-rebase-ci/002500_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/093300_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/094700_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/095600_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/101100_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/103000_code-report.mddocs/260801_0001_session_fork-pr-rebase-ci/111500_code-report.mdpackages/types/src/__tests__/usage-stats.spec.tspackages/types/src/index.tspackages/types/src/providers/qwen-code.tspackages/types/src/usage-stats.tspackages/types/src/vscode-extension-host.tsprogress.txtscripts/fix_any.pyscripts/fix_b15_types.pyscripts/fix_b15_types2.pyscripts/fix_b15_types3.pyscripts/fix_b15_types4.pyscripts/fix_b15_types5.pyscripts/fix_b15_types6.pyscripts/fix_b15_types7.pyscripts/fix_b15_types8.pyscripts/fix_mock_cast.pyscripts/fix_mock_cast2.pyscripts/fix_mock_cast3.pyscripts/insert_b04_tests.pyscripts/resolve_b05_conflicts.pyscripts/resolve_b05_test_conflicts.pysrc/__tests__/task-run-dispatch.spec.tssrc/api/providers/__tests__/moonshot.spec.tssrc/api/providers/__tests__/openai-usage-tracking.spec.tssrc/api/providers/openai-codex.tssrc/api/providers/openai.tssrc/api/transform/__tests__/vscode-lm-format.spec.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.dispose.test.tssrc/core/task/__tests__/Task.usage-stats.spec.tssrc/core/webview/ClineProvider.tssrc/services/stats/UsageAggregator.tssrc/services/stats/UsageEventStore.tssrc/services/stats/UsageRecorder.tssrc/services/stats/UsageStatsService.tssrc/services/stats/__tests__/UsageAggregator.spec.tssrc/services/stats/__tests__/UsageEventStore.spec.tssrc/services/stats/__tests__/UsageStatsService.spec.tssrc/services/stats/__tests__/costRecalculation.spec.tssrc/services/stats/costRecalculation.tssrc/services/stats/index.tssrc/shared/globalFileNames.ts
💤 Files with no reviewable changes (1)
- progress.txt
🚧 Files skipped from review as they are similar to previous changes (23)
- src/shared/globalFileNames.ts
- packages/types/src/index.ts
- packages/types/src/tests/usage-stats.spec.ts
- packages/types/src/providers/qwen-code.ts
- src/tests/task-run-dispatch.spec.ts
- src/api/providers/openai-codex.ts
- src/core/task/tests/Task.dispose.test.ts
- src/core/webview/ClineProvider.ts
- src/api/providers/tests/openai-usage-tracking.spec.ts
- src/services/stats/UsageRecorder.ts
- src/core/task/tests/Task.usage-stats.spec.ts
- src/services/stats/tests/costRecalculation.spec.ts
- packages/types/src/usage-stats.ts
- src/api/providers/tests/moonshot.spec.ts
- src/services/stats/costRecalculation.ts
- src/api/providers/openai.ts
- src/core/task/Task.ts
- src/services/stats/tests/UsageStatsService.spec.ts
- packages/types/src/vscode-extension-host.ts
- src/services/stats/UsageStatsService.ts
- src/services/stats/index.ts
- src/api/transform/tests/vscode-lm-format.spec.ts
- src/services/stats/UsageAggregator.ts
Comment on lines
+45
to
+52
| **B05 test suite (4 files, 205 tests):** | ||
| - `ShellResolver.spec.ts` — all passed | ||
| - `ShellInvocationAdapter.spec.ts` — all passed | ||
| - `TerminalProfile.spec.ts` — all passed | ||
| - `shell.spec.ts` — all passed | ||
|
|
||
| **Merge verification test (1 file, 40 tests):** | ||
| - `executeCommandTool.spec.ts` — all passed (both B04's command_output ask policy tests AND B05's cwd parameter validation tests) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the tested suite scope.
The verification section lists four B05 suites. The affected-file list also includes TerminalLifecycle.spec.ts and CommandScheduler.spec.ts, but the report does not state whether those B06 tests ran. State the tested scope explicitly before claiming complete verification.
🤖 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/260801_0001_session_fork-pr-rebase-ci/002500_code-report.md` around
lines 45 - 52, Update the verification report to explicitly state whether
TerminalLifecycle.spec.ts and CommandScheduler.spec.ts were run as part of B06
verification. Clarify the tested suite scope before claiming complete
verification, distinguishing executed tests from affected files that were not
run.
Comment on lines
+22
to
+29
| - **Edited** [`ClineProvider.ts`](src/core/webview/ClineProvider.ts:1): 5 surgical edits: | ||
| 1. Added `TaskOrganizationStore` import from `../task-persistence` | ||
| 2. Added `TaskOrganizationStateV1` + `createEmptyTaskOrganizationState` imports from `@roo-code/types` | ||
| 3. Added `taskOrganizationStore` field + `taskOrganizationStoreInitialized` flag | ||
| 4. Constructor: initialized store with `taskHistory` ref + `onChange` callback posting `taskOrganizationUpdated` to webview; added reconcile call in `TaskHistoryStore.onWrite` | ||
| 5. Added `getTaskOrganizationStore()` getter method | ||
| 6. Updated `getStateToPostToWebview()` to await store init and include `taskOrganization` state | ||
| 7. Added `taskOrganizationStore.dispose()` in provider dispose |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the surgical-edit count.
The report says there were 5 edits but lists 7 numbered edits. Change the count to 7, or regroup the list into 5 actual edits.
🤖 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/260801_0001_session_fork-pr-rebase-ci/094700_code-report.md` around
lines 22 - 29, Update the surgical-edit count in the report to match the seven
numbered changes currently listed, or regroup those changes into exactly five
numbered edits while preserving their details.
| - Total: **112 tests passed** | ||
|
|
||
| ### 6. Push to Fork | ||
| Pushed `pr/b05a-strict-reasoning-v2` to `myk1yt` remote. The pre-push hook ran `turbo check-types` across all 14 packages (11 successful, 11 total). GitHub provided PR creation URL: |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate package and task counts.
The sentence reports 14 packages but 11 total tasks. If 14 is the workspace package count and 11 is the Turbo task count, label them separately. Do not present them as one count.
🤖 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/260801_0001_session_fork-pr-rebase-ci/101100_code-report.md` at line 43,
Update the reported pre-push hook result in the session report to distinguish
the workspace package count from the Turbo task count: state 14 packages and 11
tasks separately, and avoid wording that combines them into a single count.
Comment on lines
+409
to
+423
| it("should filter events by preset 'today'", () => { | ||
| const now = new Date() | ||
| const todayIso = now.toISOString() | ||
| const pastDate = new Date(now.getTime() - 7 * 24 * 60 * 60 * 1000).toISOString() | ||
|
|
||
| const events = [ | ||
| makeEvent({ eventId: "evt-1", idempotencyKey: "idem-1", occurredAt: todayIso }), | ||
| makeEvent({ eventId: "evt-2", idempotencyKey: "idem-2", occurredAt: pastDate }), | ||
| ] | ||
| const query = makeQuery({ preset: "today", groupBy: [] }) | ||
|
|
||
| const result = aggregator.query(events, query) | ||
|
|
||
| expect(result.totals.events).toBe(1) | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'UsageAggregator|UsageAggregator\.spec\.ts|stats' . | sed 's#^\./##' | head -100
echo "== outline test == "
ast-grep outline src/services/stats/__tests__/UsageAggregator.spec.ts --view compact || true
echo "== relevant lines 380-435 =="
sed -n '380,435p' src/services/stats/__tests__/UsageAggregator.spec.ts
echo "== find UsageAggregator implementation =="
rg -n "resolveTimeRange|class UsageAggregator|query\\(" src/services/stats -S
echo "== usage aggregator source candidates =="
fd -a . src/services/stats | sed 's#^\./##'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 9202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== UsageAggregator query and resolveTimeRange =="
sed -n '65,175p' src/services/stats/UsageAggregator.ts
echo "== imports and helpers in spec =="
sed -n '1,120p' src/services/stats/__tests__/UsageAggregator.spec.ts
echo "== package test scripts and vitest availability =="
for f in package.json src/package.json; do
if [ -f "$f" ]; then
echo "-- $f --"
jq '.scripts // {}' "$f"
fi
done
node - <<'JS'
const fs = require('fs')
const src = fs.readFileSync('src/services/stats/UsageAggregator.ts','utf8')
for (const needle of ['new Date()', 'setStartOfMonth', 'setISODay']) {
console.log(needle, src.includes(needle), src.split('\n').filter((line, i) => line.includes(needle)).map((line, i) => i+1+': '+line.trim()).join('\n'))
}
const spec = fs.readFileSync('src/services/stats/__tests__/UsageAggregator.spec.ts','utf8')
for (const needle of ['vi', 'useFakeTimers', 'setSystemTime', 'Date.now']) {
console.log(needle, spec.includes(needle))
}
JS
echo "== deterministic probe for range construction across UTC day boundary =="
node - <<'JS'
const startOfUTCday = d => new Date(Date.UTC(d.getUTCFullYear(), d.getUTCMonth(), d.getUTCDate()))
const startOfUTCweek = d => {
const day = (d.getUTCDay() + 6) % 7
return new Date(Date.UTC(d.getUTCFullYear(), d.getUTCMonth(), d.getUTCDate() - day))
}
const presets = {
today: { from: startOfUTCday, to: undefined },
'7d': { from: d => new Date(d.getTime() - 7*24*60*60*1000), to: undefined }
}
const cases = [
{ name: '2024-12-31 23:59:59.999Z', now: Date.UTC(2024,11,31,23,59,59,999), evtUTC: Date.UTC(2024,11,31,23,59,59,999) },
{ name: '2025-01-01 00:00:00.000Z', now: Date.UTC(2025,0,1,0,0,0,0), evtUTC: Date.UTC(2025,0,1,0,0,0,0) },
{ name: '2025-01-01 00:00:00.000Z with evt one ms earlier', now: Date.UTC(2025,0,1,0,0,0,0), evtUTC: Date.UTC(2024,11,31,23,59,59,999) },
{ name: '2025-01-05 01:00:00.000Z', now: Date.UTC(2025,0,5,1,0,0,0), evtUTC: Date.UTC(2025,0,1,0,0,0,0) },
{ name: '2025-01-05 01:00:00.000Z', now: Date.UTC(2025,0,5,1,0,0,0), evtUTC: Date.UTC(2024,11,29,0,0,0,0) },
{ name: '2025-01-05 01:00:00.000Z', now: Date.UTC(2025,0,5,1,0,0,0), evtUTC: Date.UTC(2024,11,28,23,59,59,999) }
]
for (const { name, now, evtUTC } of cases) {
for (const [name2, range] of Object.entries(presets)) {
const out = range.from(evtUTC) <= now
console.log(name2, name, 'event-epoch <= now-epoch:', evtUTC, now, out)
}
}
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 10716
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== remaining spec imports and relevant tests =="
sed -n '1,42p' src/services/stats/__tests__/UsageAggregator.spec.ts
sed -n '640,710p' src/services/stats/__tests__/UsageAggregator.spec.ts
echo "== deterministic probe: Asia/Seoul day boundaries around UTC midnight =="
node - <<'JS'
function startOfDay(tzDate, tz) {
const d = new Date(tzDate)
d.setHours(0,0,0,0)
return d
}
function inTodayPresets(now) {
const tzNow = new Date(now.toLocaleString("en-US", {timeZone: "Asia/Seoul"}))
const from = startOfDay(tzNow, "Asia/Seoul")
const to = new Date(from)
to.setDate(to.getDate() + 1)
return {from, to}
}
const cases = [
["2024-12-31T23:59:59.999Z"],
["2025-01-01T00:00:00.000Z"],
["2025-01-01T00:00:00.001Z"],
["2025-01-01T14:59:59.999Z"],
["2025-01-01T15:00:00.000Z"],
["2025-07-07T15:59:59.999Z"],
["2025-07-07T16:00:00.000Z"],
]
for (const [iso] of cases) {
const now = new Date(iso)
const eventOneSecondEarlier = new Date(now.getTime() - 1).toISOString()
const range = inTodayPresets(now)
console.log(JSON.stringify({ iso, event: eventOneSecondEarlier, from: range.from.toISOString(), to: range.to.toISOString(), inside: range.from.getTime() <= new Date(eventOneSecondEarlier).getTime() && new Date(eventOneSecondEarlier).getTime() < range.to.getTime() }))
}
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 5391
Make the today preset test deterministic.
todayIso is created with the real system clock before UsageAggregator.query() computes its own new Date() range in Asia/Seoul. If the timezone crosses a day boundary between these calls, evt-1 falls outside the selected day and the test flakes. Use fake time for event creation and aggregation, and restore real timers in finally.
🤖 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 409 - 423,
Update the “should filter events by preset 'today'” test to use a fixed fake
clock for both event timestamp creation and aggregator.query execution,
preventing timezone-boundary flakiness. Ensure real timers are restored in a
finally block after the assertions.
Source: Coding guidelines
Comment on lines
+640
to
+658
| it("should group events by ISO week bucket", () => { | ||
| const events = [ | ||
| makeEvent({ eventId: "evt-1", idempotencyKey: "idem-1", occurredAt: "2026-07-13T10:00:00.000Z" }), | ||
| makeEvent({ eventId: "evt-2", idempotencyKey: "idem-2", occurredAt: "2026-07-15T10:00:00.000Z" }), | ||
| makeEvent({ eventId: "evt-3", idempotencyKey: "idem-3", occurredAt: "2026-07-20T10:00:00.000Z" }), | ||
| ] | ||
| const query = makeQuery({ groupBy: ["week"] }) | ||
|
|
||
| const result = aggregator.query(events, query) | ||
|
|
||
| // 2026-07-13 KST = 2026-07-13 19:00 → ISO week 28 | ||
| // 2026-07-15 KST = 2026-07-15 19:00 → ISO week 29 | ||
| // 2026-07-20 KST = 2026-07-20 19:00 → ISO week 29 | ||
| expect(result.buckets.length).toBeGreaterThanOrEqual(1) | ||
| const weekKeys = result.buckets.map((b) => b.key.week) | ||
| weekKeys.forEach((key) => { | ||
| expect(key).toMatch(/^\d{4}-W\d{2}$/) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the ISO-week buckets.
July 13 and July 15, 2026 are both 2026-W29. July 20, 2026 is 2026-W30. The current format-only assertion passes if all events merge into one bucket or if the aggregator returns incorrect week values.
Proposed fix
- expect(result.buckets.length).toBeGreaterThanOrEqual(1)
- const weekKeys = result.buckets.map((b) => b.key.week)
- weekKeys.forEach((key) => {
- expect(key).toMatch(/^\d{4}-W\d{2}$/)
- })
+ expect(result.buckets).toHaveLength(2)
+ expect(result.buckets.map((b) => b.key.week).sort()).toEqual(["2026-W29", "2026-W30"])📝 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.
Suggested change
| it("should group events by ISO week bucket", () => { | |
| const events = [ | |
| makeEvent({ eventId: "evt-1", idempotencyKey: "idem-1", occurredAt: "2026-07-13T10:00:00.000Z" }), | |
| makeEvent({ eventId: "evt-2", idempotencyKey: "idem-2", occurredAt: "2026-07-15T10:00:00.000Z" }), | |
| makeEvent({ eventId: "evt-3", idempotencyKey: "idem-3", occurredAt: "2026-07-20T10:00:00.000Z" }), | |
| ] | |
| const query = makeQuery({ groupBy: ["week"] }) | |
| const result = aggregator.query(events, query) | |
| // 2026-07-13 KST = 2026-07-13 19:00 → ISO week 28 | |
| // 2026-07-15 KST = 2026-07-15 19:00 → ISO week 29 | |
| // 2026-07-20 KST = 2026-07-20 19:00 → ISO week 29 | |
| expect(result.buckets.length).toBeGreaterThanOrEqual(1) | |
| const weekKeys = result.buckets.map((b) => b.key.week) | |
| weekKeys.forEach((key) => { | |
| expect(key).toMatch(/^\d{4}-W\d{2}$/) | |
| }) | |
| }) | |
| it("should group events by ISO week bucket", () => { | |
| const events = [ | |
| makeEvent({ eventId: "evt-1", idempotencyKey: "idem-1", occurredAt: "2026-07-13T10:00:00.000Z" }), | |
| makeEvent({ eventId: "evt-2", idempotencyKey: "idem-2", occurredAt: "2026-07-15T10:00:00.000Z" }), | |
| makeEvent({ eventId: "evt-3", idempotencyKey: "idem-3", occurredAt: "2026-07-20T10:00:00.000Z" }), | |
| ] | |
| const query = makeQuery({ groupBy: ["week"] }) | |
| const result = aggregator.query(events, query) | |
| // 2026-07-13 KST = 2026-07-13 19:00 → ISO week 28 | |
| // 2026-07-15 KST = 2026-07-15 19:00 → ISO week 29 | |
| // 2026-07-20 KST = 2026-07-20 19:00 → ISO week 29 | |
| expect(result.buckets).toHaveLength(2) | |
| expect(result.buckets.map((b) => b.key.week).sort()).toEqual(["2026-W29", "2026-W30"]) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/stats/__tests__/UsageAggregator.spec.ts` around lines 640 - 658,
Strengthen the assertions in the “should group events by ISO week bucket” test
around aggregator.query so the returned bucket keys are exactly 2026-W29 and
2026-W30, with the expected event grouping/counts for each week. Replace the
format-only weekKeys check while preserving the existing ISO-week scenario and
ordering-independent validation.
Comment on lines
+276
to
+280
| describe("error handling", () => { | ||
| it("should throw StatsStoreError with correct code on cap reached", async () => { | ||
| // 이 테스트는 cap을 강제로 설정하기 어려우므로, isCapped() 메서드 동작만 확인 | ||
| expect(store.isCapped()).toBe(false) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test does not verify what its name states.
The name says "should throw StatsStoreError with correct code on cap reached". The body only asserts isCapped() === false on a fresh store. It never reaches the cap and never asserts a throw. StatsStoreError, imported at line 9, is not used anywhere in this file. The STATS_STORE/append/003 path therefore has no coverage while appearing covered.
Make the cap injectable, or export the threshold so a test can force the state, then assert the thrown code.
💚 Proposed fix to assert the documented behavior
- it("should throw StatsStoreError with correct code on cap reached", async () => {
- // 이 테스트는 cap을 강제로 설정하기 어려우므로, isCapped() 메서드 동작만 확인
- expect(store.isCapped()).toBe(false)
- })
+ it("should report not capped for a fresh store", () => {
+ expect(store.isCapped()).toBe(false)
+ })
+
+ it("should throw STATS_STORE/append/003 once the hard cap is reached", async () => {
+ // `capped` is private; force the state to cover the guard in appendInternal.
+ ;(store as unknown as { capped: boolean }).capped = true
+
+ await expect(store.append(makeEvent())).rejects.toBeInstanceOf(StatsStoreError)
+ await expect(store.append(makeEvent())).rejects.toMatchObject({
+ code: "STATS_STORE/append/003",
+ })
+ })As per path instructions: "Add focused tests for UI binding and save behavior, persistence or normalization, and the value returned by getStateToPostToWebview(); cover both true and false/unset defaulting cases."
📝 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.
Suggested change
| describe("error handling", () => { | |
| it("should throw StatsStoreError with correct code on cap reached", async () => { | |
| // 이 테스트는 cap을 강제로 설정하기 어려우므로, isCapped() 메서드 동작만 확인 | |
| expect(store.isCapped()).toBe(false) | |
| }) | |
| describe("error handling", () => { | |
| it("should report not capped for a fresh store", () => { | |
| expect(store.isCapped()).toBe(false) | |
| }) | |
| it("should throw STATS_STORE/append/003 once the hard cap is reached", async () => { | |
| // `capped` is private; force the state to cover the guard in appendInternal. | |
| ;(store as unknown as { capped: boolean }).capped = true | |
| await expect(store.append(makeEvent())).rejects.toBeInstanceOf(StatsStoreError) | |
| await expect(store.append(makeEvent())).rejects.toMatchObject({ | |
| code: "STATS_STORE/append/003", | |
| }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/stats/__tests__/UsageEventStore.spec.ts` around lines 276 - 280,
Update the cap-reached test around UsageEventStore so it actually reaches the
configured cap, using an injectable or exported threshold, then invokes the
append path and asserts that it throws StatsStoreError with the expected
STATS_STORE/append/003 code. Remove the placeholder isCapped() assertion and
ensure both the true cap state and documented error behavior are covered.
Source: Path instructions
Comment on lines
+155
to
+186
| async initialize(): Promise<void> { | ||
| if (this.initialized) { | ||
| return | ||
| } | ||
|
|
||
| try { | ||
| await fs.mkdir(this.statsDir, { recursive: true }) | ||
| await fs.mkdir(this.quarantineDir, { recursive: true }) | ||
| } catch (err) { | ||
| throw new StatsStoreError( | ||
| "STATS_STORE/append/001", | ||
| `Failed to create stats directory: ${this.statsDir}`, | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| // manifest 로드 또는 생성 | ||
| const manifest = await this.loadOrCreateManifest() | ||
|
|
||
| // idempotency set 복원: 현재 generation의 모든 segment에서 scan | ||
| try { | ||
| await this.rebuildIdempotencySet(manifest) | ||
| } catch (err) { | ||
| // scan 실패는 치명적이지 않음: dedupe가 느슨해질 뿐 | ||
| console.warn(`[UsageEventStore] idempotency scan failed, continuing with empty set:`, err) | ||
| } | ||
|
|
||
| // hard cap 확인 | ||
| this.capped = await this.checkTotalSize() | ||
|
|
||
| this.initialized = true | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
initialize is not concurrency-safe; overlapping calls can drop idempotency keys.
this.initialized is set at line 185, after the awaits at lines 161-183. ensureInitialized is called from readAll, clear, getManifest, and appendInternal, and only the append path is serialized by the queue. If two of these run before the first initialize resolves, both observe initialized === false and both execute initialize.
A second rebuildIdempotencySet then clears the set at line 584 and rescans. Any key added by a concurrent append at line 474 during that window is lost, so a later duplicate event is accepted as new.
Memoize the in-flight promise so all callers await the same initialization.
🛡️ Proposed fix to memoize initialization
/** 초기화 완료 여부 */
private initialized = false
+
+ /** 진행 중인 초기화 promise */
+ private initPromise: Promise<void> | undefined async initialize(): Promise<void> {
if (this.initialized) {
return
}
+ if (this.initPromise) {
+ return this.initPromise
+ }
+ this.initPromise = this.initializeInternal().finally(() => {
+ this.initPromise = undefined
+ })
+ return this.initPromise
+ }
+ private async initializeInternal(): Promise<void> {
try {
await fs.mkdir(this.statsDir, { recursive: true })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async initialize(): Promise<void> { | |
| if (this.initialized) { | |
| return | |
| } | |
| try { | |
| await fs.mkdir(this.statsDir, { recursive: true }) | |
| await fs.mkdir(this.quarantineDir, { recursive: true }) | |
| } catch (err) { | |
| throw new StatsStoreError( | |
| "STATS_STORE/append/001", | |
| `Failed to create stats directory: ${this.statsDir}`, | |
| err, | |
| ) | |
| } | |
| // manifest 로드 또는 생성 | |
| const manifest = await this.loadOrCreateManifest() | |
| // idempotency set 복원: 현재 generation의 모든 segment에서 scan | |
| try { | |
| await this.rebuildIdempotencySet(manifest) | |
| } catch (err) { | |
| // scan 실패는 치명적이지 않음: dedupe가 느슨해질 뿐 | |
| console.warn(`[UsageEventStore] idempotency scan failed, continuing with empty set:`, err) | |
| } | |
| // hard cap 확인 | |
| this.capped = await this.checkTotalSize() | |
| this.initialized = true | |
| } | |
| /** 초기화 완료 여부 */ | |
| private initialized = false | |
| /** 진행 중인 초기화 promise */ | |
| private initPromise: Promise<void> | undefined | |
| async initialize(): Promise<void> { | |
| if (this.initialized) { | |
| return | |
| } | |
| if (this.initPromise) { | |
| return this.initPromise | |
| } | |
| this.initPromise = this.initializeInternal().finally(() => { | |
| this.initPromise = undefined | |
| }) | |
| return this.initPromise | |
| } | |
| private async initializeInternal(): Promise<void> { | |
| try { | |
| await fs.mkdir(this.statsDir, { recursive: true }) | |
| await fs.mkdir(this.quarantineDir, { recursive: true }) | |
| } catch (err) { | |
| throw new StatsStoreError( | |
| "STATS_STORE/append/001", | |
| `Failed to create stats directory: ${this.statsDir}`, | |
| err, | |
| ) | |
| } | |
| // manifest 로드 또는 생성 | |
| const manifest = await this.loadOrCreateManifest() | |
| // idempotency set 복원: 현재 generation의 모든 segment에서 scan | |
| try { | |
| await this.rebuildIdempotencySet(manifest) | |
| } catch (err) { | |
| // scan 실패는 치명적이지 않음: dedupe가 느슨해질 뿐 | |
| console.warn(`[UsageEventStore] idempotency scan failed, continuing with empty set:`, err) | |
| } | |
| // hard cap 확인 | |
| this.capped = await this.checkTotalSize() | |
| this.initialized = true | |
| } |
🤖 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, Make
UsageEventStore.initialize concurrency-safe by memoizing its in-flight
initialization promise. Ensure concurrent callers reuse and await the same
promise, while preserving the existing initialized fast path and setting
initialization state only after the full setup completes; clear the memoized
promise on failure so later calls can retry.
Comment on lines
+309
to
+322
| async clear(): Promise<void> { | ||
| await this.ensureInitialized() | ||
|
|
||
| let releaseLock: (() => Promise<void>) = async () => {} | ||
|
|
||
| try { | ||
| releaseLock = await this.acquireManifestLock() | ||
| } catch (err) { | ||
| throw new StatsStoreError( | ||
| "STATS_STORE/clear/001", | ||
| "Failed to acquire manifest lock for clear", | ||
| err, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how clear() and append() are invoked, and whether callers can overlap them.
set -euo pipefail
fd -e ts . src/services/stats --exclude '__tests__' --exec rg -n -C4 '\.clear\(\)|\.append\(|this\.queue'
rg -n -C4 'usageStatsService|clearUsageStats|\.clear\(\)' src/core --type=tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 1968
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Outline UsageEventStore.ts =="
ast-grep outline src/services/stats/UsageEventStore.ts --view expanded | sed -n '1,220p'
echo "== Relevant UsageEventStore.ts sections =="
sed -n '120,230p' src/services/stats/UsageEventStore.ts
sed -n '250,385p' src/services/stats/UsageEventStore.ts
echo "== Storage service consumers around append/clear =="
rg -n -C5 'UsageStatsService|clearUsageStats|restoreUsageEventsFromHistory|\.store\.clear\(\)|\.store\.append\(' src/services src/core --type=ts | sed -n '1,260p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 23997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== acquireManifestLock implementation =="
sed -n '553,578p' src/services/stats/UsageEventStore.ts
echo "== lockfile imports and retry configuration =="
rg -n -C3 'proper-lockfile|lock.*wait|sleep|sleepFor|acquireManifestLock|releaseLock' src/services/stats/UsageEventStore.ts
echo "== append/internal write file operations =="
sed -n '384,525p' src/services/stats/UsageEventStore.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 6433
Route clear through the process queue to avoid self-contention on the manifest lock.
append serializes through this.queue, but clear acquires manifest.json.lock independently. Same-process append and clear can contend, causing STATS_STORE/append/002 or STATS_STORE/clear/001 after lock retries even though the operations do not conflict across processes. Run clear through the same this.queue; extract the queue wrapper from append and reuse 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 - 322, The clear
method currently acquires the manifest lock outside the process queue, allowing
same-process append and clear operations to contend. Extract append’s this.queue
wrapper into a reusable helper, then route clear through that helper while
preserving its existing initialization, lock handling, and StatsStoreError
behavior.
myk1yt
force-pushed
the
pr/b15-usage-capture-v2
branch
from
August 7, 2026 09:19
0e51311 to
0b02cbe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/vscode-e2e/src/suite/usage-capture.test.ts (1)
38-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove detailed usage-record validation to lower-layer tests.
Direct NDJSON parsing and assertions on schema fields, token sources, and idempotency are storage and protocol tests. Keep this E2E suite as a task-to-capture smoke test. Move the detailed assertions to package-local unit or integration tests.
As per coding guidelines, “Keep E2E tests focused on high-value cross-boundary smoke coverage; do not place detailed protocol, parsing, storage, retry, or edge-case assertions there when lower-layer tests can cover them.”
Also applies to: 158-178, 209-217
🤖 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 `@apps/vscode-e2e/src/suite/usage-capture.test.ts` around lines 38 - 70, Reduce the usage-capture E2E coverage to a task-to-capture smoke test: remove direct NDJSON parsing and detailed schema, token-source, and idempotency assertions from readAllUsageEvents and the referenced tests. Move those protocol and storage validations into package-local unit or integration tests, while retaining only the cross-boundary assertion that usage capture occurs.Source: Coding guidelines
🤖 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 `@apps/vscode-e2e/src/suite/usage-capture.test.ts`:
- Around line 209-217: Update the idempotency assertion in the usage-capture
test to filter the events returned by readAllUsageEvents to records belonging to
the current taskId before extracting idempotencyKey values. Keep the uniqueness
assertion unchanged for that task-scoped subset.
---
Nitpick comments:
In `@apps/vscode-e2e/src/suite/usage-capture.test.ts`:
- Around line 38-70: Reduce the usage-capture E2E coverage to a task-to-capture
smoke test: remove direct NDJSON parsing and detailed schema, token-source, and
idempotency assertions from readAllUsageEvents and the referenced tests. Move
those protocol and storage validations into package-local unit or integration
tests, while retaining only the cross-boundary assertion that usage capture
occurs.
🪄 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: eecc0b7b-d860-4ef6-b852-1dad9f0d4d93
📒 Files selected for processing (1)
apps/vscode-e2e/src/suite/usage-capture.test.ts
Comment on lines
+209
to
+217
| // Idempotency: no two events anywhere in the store may share an | ||
| // idempotencyKey — the recorder dedupes on requestKey:status. | ||
| const all = await readAllUsageEvents(statsDir) | ||
| const keys = all.map((e) => e.idempotencyKey) | ||
| assert.strictEqual( | ||
| new Set(keys).size, | ||
| keys.length, | ||
| "Idempotency keys must be unique (no double-recorded usage events)", | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the idempotency assertion to this test's records.
The store persists across test runs, but this assertion checks every historical event. A duplicate from an earlier run can fail this test without involving taskId. Filter to records created for this task before validating idempotency.
🤖 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 `@apps/vscode-e2e/src/suite/usage-capture.test.ts` around lines 209 - 217,
Update the idempotency assertion in the usage-capture test to filter the events
returned by readAllUsageEvents to records belonging to the current taskId before
extracting idempotencyKey values. Keep the uniqueness assertion unchanged for
that task-scoped subset.
…rn (PR Zoo-Code-Org#1133) CI failures: 1. Code QA Roo Code run 31231476937 (platform-unit-test): 8 unhandled EnvironmentTeardownError rejections from core/task/__tests__/grace-retry-errors.spec.ts. The Task constructor's usage-recorder init called the mocked provider's getUsageStatsService(); the catch block's console.warn fired during the test and tripped vitest's 'Closing rpc while onUserConsoleLog was pending'. Silenced the best-effort init catch (recorder stays disabled). 2. E2E Tests (Mocked) run 31231476961: usage-capture.test.ts startNewTask calls (USAGE_CAPTURE_SMOKE, USAGE_CAPTURE_HOOK_2) had no aimock fixture ('404 No fixture matched'). Fix: - Add fixtures/usage-capture.json with attempt_completion fixtures for both tags. Runs: https://github.com/Zoo-Code-Org/Zoo-Code/actions/runs/31231476937 https://github.com/Zoo-Code-Org/Zoo-Code/actions/runs/31231476961
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/vscode-e2e/fixtures/usage-capture.json`:
- Line 5: Update the userMessage entries in the usage-capture fixture to exactly
match the prompts sent by the test, including the “: what is your name?” suffix
for both USAGE_CAPTURE_SMOKE and USAGE_CAPTURE_HOOK_2.
🪄 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: c3523a74-ce1e-470a-a00b-f59180024b38
📒 Files selected for processing (2)
apps/vscode-e2e/fixtures/usage-capture.jsonsrc/core/task/Task.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/task/Task.ts
| "fixtures": [ | ||
| { | ||
| "match": { | ||
| "userMessage": "USAGE_CAPTURE_SMOKE" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'USAGE_CAPTURE_SMOKE|USAGE_CAPTURE_HOOK_2|userMessage' \
apps/vscode-e2e/src/suite/usage-capture.test.ts \
apps/vscode-e2e/fixtures/usage-capture.jsonRepository: Zoo-Code-Org/Zoo-Code
Length of output: 2171
Align fixture matchers with the sent user messages.
The test sends USAGE_CAPTURE_SMOKE: what is your name? and USAGE_CAPTURE_HOOK_2: what is your name?, but usage-capture.json matches only the fixture labels. Use the same full prompt text in userMessage so the fixtures can match.
🤖 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 `@apps/vscode-e2e/fixtures/usage-capture.json` at line 5, Update the
userMessage entries in the usage-capture fixture to exactly match the prompts
sent by the test, including the “: what is your name?” suffix for both
USAGE_CAPTURE_SMOKE and USAGE_CAPTURE_HOOK_2.
Source: Coding guidelines
added 6 commits
August 8, 2026 17:03
…llect all recorded events
…ry on Linux runners
…path lookup to prevent I/O event loop congestion
getStatsDirs() computed repoRoot as 3 levels up from __dirname which resolves to apps/ instead of project root on Linux CI. Use vscode.extensions.getExtension() to get the authoritative globalStorageUri at runtime, matching where ClineProvider writes data.
github-actions
Bot
added
the
awaiting-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Records provider usage delta exactly once from task API attempt finalization. Handles success, error, cancel, retry, incremental usage, and duplicate finalization. Does not include query/UI.
Included Files
src/services/stats/UsageRecorder.tssrc/core/task/Task.tssrc/api/providers/openai.tssrc/api/providers/openai-codex.tsExclusion Scope
Summary by CodeRabbit