Skip to content

Email users when accounts are banned#2300

Open
Patrick-Erichsen wants to merge 8 commits into
mainfrom
pe/ban-notification-emails
Open

Email users when accounts are banned#2300
Patrick-Erichsen wants to merge 8 commits into
mainfrom
pe/ban-notification-emails

Conversation

@Patrick-Erichsen
Copy link
Copy Markdown
Contributor

@Patrick-Erichsen Patrick-Erichsen commented May 18, 2026

Summary

  • send best-effort Resend ban notification emails for manual bans and malware autobans
  • send best-effort account-restored emails for manual unbans and autoban remediation
  • include public-safe scanner context, related skill/plugin naming when resolvable, and reply-to guidance
  • enumerate every restored skill/plugin listing in restored-account emails
  • document Resend env setup and moderation email behavior

Review / testing

  • codex review --uncommitted (from origin/main autoreview skill): final pass reported no discrete correctness issues before the latest-main merge
  • bun run test convex/users.test.ts convex/autobanRemediation.test.ts --run
  • bun run format:check -- convex/users.ts convex/users.test.ts convex/autobanRemediation.test.ts docs/moderation.md specs/security-moderation.md
  • bun run lint convex/users.ts convex/users.test.ts convex/autobanRemediation.test.ts
  • bunx tsc --noEmit --pretty false
  • git diff --check

@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 18, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
clawhub Ready Ready Preview, Comment May 19, 2026 1:22am

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 18, 2026

Codex review: needs real behavior proof before merge. Reviewed June 2, 2026, 1:00 AM ET / 05:00 UTC.

Summary
Adds Resend-backed ban and restored-account email notifications, related scheduling in moderation flows, tests, env/deploy docs, and the resend dependency.

Reproducibility: yes. source inspection is enough for the main finding: a manual ban reason that does not match the special cases reaches the raw fallback in formatBanNotificationFindingValue. I did not run the behavior because this is a read-only review.

Review metrics: 2 noteworthy metrics.

  • Changed Files: 10 files changed. The PR spans backend moderation code, tests, docs/specs, env examples, package metadata, and lockfile changes.
  • Direct Dependencies: 1 added. Adding resend creates a new outbound service and supply-chain surface that maintainers should review before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • Replace raw manual ban-reason fallback with public-safe allow-listed copy.
  • Post redacted real Resend, terminal, or log proof for ban and restored-account email dispatch and payloads.
  • Refresh the branch and resolve conflicts while preserving current package ban/unban behavior.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists tests, lint/typecheck, autoreview, and a preview, but no redacted live Resend output, terminal output, logs, or artifact proves after-fix email dispatch; update the PR body with redacted proof so ClawSweeper can re-review automatically, or ask a maintainer to comment @clawsweeper re-review if it does not.

Risk before merge

  • [P1] Outbound ban emails create a security/privacy boundary because unmatched manual ban reasons can still include internal moderator notes, evidence strings, reporter context, or IDs.
  • [P1] The contributor has not provided redacted real Resend, terminal, or log proof showing actual after-fix email dispatch, headers/idempotency, and payload content.
  • [P1] The PR is dirty against current main; conflict resolution must preserve package ban/unban behavior and ban-appeal changes added after the PR base.

Maintainer options:

  1. Sanitize and Prove Before Merge (recommended)
    Replace raw manual ban-reason email fallback with allow-listed public copy, resolve current-main conflicts, and require redacted real Resend/log proof for ban and restore dispatch.
  2. Explicitly Accept Raw Reason Policy
    If maintainers want moderator-entered reasons emailed verbatim, document that security decision and still require redacted proof of the real email payloads.
  3. Pause Until Refreshed
    If the contributor cannot provide proof or rebase cleanly, keep this PR paused rather than merging a stale branch across moderation code.

Next step before merge

  • [P1] Needs contributor proof plus maintainer/security approval of outbound ban-reason copy; automation cannot supply the contributor's real Resend setup proof.

Security
Needs attention: The diff introduces outbound security emails and still permits raw manual ban reasons to cross the moderation boundary.

Review findings

  • [P1] Sanitize manual ban reasons before emailing them — convex/lib/emails.ts:369
  • [P3] Align restored-listing docs with the cap — docs/moderation.md:79-80
Review details

Best possible solution:

Merge only after the email copy is public-safe by construction, the branch is refreshed over current moderation/package behavior, and redacted real Resend or runtime logs prove both ban and restored-account emails.

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

Yes, source inspection is enough for the main finding: a manual ban reason that does not match the special cases reaches the raw fallback in formatBanNotificationFindingValue. I did not run the behavior because this is a read-only review.

Is this the best way to solve the issue?

No, not yet. The feature direction is reasonable, but the safest implementation needs allow-listed public email copy, current-main conflict resolution, and redacted live dispatch proof before merge.

