Fix integration event delivery defects#101
Conversation
|
CodeAnt AI is reviewing your PR. |
|
Warning Review limit reached
More reviews will be available in 17 minutes and 20 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 (2)
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 enhances the integration event bridge by adding path construction helpers to prevent segment duplication, ignoring agent-originated writes and temporary files, and implementing a longer deduplication TTL for logical changes based on event fingerprints. The review feedback highlights a cross-platform path resolution bug on Windows, recommends adding defensive null/undefined checks to prevent runtime crashes, and suggests optimizing the map cleanup in wasRecentlyInjected from O(N) to O(1) to improve performance under high event volumes.
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.
| } | ||
| } | ||
|
|
||
| const localPath = isAbsolute(trimmed) ? resolve(trimmed) : join(localRoot, trimmed) |
There was a problem hiding this comment.
On Windows, isAbsolute from node:path returns false for POSIX-style absolute paths (e.g., /tmp/relayfile/...). This causes localWatchEventPathsForFilename to incorrectly join the absolute path to localRoot instead of resolving it as a root-relative/absolute path. Checking if the path starts with a forward slash resolves this cross-platform discrepancy.
| const localPath = isAbsolute(trimmed) ? resolve(trimmed) : join(localRoot, trimmed) | |
| const localPath = (trimmed.startsWith('/') || isAbsolute(trimmed)) ? resolve(trimmed) : join(localRoot, trimmed) |
| function eventRecordValue(event: ChangeEvent, key: string): unknown { | ||
| const resource = isRecord(event.resource) ? event.resource : {} | ||
| const summary = isRecord(event.summary) ? event.summary : {} | ||
| return (event as Record<string, unknown>)[key] ?? resource[key] ?? summary[key] | ||
| } |
There was a problem hiding this comment.
To prevent potential runtime TypeError crashes if a malformed or null/undefined event is passed to eventRecordValue, add a defensive guard at the beginning of the function.
| function eventRecordValue(event: ChangeEvent, key: string): unknown { | |
| const resource = isRecord(event.resource) ? event.resource : {} | |
| const summary = isRecord(event.summary) ? event.summary : {} | |
| return (event as Record<string, unknown>)[key] ?? resource[key] ?? summary[key] | |
| } | |
| function eventRecordValue(event: ChangeEvent, key: string): unknown { | |
| if (!event) return undefined | |
| const resource = isRecord(event.resource) ? event.resource : {} | |
| const summary = isRecord(event.summary) ? event.summary : {} | |
| return (event as Record<string, unknown>)[key] ?? resource[key] ?? summary[key] | |
| } |
| function shouldNotifyRelayfileChange(event: ChangeEvent): boolean { | ||
| if (eventOrigin(event) === 'agent_write') return false | ||
| return shouldNotifyRelayfilePath(event.resource.path) | ||
| } |
There was a problem hiding this comment.
If event or event.resource is null or undefined, accessing event.resource.path will throw a runtime error. Adding optional chaining and a type check ensures safer execution.
| function shouldNotifyRelayfileChange(event: ChangeEvent): boolean { | |
| if (eventOrigin(event) === 'agent_write') return false | |
| return shouldNotifyRelayfilePath(event.resource.path) | |
| } | |
| function shouldNotifyRelayfileChange(event: ChangeEvent): boolean { | |
| if (eventOrigin(event) === 'agent_write') return false | |
| const path = event?.resource?.path | |
| return typeof path === 'string' && shouldNotifyRelayfilePath(path) | |
| } |
| private wasRecentlyInjected(key: string, ttlMs = RECENT_INJECTION_TTL_MS): boolean { | ||
| const now = Date.now() | ||
| for (const [entryKey, expiresAt] of this.recentInjections.entries()) { | ||
| if (expiresAt <= now) this.recentInjections.delete(entryKey) | ||
| } | ||
| if (this.recentInjections.has(key)) return true | ||
| this.recentInjections.set(key, now + RECENT_INJECTION_TTL_MS) | ||
| this.recentInjections.set(key, now + ttlMs) | ||
| return false | ||
| } |
There was a problem hiding this comment.
Iterating over the entire recentInjections map on every single call to wasRecentlyInjected results in an O(N) time complexity, which can degrade performance under high event volume. We can optimize this to O(1) by checking the specific key's expiration directly, and periodically cleaning up expired entries only when the map size exceeds a threshold.
private wasRecentlyInjected(key: string, ttlMs = RECENT_INJECTION_TTL_MS): boolean {
const now = Date.now()
const expiresAt = this.recentInjections.get(key)
if (expiresAt !== undefined && expiresAt > now) {
return true
}
if (this.recentInjections.size > 1000) {
for (const [entryKey, expiry] of this.recentInjections.entries()) {
if (expiry <= now) this.recentInjections.delete(entryKey)
}
}
this.recentInjections.set(key, now + ttlMs)
return false
}
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
Review: issue #99 delivery defects — scope-conformance + acceptance verification
Scope conformance: ✅ Every hunk maps to one of #99's four defect classes (plus minimal supporting plumbing: origin/revision/digest carried through filesystemEventToChangeEvent). Rebased on Track A merge 7678929. No scope creep.
Acceptance criteria verified at head 7abc2f4:
- No doubled event paths + regression test: ✅ Root cause was
join(localRoot, String(filename))whenfs.watchreports afilenamethat is already mount-/remote-relative.localWatchEventPathsForFilenamenow detects a filename that normalizes to a path insideremoteRootand maps it correctly. The new regression test reproduces both observed variants from #99 (absolute remote path under a message root → variant A; relativeslack/channels/...under the messages root → variant B). Path-construction code is exported and unit-tested as required. - No dotfile/
*.tmp-*events: ✅leaf.startsWith('.')+leaf.includes('.tmp-')inshouldNotifyRelayfilePath; test covers.meta.json.tmp-3507823867and.internal.json, asserting zero sends and zerolistAgentscalls (filter runs before fanout — good for Track E too). - Alias/raw dedup to one event per logical change: ✅ Pre-existing integration-scoped tail key already collapsed alias/raw paths; the new content fingerprint (
digest/revision/contentHash) at 10-min TTL kills the observed same-content re-delivery (3+ within minutes). Test: same revision via raw +__proj-cloudalias paths → one injection. - Self-echo suppression: ✅
origin === 'agent_write'filter — verifiedEventOrigin = "provider_sync" | "agent_write" | "system"is the real SDK contract (@relayfile/sdk/dist/types.d.ts:129), so this is not a guessed field. Local fallback events are stampedorigin: 'system', and the observed echo filenames (claude-1-codex-spawned.json,claude-1-issue82-ack.json) are covered by the slack/chat command-path heuristic.
Tests: ran locally at head — 41/41 pass. CI checks green (packaged-mcp-smoke still pending at review time). Unbounded-dataflow check: no new watchers/polls/fanout; net-negative event volume.
Findings (non-blocking)
- Heuristic depth —
isLikelyLocalWritebackCommandPathmatchesmessages/repliesat any depth, so a legitimate nested non-numeric.jsonrecord under a message subtree (e.g. a hypotheticalmessages/<ts>/files/<name>.json) would be silently suppressed. Both observed echo files were direct children ofmessages//replies/— consider requiring the leaf's parent segment to bemessages/repliesto cut the false-positive surface while keeping the fix. - Dedup loosening under bursts — when a fingerprint is present, two different revisions of the same path within 10s now both inject (the old path-only key suppressed the second). Correctness-wise this is arguably better, but it raises burst injection volume until Track B's queue lands — worth either also arming the plain 10s path key, or noting it as a Track B priority input.
- Remote origin coverage — the
agent_writefilter only fires if cloud actually populatesoriginon WS events. If it doesn't yet, AC4 is carried for remote events by the path heuristic alone. Worth confirming end-to-end with the relayfile contract work (codex-6) that agent writeback materializations carryorigin=agent_write. - Edge note (no action):
localWatchEventPathsForFilenameprefers the remote-path interpretation when a relative filename happens to mirror the remote root — a theoretical ambiguity with the correct bias given the observed defect.
Verdict
APPROVE — all four #99 acceptance criteria met with tests. Findings 1–3 are precision/coordination follow-ups, none blocking. Rebase note: #102 touches the same files; whoever merges second rebases per team discipline.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if (this.recentInjections.has(key)) return true | ||
| this.recentInjections.set(key, now + RECENT_INJECTION_TTL_MS) | ||
| this.recentInjections.set(key, now + ttlMs) | ||
| return false |
There was a problem hiding this comment.
Suggestion: The dedupe cache is marked as "already injected" before any message delivery happens, so a transient send failure will still suppress retries for the full TTL window. This can drop real integration events when broker delivery fails once. Record the dedupe key only after successful delivery (or remove it on failure) so failed injections can be retried. [logic error]
Severity Level: Critical 🚨
- ❌ Integration notifications dropped on transient broker send failures.
- ⚠️ Affects all providers using integration-event bridge delivery.
- ⚠️ Failures hidden behind logs; no automatic retry attempted.Steps of Reproduction ✅
1. In the test harness `makeHarness` at
`src/main/__tests__/integration-event-bridge.test.ts:79-123`, change the injected broker
so that `sendMessage` (defined at lines 104-111) throws or rejects on the first call
instead of pushing to the `sent` array.
2. Call `await harness.bridge.reconcile('project-1', [integration({...})])` as done in the
existing tests (e.g., `integration events route only to the targets` at lines 125-140).
This registers a subscription whose `onChange` callback calls `this.injectEvent(projectId,
event, specs)` in `src/main/integration-event-bridge.ts:938-953`.
3. Trigger an event using `await
harness.emit(changeEvent('/github/repos/acme/widgets.json', 'github'))` (helper at lines
46-76). The subscription callback logs `received` and calls `injectEvent` (lines 940-947).
Inside `injectEvent` (lines 1010-1099), the code computes `duplicateKey` (line 1031) and
`recentKey` (lines 1032-1033), then calls `this.wasRecentlyInjected(recentKey, ttl)` at
line 1034. On this first call, `wasRecentlyInjected` (lines 1101-1108) prunes expired
entries, finds no existing entry, sets `this.recentInjections.set(key, now + ttlMs)` at
line 1107, and returns `false`. The method then proceeds to `await
Promise.all(uniqueRecipients.map(... sendMessage ...))` at lines 1076-1098, where the
injected `sendMessage` throws. The rejected promise is caught only in the subscription's
`.catch` handler at lines 947-952, which logs `event delivery failed` but does not clear
`recentInjections`.
4. Within the TTL window (10 seconds via `RECENT_INJECTION_TTL_MS` at line 18 or 10
minutes via `RECENT_LOGICAL_CHANGE_TTL_MS` at line 19 when a fingerprint is present), emit
the same logical event again using `await
harness.emit(changeEvent('/github/repos/acme/widgets.json', 'github'))`. The subscription
callback again reaches `injectEvent` (line 1010), recomputes the same `recentKey`, and
calls `wasRecentlyInjected` (line 1034). This time `this.recentInjections.has(key)` at
line 1106 is `true`, so `wasRecentlyInjected` returns `true`, causing `injectEvent` to log
`skipped duplicate path` (lines 1035-1040) and return early without calling `sendMessage`.
The original delivery never succeeded, but all retries for the same key during the TTL are
suppressed, dropping the integration event.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:** 1106:1108
**Comment:**
*Logic Error: The dedupe cache is marked as "already injected" before any message delivery happens, so a transient send failure will still suppress retries for the full TTL window. This can drop real integration events when broker delivery fails once. Record the dedupe key only after successful delivery (or remove it on failure) so failed injections can be retried.
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. |
|
✅ pr-reviewer applied fixes — committed and pushed Implemented two review fixes:
Validated locally:
Only the two intended files are left modified. |
1 participant
User description
Fixes #99.
Summary
*.tmp-*, discovery/index/schema files, and local Slack/chat writeback command files.agent_writeevents before delivery, preventing an agent from receiving its own writeback as an incoming integration event.Acceptance Criteria
local watcher path construction does not duplicate remote path segments, including both variants from Integration event delivery defects: doubled paths, tmp-file leaks, duplicate alias delivery, self-echo of writeback files #99.*.tmp-*delivery: covered byintegration events ignore index, discovery, tmp, dotfile, and local writeback command files.agent_writeorigin filtering and local Slack/chat writeback filename filtering regressions.All four defect classes root-caused to Pear event bridge behavior; no cloud or Relayfile SDK changes were required.
Validation
node --experimental-strip-types --no-warnings --test src/main/__tests__/integration-event-bridge.test.tsnpm testnpm run buildgit diff --checkCodeAnt-AI Description
Fix duplicate and self-echoed integration events from local file changes
What Changed
Impact
✅ Fewer duplicate integration notifications✅ No self-echo from agent writebacks✅ Cleaner Slack and chat event delivery🔄 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.