feat(gmail): reference attachments by index (--use-indexed-attachment-ids) - #963
Conversation
![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: 17bed7fbce
ℹ️ 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".
| attachments := collectAttachments(msg.Payload) | ||
| if len(attachments) > 0 { | ||
| payload["attachments"] = attachmentOutputs(attachments) | ||
| payload["attachments"] = attachmentOutputs(attachments, c.UseIndexedAttachmentIDs) |
There was a problem hiding this comment.
When gmail get --json --use-indexed-attachment-ids runs without --sanitize-content, this converts only the convenience attachments array while payload["message"] still serializes every original message.payload...body.attachmentId. The same issue is more severe in thread get and drafts get, where non-sanitized JSON without downloads has no indexed attachment list at all, so enabling the flag still outputs only opaque IDs. Rewrite or omit attachment IDs in the embedded API objects whenever indexed mode is enabled.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func downloadAttachmentOutputs(ctx context.Context, svc *gmail.Service, messageID string, attachments []attachmentInfo, dir string) ([]attachmentDownloadOutput, error) { | ||
| func downloadAttachmentOutputs(ctx context.Context, svc *gmail.Service, messageID string, attachments []attachmentInfo, dir string, useIndexedAttachmentIDs bool) ([]attachmentDownloadOutput, error) { |
There was a problem hiding this comment.
When indexed mode is used with drafts get --download, thread get --download, or thread attachments --download, this function uses the new flag only to format the returned metadata; it still calls downloadAttachment, which constructs the saved filename from the first eight characters of the opaque attachment ID. Consequently the downloaded path does not contain the advertised index and continues exposing the opaque ID, unlike the single-attachment command. Pass an index-aware filename reference into the download path construction.
Useful? React with 👍 / 👎.
17bed7f to
0350e79
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed August 8, 2026, 11:33 PM ET / August 9, 2026, 03:33 UTC. ClawSweeper reviewWhat this changesThis PR adds an opt-in Gmail flag and environment variable that replace attachment IDs with zero-based indexes in attachment listings and downloads. Merge readiness⛔ Blocked until real behavior proof is added - 5 items remain Keep open. The current main branch does not implement this opt-in attachment-index contract, and the current patch resolves the prior output-path findings; it still needs maintainer product direction and inspectable real Gmail proof before merge. Priority: P2 Review scores
Verification
How this fits togetherGmail commands fetch MIME payloads, collect attachment metadata, render it in text or JSON, and pass a selected attachment reference to Gmail’s download API. This PR changes that shared metadata and reference boundary across message, thread, draft, and download commands. flowchart LR
A[Gmail message or thread] --> B[Attachment collection]
B --> C[Indexed-reference option]
C --> D[Text and JSON output]
C --> E[Attachment download lookup]
E --> F[Gmail attachment API]
F --> G[Saved file]
Decision needed
Why: The patch adds a new CLI and environment-backed machine contract rather than repairing an established behavior; source inspection cannot choose its long-term API direction. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Approve the opt-in index contract only if desired, preserve the existing attachment-ID default, and add redacted current-head Gmail evidence showing indexed listing output followed by a successful indexed download. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug reproduction: this is a new opt-in feature. The branch contains focused mocked command coverage, but no inspectable current-head live Gmail transcript or artifact. Is this the best way to solve the issue? Unclear: the implementation consistently keeps the existing default and uses an explicit mode, but maintainers must decide whether this new CLI and environment contract belongs in the product. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1e9a712c91b5. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (22 earlier review cycles; latest 8 shown)
|
0350e79 to
abfd1c6
Compare
clawsweeper
Bot
added
rating: 🧂 unranked krab
… of renumbering them
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
clawsweeper
Bot
removed
the
merge-risk: 🚨 compatibility
|
Landed in Maintainer work before landing:
Proof:
Thanks @ronny-rentner for the compact-reference design and persistent iteration. |
2 participants
Summary
Lets Gmail attachments be referenced by a stable 0-based index instead of the long, opaque
attachmentId, behind a single opt-in flag.--use-indexed-attachment-ids(envGOG_GMAIL_USE_INDEXED_ATTACHMENT_IDS) is added to the six commands that render or accept an attachment id:messages search,get,thread get,thread attachments,drafts get, andattachment. When it's on:attachmentIndex(a number) in place ofattachmentId(a ~200-char opaque string), in both JSON and text. The long id is not emitted at all.gmail attachment <messageId> <index>takes the 0-based index; the index resolves to the real attachment (the real id still drives the actual download API call). A non-numeric argument is rejected in this mode.<messageId>_<index>_...).Default behaviour is unchanged: without the flag the argument and the output are the real
attachmentId, and a numeric argument is treated as a real id, not an index (the index and the id are never silently mixed).Motivation
The
attachmentIdis a ~200-char opaque base64url string, so emitting one per attachment in a listing — and passing it back to download — is token-expensive for an agent. A 0-based position is 1–2 characters and is a stable reference: a message's MIME structure is fixed, so the Nth attachment is the same across calls, andgmail attachmentre-resolves the index to the currentattachmentIdat download time. With the flag on, an agent lists attachments and downloads them purely by index and never handles a long id.User-facing changes
--use-indexed-attachment-ids(envGOG_GMAIL_USE_INDEXED_ATTACHMENT_IDS) on the six attachment-handling gmail commands.attachmentIndex(number) instead ofattachmentId;gmail attachmenttakes the index; the saved filename uses the index.Implementation notes
attachmentInfocarries bothAttachmentIDand a 0-basedAttachmentIndex, stamped once when attachments are collected (collectAttachments).attachmentOutputand the download-result summaries) have distinctattachmentId(string, omitempty) andattachmentIndex(number, omitempty) fields; a stringified index is never written into an id field.gmail attachmentargument. The env var applies the setting globally through each command's env-defaulted flag, mirroring gogcli's existing--include-passwords/GOG_ZOOM_INCLUDE_PASSWORDS.Testing
attachmentByIndex(valid / out-of-range / negative); index resolves and downloads; index out of range errors.attachmentIndex(unit +gmail getend-to-end), a non-numeric argument is rejected, the filename uses the index; a numeric argument without the flag is treated as a raw id (download fails); env-var enables the mode.make fmt/make lintclean; command docs regenerated. Verified against a live account.