Repository navigation
refactor(memory_sync): convert Gmail to the generic sync orchestrator (Refs #3333, stacked on #3873) - #3874
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe Gmail incremental sync implementation is extracted from ChangesGmail IncrementalSource migration and orchestrator extensions
Sequence Diagram(s)sequenceDiagram
participant Provider as GmailProvider
participant Entry as run_gmail_sync
participant Orch as orchestrator::run_sync
participant Source as GmailSource
participant API as Composio API
rect rgba(100, 149, 237, 0.5)
note over Provider,Entry: Sync entry
Provider->>Entry: run_gmail_sync(ctx, reason)
Entry->>Orch: run_sync(ctx, reason, GmailSource)
end
rect rgba(144, 238, 144, 0.5)
note over Orch,API: Preamble — resolve account email
Orch->>Source: preamble()
Source->>API: ACTION_GET_PROFILE
API-->>Source: email address (cached in OnceLock)
end
loop Per scope / per page (up to page_ceiling)
Orch->>Source: page_ceiling(reason, state)
Source-->>Orch: effective_max_pages
Orch->>Source: fetch_page(cursor or depth window)
Source->>API: ACTION_FETCH_EMAILS(query, max_results, pageToken)
API-->>Source: message items + nextPageToken
Source-->>Orch: PageFetch
Orch->>Orch: select_pending (dedup)
alt pending empty AND stop_on_empty_pending()
Orch->>Orch: break scope loop
else pending non-empty
Orch->>Source: ingest_page(pending items)
Source->>Source: ingest_page_into_memory_tree_with_outcome
end
end
Orch-->>Entry: SyncOutcome
Entry->>Entry: record_periodic_success("gmail")
Entry-->>Provider: SyncOutcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Walkthrough
4th increment of the #3333 epic. GmailProvider::sync becomes a one-line delegate to run_gmail_sync; the bespoke ~536-line loop moves into a new gmail/source.rs IncrementalSource impl. Two new defaulted orchestrator hooks land: page_ceiling(reason, state) (gmail's adaptive 2-page cap when the prior sync ran <5min ago) and stop_on_empty_pending() (halt a scope once a page dedups to nothing). Gmail sets server_side_depth=true (injects after:<epoch>), detail_noun="messages", dedup by bare message id. The last_seen_id head-unchanged micro-opt is intentionally dropped as redundant with stop_on_empty_pending. PR is stacked on the now-merged #3873 but not yet rebased (DIRTY).
Actionable
🔴 Blocker — repo policy: all 7 commits carry Co-Authored-By: Claude <noreply@anthropic.com> trailers. This repo forbids AI co-author trailers on commits. Please rebase to drop every Co-Authored-By: Claude trailer before merge. The rebase onto merged #3873 also collapses the inflated +3592/-3044 to the true gmail/orchestrator/e2e delta — do both in one pass.
🟡 Question — last_seen_id drop budget-neutrality (gmail/source.rs doc block; orchestrator stop logic). The claim is sound for a quiet inbox: both old head_unchanged and new stop_on_empty_pending fetch exactly one page then stop. One behavioral nuance to confirm intentional: the old path broke before ingest on a head-id match; the new path stops after select_pending/ingest-attempt of page 0. On a newest-first inbox where the head is already-synced, any new mail sorts above the head, so the dedup'd-empty signal is equivalent — but please confirm Gmail's result ordering is reliably newest-first (no from:me/SENT interleaving that could place a new item below an already-synced head). If ordering can interleave, the dropped opt and the new stop are not strictly equivalent.
🟡 P2 — test coverage gap: the new e2e (gmail_sync_depth_days_injects_after_floor_into_query) and the migrated cap e2e prove depth + cap on the shared path, and a generic FakeSource unit test covers stop_on_empty_pending. But nothing exercises gmail's own early-stop on an all-already-synced page — the exact behavior replacing the dropped last_seen_id opt. Consider a gmail e2e that pre-seeds synced_ids, returns one all-synced page, and asserts only one GMAIL_FETCH_EMAILS round-trip is issued.
Verified
source.rscallssuper::post_process::post_process(...)directly; gmail'spost_process_action_resultis a thin delegate to the same fn — equivalent to the oldself.post_process_action_resultcall.resolve_account_emailuses["emailAddress","email","profile.emailAddress"], matching the originalfetch_user_profileextraction; not budget-counted (parity).- Both new hooks are defaulted (
page_ceiling→max_pages,stop_on_empty_pending→false); notion/github/linear/clickup inherit defaults and keep prior behavior. Orchestrator unit tests pin both. run_gmail_syncpreserves therecord_sync_successscheduler-heartbeat bump.- Cursor hold-on-failure / cap-truncation and
after:<cursor-epoch>incremental query carry over faithfully.
Requesting changes: the Co-Authored-By: Claude trailers are a merge-blocker per repo policy, and the budget-neutrality of the last_seen_id removal warrants explicit author confirmation re: Gmail result ordering.
6ea8489 to
d722e14
Compare
|
Thanks @oxoxDev — all three addressed (force-pushed 🔴 Co-Authored-By trailers + rebase — done. Rebased onto the now-merged #3873, so the branch is a single clean commit on 🟡 🟡 P2 test gap — added. New e2e Also noted for future PRs: I've recorded that this repo strips/forbids AI co-author trailers, so they won't reappear. |
coderabbitai
Bot
added
memory
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/memory_sync/composio/providers/gmail/source.rs (1)
96-104: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd debug logging for entry/exit and external calls.
Per coding guidelines, new flows should include debug-level logging for entry/exit, branches, and external calls with grep-friendly prefixes. Key methods like
resolve_account_email,run_gmail_sync,preamble,fetch_page, andingestwould benefit fromtracing::debug!statements to aid production troubleshooting.Example pattern matching the existing warn logs:
tracing::debug!( connection_id = ?ctx.connection_id, "[composio:gmail] fetch_page: starting, cursor={:?}", cursor );Also applies to: 111-122, 170-184, 186-249, 263-310
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/memory_sync/composio/providers/gmail/source.rs` around lines 96 - 104, Add debug-level logging to key methods in the Gmail provider source file using tracing::debug! statements. For methods resolve_account_email, run_gmail_sync, preamble, fetch_page, and ingest, add entry/exit logging as well as logging for external calls and branching logic. Include the connection_id from the provided context parameter in log statements and use grep-friendly prefixes like "[composio:gmail]" followed by the method name to aid production troubleshooting, following the pattern shown in the comment example.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/memory_sync/composio/providers/gmail/source.rs`:
- Around line 96-104: Add debug-level logging to key methods in the Gmail
provider source file using tracing::debug! statements. For methods
resolve_account_email, run_gmail_sync, preamble, fetch_page, and ingest, add
entry/exit logging as well as logging for external calls and branching logic.
Include the connection_id from the provided context parameter in log statements
and use grep-friendly prefixes like "[composio:gmail]" followed by the method
name to aid production troubleshooting, following the pattern shown in the
comment example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6725e453-1203-4991-a54d-f89b15c81780
📒 Files selected for processing (5)
src/openhuman/memory_sync/composio/providers/gmail/mod.rssrc/openhuman/memory_sync/composio/providers/gmail/provider.rssrc/openhuman/memory_sync/composio/providers/gmail/source.rssrc/openhuman/memory_sync/composio/providers/orchestrator.rstests/memory_sync_providers_raw_coverage_e2e.rs
d722e14 to
0a3979b
Compare
… (Refs tinyhumansai#3333) GmailProvider::sync is now a one-line delegate; the bespoke loop moves to slim IncrementalSource primitives in gmail/source.rs (provider.rs shrinks ~536 lines). Gmail needed two small, generic, defaulted orchestrator hooks (no impact on the other providers): - page_ceiling(reason, state) -> u32 — a state-aware page ceiling, so gmail keeps its adaptive cap (cap to 2 pages when the previous sync ran <5min ago). - stop_on_empty_pending() -> bool — stop a scope once a fetched page yields no new items, so gmail keeps its all-already-synced early stop. Gmail-specific shapes in the source: - preamble resolves the account email (NOT budget-counted, parity) and stashes it in a OnceLock for ingest to stamp the per-account source_id. - server_side_depth = true: the sync_depth_days window is injected as an `after:<epoch>` query qualifier on the first sync (incremental syncs use `after:<cursor-epoch>`). - batch page ingest via ingest_page_into_memory_tree_with_outcome; dedup by the bare message id (emails are immutable); detail_noun = "messages". - run_gmail_sync wrapper calls record_sync_success on completion (heartbeat). One intentional drop: the last_seen_id "head-unchanged" micro-optimization is redundant with stop_on_empty_pending. Gmail's GMAIL_FETCH_EMAILS query is just `-in:spam -in:trash` (no from:me/SENT interleaving) and Gmail returns results newest-first, so any new mail sorts above an already-synced head — the dedup'd-empty page is the same signal head_unchanged keyed on. On a quiet inbox both fetch exactly one page and stop, so API-budget behavior is unchanged. Tests: orchestrator page_ceiling + stop_on_empty_pending unit tests; gmail dedup-key/server-side-depth/adaptive-ceiling unit tests; existing gmail cap e2e now rides run_sync; new gmail sync_depth_days e2e (asserts the `after:` injection); new gmail_sync_stops_after_an_all_already_synced_page e2e (pre-seeds synced_ids, cursor None so only stop_on_empty can halt, asserts a single GMAIL_FETCH_EMAILS round-trip against a perpetual nextPageToken).
0a3979b to
9ebb6cc
Compare
Summary
GmailProvider::syncis now a one-line delegate; the bespoke loop moves to slimIncrementalSourceprimitives ingmail/source.rs(provider.rs shrinks ~536 lines).page_ceiling(reason, state)(state-aware page ceiling) andstop_on_empty_pending()(stop a scope at an all-already-synced page).Problem
Gmail is the most optimization-laden provider: an adaptive page cap (cap to a couple of pages when it just synced), an all-already-synced early stop, a server-side
after:depth window, an up-front (non-budget-counted) profile fetch for the account email, and batch page ingest. Converting it without those becoming behavior regressions is what made it a deferred case — handled here with two generic hooks plus provider-specific source code.Solution
gmail/source.rs(GmailSource):preambleresolves the account email viaGMAIL_GET_PROFILE(not budget-counted, parity) and stashes it in aOnceLocksoingestcan stamp the per-accountsource_id; returns the single flat scope.server_side_depth() = true:sync_depth_daysis injected asafter:<epoch>on the first sync; incremental syncs useafter:<cursor-epoch>.page_ceiling(reason, state)→ adaptive cap (2 pages when the previous sync ran within 5 min, else the full ceiling).stop_on_empty_pending() = true→ stop once a page's messages are all already-synced (Gmail is newest-first).ingest_page_into_memory_tree_with_outcome; dedup by the bare message id (emails are immutable);detail_noun() = "messages".run_gmail_syncwrapper callsrecord_sync_successon completion (scheduler-heartbeat parity).Orchestrator change (
orchestrator.rs):page_ceiling(reason, state)(default =max_pages()) andstop_on_empty_pending()(defaultfalse). Both are generic concepts, defaulted so notion/github/linear/clickup are unaffected.Intentional behavior drop
The legacy loop's
last_seen_id"head-unchanged" micro-optimization (stop before post-processing page 0 when its first id matches the last sync's freshest id) is redundant withstop_on_empty_pending: on a quiet inbox both fetch exactly one page and stop, so the API-budget behavior is unchanged. It's removed (andlast_seen_idis no longer written) rather than adding a gmail-only orchestrator hook for a no-op-on-budget optimization. No test asserts gmail'sdetails/last_seen_id.Submission Checklist
page_ceiling+stop_on_empty_pendingunit tests (incl. the all-synced control); gmail dedup-key / server-side-depth / adaptive-ceiling unit tests; existing gmail cap e2e now ridesrun_sync; newgmail_sync_depth_days_injects_after_floor_into_querye2e.cargo testforcomposio::providers(470 lib tests) +--test memory_sync_providers_raw_coverage_e2e(16) locally — all green.N/A: behaviour-only refactor.## Related—N/A: behaviour-only refactor.N/A: internal sync refactor.Closes #NNN: increment of the multi-PR Generic Composio sync orchestrator so every toolkit enforces max_items and sync_depth_days #3333 migration, references (not closes) the issue.Impact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check—N/A: no frontend changespnpm typecheck—N/A: no frontend changescargo test --lib -- composio::providers(470 passed) +cargo test --test memory_sync_providers_raw_coverage_e2e -- --test-threads=1(16 passed)cargo fmt --checkclean;cargo check+cargo clippy --libclean on changed filesN/A: no Tauri changesValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
last_seen_idhead-unchanged micro-optimization (API-budget-neutral; covered by stop_on_empty_pending).Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
Refactor
Tests
sync_depth_daysto ensure the correctafter:filtering is applied.