Skip to content

fix(gmail): enforce per-account no-send guard before dry-run exit#916

Merged
steipete merged 8 commits into
openclaw:mainfrom
veteranbv:fix/no-send-account-dry-run
Jul 13, 2026
Merged

fix(gmail): enforce per-account no-send guard before dry-run exit#916
steipete merged 8 commits into
openclaw:mainfrom
veteranbv:fix/no-send-account-dry-run

Conversation

@veteranbv

@veteranbv veteranbv commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #915.

What Problem This Solves

The per-account no-send guard (gog config no-send set <email>) is enforced only inside each send command's Run(), after auth resolution — and every send path calls dryRunExit before that point. --dry-run, the safe way to probe a guard, reported would gmail.send for an account whose sends are blocked. The --gmail-no-send flag and global gmail_no_send config layers block under --dry-run; the per-account layer silently did not.

Why This Change Was Made

An operator or agent verifying its guardrails with --dry-run is told the guard is absent when it isn't — the exact wrong answer for a safety feature. Real (non-dry-run) sends were never at risk: checkAccountNoSend blocks before any API call on the real path. The failure is the safety signal.

The fix enforces the per-account guard in enforceGmailNoSend, at the same pre-Run() layer as its two siblings, and applies the same pre-dry-run enforcement to the Discovery path (gog api call gmail v1 gmail.users.messages.send / ...drafts.send). Per Codex review follow-ups: aliases resolve correctly (a config-backed store resolver is installed before the enforcement hooks), and account resolution is skipped entirely when no per-account guards exist, so a plain dry-run never reads the keyring. Post-auth checkAccountNoSend remains as defense-in-depth everywhere.

User Impact

  • --dry-run on any Gmail send path (first-class or Discovery) now reports the per-account block instead of would gmail.send / would api.call.
  • No new flags; no behavior change for accounts without guards, non-send commands, or the existing flag/global layers.
  • A plain dry-run with no guards configured no longer resolves an account at all (no keyring inference).

Evidence

Baseline gog-main built from upstream main @ 5800e0ef; gog-pr built from this PR @ 0fe4595bdf16508e8eae99295757b706af996f06. All runs fully isolated: env -i with throwaway HOME, mktemp config dirs, synthetic accounts only, no credentials. Guard config:

{"account_aliases":{"work":"[email protected]"},"no_send_accounts":{"[email protected]":true}}

before/after: guarded dry-run leaks on main, blocks on PR

Claims proved live (24/24 checks):

  • All five first-class send paths (send, drafts send, forward, reply, autoreply) and the Discovery gmail.users.messages.send call report would ... on main for a guarded account under --dry-run, and Gmail sending is blocked for [email protected] (config no-send) on this PR.
  • Alias form (--account work[email protected]) blocked on this PR.
  • No over-blocking: non-guarded account dry-runs normally; drafts create for the guarded account dry-runs normally; empty-guards config with no --account dry-runs normally — all identical on both binaries.
  • Existing layers unchanged: --gmail-no-send and global gmail_no_send block identically on both binaries.
