feat(sources): acquire live Drive attachment bytes into the blob store#3073
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2e0f2bb17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return {"id": doc, DRIVE_LIVE_FETCH_DATA_KEY: data_b64} | ||
| if isinstance(doc, dict): | ||
| updated = dict(doc) | ||
| updated[DRIVE_LIVE_FETCH_DATA_KEY] = data_b64 |
There was a problem hiding this comment.
When a live fetch succeeds, this sidecar remains inside the driveDocument/driveImage object. The typed Gemini parser accepts that object and _drive_media_content_block copies it wholesale into ContentBlock.raw, which parsed_blocks_from_meta then persists as block metadata; every acquired attachment can therefore be duplicated as base64 metadata in the index, up to the 50 MB cap, and exposed through block metadata surfaces instead of only the blob path. Strip DRIVE_LIVE_FETCH_DATA_KEY before constructing blocks/metadata after decoding it into inline_bytes.
Useful? React with 👍 / 👎.
| if isinstance(doc, str): | ||
| return doc or None | ||
| if isinstance(doc, dict): | ||
| for key in ("id", "fileId", "driveId"): |
There was a problem hiding this comment.
For Gemini/Drive attachment records that carry both id and fileId, the existing parser treats id as the provider attachment id and fileId as the actual Drive file id, while DriveSourceClient.download_bytes ultimately calls Drive get_media(fileId=...). This loop tries id first, so those payloads request the attachment handle instead of the Drive file and the new acquisition path logs a failure and leaves an otherwise fetchable attachment unfetched. Prefer fileId over id, and avoid falling back to driveId as a file id.
Useful? React with 👍 / 👎.
| exc, | ||
| ) | ||
| continue | ||
| raw_bytes = _inject_live_drive_attachment_bytes(raw_bytes, drive_client, file_meta) |
There was a problem hiding this comment.
Because this injector is only called in the download branch, any existing Drive cache file takes the cache_path.exists() branch above and is blob-stored unchanged. In archives with cache files from earlier ingests, re-acquiring the source will still parse the old JSON without the sidecar, so Drive-hosted attachments remain unfetched until the user manually deletes the cache. Run cached bytes through the same injection/upgrade path, or invalidate legacy cache entries when the sidecar is absent.
Useful? React with 👍 / 👎.
|
Addressed all 3 Codex findings in 74da2f3:
200 focused tests pass ( Re: |
Problem: Drive-hosted attachment references (driveDocument/driveImage/ driveAudio/driveVideo) were never fetched by the live ingest pipeline. The only code that called DriveSourceClient.download_bytes for an attachment (_apply_drive_attachments + iter_drive_sessions) had zero callers — the real path (iter_drive_raw_data -> generic dispatch parse) never wired it in, and that dead code wrote to attachment.path/local disk rather than inline_bytes, so even if revived it would not reach the existing true-hash blob-publish path. Solution: iter_drive_raw_data already owns the one live DriveSourceAPI client instance per file, inside the iterator scope, before the acquire and parse stages decouple for memory-bounded streaming. A new module (sources/drive/attachment_fetch.py) walks the raw session JSON for Drive-hosted doc/media references *at that point*, downloads each via the same live client, and injects the fetched bytes as base64 under a sidecar key (DRIVE_LIVE_FETCH_DATA_KEY). attachment_from_doc (the existing chunk parser) decodes that sidecar into ParsedAttachment.inline_bytes with no other parser changes, so the already-shipped publish-then-write path (ingest_batch/_core.py) picks it up as a real content-addressed blob with a true SHA-256 and acquisition_status='acquired' — zero downstream changes needed. Fetch failures/oversize files leave the attachment honestly unfetched, never fabricating a hash. Removed the dead _apply_drive_attachments/ iter_drive_sessions path they replace (zero live callers). Scope: this lands only the Drive sub-case of polylogue-83u.2. The export-zip-member and local-path sub-cases have no current producer (no parser ever emits a zip-embedded or local-filesystem attachment) — tracked as a parent-epic note rather than speculative beads. Verification: devtools test tests/unit/sources/test_drive_ops.py tests/unit/sources/test_drive_attachment_fetch.py tests/unit/storage/test_attachment_acquisition.py tests/unit/sources/test_source_laws.py -> 143 passed. devtools verify --quick -> exit 0 (ruff format/check, mypy --strict, render all --check, topology, layering, closure-matrix, manifests, ci-workflows, doc-commands, docs-coverage, test-infra-currency, test-clock-hygiene, pytest-timeout-overrides, degrade-loudly all green). devtools test tests/unit/sources/ -> 1705 passed, 2 pre-existing failures in test_chatgpt_normalization_survivors.py confirmed unrelated (same failures reproduce with this change stashed out). Ref polylogue-83u.2 Co-Authored-By: Claude <noreply@anthropic.com>
Drive live attachment byte acquisition (this branch, commit above) is the Drive sub-case of polylogue-83u.2. Export-zip-member and local-path sub-cases have no current producer (no parser has ever emitted an attachment with real zip-member or local-filesystem bytes) -- noted on parent polylogue-83u as inapplicable rather than deferred. Hand-merged the two touched records from a fresh `bd export` rather than copying the whole file: bd resolves its export target independent of invocation cwd, so it also carries unrelated concurrent-session bead activity that doesn't belong on this branch. Co-Authored-By: Claude <noreply@anthropic.com>
Problem: Codex/CodeRabbit review of the Drive live attachment byte acquisition change (previous commit) found three real gaps. - P1: the live-fetch sidecar (DRIVE_LIVE_FETCH_DATA_KEY) survived unstripped into ContentBlock.raw for driveDocument/driveImage/ driveAudio/driveVideo blocks (gemini_message.py's _drive_media_content_block), unlike inlineData/inlineFile/inlineImage which already strip their `data` payload via _without_inline_bytes. Every acquired Drive attachment would duplicate its full base64 bytes into block metadata -- persisted in index.db, which is not content-addressed, up to the 50MB cap. - P2: the live-fetch file-id resolution tried `id` before `fileId` (and fell back to `driveId`, a shared-drive CONTAINER id, never a file id), the reverse of attachment_from_doc's own native-id precedence (#1252: `fileId` is the real Drive file id DriveSourceClient.download_bytes needs; `id` is a provider attachment handle). Payloads carrying both fields would request the wrong handle and silently fail live. - P2: the injector only ran on a fresh download; a cache_path.exists() hit skipped it entirely, so a cache file written before this feature (or where a Drive-hosted attachment failed/was oversize at the time) stayed unfetched forever even on repeated re-ingest. What changed: - _drive_media_content_block strips DRIVE_LIVE_FETCH_DATA_KEY from the record before building ContentBlock.raw, mirroring _without_inline_bytes's existing treatment of inline data. - _doc_file_id now tries `fileId` before `id` and never considers `driveId`. - _inject_live_drive_attachment_bytes now returns (bytes, mutated) and iter_drive_raw_data runs it unconditionally on every read (cache hit or fresh download), refreshing the cache file only when something new was actually resolved. Verification: devtools test tests/unit/sources/test_drive_ops.py tests/unit/sources/test_drive_attachment_fetch.py tests/unit/sources/test_parsers_drive.py tests/unit/storage/test_attachment_acquisition.py tests/unit/sources/test_source_laws.py tests/unit/sources/test_gemini_chunked_prompt_contract.py -> 200 passed (4 new tests covering each finding). devtools verify --quick + public-claims -> exit 0. devtools test tests/unit/sources/ -> 1709 passed (same 2 pre-existing unrelated failures). Co-Authored-By: Claude <noreply@anthropic.com>
74da2f3 to
74ba3f4
Compare
1 participant
Summary
Implements the Drive sub-case of
polylogue-83u.2: live Drive-hosted attachment references (driveDocument(s)/driveImage/driveAudio/driveVideo) are now fetched during ingest and land in the content-addressed blob store with a true SHA-256, instead of never being acquired.Problem
A 2026-07-08 investigation (recorded on the bead) found the only code that ever called
DriveSourceClient.download_bytesfor an attachment —_apply_drive_attachments+iter_drive_sessions— had zero live callers. The real ingest path (iter_drive_raw_data→ generic dispatch parse) never wired attachment acquisition in at all. Even if that dead code had been revived, it wrote bytes toattachment.path/local disk rather thanParsedAttachment.inline_bytes, so it would not have reached the existing true-hash blob-publish path anyway (schema v13'sacquisition_status).Solution
iter_drive_raw_data(polylogue/sources/drive/__init__.py) already owns the one liveDriveSourceAPIclient instance per file, inside the iterator scope, before the acquire and parse stages decouple for memory-bounded streaming. A new module,polylogue/sources/drive/attachment_fetch.py, walks the raw session JSON for Drive-hosted doc/media references at that exact point, downloads each via the same live client, and injects the fetched bytes as base64 under a sidecar key (DRIVE_LIVE_FETCH_DATA_KEY, defined inpolylogue/sources/parsers/drive_support_attachments.py). The existing chunk parser (attachment_from_doc) decodes that sidecar intoParsedAttachment.inline_bytes— no other parser changes — so the already-shipped publish-then-write path (ingest_batch/_core.py) picks it up as a real content-addressed blob withacquisition_status='acquired', with zero downstream changes needed.Fetch failures and oversize files (50MB cap) leave the attachment honestly
upload_origin="drive"with noinline_bytes— never a fabricated hash.Removed
_apply_drive_attachments/iter_drive_sessions(and the now-deadsources/__init__.pyre-export) — the obsolete, zero-caller path this change replaces, per surgical-renewal doctrine.Non-goals (explicitly out of scope)
polylogue-83u.2also named export-zip-member and local-path attachment sub-cases. Both were re-confirmed to have zero current producer — no parser has ever emitted an attachment whose bytes live as a sibling zip member or a real local filesystem path. There is nothing live to un-bypass for either, so this PR does not touch them; noted on parentpolylogue-83urather than filing speculative follow-up beads with no defined producer.Verification
devtools test tests/unit/sources/test_drive_ops.py tests/unit/sources/test_drive_attachment_fetch.py tests/unit/storage/test_attachment_acquisition.py tests/unit/sources/test_source_laws.py→ 143 passeddevtools verify --quick→ exit 0 (ruff format/check, mypy --strict, render all --check, topology, layering, closure-matrix, manifests, ci-workflows, doc-commands, docs-coverage, test-infra-currency, test-clock-hygiene, pytest-timeout-overrides, degrade-loudly)devtools test tests/unit/sources/→ 1705 passed, 2 pre-existing failures intest_chatgpt_normalization_survivors.pyconfirmed unrelated (reproduce identically with this change stashed out)Ref polylogue-83u.2
Co-Authored-By: Claude noreply@anthropic.com