Full review comments:

  • [P1] Sanitize manual ban reasons before emailing them — convex/lib/emails.ts:369
    When source === "manual" and the reason does not match the two special cases above, this returns the moderator-provided reason directly into the user email. Manual ban reasons can contain internal evidence, reporter context, or IDs, so the outbound email should use an allow-listed public summary or a separate public reason field instead of the raw stored reason.
    Confidence: 0.88
  • [P3] Align restored-listing docs with the cap — docs/moderation.md:79-80
    The renderer now caps restored listings at ten and the tests assert listings 11 and 12 are omitted with an overflow count, but the public docs still promise every restored listing. Please update the docs/spec wording to describe the cap and summary, or remove the cap if every listing is required.
    Confidence: 0.93

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against dcbc38999f1a.

Label changes

Label changes:

  • add merge-risk: 🚨 compatibility: The PR changes ban/unban functions that current main has since extended for package resources, and the dirty branch must preserve those behaviors during conflict resolution.

Label justifications:

  • P2: This is a normal-priority moderation improvement with a concrete security/privacy blocker but limited blast radius.
  • merge-risk: 🚨 security-boundary: Merging as-is could expose internal manual moderation reasons through outbound user emails.
  • merge-risk: 🚨 compatibility: The PR changes ban/unban functions that current main has since extended for package resources, and the dirty branch must preserve those behaviors during conflict resolution.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists tests, lint/typecheck, autoreview, and a preview, but no redacted live Resend output, terminal output, logs, or artifact proves after-fix email dispatch; update the PR body with redacted proof so ClawSweeper can re-review automatically, or ask a maintainer to comment @clawsweeper re-review if it does not.
Evidence reviewed

Security concerns:

  • [high] Raw ban reasons can leave the moderation boundary — convex/lib/emails.ts:369
    The manual-ban fallback emails the raw normalized reason for any unmatched string, which can expose moderator notes or internal evidence to the banned user.
    Confidence: 0.88

Acceptance criteria:

  • [P1] bun run test convex/users.test.ts convex/autobanRemediation.test.ts --run.
  • [P1] bun run format:check -- convex/users.ts convex/users.test.ts convex/autobanRemediation.test.ts docs/moderation.md specs/security-moderation.md.
  • [P1] bun run lint convex/users.ts convex/users.test.ts convex/autobanRemediation.test.ts.
  • [P1] bunx tsc --noEmit --pretty false.
  • [P1] Provide redacted live Resend, terminal, or log proof for manual ban, malware autoban, manual unban, and autoban-remediation email dispatch.

What I checked:

  • PR branch adds outbound moderation email path: The PR head adds Resend-backed sendBanNotificationEmail and sendUnbanNotificationEmail helpers with idempotency keys and text/HTML payloads. (convex/lib/emails.ts:64, 8c139603adfe)
  • Manual reason fallback is still raw: For unmatched manual ban reasons, formatBanNotificationFindingValue returns the normalized moderator-provided reason directly into the outbound email body. (convex/lib/emails.ts:369, 8c139603adfe)
  • Current main does not already implement the feature: Current main has existing security contact strings but no Resend, sendBanNotification, or sendUnbanNotification implementation, so the central feature is not already implemented on main. (convex/auth.ts:12, dcbc38999f1a)
  • Latest CLI release does not contain Resend notification code: The v0.18.0 release package.json has no resend dependency and its users module has no ban/unban notification functions. (package.json:73, 875f026a2300)
  • Real behavior proof is absent: The PR body lists tests, lint, typecheck, diff check, autoreview, and a Vercel preview, but no redacted live Resend output, terminal output, logs, or artifact proving actual ban/restoration email dispatch and payloads. (8c139603adfe)
  • Restored listing docs overpromise relative to implementation: Docs say the restored-account notice lists every restored listing, while the implementation and tests cap visible restored listings at ten and summarize the remainder. (docs/moderation.md:79, 8c139603adfe)

Likely related people:

  • Patrick-Erichsen: Current-main history and blame tie Patrick Erichsen to the ban/autoban moderation paths and the PR also proposes the email implementation. (role: recent area contributor; confidence: high; commits: 07fed45f425e, 6814af95dff0, fd0b390ae76f; files: convex/users.ts, convex/users.test.ts, convex/lib/emails.ts)
  • Vyctor H. Brzezowski: Recent current-main commits added package ban/unban handling in the same moderation functions that this PR must now merge with safely. (role: recent adjacent contributor; confidence: high; commits: 87f2b846efce, 18acbc120915, 97023d3123f4; files: convex/users.ts, convex/packages.ts, convex/schema.ts)
  • Shadow: Recent current-main ban appeals service work changed the user ban/unban surface adjacent to this PR's notification behavior. (role: recent adjacent contributor; confidence: medium; commits: a20e2efd6832; files: convex/users.ts, specs/security-moderation.md)
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.

@clawsweeper clawsweeper Bot added 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. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels May 18, 2026
@socket-security
Copy link
Copy Markdown

socket-security Bot commented May 19, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedresend@​6.12.39810010099100

View full report

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 21, 2026

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 1, 2026

This pull request has been automatically marked as stale due to inactivity.
Please update it or it will be closed.

@github-actions github-actions Bot added the stale label Jun 1, 2026
@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. and removed P1 High-priority user-facing bug, regression, or broken workflow. labels Jun 1, 2026
@github-actions github-actions Bot removed the stale label Jun 2, 2026
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant