Skip to content

feat(gmail): add bounded inline attachment output#919

Merged
steipete merged 6 commits into
openclaw:mainfrom
chrischall:claude/gmail-attachment-inline
Jul 16, 2026
Merged

feat(gmail): add bounded inline attachment output#919
steipete merged 6 commits into
openclaw:mainfrom
chrischall:claude/gmail-attachment-inline

Conversation

@chrischall

@chrischall chrischall commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add gog gmail attachment --inline for base64 content in JSON and plain output.
  • Keep writing the attachment to the requested path; attachments above 3 MiB remain path-only with an explanatory reason.
  • Return best-effort filename and MIME type metadata without changing the default output shape.
  • Narrow the original proposal to transport only: PDF/text parsing, its dependency, and its subprocess boundary are removed.

Safety and compatibility

  • Inline mode always fetches provider bytes instead of trusting the size-only local cache.
  • Base64 is generated from the fetched bytes retained in memory, not by reopening the caller-controlled destination path.
  • Default cache validation still requires an exact attachment ID.
  • Sender-controlled metadata is quoted when it contains terminal control characters.
  • No new dependency; default {path, cached, bytes} JSON remains unchanged.

Proof

  • make ci: lint, deadcode, all Go and Node tests, generated command docs, docs coverage, and agent-skills checks pass.
  • Structured final-tree review: clean.
  • Signed exact-head live Gmail validation on 6401d196bfb24c0225e4fdb1d6ae4eb3dacef201: help, credential-free dry-run, small inline round-trip, unchanged default shape, invalid-ID cache rejection, same-size stale-file refresh, and oversized fallback all pass. Temporary drafts and files were deleted; post-check found zero drafts.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 13, 2026
@clawsweeper

clawsweeper Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 14, 2026, 10:32 AM ET / 14:32 UTC.

Summary
Adds --inline base64 output and --text extraction to Gmail attachment downloads, with metadata fallback, bounded subprocess-isolated PDF parsing, tests, docs, and a new PDF dependency.

Reproducibility: not applicable. This PR fills a remote-client capability gap rather than fixing a violation of an existing documented contract. Current main’s path-only response clearly establishes the motivation.

Review metrics: 2 noteworthy metrics.

  • Patch surface: 12 files, +819/-15. A two-flag Gmail feature adds a dependency, hidden command, subprocess boundary, generated docs, and substantial tests.
  • Validation surface: 12 command scenarios plus subprocess tests. Coverage spans compatibility, content modes, metadata fallback, parse failures, bounds, and hard timeout termination.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] Merging --text makes attacker-controlled PDF parsing and a new third-party parser part of the supported core Gmail/MCP trust boundary, despite the patch’s strong isolation and resource bounds.
  • [P1] The hidden child-process design becomes supported cross-platform and packaged-binary behavior; maintainers should explicitly own its interaction with safety profiles and long-lived MCP deployments.

Maintainer options:

  1. Narrow to inline transport (recommended)
    Remove --text, the PDF dependency, and the hidden extraction command, then retain the proven bounded base64 response path.
  2. Own the parser boundary
    Keep both modes after explicit maintainer acceptance of untrusted PDF parsing and confirmation that packaged, safety-profile, and cross-platform subprocess behavior is supported.
  3. Pause the combined feature
    Leave the PR open or close it if attachment parsing is not a core boundary the project wants to maintain.

Next step before merge

  • [P2] A maintainer should choose inline-only versus accepting core text/PDF parsing; no remaining narrow mechanical defect is suitable for the repair lane.

Maintainer decision needed

  • Question: Should core gog gmail attachment support PDF/HTML/plain-text extraction, or should this PR be narrowed to the bounded --inline transport mode?
  • Rationale: The patch is correct and proven, but accepting an untrusted PDF parser and hidden subprocess as permanent core behavior is a product and security-boundary choice that review automation cannot authorize.
  • Likely owner: steipete — Release ownership and repository-wide dependency/security-boundary authority make this the best available decision owner.
  • Options:
    • Land inline only (recommended): Keep the bounded base64 transport capability and remove the parser dependency, hidden child command, and text-extraction surface from this PR.
    • Accept text extraction: Merge both modes and explicitly own parser updates, subprocess behavior, safety-profile compatibility, and ongoing untrusted-attachment hardening in core.