Full result matrix (24 cases, JSON)
{"case":"guarded-email send --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.send","pass":true}
{"case":"guarded-alias send --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.send","pass":true}
{"case":"guarded drafts send --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.drafts.send","pass":true}
{"case":"guarded forward --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.forward","pass":true}
{"case":"guarded reply --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.reply","pass":true}
{"case":"guarded autoreply --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.autoreply","pass":true}
{"case":"guarded discovery messages.send --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would api.call","pass":true}
{"case":"other-account send --dry-run (allowed)","binary":"gog-main","expect_contains":"Dry run: would gmail.send","first_line":"Dry run: would gmail.send","pass":true}
{"case":"guarded drafts create --dry-run (allowed)","binary":"gog-main","expect_contains":"Dry run: would gmail.drafts.create","first_line":"Dry run: would gmail.drafts.create","pass":true}
{"case":"no-guards no-account send --dry-run (allowed)","binary":"gog-main","expect_contains":"Dry run: would gmail.send","first_line":"Dry run: would gmail.send","pass":true}
{"case":"flag --gmail-no-send --dry-run (blocked)","binary":"gog-main","expect_contains":"blocked by --gmail-no-send","first_line":"Gmail sending is blocked by --gmail-no-send","pass":true}
{"case":"global gmail_no_send --dry-run (blocked)","binary":"gog-main","expect_contains":"blocked by config gmail_no_send","first_line":"Gmail sending is blocked by config gmail_no_send","pass":true}
{"case":"guarded-email send --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true}
{"case":"guarded-alias send --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true}
{"case":"guarded drafts send --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true}
{"case":"guarded forward --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true}
{"case":"guarded reply --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true}
{"case":"guarded autoreply --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true}
{"case":"guarded discovery messages.send --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true}
{"case":"other-account send --dry-run (allowed)","binary":"gog-pr","expect_contains":"Dry run: would gmail.send","first_line":"Dry run: would gmail.send","pass":true}
{"case":"guarded drafts create --dry-run (allowed)","binary":"gog-pr","expect_contains":"Dry run: would gmail.drafts.create","first_line":"Dry run: would gmail.drafts.create","pass":true}
{"case":"no-guards no-account send --dry-run (allowed)","binary":"gog-pr","expect_contains":"Dry run: would gmail.send","first_line":"Dry run: would gmail.send","pass":true}
{"case":"flag --gmail-no-send --dry-run (blocked)","binary":"gog-pr","expect_contains":"blocked by --gmail-no-send","first_line":"Gmail sending is blocked by --gmail-no-send","pass":true}
{"case":"global gmail_no_send --dry-run (blocked)","binary":"gog-pr","expect_contains":"blocked by config gmail_no_send","first_line":"Gmail sending is blocked by config gmail_no_send","pass":true}

Scope note: the live matrix intentionally exercises only dry-run and pre-auth paths (no credentialed runs, nothing sent). The real-send guard behavior is unchanged and covered by the existing and new unit tests.

Hygiene, pinned to 0fe4595b:

  • git diff --check origin/main...HEAD — clean
  • make fmt — zero files changed
  • make lint — pass
  • Focused regression: go test ./internal/cmd/ -run 'NoSend|DiscoveryGmailSend|GmailSendDryRun' -v — 10 passed, 0 failed
  • Full package: go test ./internal/cmd/ — pass; upstream CI green across all seven checks

Test plan

  • TestConfigNoSendAccountBlocksBeforeDryRun — guarded send paths + alias blocked under --dry-run (fails on unpatched main, passes with fix)
  • TestConfigNoSendAccountDoesNotOverblockDryRun — other accounts and non-send commands unaffected
  • TestGmailSendDryRunWithoutGuardsSkipsAccountResolution — no guards → no account resolution on dry-run
  • TestDiscoveryGmailSendHonorsPerAccountNoSend — Discovery messages.send/drafts.send blocked per-account; non-send methods and other accounts unaffected
  • TestDiscoveryGmailSendHonorsNoSendFlag — updated for the parameter-less guard signature
  • make fmt / make lint / make test — clean

User-facing change: --dry-run on a send command now reports the per-account block instead of Dry run: would gmail.send. No new flags. CHANGELOG and docs/spec.md updated.

The per-account no-send guard was only checked post-auth inside each
send command's Run(), which --dry-run exits before reaching. A dry run
against a guarded account reported 'would gmail.send' while the flag
and global-config layers both block under the same conditions.

Enforce the per-account guard in enforceGmailNoSend alongside the flag
and global config key. The account resolves without auth via
requireAccount; when no account is resolvable the command keeps
ownership of that failure mode, and the post-auth checkAccountNoSend
call remains as defense-in-depth for real sends.

Fixes openclaw#915

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 69f31a012a

ℹ️ 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".

Comment thread internal/cmd/gmail_no_send.go
The early no-send hook ran before Execute installed
cli.configStoreResolver, so alias resolution inside requireAccount
returned an internal resolver error and the per-account guard was
skipped for --account <alias>. Install a config-backed resolver before
the enforcement hooks; the context-backed resolver installed later is
equivalent (both reduce to configureRuntimeConfig + runtime.Config).

Flagged by Codex review on openclaw#916.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef0e13566b

ℹ️ 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".

Comment thread internal/cmd/gmail_no_send.go
Comment thread docs/spec.md Outdated
…ery sends

Two follow-ups from Codex review:

- The pre-Run per-account check called requireAccount even when
  no_send_accounts was empty, so a plain dry-run could read the keyring
  through default-account inference with no guard to evaluate. Skip
  account resolution entirely unless a per-account guard exists.

- The Discovery path (gog api call gmail v1 ...messages.send/drafts.send)
  ran dryRunExit before checkDiscoveryGmailNoSend, so the same dry-run
  blind spot existed there. Hoist the guard above the dry-run exit with
  the same skip and error-tolerance rules, and keep a post-auth
  checkAccountNoSend for the resolved account, mirroring the
  first-class send commands.
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. labels Jul 12, 2026
@clawsweeper

clawsweeper Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 13, 2026, 3:20 AM ET / 07:20 UTC.

Summary
The PR enforces per-account Gmail no-send policy before dry-run exits for first-class and Discovery send paths, retains post-auth defense-in-depth checks, and adds focused regression coverage.

Reproducibility: yes. Current main exits through dry-run before the per-account check, and the submitted isolated baseline binary reproduces the incorrect would result across the affected send paths.

Review metrics: 2 noteworthy metrics.

  • Live behavior matrix: 24/24 cases passed. The proof covers guarded and allowed first-class and Discovery dry-run paths, including alias resolution and no-guard keyring avoidance.
  • Patch surface: 6 files, +207/-6. The change remains bounded to guard ordering, early account resolution, regression tests, and a repository-owner-added release note.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #915
Summary: This PR is the focused candidate fix for the linked bug; the earlier no-send PR is historical feature provenance rather than competing canonical work.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster ✨ media proof bonus
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.

Next step before merge

  • [P2] No repair job is needed; normal required checks can gate the exact reviewed head before merge.

Security
Cleared: The patch strengthens an existing send-safety boundary and adds no dependency, permission, secret-handling, download, or supply-chain execution changes.

Review details

Best possible solution:

Land the pre-dry-run policy enforcement with its post-auth defense-in-depth checks and focused regression coverage once required checks pass.

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

Yes. Current main exits through dry-run before the per-account check, and the submitted isolated baseline binary reproduces the incorrect would result across the affected send paths.

Is this the best way to solve the issue?

Yes. Hoisting the existing per-account policy to the same pre-Run layer as its global siblings is the narrowest consistent fix, while retaining the resolved-account check after authentication as defense in depth.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: The PR fixes a misleading Gmail safety signal with limited blast radius; real sends were already blocked before API calls.
  • 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 (recording): The before-and-after terminal recording and isolated 24-case live-binary matrix convincingly demonstrate the current failure and corrected guarded and allowed behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The before-and-after terminal recording and isolated 24-case live-binary matrix convincingly demonstrate the current failure and corrected guarded and allowed behavior.
  • proof: 🎥 video: Contributor real behavior proof includes video or recording evidence. The before-and-after terminal recording and isolated 24-case live-binary matrix convincingly demonstrate the current failure and corrected guarded and allowed behavior.
Evidence reviewed

What I checked:

  • Current-main ordering gap: On the reviewed base, Discovery API calls reach the dry-run exit before account resolution and per-account no-send enforcement, matching the reported false safety signal. (internal/cmd/api.go:118, 5800e0ef0d9f)
  • Pre-dry-run enforcement: The PR evaluates Discovery Gmail send methods before dryRunExit and adds per-account evaluation to the existing pre-Run Gmail guard layer. (internal/cmd/api.go:121, 3b7e8a11eead)
  • Alias and keyring safety: The head installs config-backed alias resolution before enforcement and skips account resolution when no active per-account guard exists, addressing the earlier alias and unnecessary keyring-access findings. (internal/cmd/root.go:167, 3b7e8a11eead)
  • Regression coverage: Focused tests cover guarded email and alias forms, unguarded accounts, non-send commands, inactive-guard keyring avoidance, and Discovery messages.send and drafts.send behavior. (internal/cmd/root_more_test.go:397, 3b7e8a11eead)
  • Real behavior proof: The PR body includes an isolated before-and-after terminal recording and a 24-of-24 live-binary matrix covering first-class sends, Discovery send, aliases, allowed operations, and the existing global guard layers. (0fe4595bdf16)
  • Review continuity: The commit sequence addresses the earlier alias-resolution, no-guard account-inference, Discovery-path, and PR-owned changelog findings; the later release note was added by the repository owner. (CHANGELOG.md:4, d5036364cfe8)

Likely related people:

  • veteranbv: The earlier no-send contribution established this behavior, and they authored the central fix plus the alias, Discovery, and keyring-safety follow-ups. (role: feature owner and prior area contributor; confidence: high; commits: 60b794b4f764, ef0e13566b91, 0fe4595bdf16; files: internal/cmd/gmail_no_send.go, internal/cmd/api.go, internal/cmd/root.go)
  • steipete: Authored the latest head commits refining inactive-guard behavior, adjusting its test, and adding the release-owned changelog entry. (role: merger and recent area contributor; confidence: high; commits: d5036364cfe8, 72fcae10ceb1, 3b7e8a11eead; files: CHANGELOG.md, internal/cmd/gmail_no_send.go, internal/cmd/root_more_test.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-12T17:24:51.681Z sha 0fe4595 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-12T18:38:06.305Z sha 0fe4595 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-12T18:44:24.393Z sha 0fe4595 :: needs changes before merge. :: [P3] Remove the PR-owned changelog entry
  • reviewed 2026-07-12T19:51:32.556Z sha 55ee3a4 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-12T19:57:48.731Z sha 55ee3a4 :: needs maintainer review before merge. :: none

@veteranbv

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

PR body upgraded with live-behavior proof: 24/24-case matrix run against real binaries (baseline main @ 5800e0e vs PR @ 0fe4595) under full isolation, a before/after terminal recording, and hygiene receipts pinned to the head SHA. No code changes since your last review.

@clawsweeper

clawsweeper Bot commented Jul 12, 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: 🎥 video Contributor real behavior proof includes video or recording evidence. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed 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. labels Jul 12, 2026
CHANGELOG.md is release-owned for normal PRs; the user-facing release
context stays in the PR body for the landing workflow to consume.
@veteranbv

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Dropped the PR-owned CHANGELOG entry in 55ee3a4 per the release-note policy — the user-facing release context stays in the PR body for the landing workflow. Only change since the last review; code, tests, and docs/spec are untouched.

@clawsweeper

clawsweeper Bot commented Jul 12, 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: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 12, 2026
@steipete
steipete merged commit e4239f8 into openclaw:main Jul 13, 2026
5 checks passed
@steipete

Copy link
Copy Markdown
Collaborator

Landed in e4239f8c57a5.

Maintainer proof on exact PR head 3b7e8a11eead:

  • make ci passed: format, lint/security, deadcode, full Go/Node tests, docs generation/coverage, and agent-skill checks.
  • Focused no-send/Discovery/dry-run regression tests passed.
  • Source-blind built CLI matrix passed 20/20 across first-class send paths, aliases, Discovery sends, direct/alias/environment account selection, inactive guards, and allowed controls; no mail was sent.
  • go mod verify and govulncheck ./... passed with zero reachable/imported-package vulnerabilities.
  • AutoReview converged clean after fixing inactive boolean-map guard handling.
  • Exact-head hosted checks passed on Ubuntu, macOS, Windows, worker, and Docker build-only.
  • Final merge paths exclude docs/**; no Pages or publication workflow was triggered by the candidate.

Thank you @veteranbv for the report, fix, and thorough initial proof.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. proof: 🎥 video Contributor real behavior proof includes video or recording evidence. 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.

Per-account no-send guard is not enforced under --dry-run

2 participants