Conversation
Add default-off client performance and memory telemetry with lifecycle, provider, render, and stdout instrumentation. Persist versioned local records with bounded retention, tolerant reporting, token-usage joins, and inspect/delete controls through /perf. Cover settings, cleanup, failure handling, concurrency, memory trends, reporting, and TUI integration with Bun behavioral tests.
github-actions
Bot
added
the
maintainer:e2e:ok
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:
📝 WalkthroughWalkthroughChangesThe PR adds optional local performance telemetry. It adds configuration, lifecycle instrumentation, memory sampling, JSONL storage and retention, reports, and Performance telemetry configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/ui/hooks/useMemoryMonitor.test.tsx (1)
19-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the
rssspy after the suite.
vi.spyOn(process.memoryUsage, 'rss')patches a global and is never restored.vi.clearAllMocks()clears call state, but it does not reinstall the original implementation. Restore the spy so other suites in the same worker observe the realprocess.memoryUsage.rss. Per repository learnings, mock cleanup must be scoped locally because no globalrestoreMocksbehavior is assumed.🧹 Proposed cleanup
afterEach(() => { vi.useRealTimers(); }); + + afterAll(() => { + rssSpy.mockRestore(); + });Add
afterAllto thebun:testimport list.🤖 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/cli/src/ui/hooks/useMemoryMonitor.test.tsx` around lines 19 - 28, Add afterAll to the bun:test imports and restore the process.memoryUsage.rss spy in an afterAll hook, ensuring the original global implementation is reinstated after the suite while preserving the existing per-test timer and mock cleanup.Source: Learnings
🧹 Nitpick comments (32)
packages/telemetry/src/perf/perfReport.tokenJoin.behavior.test.ts (2)
26-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two imports from the same module.
-import { buildReport, assembleReport } from './perfReport.js'; -import { aggregateTokenUsageByOperation } from './perfReport.js'; +import { + buildReport, + assembleReport, + aggregateTokenUsageByOperation, +} from './perfReport.js';🤖 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/telemetry/src/perf/perfReport.tokenJoin.behavior.test.ts` around lines 26 - 27, Merge the separate imports from perfReport.js into a single import declaration containing buildReport, assembleReport, and aggregateTokenUsageByOperation.
163-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
mapinstead offlatMapfor scalar values.
g.sampleCountis a number, soflatMapbehaves asmaphere and hides the intent.- const ops = report.groups.flatMap((g) => g.sampleCount); + const ops = report.groups.map((g) => g.sampleCount);🤖 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/telemetry/src/perf/perfReport.tokenJoin.behavior.test.ts` around lines 163 - 165, Replace flatMap with map when collecting scalar sampleCount values from report.groups in the test, preserving the expected [1] assertion and the existing single-operation-group behavior.packages/cli/src/session/buildPerfOwner.behavior.test.ts (2)
43-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused temporary directory fixture.
No test in this file reads or writes
dir. ThemkdtempSyncinbeforeEachand therminafterEachadd filesystem work without coverage value.♻️ Proposed cleanup
-let dir: string; - beforeEach(() => { - dir = fs.mkdtempSync(join(tmpdir(), 'perf-disabled-')); setInteractiveStdoutObserver(null); setInteractiveRenderObserver(null); setPerfPhaseObserver(null); }); -afterEach(async () => { +afterEach(() => { setInteractiveStdoutObserver(null); setInteractiveRenderObserver(null); setPerfPhaseObserver(null); - try { - await fsp.rm(dir, { recursive: true, force: true }); - } catch { - // ignore - } });The
node:fs,node:fspromises,node:path, andnode:osimports then become unnecessary.🤖 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/cli/src/session/buildPerfOwner.behavior.test.ts` around lines 43 - 61, Remove the unused dir fixture by deleting the dir declaration, its mkdtempSync setup in beforeEach, and the corresponding fsp.rm cleanup in afterEach; then remove the now-unused node:fs, node:fs promises, node:path, and node:os imports while preserving the observer resets.
167-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test cannot fail if the implementation changes format.
The assertion recomputes the expected value with the same expression the implementation uses. Any change in
resolvePlatformArchthat still usesprocess.platformandprocess.archpasses, and a swap of the two fields would need a separate check. Consider asserting the literal shape, for example that the string contains-and starts withprocess.platform.🤖 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/cli/src/session/buildPerfOwner.behavior.test.ts` around lines 167 - 176, Update the test around resolvePlatformArch so it independently validates the output format instead of constructing the expected value with the same process.platform-process.arch expression. Assert that the result starts with process.platform, contains the separator, and places process.arch after it, while keeping the test focused on the platform string format.packages/telemetry/src/perf/perfReport.ts (3)
514-524: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCount the terminal statuses in one pass.
countTerminalStatusesruns sevenfilterpasses over the operation list. A single loop with a pre-seeded record gives the same result with one pass.♻️ Proposed refactor
function countTerminalStatuses( opsRecords: readonly PerfOperationRecord[], ): Record<PerfTerminalStatus, number> { const terminalStatusCounts = {} as Record<PerfTerminalStatus, number>; for (const status of TERMINAL_STATUSES) { - terminalStatusCounts[status] = opsRecords.filter( - (o) => o.status === status, - ).length; + terminalStatusCounts[status] = 0; } + for (const o of opsRecords) { + terminalStatusCounts[o.status] += 1; + } return terminalStatusCounts; }🤖 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/telemetry/src/perf/perfReport.ts` around lines 514 - 524, Refactor countTerminalStatuses to initialize every TERMINAL_STATUSES entry to zero, then iterate over opsRecords once and increment the count for each record whose status is terminal. Preserve the returned Record shape and zero counts for terminal statuses absent from the input.
676-683: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
foundreports the presence of baseline rows, not a dimension match.
foundistruewhen any baseline operation exists, even when no non-baseline group shares its dimensions. The formatter then prints "matched" while every group prints the UNMATCHED warning. Consider naming the field to reflect "baseline rows exist", or deriving the printed label from the per-group match 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/telemetry/src/perf/perfReport.ts` around lines 676 - 683, Correct the baseline status reporting around the returned baselineInfo.found field so it reflects whether baseline dimensions match the compared groups, not merely whether pooledBaselineByDims contains rows. Either rename the field to represent baseline-row presence and update consumers, or derive the formatter’s matched label from each group’s actual match result, keeping the UNMATCHED warning consistent.
469-509: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueResolve the join once per operation instead of once per metric.
computeP50Valuesiterates the 20 metric keys in the outer loop and callsjoinedTokenValuefor every operation inside. That performs 20 map lookups per operation, and 18 of them returnundefinedbecause onlycontext_tokensandoutput_tokenscan join. Restrict the lookup to the two token keys.♻️ Proposed refactor
function computeP50Values( opsRecords: readonly PerfOperationRecord[], aggregatedTokens?: ReadonlyMap<string, AggregatedTokenUsage>, ): Record<string, number | null> { const p50Values: Record<string, number | null> = {}; for (const metricKey of P50_METRIC_KEYS) { + const isTokenKey = + metricKey === 'context_tokens' || metricKey === 'output_tokens'; const values = opsRecords.map((o) => { - if (aggregatedTokens !== undefined) { + if (isTokenKey && aggregatedTokens !== undefined) { const joined = joinedTokenValue(o, metricKey, aggregatedTokens); if (joined !== undefined) return joined; } return extractMetric(o, metricKey); }); p50Values[metricKey] = p50(values); } return p50Values; }🤖 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/telemetry/src/perf/perfReport.ts` around lines 469 - 509, Refactor computeP50Values so each operation resolves aggregatedTokens.get(o.operation_id) at most once, then reuse that joined record while computing all metric keys. Apply joined contextTokens/outputTokens only for context_tokens and output_tokens, preserving extractMetric fallback for unmatched operations and non-token metrics; update joinedTokenValue or remove it as needed.packages/cli/src/session/interactiveUI.startup.transaction.behavior.test.ts (1)
523-526: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
replacePreviousInstanceAndOwnerstatically.The file already imports from
./interactiveUI.jsat line 28. A dynamic import here adds no isolation, because the module is already loaded and its state is shared. AddreplacePreviousInstanceAndOwnerto the static import list for consistency.🤖 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/cli/src/session/interactiveUI.startup.transaction.behavior.test.ts` around lines 523 - 526, Update the existing static import from ./interactiveUI.js to include replacePreviousInstanceAndOwner, then call the imported function directly in the test instead of using the dynamic import expression.packages/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.cancellation.bun.tsx (1)
165-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
satisfies ObservableAgentEventhere for type safety.The other three tests in this file assert the event shape with
satisfies ObservableAgentEvent. This case usesas never, which disables type checking on the payload (including the extraname: 'shell'field). If the event shape drifts, this test keeps compiling and silently loses coverage.♻️ Proposed change
registry.observeAgentEvent( { type: 'tool-status', - update: { id: 'c1', name: 'shell', status: 'executing' }, - } as never, + update: { id: 'c1', status: 'executing' }, + } satisfies ObservableAgentEvent, signal, 0, );🤖 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/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.cancellation.bun.tsx` around lines 165 - 174, In the cancellation test’s registry.observeAgentEvent call, replace the `as never` cast with a `satisfies ObservableAgentEvent` assertion on the event payload. Preserve the existing event values while enabling compile-time validation of the full tool-status shape, including the nested update fields.packages/telemetry/src/perf/tokenUsageReader.behavior.test.ts (1)
25-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the duplicated import and correct the comment.
streamTokenUsageFromReadableis imported from the same specifier as the other symbols, so the second import statement is redundant. The comment states the seam is imported "directly from the internal module", but both statements target./tokenUsageReader.js.♻️ Proposed change
import { consumeTokenUsageDirectory, streamTokenUsageDirectory, streamTokenUsageRecords, + // Package-private seam: not re-exported from the package barrel. + streamTokenUsageFromReadable, } from './tokenUsageReader.js'; import type { TokenUsageStreamEntry } from './tokenUsageReader.js'; -// The controlled-readable seam is package-private (not exported from the -// barrel); same-package tests import it directly from the internal module. -import { streamTokenUsageFromReadable } from './tokenUsageReader.js';🤖 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/telemetry/src/perf/tokenUsageReader.behavior.test.ts` around lines 25 - 33, Merge streamTokenUsageFromReadable into the existing import from ./tokenUsageReader.js and remove the redundant second import. Update the adjacent comment to accurately describe the package-private seam without claiming it is imported from a different internal module.packages/cli/src/ui/hooks/memoryTrend/memoryTelemetry.behavior.test.ts (2)
283-308: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueChoose distinct fixture values so the assertion discriminates pre- and post-operation idle.
The first tick runs at
mono = 10_000before any operation ends, so itsms_since_last_operationequals uptime, which is10_000. The second tick computes25_000 - 15_000, which is also10_000. Both assertions at Lines 305-306 pass with the same number, so the test cannot detect a regression that returns uptime instead of the idle interval.Move the first tick to a different uptime, for example
mono = 4_000, and assert4_000forrecords[0].🤖 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/cli/src/ui/hooks/memoryTrend/memoryTelemetry.behavior.test.ts` around lines 283 - 308, Update the test around MemoryTelemetryController so the pre-operation tick uses a distinct monotonic time, such as 4,000, before markOperationEnd; assert records[0].ms_since_last_operation is 4,000 while preserving the post-operation tick at 25,000 and its 10,000 idle assertion.
390-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth tests parse a newline-delimited JSON file with a single
JSON.parsecall.PerfSinkappends one JSON document per line. Reading the whole file and callingJSON.parseonce works only while exactly one record exists. As soon as a second record lands in the same file, the call throws a syntax error that hides the real assertion.
packages/cli/src/ui/hooks/memoryTrend/memoryTelemetry.behavior.test.ts#L390-L398: split the file body on newlines, drop empty lines, and run theslopekey check over every parsed record.packages/cli/src/ui/hooks/agentStream/operationLifecycle.p10.memory.behavior.test.ts#L238-L243: apply the same line-by-line parse, and iterate every.jsonlfile instead of onlyfiles[0].🤖 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/cli/src/ui/hooks/memoryTrend/memoryTelemetry.behavior.test.ts` around lines 390 - 398, Update the JSONL parsing in packages/cli/src/ui/hooks/memoryTrend/memoryTelemetry.behavior.test.ts:390-398 to split the file contents into non-empty lines, parse each record individually, and check the slope key across every parsed record. Apply the same line-by-line parsing in packages/cli/src/ui/hooks/agentStream/operationLifecycle.p10.memory.behavior.test.ts:238-243, iterating over every .jsonl file rather than only files[0].packages/cli/src/ui/hooks/agentStream/operationLifecycle.p10.memory.behavior.test.ts (1)
41-43: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDispose the sink and retention in
afterEach.Each test creates a
PerfRetentionwithmaintenanceIntervalMs: 60_000, which starts an interval timer once the sink starts. Tests 1, 2, and 4 dispose only on the success path. If an assertion throws first, the timer and the claim file survive the test, andfs.rmSyncremoves the directory underneath a live retention owner.Track the created sinks and dispose them in
afterEach, in the same waypackages/cli/src/ui/hooks/memoryTrend/memoryTelemetry.behavior.test.tsLines 34-52 does withactiveSinks.🤖 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/cli/src/ui/hooks/agentStream/operationLifecycle.p10.memory.behavior.test.ts` around lines 41 - 43, Track each created PerfRetention sink in an active-sinks collection, and update afterEach to dispose every tracked sink before removing the test directory. Register sinks as they are created and clear the collection after disposal, following the activeSinks pattern used by the memoryTelemetry behavior test so cleanup also runs when assertions fail.packages/telemetry/src/perf/PerfSink.ts (1)
289-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
byteCountor document that it resets on each file roll.The getter is named
byteCount, but it returnsbytesSinceStat, which Line 331 resets to zero on every roll. A consumer that readsbyteCountas a per-run total gets the bytes written since the last day roll only. Either rename the getter tobytesSinceRoll, or add a doc comment that states the reset behavior.🤖 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/telemetry/src/perf/PerfSink.ts` around lines 289 - 291, Clarify the reset semantics of PerfSink’s byte-count getter by either renaming byteCount to bytesSinceRoll and updating its consumers, or adding a doc comment to byteCount stating that it resets to zero after each file roll. Keep the existing bytesSinceStat behavior unchanged.packages/cli/src/ui/inkRenderOptions.observer.behavior.test.ts (1)
112-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider resetting the observers in an
afterEach.This test leaves both module-level observers installed when it ends.
setInteractiveStdoutObserveralso leaves a cached stdio built with the observer. Every describe here resets inbeforeEach, so this suite stays correct. A different suite that runs later in the same Bun process and does not reset first would observe the leftover state.An
afterEachthat clears both observers makes the isolation independent of test-file ordering.♻️ Proposed cleanup hook
+ afterEach(() => { + setInteractiveStdoutObserver(null); + setInteractiveRenderObserver(null); + });Import
afterEachfrombun:testif you adopt this.🤖 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/cli/src/ui/inkRenderOptions.observer.behavior.test.ts` around lines 112 - 128, Import afterEach from bun:test and add a cleanup hook for this suite that clears both interactive render and stdout observers after every test, including any cached stdio state created by setInteractiveStdoutObserver. Preserve the existing beforeEach setup and test behavior.packages/cli/src/ui/hooks/perf/interactivePerfRuntime.ts (1)
361-371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the exported
PERF_MAINTENANCE_INTERVAL_MSconstant instead of the literal60_000.
packages/telemetry/src/perf/index.tsexportsPERF_MAINTENANCE_INTERVAL_MSfor this purpose. A literal here can drift from the retention policy constant.♻️ Proposed refactor
- maintenanceIntervalMs: 60_000, + maintenanceIntervalMs: PERF_MAINTENANCE_INTERVAL_MS,Add the import:
import { PerfSink, PerfRetention, + PERF_MAINTENANCE_INTERVAL_MS, } from '`@vybestack/llxprt-code-telemetry/perf/index.js`';🤖 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/cli/src/ui/hooks/perf/interactivePerfRuntime.ts` around lines 361 - 371, Update the PerfRetention configuration in the interactive performance runtime to use the exported PERF_MAINTENANCE_INTERVAL_MS constant for maintenanceIntervalMs instead of the 60_000 literal, importing it from the telemetry perf module.packages/cli/src/ui/cliUiRuntime.ts (1)
436-442: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify where
getPerfSnapshotCapabilityis actually supplied.
AppStateRuntimedeclaresgetPerfSnapshotCapability, butbuildUiRuntimeFromSourcenever maps it into theappslice. OnlybuildSlashCommandRuntimeattaches it, from its own parameter. A reader who follows the interface will expect theappslice to carry it. State in the doc comment that the flattenedCliUiRuntimesupplies this member, not theappslice.🤖 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/cli/src/ui/cliUiRuntime.ts` around lines 436 - 442, Update the doc comment for getPerfSnapshotCapability in AppStateRuntime to clarify that this member is supplied by the flattened CliUiRuntime, not mapped into the app slice by buildUiRuntimeFromSource. Keep the existing optional/null behavior and references to the interactive perf runtime owner unchanged.packages/cli/src/session/interactiveUI.tsx (1)
247-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated alternate-buffer condition.
The expression
settings.merged.ui.useAlternateBuffer === true && !config.getScreenReader()appears three times inside the same callback. Compute it once beforeresolveRenderMode.♻️ Proposed refactor
renderMode: () => { - resolveRenderMode( - config.getScreenReader(), - settings.merged.ui.useAlternateBuffer === true && - !config.getScreenReader(), - settings.merged.ui.useAlternateBuffer === true && - !config.getScreenReader() && - settings.merged.ui.incrementalRendering !== false, - ), + const screenReader = config.getScreenReader(); + const altBuffer = + settings.merged.ui.useAlternateBuffer === true && !screenReader; + return resolveRenderMode( + screenReader, + altBuffer, + altBuffer && settings.merged.ui.incrementalRendering !== 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 `@packages/cli/src/session/interactiveUI.tsx` around lines 247 - 255, In the renderMode callback, compute the repeated condition combining settings.merged.ui.useAlternateBuffer and !config.getScreenReader() once in a local variable, then reuse it for both alternate-buffer arguments to resolveRenderMode while preserving the incrementalRendering check.packages/cli/src/ui/commands/perfCommand.ts (1)
433-438: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
perfCommandexport and comment.
BuiltinCommandLoaderregisters a command created bycreatePerfCommand(...). No in-repository code importsperfCommand, which otherwise resolvesStorage.getGlobalLogDir()during module initialization.🤖 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/cli/src/ui/commands/perfCommand.ts` around lines 433 - 438, Remove the unused exported perfCommand constant and its accompanying comment. Keep command creation exclusively in BuiltinCommandLoader through createPerfCommand({ snapshotCapability}), and do not trigger Storage.getGlobalLogDir() during module initialization.packages/telemetry/src/perf/perfRecords.ts (1)
200-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate
joinKeyFromPromptIdtoderiveOperationId.The two functions have byte-identical bodies. The join contract requires both sides to use the same split rule. If one body changes, the write-time key and the read-time key diverge silently. Delegation makes the shared rule structural instead of conventional, and keeps both public names for intent.
♻️ Proposed refactor
export function joinKeyFromPromptId(promptId: string): string { - return promptId.split('`#continuation`#')[0]; + return deriveOperationId(promptId); }🤖 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/telemetry/src/perf/perfRecords.ts` around lines 200 - 213, Update joinKeyFromPromptId to delegate directly to deriveOperationId(promptId) instead of duplicating the split logic, while preserving both public function names and the existing continuation join-key behavior.packages/telemetry/src/perf/perfConsumer.behavior.test.ts (1)
68-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo perf test helpers hand-roll temp-directory names instead of calling
fs.mkdtemp. Both helpers build a name fromDate.now()andMath.random()and then callfs.mkdir.fs.mkdtempcreates the directory atomically and guarantees uniqueness.packages/telemetry/src/perf/perfInspect.behavior.test.tsin the same directory already usesfs.mkdtemp, so the suite is inconsistent.
packages/telemetry/src/perf/perfConsumer.behavior.test.ts#L68-L75: replace the body ofmakeTempDirwithreturn fs.mkdtemp(join(tmpdir(), 'perf-test-'));.packages/telemetry/src/perf/perfRecords.v0.behavior.test.ts#L229-L236: replace the body ofmakeTempDirwithreturn fs.mkdtemp(join(tmpdir(), 'perf-v0-'));.🤖 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/telemetry/src/perf/perfConsumer.behavior.test.ts` around lines 68 - 75, Update makeTempDir in packages/telemetry/src/perf/perfConsumer.behavior.test.ts (lines 68-75) to return fs.mkdtemp(join(tmpdir(), 'perf-test-')). Update makeTempDir in packages/telemetry/src/perf/perfRecords.v0.behavior.test.ts (lines 229-236) to return fs.mkdtemp(join(tmpdir(), 'perf-v0-')), removing the Date.now(), Math.random(), and fs.mkdir-based naming.packages/telemetry/src/perf/perfConsumer.ts (1)
162-195: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRetain only
okentries inconsumePerfDirectory; keep all counters.
perfReport.tsandperfInspect.tsskip non-okentries. Moveentries.push(...)into theokcase. This reduces memory use without changing diagnostic counts.🤖 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/telemetry/src/perf/perfConsumer.ts` around lines 162 - 195, Update consumePerfDirectory’s entry collection so entries.push(...) runs only within the 'ok' case of the entry.kind switch. Preserve all existing diagnostic counter increments for non-ok kinds and retain the exhaustive default handling.packages/telemetry/src/perf/retention.claim.behavior.test.ts (1)
214-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDispose the retention instance in the crash test.
This test calls
start()and never callsdispose(). The instance keeps a real 60 s interval and a claim file for the remainder of the test process. The interval isunref'd, so it does not block exit, but the leaked owner can touch the claim during later tests in the same file. Capture the assertion first, then dispose.🤖 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/telemetry/src/perf/retention.claim.behavior.test.ts` around lines 214 - 229, Update the crash test around PerfRetention.start so it captures the stale-claim assertion result, then calls retention.dispose() before the test exits. Preserve the existing claim-file assertions and ensure disposal occurs after validation.packages/telemetry/src/perf/retention.ts (1)
628-628: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
isClaimFileinstead ofendsWith('.claim').
maintainclassifies claims withisClaimFile(backed byCLAIM_FILE_RE), butcountNonStaleClaimsuses a raw suffix test. A file named exactly.claimmatches the suffix test but notCLAIM_FILE_RE, so the two paths can disagree. The file header statesperfArtifacts.tsis the single source of truth for artifact predicates.♻️ Proposed change
- const claimNames = names.filter((n) => n.endsWith('.claim')); + const claimNames = names.filter(isClaimFile);🤖 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/telemetry/src/perf/retention.ts` at line 628, Update countNonStaleClaims to filter claim names with the shared isClaimFile predicate instead of a raw .claim suffix check, keeping claim classification consistent with maintain and the perfArtifacts.ts source of truth.packages/telemetry/src/perf/retention.eviction.behavior.test.ts (2)
38-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared retention test helpers into one module.
writePerfFile,createClaimFile,setMtime, andlistFilesare duplicated inretention.claim.behavior.test.ts,retention.eviction.behavior.test.ts,retention.faults.behavior.test.ts,retention.multiowner.behavior.test.ts, andretention.scheduling.behavior.test.ts. Thedirvariable binding differs per file, so the helpers should takediras a parameter.Move them to a shared file such as
packages/telemetry/src/perf/__tests__/retentionTestHelpers.tsand import them in each suite.🤖 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/telemetry/src/perf/retention.eviction.behavior.test.ts` around lines 38 - 74, Extract writePerfFile, createClaimFile, setMtime, and listFiles into a shared retentionTestHelpers module, adding dir as an explicit parameter to each helper. Remove the duplicated local definitions from all five retention behavior suites and import the shared helpers, updating every call site to pass the suite’s dir binding.
342-348: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
utcDayKeyfromperfArtifacts.tsinstead of redefining it.
packages/telemetry/src/perf/perfArtifacts.tsexportsutcDayKeywith the same implementation. The test currently keeps a private copy. If the production day-key derivation changes, this copy hides the change instead of failing.♻️ Proposed change
-function utcDayKey(now: number): string { - const date = new Date(now); - const year = date.getUTCFullYear(); - const month = String(date.getUTCMonth() + 1).padStart(2, '0'); - const day = String(date.getUTCDate()).padStart(2, '0'); - return `${year}${month}${day}`; -}Add to the imports:
import { utcDayKey } from './perfArtifacts.js';🤖 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/telemetry/src/perf/retention.eviction.behavior.test.ts` around lines 342 - 348, Remove the local utcDayKey definition from the test and import the existing utcDayKey export from perfArtifacts.ts, preserving all current call sites and behavior.packages/telemetry/src/perf/retention.lifecycle.behavior.test.ts (1)
615-625: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
concurrentReaddirscounter leaks ifreaddirrejects.The decrement at Line 623 runs only on the success path. If
fsp.readdirthrows, the counter stays incremented andmaxConcurrentbecomes permanently wrong for the rest of the test. Usetry/finally.♻️ Proposed change
async readdir(d: string): Promise<string[]> { concurrentReaddirs += 1; maxConcurrent = Math.max(maxConcurrent, concurrentReaddirs); - // Only the first readdir is gated; subsequent ones proceed. - if (!gateReleased) { - await gate.promise; - } - const result = await fsp.readdir(d); - concurrentReaddirs -= 1; - return result; + try { + // Only the first readdir is gated; subsequent ones proceed. + if (!gateReleased) { + await gate.promise; + } + return await fsp.readdir(d); + } finally { + concurrentReaddirs -= 1; + } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/telemetry/src/perf/retention.lifecycle.behavior.test.ts` around lines 615 - 625, Update the readdir mock method to decrement concurrentReaddirs in a finally block surrounding the awaited fsp.readdir call, ensuring the counter is restored whether the filesystem operation resolves or rejects while preserving the existing result return behavior.packages/telemetry/src/perf/retention.capSelection.behavior.test.ts (1)
89-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the disk volume of this test and correct the byte comment.
This test writes 8 files of
Math.ceil(PERF_MAX_BYTES / 7)bytes, which is about 77 MiB total. The comment on Line 90 says "8 files × ~8 MiB each = ~64 MiB", which does not match Line 91. Large temporary writes can fail withENOSPCon CI runners that use a smalltmpfsforos.tmpdir().The cap is injectable, so you can prove the same binding behavior with a scaled-down
maxByteswhile keeping the constant assertion separate.♻️ Proposed change
- // Create fewer than MAX_FILES files but exceeding MAX_BYTES total. - // 8 files × ~8 MiB each = ~64 MiB ≈ MAX_BYTES. - const bytesPerFile = Math.ceil(PERF_MAX_BYTES / 7); // ~9.6 MiB each + // Create fewer than MAX_FILES files but exceeding the byte cap total. + // 8 files × (cap / 7) bytes ≈ 8/7 × cap, so the byte cap binds first. + const maxBytes = 8 * 1024 * 1024; // scaled-down cap: 8 files ≈ 9.1 MiB + const bytesPerFile = Math.ceil(maxBytes / 7);Then pass
maxBytesto thePerfRetentionoptions and assert against it instead ofPERF_MAX_BYTES.🤖 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/telemetry/src/perf/retention.capSelection.behavior.test.ts` around lines 89 - 101, Reduce the temporary data written by this test by introducing a small injectable maxBytes value for the PerfRetention setup, generating the eight files relative to that scaled cap, and asserting retention against the injected value rather than PERF_MAX_BYTES. Update the size comment to accurately describe the scaled totals, while keeping any separate PERF_MAX_BYTES constant assertion unchanged.packages/telemetry/src/perf/perfDelete.ts (1)
96-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
perfDeleteJSDoc to theperfDeletefunction.This documentation block describes
perfDelete, but it is attached tointerface StatedArtifact. Editors and API docs will show the delete contract on the wrong symbol.perfDeleteis declared at Line 295 with no documentation.♻️ Proposed change
-/** - * Deletes owned perf JSONL and stale claim artifacts from a directory, - * respecting live-writer safety. - * - * - A missing directory is a no-op (fail open, returns zero counts). - * - External filesystem failures fail open and are counted. - * - Internal invalid options (NaN/negative timing) fail fast. - * - * Protection rules (shared with retention via `perfArtifacts.ts`): - * - A perf JSONL whose day-key is today UTC AND mtime is within the - * maintenance interval is protected (active writer). - * - A perf JSONL whose run UUID has a non-stale/future claim is protected. - * - A claim that is non-stale (now - mtime ≤ lease) is protected. - */ interface StatedArtifact {Then add the same block immediately above
export async function perfDelete(at Line 295.🤖 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/telemetry/src/perf/perfDelete.ts` around lines 96 - 115, Move the existing deletion-contract JSDoc from the StatedArtifact interface to immediately above the export async function perfDelete declaration, leaving the interface undocumented and preserving the documentation text unchanged.packages/cli/src/ui/hooks/agentStream/operationLifecycle.ts (2)
245-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
apiActiveis written but never read.
enterApiPhasesetsop.apiActive = true, andPendingOpdeclares the field. No code reads it.classifyCancellationderives the API case from the absence of approval/tool state instead. Remove the field and the assignment, or read it inclassifyCancellationif the API phase must be distinguished from "no phase started".Also applies to: 585-588
🤖 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/cli/src/ui/hooks/agentStream/operationLifecycle.ts` around lines 245 - 246, Remove the unused apiActive field from PendingOp and remove its assignment in enterApiPhase, since classifyCancellation already derives the API case without reading it.
889-900: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffConsider bounding the poisoned lifecycle chain.
queueWritechains every write onlifecycleChain. After one internal (non-errno) rejection, the chain stays rejected for the remaining session. Every laterfinalisethen rejects, and those rejections propagate throughsubmitQueryTurnLifecycleinto the user's submit path. One instrumentation fault therefore fails all later turns, not just the failed record.The behavior is intentional and tested (D8 fail-fast), so this is a design question rather than a defect. Consider recording the first internal failure, disabling further instrumentation, and resetting
lifecycleChainto a resolved promise so telemetry degrades instead of breaking interactive turns.🤖 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/cli/src/ui/hooks/agentStream/operationLifecycle.ts` around lines 889 - 900, Update queueWrite to record the first non-errno persistence failure, disable subsequent lifecycle instrumentation, and reset lifecycleChain to a resolved promise after that failure. Preserve D8 fail-fast behavior for the failed write and drain(), while ensuring later finalise calls no longer reject user submit paths once instrumentation has been disabled.packages/cli/src/ui/hooks/agentStream/operationLifecycle.behavior.test.ts (1)
157-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
runUuidoverride does not reach the sink.
startAndCreateappliesoverrides.runUuidtoPerfRetentiononly. ThePerfSinkalways receives a freshcrypto.randomUUID(). A future test that passesrunUuidto correlate the claim file with the record file will silently get mismatched UUIDs. PassrunUuidto the sink as well, or remove the unused override.🧹 Proposed fix
const sink = overrides.sink ?? new PerfSink({ dir, - runUuid: crypto.randomUUID(), + runUuid, retention, });🤖 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/cli/src/ui/hooks/agentStream/operationLifecycle.behavior.test.ts` around lines 157 - 171, Update startAndCreate so the PerfSink constructor receives the same runUuid resolved from overrides and used by PerfRetention, instead of generating a second UUID; preserve override behavior and shared UUID correlation between retention and sink.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 04aa8be8-abce-4df7-aa45-0b7729b43a65
⛔ Files ignored due to path filters (27)
project-plans/issue3167/.completed/P13.mdis excluded by!project-plans/**project-plans/issue3167/acceptance-criteria.mdis excluded by!project-plans/**project-plans/issue3167/analysis/domain-model.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/01-schema-and-reader.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/02-perfsink-and-interval-union.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/03-stdout-observer.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/04-operation-lifecycle.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/05-client-phases.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/06-retention.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/07-memory-trend.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/08-consumer-and-perf-command.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/09-overhead-harness.mdis excluded by!project-plans/**project-plans/issue3167/execution-tracker.mdis excluded by!project-plans/**project-plans/issue3167/plan/00-overview.mdis excluded by!project-plans/**project-plans/issue3167/plan/01-preflight-verification.mdis excluded by!project-plans/**project-plans/issue3167/plan/02-analysis-pseudocode.mdis excluded by!project-plans/**project-plans/issue3167/plan/03-interval-union.mdis excluded by!project-plans/**project-plans/issue3167/plan/04-schema-perfsink-reader.mdis excluded by!project-plans/**project-plans/issue3167/plan/05-stdout-observer-onrender.mdis excluded by!project-plans/**project-plans/issue3167/plan/06-operation-lifecycle-identity.mdis excluded by!project-plans/**project-plans/issue3167/plan/07-client-phases.mdis excluded by!project-plans/**project-plans/issue3167/plan/08-retention.mdis excluded by!project-plans/**project-plans/issue3167/plan/09-settings.mdis excluded by!project-plans/**project-plans/issue3167/plan/10-memory-trend.mdis excluded by!project-plans/**project-plans/issue3167/plan/11-reader-consumer-perf-command.mdis excluded by!project-plans/**project-plans/issue3167/plan/12-integration-overhead-harness.mdis excluded by!project-plans/**project-plans/issue3167/plan/13-final-verification.mdis excluded by!project-plans/**
📒 Files selected for processing (125)
docs/telemetry-privacy.mdpackages/cli/src/__tests__/cliSessionDispatch.characterization.test.tsxpackages/cli/src/cli.provider-init.test.tspackages/cli/src/cli.startInteractiveUI.test.tsxpackages/cli/src/cli.test.tsxpackages/cli/src/config/configBuilder.tspackages/cli/src/config/perfSettingsMerge.behavior.test.tspackages/cli/src/config/perfSettingsValidation.behavior.test.tspackages/cli/src/config/settingsSchema.tspackages/cli/src/services/BuiltinCommandLoader.test.tspackages/cli/src/services/BuiltinCommandLoader.tspackages/cli/src/services/cliCommandApiMap.tspackages/cli/src/services/commandApiMapCompleteness.test.tspackages/cli/src/session/buildPerfOwner.behavior.test.tspackages/cli/src/session/interactiveUI.startup.transaction.behavior.test.tspackages/cli/src/session/interactiveUI.tsxpackages/cli/src/session/interactiveUiLifecycle.tspackages/cli/src/ui/App.tsxpackages/cli/src/ui/AppContainerRuntime.tsxpackages/cli/src/ui/cliUiRuntime.tspackages/cli/src/ui/commands/perfCommand.behavior.test.tspackages/cli/src/ui/commands/perfCommand.tspackages/cli/src/ui/commands/perfCommand.wiring.behavior.test.tspackages/cli/src/ui/containers/AppContainer/hooks/useAppBootstrap.tspackages/cli/src/ui/containers/AppContainer/hooks/useAppInput.tspackages/cli/src/ui/hooks/agentStream/__tests__/lifecyclePerfFixtures.tspackages/cli/src/ui/hooks/agentStream/__tests__/overheadHarness.useSubmitQuery.test.tsxpackages/cli/src/ui/hooks/agentStream/__tests__/useAgentEventStream.defaultoff.p07.bun.tsxpackages/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.cancellation.bun.tsxpackages/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.failfast.bun.tsxpackages/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.test.tsxpackages/cli/src/ui/hooks/agentStream/operationLifecycle.behavior.test.tspackages/cli/src/ui/hooks/agentStream/operationLifecycle.p07.behavior.test.tspackages/cli/src/ui/hooks/agentStream/operationLifecycle.p07.contract.behavior.test.tspackages/cli/src/ui/hooks/agentStream/operationLifecycle.p10.memory.behavior.test.tspackages/cli/src/ui/hooks/agentStream/operationLifecycle.snapshot.behavior.test.tspackages/cli/src/ui/hooks/agentStream/operationLifecycle.tspackages/cli/src/ui/hooks/agentStream/submitQueryTurnLifecycle.tspackages/cli/src/ui/hooks/agentStream/useAgentEventStream.tspackages/cli/src/ui/hooks/agentStream/useAgentStream.tspackages/cli/src/ui/hooks/agentStream/useAgentStreamOrchestration.tspackages/cli/src/ui/hooks/agentStream/useSubmitQuery.tspackages/cli/src/ui/hooks/memoryTrend/index.tspackages/cli/src/ui/hooks/memoryTrend/memoryRing.behavior.test.tspackages/cli/src/ui/hooks/memoryTrend/memoryRing.tspackages/cli/src/ui/hooks/memoryTrend/memorySlope.behavior.test.tspackages/cli/src/ui/hooks/memoryTrend/memorySlope.tspackages/cli/src/ui/hooks/memoryTrend/memoryTelemetry.behavior.test.tspackages/cli/src/ui/hooks/memoryTrend/memoryTelemetry.tspackages/cli/src/ui/hooks/memoryTrend/useMemoryMonitor.behavior.test.tspackages/cli/src/ui/hooks/perf/dynamicIdentity.behavior.test.tspackages/cli/src/ui/hooks/perf/interactiveLifecycle.behavior.test.tspackages/cli/src/ui/hooks/perf/interactivePerfRuntime.behavior.test.tspackages/cli/src/ui/hooks/perf/interactivePerfRuntime.startup.behavior.test.tspackages/cli/src/ui/hooks/perf/interactivePerfRuntime.tspackages/cli/src/ui/hooks/useMemoryMonitor.test.tsxpackages/cli/src/ui/hooks/useMemoryMonitor.tspackages/cli/src/ui/inkRenderOptions.observer.behavior.test.tspackages/cli/src/ui/inkRenderOptions.tspackages/core/package.jsonpackages/core/src/config/config.tspackages/core/src/config/configBaseCore.tspackages/core/src/config/configConstructor.tspackages/core/src/config/configPerfGetters.behavior.test.tspackages/core/src/config/configTypes.tspackages/core/src/config/perfSettings.behavior.test.tspackages/core/src/config/telemetryConfigCopy.behavior.test.tspackages/core/src/config/telemetrySettingsCopy.behavior.test.tspackages/core/src/index.tspackages/core/src/perf/perfPhaseObserver.tspackages/core/src/utils/stdio.observer.behavior.test.tspackages/core/src/utils/stdio.tspackages/providers/src/__tests__/attemptRecorder.perf.behavior.test.tspackages/providers/src/logging/attemptRecorder.tspackages/telemetry/index.tspackages/telemetry/package.jsonpackages/telemetry/src/perf/PerfSink.tspackages/telemetry/src/perf/index.tspackages/telemetry/src/perf/perfArtifacts.tspackages/telemetry/src/perf/perfConsumer.behavior.test.tspackages/telemetry/src/perf/perfConsumer.tspackages/telemetry/src/perf/perfDelete.behavior.test.tspackages/telemetry/src/perf/perfDelete.tspackages/telemetry/src/perf/perfInspect.behavior.test.tspackages/telemetry/src/perf/perfInspect.tspackages/telemetry/src/perf/perfPhaseObserver.behavior.test.tspackages/telemetry/src/perf/perfPhaseObserver.tspackages/telemetry/src/perf/perfReader.join.behavior.test.tspackages/telemetry/src/perf/perfReader.streaming.behavior.test.tspackages/telemetry/src/perf/perfReader.tolerant.behavior.test.tspackages/telemetry/src/perf/perfRecordSize.bench.tspackages/telemetry/src/perf/perfRecords.behavior.test.tspackages/telemetry/src/perf/perfRecords.tspackages/telemetry/src/perf/perfRecords.v0.behavior.test.tspackages/telemetry/src/perf/perfRecordsStream.tspackages/telemetry/src/perf/perfReport.behavior.test.tspackages/telemetry/src/perf/perfReport.tokenJoin.behavior.test.tspackages/telemetry/src/perf/perfReport.tspackages/telemetry/src/perf/perfSchema.boundary.behavior.test.tspackages/telemetry/src/perf/perfSelfHealth.behavior.test.tspackages/telemetry/src/perf/perfSelfHealth.model.behavior.test.tspackages/telemetry/src/perf/perfSink.failopen.behavior.test.tspackages/telemetry/src/perf/perfSink.retention.behavior.test.tspackages/telemetry/src/perf/perfSink.roundtrip.behavior.test.tspackages/telemetry/src/perf/perfSlopeBridge.tspackages/telemetry/src/perf/retention.capSelection.behavior.test.tspackages/telemetry/src/perf/retention.claim.behavior.test.tspackages/telemetry/src/perf/retention.eviction.behavior.test.tspackages/telemetry/src/perf/retention.faults.behavior.test.tspackages/telemetry/src/perf/retention.lifecycle.behavior.test.tspackages/telemetry/src/perf/retention.multiowner.behavior.test.tspackages/telemetry/src/perf/retention.scheduling.behavior.test.tspackages/telemetry/src/perf/retention.tspackages/telemetry/src/perf/retention.validation.behavior.test.tspackages/telemetry/src/perf/tokenUsageReader.behavior.test.tspackages/telemetry/src/perf/tokenUsageReader.tspackages/telemetry/src/telemetry/events/tool-events.tspackages/telemetry/src/telemetry/events/toolEvents.boundaries.behavior.test.tspackages/telemetry/src/telemetry/index.tspackages/telemetry/src/telemetry/intervalUnion.behavior.test.tspackages/telemetry/src/telemetry/intervalUnion.tspackages/telemetry/src/telemetry/loggers.perf.behavior.test.tspackages/telemetry/src/telemetry/loggers.tspackages/telemetry/src/telemetry/sessionMetricsAggregator.tsschemas/settings.schema.json
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. |
Contributor
WalkthroughBefore this PR, the CLI had no built-in mechanism to capture, retain, or inspect local performance telemetry; operation timing, memory trends, and token usage were either unobserved or only surfaced indirectly. After this PR, users can opt into local performance telemetry through settings, with a new perf sink/reader/consumer pipeline that retains records, tracks operation lifecycles, monitors memory trends, and exposes everything through a dedicated Release NotesNew Features
Tests
Documentation
Refactor
Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI/App
participant Submit as useSubmitQuery
participant Stream as useAgentEventStream
participant Agent as Agent
participant Ink as Ink/onRender
participant Stdio as StdioProxy
participant Memory as useMemoryMonitor
participant Sink as PerfSink
participant Cmd as /perf Command
User->>CLI: submit prompt
CLI->>Submit: initTurn, mint operation_id
Submit->>Stream: runStream(query, signal, promptId)
Stream->>Agent: agent.stream()
Agent-->>Stream: AgentEvent delta stream
Stream->>Ink: render frame
Ink-->>Stream: onRender metrics
Stream->>Stdio: write rendered output
Stdio-->>Stream: bytes written, write count
loop every 60s
Memory->>Memory: process.memoryUsage().rss
Memory->>Sink: append memory_sample record
end
Stream->>Sink: append operation perf record
Agent-->>Stream: stream complete
Stream-->>Submit: runStream resolved
Submit-->>CLI: turn finalized
User->>Cmd: /perf
Cmd->>Sink: read JSONL records
Sink-->>Cmd: perf records
Cmd-->>User: trend report
Magnitude🎯 4 (XL) Related
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
packages/cli/src/session/interactiveUI.tsx (1)
334-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one mouse-enablement decision for staging and activation.
commitInteractiveStartupcomputesstate.mouseStagedthroughports.isMouseEnabled, butsetupTerminalExitHandlerscalls the module-levelisMouseEventsEnableddirectly. A test that overrides only theisMouseEnabledport makes staging state and actual activation diverge, which can hide a rollback defect. Pass the computed value intosetupTerminalinstead.♻️ Proposed refactor
function setupTerminalExitHandlers( renderOptions: ReturnType<typeof inkRenderOptions>, - settings: LoadedSettings, + mouseEventsEnabled: boolean, ): void { - const mouseEventsEnabled = isMouseEventsEnabled(renderOptions, settings); if (mouseEventsEnabled) {state.mouseStaged = ports.isMouseEnabled(renderOptions, args.settings); - ports.setupTerminal(renderOptions, args.settings); + ports.setupTerminal(renderOptions, state.mouseStaged);Also applies to: 536-537
🤖 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/cli/src/session/interactiveUI.tsx` around lines 334 - 349, Update the interactive startup flow so the mouse-enabled decision computed by commitInteractiveStartup through ports.isMouseEnabled is passed into setupTerminal and setupTerminalExitHandlers, rather than recomputed with isMouseEventsEnabled. Use that single value for both staging and activation while preserving the existing exit-handler registration behavior.packages/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.failfast.bun.tsx (1)
182-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated
AggregateErrorbranches.The
cleanupErrors.length === 1branch produces the same value as the> 1branch.[err, ...cleanupErrors]equals[err, cleanupErrors[0]]when the array holds one item. One branch is enough.♻️ Proposed change
- if (cleanupErrors.length === 1) { - throw new AggregateError( - [err, cleanupErrors[0]], - 'setup cleanup also failed', - ); - } - if (cleanupErrors.length > 1) { + if (cleanupErrors.length > 0) { throw new AggregateError( [err, ...cleanupErrors], 'setup cleanup also failed', ); }🤖 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/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.failfast.bun.tsx` around lines 182 - 193, Collapse the two cleanupErrors branches into a single condition handling any non-empty cleanupErrors array, constructing AggregateError with [err, ...cleanupErrors] and preserving the existing message and throw behavior.packages/telemetry/src/perf/tokenUsageReader.behavior.test.ts (1)
25-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the duplicate import and correct the comment.
Lines 25-29 and line 33 import from the same specifier
./tokenUsageReader.js. The comment says the seam is imported "directly from the internal module", but both statements target the same module. Merge them into one import statement to avoid an ESLintno-duplicate-importsfailure and to remove the misleading note.♻️ Proposed change
import { consumeTokenUsageDirectory, streamTokenUsageDirectory, streamTokenUsageRecords, + // Package-private seam: not re-exported from the barrel. + streamTokenUsageFromReadable, } from './tokenUsageReader.js'; import type { TokenUsageStreamEntry } from './tokenUsageReader.js'; -// The controlled-readable seam is package-private (not exported from the -// barrel); same-package tests import it directly from the internal module. -import { streamTokenUsageFromReadable } from './tokenUsageReader.js';🤖 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/telemetry/src/perf/tokenUsageReader.behavior.test.ts` around lines 25 - 33, Merge the named imports from ./tokenUsageReader.js into a single import declaration, including streamTokenUsageFromReadable and the existing functions and type as appropriate. Remove the misleading controlled-readable seam comment and preserve all imported symbols and their type-only semantics.packages/telemetry/src/perf/retention.capSelection.behavior.test.ts (1)
87-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the comments and strengthen the byte-cap assertion.
Three points in this test are inaccurate:
- Line 90 says "8 files × ~8 MiB each"; line 91 computes
PERF_MAX_BYTES / 7(~9.14 MiB). Only one value can be right.- Line 116 asserts
remaining.length < PERF_MAX_FILES. The test creates 8 files, so this holds without any retention work. It does not prove the byte cap bound.- The real evidence is line 118 plus the fact that files were removed. Assert the removal directly.
♻️ Proposed adjustment
- // Create fewer than MAX_FILES files but exceeding MAX_BYTES total. - // 8 files × ~8 MiB each = ~64 MiB ≈ MAX_BYTES. - const bytesPerFile = Math.ceil(PERF_MAX_BYTES / 7); // ~9.6 MiB each + // Create far fewer than MAX_FILES files but exceeding MAX_BYTES total. + // 8 files × (MAX_BYTES / 7) ≈ 73 MiB > MAX_BYTES. + const bytesPerFile = Math.ceil(PERF_MAX_BYTES / 7); @@ - // The byte cap should be enforced (files count < MAX_FILES proves byte cap bound). - expect(remaining.length).toBeLessThan(PERF_MAX_FILES); + // The byte cap bound: files were deleted even though the count cap (128) + // was never reached. + expect(remaining.length).toBeLessThan(8); // Total bytes should be under the byte cap. expect(remainingBytes).toBeLessThanOrEqual(PERF_MAX_BYTES);🤖 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/telemetry/src/perf/retention.capSelection.behavior.test.ts` around lines 87 - 118, Correct the setup comment in the high-volume retention test to match the actual bytesPerFile calculation. Replace the ineffective remaining.length assertion with a direct assertion that retention removed at least one file, while preserving the remainingBytes <= PERF_MAX_BYTES assertion as the byte-cap check.
🤖 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/cli/src/session/interactiveUI.tsx`:
- Around line 221-267: Update buildAndStartPerfOwner around owner.start() to
catch startup failures, dispose the constructed owner, and rethrow the original
error. Preserve successful startup behavior, and first verify whether
InteractivePerfRuntime.start already guarantees cleanup to avoid duplicate
disposal.
In `@packages/cli/src/ui/hooks/agentStream/operationLifecycle.behavior.test.ts`:
- Around line 157-171: Ensure each test setup uses one shared runUuid for both
PerfRetention and PerfSink, matching production wiring. In
packages/cli/src/ui/hooks/agentStream/operationLifecycle.behavior.test.ts#L157-L171,
pass the existing runUuid to PerfSink; add and reuse a shared runUuid in
createRegistry at `#L119-L141` and in inline sink constructions at `#L687-L692`,
`#L760-L765`, `#L791-L796`, and `#L829-L834`. In
packages/cli/src/ui/hooks/agentStream/operationLifecycle.p10.memory.behavior.test.ts#L100-L109,
`#L135-L144`, `#L166-L175`, and `#L209-L218`, replace each pair of random UUID calls
with one shared value.
In `@packages/cli/src/ui/inkRenderOptions.ts`:
- Around line 61-79: Ensure clearing or replacing observers detaches them from
already-built Ink resources: in packages/cli/src/ui/inkRenderOptions.ts:61-79,
update the cached stdio path so writes consult the current observer or otherwise
support detaching instead of capturing a stale observer; in
packages/cli/src/ui/inkRenderOptions.ts:136-145, have the onRender callback read
interactiveRenderObserver at invocation time rather than capturing
renderObserver, while preserving fail-fast behavior for the currently active
observer.
In `@packages/telemetry/src/perf/perfSink.failopen.behavior.test.ts`:
- Around line 285-295: Add an assertion on openAttempts in the fail-open write
test after the second sink.write call, verifying that exclusive open was
attempted again. Keep the existing diagnostics, dispose, and file-count
assertions unchanged.
In `@packages/telemetry/src/perf/perfSink.retention.behavior.test.ts`:
- Around line 278-318: Rename the AC-7 describe block and test so they describe
manual-sweep retention convergence rather than concurrent overshoot, since the
body performs only sequential setup and one retention.maintain call. Preserve
the existing assertions and test behavior; do not introduce concurrency.
- Around line 108-127: Update the PerfSink filename generation used by
sink.write so the perf-<day>-<runUuid>.jsonl day key is derived from the
operation record’s ts, via the existing perfArtifacts.utcDayKey helper, rather
than the wall clock. Ensure all related retention behavior tests use
deterministic record timestamps and preserve the expected 20260808 filename
without depending on the current date.
In `@packages/telemetry/src/perf/retention.eviction.behavior.test.ts`:
- Around line 160-227: Update the tests a fresh claim is never evicted and a
future-mtime claim is protected until it becomes eligible so each created claim
UUID differs from the PerfRetention runUuid, ensuring own-run protection does
not determine the result. Add a separate fresh owner claim only if required by
the sweep setup, while preserving the existing assertions for the non-owner
claims.
- Around line 154-158: Strengthen the assertion after retention.maintain in the
retention test to verify the exact surviving artifact set rather than only its
size. Assert that the expected claim artifact remains, with the intended
filename, and thereby confirm the claim counted toward the cap without being
JSONL-parsed.
---
Nitpick comments:
In `@packages/cli/src/session/interactiveUI.tsx`:
- Around line 334-349: Update the interactive startup flow so the mouse-enabled
decision computed by commitInteractiveStartup through ports.isMouseEnabled is
passed into setupTerminal and setupTerminalExitHandlers, rather than recomputed
with isMouseEventsEnabled. Use that single value for both staging and activation
while preserving the existing exit-handler registration behavior.
In
`@packages/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.failfast.bun.tsx`:
- Around line 182-193: Collapse the two cleanupErrors branches into a single
condition handling any non-empty cleanupErrors array, constructing
AggregateError with [err, ...cleanupErrors] and preserving the existing message
and throw behavior.
In `@packages/telemetry/src/perf/retention.capSelection.behavior.test.ts`:
- Around line 87-118: Correct the setup comment in the high-volume retention
test to match the actual bytesPerFile calculation. Replace the ineffective
remaining.length assertion with a direct assertion that retention removed at
least one file, while preserving the remainingBytes <= PERF_MAX_BYTES assertion
as the byte-cap check.
In `@packages/telemetry/src/perf/tokenUsageReader.behavior.test.ts`:
- Around line 25-33: Merge the named imports from ./tokenUsageReader.js into a
single import declaration, including streamTokenUsageFromReadable and the
existing functions and type as appropriate. Remove the misleading
controlled-readable seam comment and preserve all imported symbols and their
type-only semantics.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: be085559-9bb0-4f14-a049-e1e51990e4b6
⛔ Files ignored due to path filters (27)
project-plans/issue3167/.completed/P13.mdis excluded by!project-plans/**project-plans/issue3167/acceptance-criteria.mdis excluded by!project-plans/**project-plans/issue3167/analysis/domain-model.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/01-schema-and-reader.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/02-perfsink-and-interval-union.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/03-stdout-observer.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/04-operation-lifecycle.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/05-client-phases.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/06-retention.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/07-memory-trend.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/08-consumer-and-perf-command.mdis excluded by!project-plans/**project-plans/issue3167/analysis/pseudocode/09-overhead-harness.mdis excluded by!project-plans/**project-plans/issue3167/execution-tracker.mdis excluded by!project-plans/**project-plans/issue3167/plan/00-overview.mdis excluded by!project-plans/**project-plans/issue3167/plan/01-preflight-verification.mdis excluded by!project-plans/**project-plans/issue3167/plan/02-analysis-pseudocode.mdis excluded by!project-plans/**project-plans/issue3167/plan/03-interval-union.mdis excluded by!project-plans/**project-plans/issue3167/plan/04-schema-perfsink-reader.mdis excluded by!project-plans/**project-plans/issue3167/plan/05-stdout-observer-onrender.mdis excluded by!project-plans/**project-plans/issue3167/plan/06-operation-lifecycle-identity.mdis excluded by!project-plans/**project-plans/issue3167/plan/07-client-phases.mdis excluded by!project-plans/**project-plans/issue3167/plan/08-retention.mdis excluded by!project-plans/**project-plans/issue3167/plan/09-settings.mdis excluded by!project-plans/**project-plans/issue3167/plan/10-memory-trend.mdis excluded by!project-plans/**project-plans/issue3167/plan/11-reader-consumer-perf-command.mdis excluded by!project-plans/**project-plans/issue3167/plan/12-integration-overhead-harness.mdis excluded by!project-plans/**project-plans/issue3167/plan/13-final-verification.mdis excluded by!project-plans/**
📒 Files selected for processing (125)
docs/telemetry-privacy.mdpackages/cli/src/__tests__/cliSessionDispatch.characterization.test.tsxpackages/cli/src/cli.provider-init.test.tspackages/cli/src/cli.startInteractiveUI.test.tsxpackages/cli/src/cli.test.tsxpackages/cli/src/config/configBuilder.tspackages/cli/src/config/perfSettingsMerge.behavior.test.tspackages/cli/src/config/perfSettingsValidation.behavior.test.tspackages/cli/src/config/settingsSchema.tspackages/cli/src/services/BuiltinCommandLoader.test.tspackages/cli/src/services/BuiltinCommandLoader.tspackages/cli/src/services/cliCommandApiMap.tspackages/cli/src/services/commandApiMapCompleteness.test.tspackages/cli/src/session/buildPerfOwner.behavior.test.tspackages/cli/src/session/interactiveUI.startup.transaction.behavior.test.tspackages/cli/src/session/interactiveUI.tsxpackages/cli/src/session/interactiveUiLifecycle.tspackages/cli/src/ui/App.tsxpackages/cli/src/ui/AppContainerRuntime.tsxpackages/cli/src/ui/cliUiRuntime.tspackages/cli/src/ui/commands/perfCommand.behavior.test.tspackages/cli/src/ui/commands/perfCommand.tspackages/cli/src/ui/commands/perfCommand.wiring.behavior.test.tspackages/cli/src/ui/containers/AppContainer/hooks/useAppBootstrap.tspackages/cli/src/ui/containers/AppContainer/hooks/useAppInput.tspackages/cli/src/ui/hooks/agentStream/__tests__/lifecyclePerfFixtures.tspackages/cli/src/ui/hooks/agentStream/__tests__/overheadHarness.useSubmitQuery.test.tsxpackages/cli/src/ui/hooks/agentStream/__tests__/useAgentEventStream.defaultoff.p07.bun.tsxpackages/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.cancellation.bun.tsxpackages/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.failfast.bun.tsxpackages/cli/src/ui/hooks/agentStream/__tests__/useSubmitQuery.lifecycle.test.tsxpackages/cli/src/ui/hooks/agentStream/operationLifecycle.behavior.test.tspackages/cli/src/ui/hooks/agentStream/operationLifecycle.p07.behavior.test.tspackages/cli/src/ui/hooks/agentStream/operationLifecycle.p07.contract.behavior.test.tspackages/cli/src/ui/hooks/agentStream/operationLifecycle.p10.memory.behavior.test.tspackages/cli/src/ui/hooks/agentStream/operationLifecycle.snapshot.behavior.test.tspackages/cli/src/ui/hooks/agentStream/operationLifecycle.tspackages/cli/src/ui/hooks/agentStream/submitQueryTurnLifecycle.tspackages/cli/src/ui/hooks/agentStream/useAgentEventStream.tspackages/cli/src/ui/hooks/agentStream/useAgentStream.tspackages/cli/src/ui/hooks/agentStream/useAgentStreamOrchestration.tspackages/cli/src/ui/hooks/agentStream/useSubmitQuery.tspackages/cli/src/ui/hooks/memoryTrend/index.tspackages/cli/src/ui/hooks/memoryTrend/memoryRing.behavior.test.tspackages/cli/src/ui/hooks/memoryTrend/memoryRing.tspackages/cli/src/ui/hooks/memoryTrend/memorySlope.behavior.test.tspackages/cli/src/ui/hooks/memoryTrend/memorySlope.tspackages/cli/src/ui/hooks/memoryTrend/memoryTelemetry.behavior.test.tspackages/cli/src/ui/hooks/memoryTrend/memoryTelemetry.tspackages/cli/src/ui/hooks/memoryTrend/useMemoryMonitor.behavior.test.tspackages/cli/src/ui/hooks/perf/dynamicIdentity.behavior.test.tspackages/cli/src/ui/hooks/perf/interactiveLifecycle.behavior.test.tspackages/cli/src/ui/hooks/perf/interactivePerfRuntime.behavior.test.tspackages/cli/src/ui/hooks/perf/interactivePerfRuntime.startup.behavior.test.tspackages/cli/src/ui/hooks/perf/interactivePerfRuntime.tspackages/cli/src/ui/hooks/useMemoryMonitor.test.tsxpackages/cli/src/ui/hooks/useMemoryMonitor.tspackages/cli/src/ui/inkRenderOptions.observer.behavior.test.tspackages/cli/src/ui/inkRenderOptions.tspackages/core/package.jsonpackages/core/src/config/config.tspackages/core/src/config/configBaseCore.tspackages/core/src/config/configConstructor.tspackages/core/src/config/configPerfGetters.behavior.test.tspackages/core/src/config/configTypes.tspackages/core/src/config/perfSettings.behavior.test.tspackages/core/src/config/telemetryConfigCopy.behavior.test.tspackages/core/src/config/telemetrySettingsCopy.behavior.test.tspackages/core/src/index.tspackages/core/src/perf/perfPhaseObserver.tspackages/core/src/utils/stdio.observer.behavior.test.tspackages/core/src/utils/stdio.tspackages/providers/src/__tests__/attemptRecorder.perf.behavior.test.tspackages/providers/src/logging/attemptRecorder.tspackages/telemetry/index.tspackages/telemetry/package.jsonpackages/telemetry/src/perf/PerfSink.tspackages/telemetry/src/perf/index.tspackages/telemetry/src/perf/perfArtifacts.tspackages/telemetry/src/perf/perfConsumer.behavior.test.tspackages/telemetry/src/perf/perfConsumer.tspackages/telemetry/src/perf/perfDelete.behavior.test.tspackages/telemetry/src/perf/perfDelete.tspackages/telemetry/src/perf/perfInspect.behavior.test.tspackages/telemetry/src/perf/perfInspect.tspackages/telemetry/src/perf/perfPhaseObserver.behavior.test.tspackages/telemetry/src/perf/perfPhaseObserver.tspackages/telemetry/src/perf/perfReader.join.behavior.test.tspackages/telemetry/src/perf/perfReader.streaming.behavior.test.tspackages/telemetry/src/perf/perfReader.tolerant.behavior.test.tspackages/telemetry/src/perf/perfRecordSize.bench.tspackages/telemetry/src/perf/perfRecords.behavior.test.tspackages/telemetry/src/perf/perfRecords.tspackages/telemetry/src/perf/perfRecords.v0.behavior.test.tspackages/telemetry/src/perf/perfRecordsStream.tspackages/telemetry/src/perf/perfReport.behavior.test.tspackages/telemetry/src/perf/perfReport.tokenJoin.behavior.test.tspackages/telemetry/src/perf/perfReport.tspackages/telemetry/src/perf/perfSchema.boundary.behavior.test.tspackages/telemetry/src/perf/perfSelfHealth.behavior.test.tspackages/telemetry/src/perf/perfSelfHealth.model.behavior.test.tspackages/telemetry/src/perf/perfSink.failopen.behavior.test.tspackages/telemetry/src/perf/perfSink.retention.behavior.test.tspackages/telemetry/src/perf/perfSink.roundtrip.behavior.test.tspackages/telemetry/src/perf/perfSlopeBridge.tspackages/telemetry/src/perf/retention.capSelection.behavior.test.tspackages/telemetry/src/perf/retention.claim.behavior.test.tspackages/telemetry/src/perf/retention.eviction.behavior.test.tspackages/telemetry/src/perf/retention.faults.behavior.test.tspackages/telemetry/src/perf/retention.lifecycle.behavior.test.tspackages/telemetry/src/perf/retention.multiowner.behavior.test.tspackages/telemetry/src/perf/retention.scheduling.behavior.test.tspackages/telemetry/src/perf/retention.tspackages/telemetry/src/perf/retention.validation.behavior.test.tspackages/telemetry/src/perf/tokenUsageReader.behavior.test.tspackages/telemetry/src/perf/tokenUsageReader.tspackages/telemetry/src/telemetry/events/tool-events.tspackages/telemetry/src/telemetry/events/toolEvents.boundaries.behavior.test.tspackages/telemetry/src/telemetry/index.tspackages/telemetry/src/telemetry/intervalUnion.behavior.test.tspackages/telemetry/src/telemetry/intervalUnion.tspackages/telemetry/src/telemetry/loggers.perf.behavior.test.tspackages/telemetry/src/telemetry/loggers.tspackages/telemetry/src/telemetry/sessionMetricsAggregator.tsschemas/settings.schema.json
🚧 Files skipped from review as they are similar to previous changes (87)
- packages/cli/src/config/configBuilder.ts
- packages/cli/src/services/BuiltinCommandLoader.test.ts
- packages/cli/src/tests/cliSessionDispatch.characterization.test.tsx
- packages/telemetry/package.json
- packages/cli/src/services/cliCommandApiMap.ts
- packages/cli/src/cli.provider-init.test.ts
- packages/core/package.json
- packages/cli/src/ui/hooks/useMemoryMonitor.test.tsx
- packages/cli/src/ui/containers/AppContainer/hooks/useAppInput.ts
- packages/core/src/config/telemetrySettingsCopy.behavior.test.ts
- packages/telemetry/index.ts
- packages/cli/src/ui/inkRenderOptions.observer.behavior.test.ts
- packages/core/src/config/configPerfGetters.behavior.test.ts
- packages/cli/src/ui/hooks/agentStream/useAgentStream.ts
- packages/cli/src/ui/containers/AppContainer/hooks/useAppBootstrap.ts
- packages/cli/src/cli.startInteractiveUI.test.tsx
- packages/cli/src/ui/App.tsx
- packages/core/src/index.ts
- packages/cli/src/services/commandApiMapCompleteness.test.ts
- packages/cli/src/services/BuiltinCommandLoader.ts
- packages/cli/src/ui/hooks/agentStream/useAgentStreamOrchestration.ts
- packages/cli/src/cli.test.tsx
- packages/cli/src/ui/hooks/agentStream/tests/overheadHarness.useSubmitQuery.test.tsx
- packages/cli/src/ui/hooks/agentStream/tests/useAgentEventStream.defaultoff.p07.bun.tsx
- packages/cli/src/ui/AppContainerRuntime.tsx
- packages/cli/src/ui/hooks/agentStream/operationLifecycle.p07.contract.behavior.test.ts
- packages/cli/src/config/settingsSchema.ts
- packages/cli/src/ui/hooks/perf/interactivePerfRuntime.startup.behavior.test.ts
- packages/cli/src/ui/hooks/perf/interactivePerfRuntime.behavior.test.ts
- packages/core/src/perf/perfPhaseObserver.ts
- packages/cli/src/ui/hooks/agentStream/useAgentEventStream.ts
- packages/cli/src/ui/hooks/agentStream/operationLifecycle.snapshot.behavior.test.ts
- packages/core/src/config/configConstructor.ts
- packages/telemetry/src/telemetry/loggers.ts
- packages/telemetry/src/telemetry/index.ts
- packages/cli/src/ui/hooks/agentStream/tests/lifecyclePerfFixtures.ts
- packages/cli/src/ui/commands/perfCommand.ts
- packages/cli/src/config/perfSettingsValidation.behavior.test.ts
- packages/telemetry/src/perf/perfPhaseObserver.behavior.test.ts
- packages/telemetry/src/perf/perfInspect.ts
- packages/cli/src/session/buildPerfOwner.behavior.test.ts
- packages/cli/src/ui/hooks/agentStream/tests/useSubmitQuery.lifecycle.test.tsx
- packages/core/src/config/config.ts
- packages/telemetry/src/perf/perfRecordsStream.ts
- packages/cli/src/ui/hooks/agentStream/useSubmitQuery.ts
- packages/telemetry/src/perf/perfRecords.behavior.test.ts
- packages/telemetry/src/perf/perfSink.roundtrip.behavior.test.ts
- packages/telemetry/src/perf/perfSchema.boundary.behavior.test.ts
- packages/cli/src/ui/hooks/memoryTrend/useMemoryMonitor.behavior.test.ts
- packages/cli/src/ui/hooks/memoryTrend/memorySlope.ts
- packages/cli/src/config/perfSettingsMerge.behavior.test.ts
- packages/telemetry/src/telemetry/events/toolEvents.boundaries.behavior.test.ts
- packages/cli/src/ui/hooks/memoryTrend/index.ts
- packages/telemetry/src/perf/perfReader.join.behavior.test.ts
- packages/telemetry/src/perf/PerfSink.ts
- packages/cli/src/ui/hooks/agentStream/operationLifecycle.p07.behavior.test.ts
- packages/cli/src/ui/hooks/perf/interactivePerfRuntime.ts
- packages/providers/src/logging/attemptRecorder.ts
- packages/telemetry/src/perf/perfRecords.ts
- packages/telemetry/src/perf/perfRecordSize.bench.ts
- packages/telemetry/src/perf/perfPhaseObserver.ts
- packages/telemetry/src/telemetry/intervalUnion.behavior.test.ts
- packages/core/src/config/telemetryConfigCopy.behavior.test.ts
- packages/core/src/config/configBaseCore.ts
- packages/cli/src/ui/hooks/useMemoryMonitor.ts
- packages/cli/src/ui/hooks/agentStream/tests/useSubmitQuery.lifecycle.cancellation.bun.tsx
- packages/core/src/utils/stdio.ts
- packages/cli/src/ui/hooks/memoryTrend/memoryRing.ts
- packages/telemetry/src/perf/perfDelete.ts
- packages/cli/src/ui/hooks/agentStream/submitQueryTurnLifecycle.ts
- packages/cli/src/ui/hooks/memoryTrend/memoryTelemetry.ts
- packages/cli/src/ui/cliUiRuntime.ts
- packages/telemetry/src/perf/retention.validation.behavior.test.ts
- packages/cli/src/session/interactiveUiLifecycle.ts
- packages/telemetry/src/telemetry/intervalUnion.ts
- docs/telemetry-privacy.md
- packages/cli/src/ui/hooks/memoryTrend/memoryRing.behavior.test.ts
- packages/cli/src/ui/hooks/memoryTrend/memorySlope.behavior.test.ts
- packages/cli/src/ui/hooks/agentStream/operationLifecycle.ts
- packages/providers/src/tests/attemptRecorder.perf.behavior.test.ts
- packages/telemetry/src/telemetry/events/tool-events.ts
- packages/core/src/config/perfSettings.behavior.test.ts
- packages/telemetry/src/perf/tokenUsageReader.ts
- packages/telemetry/src/perf/perfReport.ts
- packages/telemetry/src/perf/perfConsumer.ts
- packages/core/src/config/configTypes.ts
- packages/telemetry/src/perf/retention.ts
Contributor
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
Collaborator
Author
CodeRabbit pre-merge summary evaluationLinked Issues warning: no code change is warranted. The warning is based on the superseded revision in the original issue body. The issue author explicitly states in the issue comments that the committed specification and plan are authoritative and records these corrections:
Docstring Coverage warning: no broad documentation churn is warranted solely to satisfy CodeRabbit's generated percentage. This repository does not enforce that metric as a merge gate. New public contracts and non-obvious lifecycle/failure semantics are documented, while the extensive Bun behavioral tests provide executable contract evidence. Adding repetitive docstrings to private test fixtures and self-explanatory helpers would reduce signal without addressing issue intent. |
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
1 participant
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.
TLDR
Adds strictly opt-in, local-only client performance and memory telemetry so LLxprt users can measure operation trends, inspect retained data, and diagnose regressions without uploading telemetry. Collection remains disabled by default, and memory sampling requires a separate opt-in.
Dive Deeper
This PR implements the accepted issue 3167 design end to end:
The package dependency direction is preserved: packages/agents is unchanged, telemetry owns persistence/reporting, core owns narrow observer bridges, and CLI owns lifecycle and user-facing controls.
Internal observer and programming errors fail fast. Only genuinely external filesystem persistence and maintenance failures fail open.
Reviewer Test Plan
Confirm the default path is inert:
Enable collection in settings:
Start an interactive session, submit prompts including a tool-using prompt, then run /perf and /perf report. Verify operation counts and health are shown and the local perf directory contains versioned NDJSON records.
Enable telemetry.perf.memory, repeat a session, and verify memory fields/samples appear. Disable memory again and verify they are omitted.
Run /perf inspect, /perf report with an exact version or SHA baseline, and /perf delete. Verify delete preserves live writers while removing eligible retained artifacts.
Exercise cancellation during provider, tool, and approval phases and verify each operation finalizes once with the appropriate status.
Run the repository gates:
Local validation completed on macOS: the full workspace suite, lint, typecheck before and after build, format, build, StepFun smoke test, mechanical policy guards, and real tmux TUI scenarios for /perf, /perf inspect, and /perf report all passed.
Testing Matrix
Linked issues / bugs
Fixes #3167
Summary by CodeRabbit
/perfcommands for inspecting, reporting, snapshotting, and deleting telemetry data.