Security
Cleared: No concrete remaining exploit or supply-chain defect was found after the pinned module checksums, input/output caps, panic recovery, timeout, killable subprocess, and untrusted-output integration were inspected.

Review details

Best possible solution:

Land the bounded --inline mode as the narrow remote-client fix, and include --text only if maintainers explicitly choose to support attachment parsing in core with the dependency, subprocess, safety-profile, and untrusted-content obligations that entails.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR fills a remote-client capability gap rather than fixing a violation of an existing documented contract. Current main’s path-only response clearly establishes the motivation.

Is this the best way to solve the issue?

Unclear: bounded inline bytes are the narrowest maintainable solution to the remote-path problem, while built-in text extraction is a larger optional parsing surface whose value depends on maintainer product and security intent.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against e4239f8c57a5.

Label changes

Label justifications:

  • P2: This is a well-proven optional Gmail/MCP improvement with limited blast radius but a real maintainer scope decision.
  • merge-risk: 🚨 security-boundary: Merging --text makes untrusted attachment parsing and a new PDF library part of the core CLI security boundary.
  • merge-risk: 🚨 availability: Malformed attachments exercise a parser subprocess in long-lived MCP workflows, so resource exhaustion and process behavior remain operator-relevant merge risks despite the added bounds.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Redacted live Gmail CLI and gogcli-mcp output demonstrates the changed behavior after the final fix, including exact inline-byte round-trip and subprocess-backed PDF text extraction.
  • proof: sufficient: Contributor real behavior proof is sufficient. Redacted live Gmail CLI and gogcli-mcp output demonstrates the changed behavior after the final fix, including exact inline-byte round-trip and subprocess-backed PDF text extraction.
Evidence reviewed

What I checked:

  • Current main lacks the requested modes: At the reviewed main SHA, GmailAttachmentCmd has only message, attachment, output, and name inputs, and returns only path, cached, and byte count. (internal/cmd/gmail_attachment.go:21, e4239f8c57a5)
  • Default compatibility is preserved: The branch keeps the no-flag response fields unchanged and adds focused coverage asserting the existing JSON result remains path/cached/bytes. (internal/cmd/gmail_attachment.go:27, 1d59b5bf2f4a)
  • Prior output-contract defect is resolved: The branch now Go-quotes tab/newline-bearing plain values so extracted multiline text remains one parseable TSV record, matching the repository stdout policy. (internal/cmd/gmail_attachment.go:45, 5e4cd61c2805)
  • Runaway parser work is resolved: PDF extraction now re-execs the binary under exec.CommandContext, caps input and output, propagates child errors, and has a real subprocess timeout-kill test. (internal/cmd/gmail_attachment_text.go:67, 1d59b5bf2f4a)
  • Untrusted-output integration exists: The existing JSON output wrapper recognizes a text field as untrusted content when --wrap-untrusted is enabled, so the new extracted text uses the established agent-safety path. (internal/outfmt/untrusted.go, e4239f8c57a5)
  • Real behavior proof: The PR body includes redacted live Gmail CLI and MCP transcripts showing unchanged default output, successful text extraction, exact base64 round-trip, stable multiline plain output, and re-verification after subprocess isolation. (1d59b5bf2f4a)

