Add targeted integration context reads#105
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
CodeAnt AI is reviewing your PR. |
|
Warning Review limit reached
More reviews will be available in 7 minutes and 8 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 (7)
📝 WalkthroughWalkthroughThis PR adds remote file reading capability to the Pear integrations system and uses it to enrich injected integration events with metadata and file context previews. The feature spans from a new IPC API endpoint through event bridge enrichment to comprehensive test coverage of edge cases. ChangesRemote file context preview and event enrichment
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 |
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track D review (head 65c6f9b)
Comment-review per shared-bot-account limitation; verdict line binding per the issue #82 team charter.
Scope conformance: PASS. Bridge diff is additive payload metadata + guidance text exactly as pre-announced (coordinator approved that shape as conflict-trivial); the targeted-read primitive lives in integrations.ts + IPC/preload/shared-types; no subscription/queue/watch restructuring. Rebase on 3895f1c verified.
Good:
integrationEventMetadata(integration-event-bridge.ts:794) flattensprovider/resourcePath/resourceId/title/status/actorinto messagedata— matches the issue's "event payload should include provider, resource path, resource id, title/status/actor when available" verbatim; no key collisions with the existingdatashape; bridge test asserts the new fields.readRemoteFile(integrations.ts:749) is properly scoped: project existence check, path normalization, mount-scope containment check before any network call (tested), no local mount reconciliation (tested viaensureMountednot called), 1MB preview cap + NUL-byte binary detection.
Findings
-
[blocking-as-disposition] The core Track D acceptance criterion is not yet reachable by an agent, and nothing consumes the new read path. Acceptance: "Agent can handle a Slack message event and fetch the specific message/thread context without historical download enabled." Post-#98 (inline Slack context removed) an agent's only context source is
.integrations— which requires historical download. This PR adds the right primitive (integrations:read-remote-file— the issue explicitly sanctions "a small Pear IPC bridge"), butgrepfinds zero consumers: no renderer call site and no agent-facing surface (the injected guidance says "use targeted Relayfile context reads … when available" — today it never is). Resolve by ONE of:- (a) wire a consumer in this PR (agent-facing read tool or renderer surface that serves agent context requests), or
- (b) per the Track C precedent: post an explicit partial-acceptance disposition on #82 — Track D delivers payload metadata + the scoped read primitive; agent-facing wiring is a named follow-up (with issue ref) — and soften the injected guidance text so agents aren't told to use a path that doesn't exist yet (e.g. mention the metadata fields instead).
I'll APPROVE same-day on either.
-
[note, non-blocking] Preview cap is post-fetch:
readFilepulls the full content into main-process memory beforeremoteFileReadToPreviewdiscards >1MB bodies — the cap bounds renderer transfer, not network/memory. Fine while the SDK lacks size/HEAD or range reads; worth one comment line so it isn't mistaken for a transfer budget (same temporary-pending-SDK-contract family as #103). -
[nit, non-blocking] Missing branch tests for
remoteFileReadToPreview'too-large' and 'binary' outcomes (base64 + NUL-byte path). Cheap to add while you're in there. -
[merge-order] Same bridge/test file as #100/#103 (both approved, at the merge gate ahead of you) — standing rebase convention applies; expect a rebase before merge.
Verdict: REQUEST CHANGES
Single substantive item (finding 1) with a text-only resolution path available. Metadata work and the read primitive itself are exactly right.
65c6f9b to
d159b4a
Compare
| return this.withWorkspaceHandle(async (handle) => { | ||
| const file = await handle.client().readFile(handle.workspaceId, path) | ||
| return remoteFileReadToPreview(file) | ||
| }) |
There was a problem hiding this comment.
Suggestion: readRemoteFile lets SDK errors (for example not-found/permission errors) reject the IPC call instead of returning a FilePreview result, which breaks parity with readMountPreview that always returns a preview shape (kind: 'missing' on failures). Wrap the remote read in error handling and map failures to the existing preview contract so renderer callers don't need a different error path. [api mismatch]
Severity Level: Major ⚠️
- ❌ Remote integration previews throw on missing remote files.
- ⚠️ Renderer FsReadPreviewResult callers see inconsistent error handling.
- ⚠️ IPC rejections can appear as uncaught errors in UI.Steps of Reproduction ✅
1. From the renderer, call `window.pear.integrations.readRemoteFile(projectId,
remotePath)` which is wired in `src/preload/index.ts:372-383` to
`invoke<FsReadPreviewResult>('integrations:read-remote-file', projectId, remotePath)`.
2. Electron IPC routes this to the main process handler
`ipcMain.handle('integrations:read-remote-file', ...)` in
`src/main/ipc-handlers.ts:25-31`, which calls
`integrationsManager.readRemoteFile(projectId, remotePath)`.
3. In `IntegrationsManager.readRemoteFile` (`src/main/integrations.ts:749-761`), after
project and scope checks, the method executes
`handle.client().readFile(handle.workspaceId, path)` inside `return
this.withWorkspaceHandle(async (handle) => { ... })` without any try/catch or error
mapping.
4. If the Relayfile SDK `readFile` call rejects (for example the remote file is missing or
access is denied), the promise from `readRemoteFile` rejects and the IPC `invoke` in the
renderer receives a rejected promise instead of an `FsReadPreviewResult`. In contrast,
`readMountPreview` (`src/main/integrations.ts:744-747`) calls
`filesystem.readTextPreview`, which wraps `stat`/`readFile` in a try/catch and returns `{
kind: 'missing', content, size: 0 }` on errors (`src/main/filesystem.ts:33-65), so local
previews never reject. This discrepancy forces renderer callers to handle a different
error path for remote reads or risk uncaught promise rejections.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/integrations.ts
**Line:** 758:761
**Comment:**
*Api Mismatch: `readRemoteFile` lets SDK errors (for example not-found/permission errors) reject the IPC call instead of returning a `FilePreview` result, which breaks parity with `readMountPreview` that always returns a preview shape (`kind: 'missing'` on failures). Wrap the remote read in error handling and map failures to the existing preview contract so renderer callers don't need a different error path.
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. |
d159b4a to
0a97c80
Compare
|
Fixed the PR issues I found:
Local verification passed:
|
|
Fixed the failing changed-file Vitest coverage in src/main/integrations.test.ts. The test harness now mocks the cloud integrations list endpoint used by local mount hydration, and the async Validation run locally:
I could not access GitHub review/comment APIs for this private PR from the sandbox; they returned |
|
✅ pr-reviewer applied fixes — committed and pushed Fixed the failing changed-file Vitest coverage in src/main/integrations.test.ts. The test harness now mocks the cloud integrations list endpoint used by local mount hydration, and the async Validation run locally:
I could not access GitHub review/comment APIs for this private PR from the sandbox; they returned |
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track D re-review (delta 65c6f9b → 0a97c80, rebase over cb474f6)
Comment-review per shared-bot-account limitation; verdict line binding per the issue #82 team charter.
The previous blocking item is substantively resolved: the real consumer is IntegrationEventBridge.readEventContextPreview() — the bridge targeted-reads the changed record via SDK readFile at injection time and delivers the context inline (text + structured data.contextPreview), so the Track D acceptance ("agent can handle a Slack message event and fetch the specific message/thread context without historical download enabled") is now actually met by delivery: context arrives with the event, no local mirror involved, graceful degradation on read failure via aggregated warn, optional-readFile guard for clients without it. Binary/too-large branches implemented AND tested (closes my prior nit), post-fetch cap comment added (closes the other). Rebase composition over #100/#102 is clean in range-diff (reviewed in full).
Three new, small items arising from the new mechanism — all in the budget/reachability class this issue exists to fix:
-
[should-fix] Skip the preview read for
file.deletedevents.readEventContextPreviewruns for every injected event regardless of type; deletions are notified (terminal states are normal flow), so every legitimate delete now costs a doomed remote read plus a spuriousevent context read failedwarn (first + every 25th). Oneif (event.type === 'file.deleted') return undefinedand a deletion-path test. -
[should-fix] Don't ship the full preview content twice per recipient.
contextPreview.content(≤32KB) is inlined in the message text AND duplicated insidedata.contextPreview— every recipient message carries up to ~64KB for one event, multiplied by recipient count, with Track B's bounded dispatcher not yet landed. Keep content in ONE place (text is what agents read; reducedata.contextPreviewto{path, kind, size, contentType}), or cap the inline text segment harder (pre-#98 inline context capped at 4KB — 32KB is 8× that). #82's core complaint is injection volume; let's not reintroduce it through the context channel. -
[should-fix] Agent guidance again references surfaces agents can't invoke. The injected lines
Targeted read IPC: integrations:read-remote-file(...)andRenderer helper: pear.integrations.readRemoteFile(...)are renderer/preload APIs — a terminal agent session cannot call either; "use the targeted read IPC above for fresh context" instructs an unreachable action (same finding class as round 1). Since the inline preview now delivers the context, reframe: present the preview as the context, note the path can be re-read on demand via the Pear UI/renderer helper (for humans/tools that have it), and drop the imperative phrasing toward agents.
CI at 0a97c80 was still starting at review time (CodeRabbit pending, checks/smoke not yet reported) — full green required before merge regardless.
Verdict: REQUEST CHANGES
All three are small, mechanical, and in code you just touched; the consumer architecture itself is right and I expect this to be the last round. FIFO: you're behind #103 (approved, active) and #104 (approved); a rebase will follow those merges — standing convention applies.
0ba49b1 to
5e4fab8
Compare
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track D final re-check (delta 0a97c80 → 5e4fab8)
Comment-review per shared-bot-account limitation; verdict line binding per the issue #82 team charter.
All three REQUEST CHANGES items resolved exactly as specified, verified by direct diff of the two heads:
- ✅ Deleted-event reads:
readEventContextPreviewreturns early onfile.deleted(integration-event-bridge.ts:1277), with the test asserting zeroreadFilecalls ANDdata.contextPreview === undefinedon the deletion path. - ✅ No double-shipped content:
data.contextPreviewnow carriesEventContextPreviewMetadata(Omit<…, 'content'>— path/kind/size/contentType only) viaeventContextPreviewMetadata()at the injection call site; preview content lives solely in the message text. Test assertsdata.contextPreview.contentisundefined. - ✅ Agent-reachable guidance: renderer/IPC invocation lines removed from the event text; replaced with a plain
Targeted context path:line and the closing guidance now presents the inline preview as the context source. No more unreachable imperatives.
Author validation: bridge 24/24, build pass.
Verdict: APPROVE (content-based at head 5e4fab8)
Merge gates: FIFO turn (behind #103 active and #104 approved), full green CI at the merge head (checks + packaged-mcp-smoke hadn't reported on 5e4fab8 at review time), and the post-#103/#104 rebase under the standing convention — mergeState is already DIRTY as you noted, so expect a rebase; clean → "diff unchanged" note, conflicts touching logic → delta re-check with both SHAs. After merge: Track D fix note on #82 (acceptance: event payload metadata fields, inline targeted-read context preview without historical download, scoped renderer read surface) and announce in #general.
5764e49 to
76b506e
Compare
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track D delta re-check #2 (5e4fab8 → 76b506e, rebase/composition over #97)
Comment-review per shared-bot-account limitation; verdict line binding per the issue #82 team charter.
The #97 composition implements the coordinator's disposition exactly, verified at code and test level:
- Single context source: #97's
eventContextLines/slackRecordContextLines/readLocalEventRecord(expand-path + local-mount fallback context) are fully retired from the bridge —grepconfirms onlyreadEventContextPreview+ the 32KB cap remain. One eager read, one inline block per event. - Preserved #97 behaviors agents depend on: DM glob subscription/delivery (D-channel +
dms/*tests) and the Slack staleness filters stay intact. - Required test coverage all present: history-off Slack context resolves through ONE targeted remote preview (
test:356asserts exactly oneInline context preview:match anddoesNotMatch /Slack text:/),data.contextPreview.contentundefined, deleted events make zeroreadFilecalls.
CI fully green at 76b506e (checks ✅, smoke ✅, CodeRabbit ✅). Author validation: bridge 27/27, npm test 53/53, build, diff-check.
Verdict: APPROVE (content-based at head 76b506e)
Known forward path: #103 merged at fcb1283 (SDK 0.8.10) and #104 is FIFO-active ahead of you — this branch rebases at least once more. This approval carries forward content-wise under the standing convention: clean rebase → merge with note; composition touching logic (likely, given the SDK bump touches FileReadResponse and #103 reshaped subscribe options) → SHA pair for delta re-check. Revalidate against 0.8.10 explicitly (your readFile/FileReadResponse usage) and confirm the retired #97 context path stays retired through the rebase. After merge: Track D fix note on #82 + announce.
1920bed to
30de4ae
Compare
|
CodeAnt AI is running Incremental review |
|
CodeAnt AI Incremental review completed. |
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track D delta re-check #3 (76b506e → 30de4ae, composition over #103+#104 / main cf39826)
Comment-review per shared-bot-account limitation; verdict line binding per the issue #82 team charter.
Not a clean rebase (as expected — #103 reshaped the bridge underneath), reviewed as full composition via git range-diff 4ec4a71..76b506e cf39826..30de4ae:
- #103 semantics correctly absorbed: the raw-channel-path case flips from #97-era delivery to ignored (
assert.deepEqual(harness.sent, [])), stale-backfill test assertseventsDroppedcounters viawaitForDropped, fixtures move to slack-ts-consistent timestamps +withMockedNowreconciles. All consistent with the merged Track C baseline. - Retired stays retired:
grepcount foreventContextLines|slackRecordContextLines|readLocalEventRecordat 30de4ae = 0; #97's local/expand context tests are gone and replaced by the preview suite. Single bounded read/block holds. - Carry-forward checks pass: history-off Slack thread context resolves through ONE targeted remote
readFile(exactly-one-block +doesNotMatch /Slack text:/assertions intact, now with explicitdownloadHistoricalData: false); binary/too-large branches preserved;data.contextPreview.contentundefined. - Unadopted bot drift
1920bedconfirmed absent (head is owner-composed 30de4ae) — per the standing bot-delta rule.
Author validation: integrations 9/9, bridge 32/32, npm test 58/58, build, diff-check. CI at 30de4ae: checks ✅, CodeRabbit ✅, packaged-mcp-smoke pending at review time.
Verdict: APPROVE (content-based at head 30de4ae)
Merge gate: packaged-mcp-smoke green at 30de4ae. On green: merge, post the Track D fix note on #82 (acceptance: structured event metadata, single bounded inline context preview without historical download, scoped renderer read surface, deleted-event read skip), announce in #general so #106 starts its final rebase.
|
Fixed one production bug in integration-event-bridge.ts: deleted Relayfile filesystem events are normalized to Updated the deleted-file regression in integration-event-bridge.test.ts to match the production event shape. Validated locally:
GitHub review/comment APIs returned |
|
✅ pr-reviewer applied fixes — committed and pushed Fixed one production bug in integration-event-bridge.ts: deleted Relayfile filesystem events are normalized to Updated the deleted-file regression in integration-event-bridge.test.ts to match the production event shape. Validated locally:
GitHub review/comment APIs returned |
User description
Summary
integrations:read-remote-fileIPC/preload path backed by Relayfile SDKreadFileso Pear can read one provider record by Relayfile path without local provider-history mountsprovider,resourcePath,resourceId,title,status,actor)IntegrationEventBridge.readEventContextPreview()performs a targeted SDKreadFilefor the eventresource.path, injects a bounded inline text preview for agents, and stores preview metadata only indata.contextPreview#97/#103/#104 Composition
cf39826after Track C (Mitigate integration replay subscriptions #103, SDK@relayfile/sdk@0.8.10) and Track G (Bound local integration history mounts #104).readEventContextPreview()remains the single Slack inline context source after composing with Inline Slack thread event context #97.expand('full')Slack context path and local-mount context fallback remain retired, so each event performs one eager context read and emits one inline context block.Bot Drift
1920bedduring the rebase; this branch is now owner-composed at30de4ae.Acceptance
Notes
readFile, so no SDK fork was needed. The preview cap is post-fetch because the SDK returns the full file today; Pear bounds only injected context size.Fixes part of #82 (Track D).
Validation
npx vitest run src/main/integrations.test.ts(9/9)node --experimental-strip-types --no-warnings --test src/main/__tests__/integration-event-bridge.test.ts(32/32)npm test(58/58)npm run buildgit diff --checkCodeAnt-AI Description
Read remote integration records directly and show targeted context in event messages
What Changed
Impact
✅ Faster Slack event context✅ Fewer missing previews when local mounts are off✅ Clearer integration event details🔄 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.