Likely related people:

  • steipete: The available release history shows Peter Steinberger signs and publishes gogcli releases, making him the strongest routing candidate for accepting a new core parser dependency and trust boundary. (role: release and product-boundary owner; confidence: high; commits: 836378754862; files: go.mod, internal/cmd/gmail.go)
  • salmonumbrella: Repository release history credits substantial Gmail attachment path, cache, and cancellation hardening in the same command area. (role: recent Gmail attachment area contributor; confidence: medium; files: internal/cmd/gmail_attachment.go)
  • zerone0x: Repository release history credits the directory-output and cache-hit behavior that the PR preserves and extends. (role: attachment output behavior contributor; confidence: medium; files: internal/cmd/gmail_attachment.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (5 earlier review cycles)
  • reviewed 2026-07-13T21:33:55.542Z sha e0dd518 :: needs real behavior proof before merge. :: [P2] Preserve parseable plain output for extracted text
  • reviewed 2026-07-13T21:52:45.258Z sha e0dd518 :: needs real behavior proof before merge. :: [P2] Preserve parseable plain output for extracted text
  • reviewed 2026-07-13T22:08:57.415Z sha 5e4cd61 :: found issues before merge. :: [P1] Make the PDF timeout terminate parser work
  • reviewed 2026-07-13T22:29:15.230Z sha 1d59b5b :: needs maintainer review before merge. :: none
  • reviewed 2026-07-13T22:36:31.595Z sha 1d59b5b :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 13, 2026
@chrischall

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review — pushed 5e4cd61 fixing the --text --plain TSV contract (Go-quoted single field + regression test) and bounding PDF extraction with a 10s timeout + adversarial corrupt-PDF fixture; the PR body now includes redacted live CLI and MCP transcripts for default/--inline/--text and the fixed --plain output.

@clawsweeper

clawsweeper Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 13, 2026
@chrischall

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review — 1d59b5b replaces the goroutine timeout with a killable subprocess boundary: PDF parsing re-execs gog as a hidden gmail pdf-extract child under exec.CommandContext, so timeout = SIGKILL (no lingering parser work in long-lived MCP servers). Child re-checks the 3 MiB input cap, parent caps extracted text at 4 MiB. Tests spawn the real subprocess including a hung-child kill test; PR body has details and live re-verification.

@clawsweeper

clawsweeper Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 13, 2026
chrischall and others added 5 commits July 16, 2026 00:27
gog gmail attachment previously only wrote the attachment to a local file
and returned the path — a dead end for remote callers (MCP clients) that
cannot read the host filesystem.

- --inline: returns the content base64-encoded in a contentBase64 field
  (JSON) for attachments up to 3 MiB; larger ones keep the path-only
  behavior with an explanatory `reason` instead of failing silently.
- --text: returns extracted text in a `text` field — PDFs via a pure-Go
  text extractor (with a `note` when the PDF is scanned/image-only),
  HTML with tags stripped, plain text verbatim. Unsupported types return
  a clear `reason`. Mutually exclusive with --inline.
- Both modes resolve filename/mimeType from the message payload, with a
  single-attachment fallback (Gmail attachment IDs are not stable across
  API responses) and content sniffing when metadata is unavailable.
- Default behavior (no new flags) is unchanged: {path, cached, bytes}.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address review on the --inline/--text attachment modes:

- --plain output stays a stable one-record-per-line TSV: values containing
  tabs/newlines (extracted text) are emitted as a single Go-quoted field,
  with a regression test that round-trips multiline text via strconv.Unquote.
- PDF parsing of attacker-controlled attachments is now bounded: a 10s
  timeout (on top of the existing 3 MiB input cap and panic recovery) so a
  pathological PDF cannot hang the command or a long-lived MCP server.
- Adversarial fixture test: corrupt PDF input surfaces a clear reason
  instead of text, without panicking.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address the remaining review P1: a goroutine timeout cannot terminate
parser work, so repeated hostile PDFs could leave CPU/memory work behind
in a long-lived MCP server.

- gmail attachment --text now re-execs the gog binary as a hidden
  `gmail pdf-extract` child (PDF bytes on stdin, text on stdout) under
  exec.CommandContext, so the 10s timeout SIGKILLs the parser outright.
- Child re-checks the input size cap; parent caps extracted-text output
  at 4 MiB (compressed streams can inflate past the input cap) and
  surfaces child stderr as the failure reason.
- Tests spawn the real subprocess boundary (test binary dispatches via
  a TestMain hook): extraction through the child, a hung child killed
  on timeout, corrupt-PDF error propagation, direct child-command IO,
  and oversized-input rejection.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Chris Hall <chris.c.hall@gmail.com>
@steipete
steipete force-pushed the claude/gmail-attachment-inline branch from 1d59b5b to 6401d19 Compare July 16, 2026 16:21
@steipete steipete changed the title feat(gmail): add --inline and --text content modes to gmail attachment feat(gmail): add bounded inline attachment output Jul 16, 2026
@steipete
steipete merged commit 77200b0 into openclaw:main Jul 16, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants