Skip to content

Brand transactional auth emails per app - #2439

Open
timmilazzo wants to merge 22 commits into
mainfrom
ai_main_a22984fd7a074c0f90c5
Open

Brand transactional auth emails per app#2439
timmilazzo wants to merge 22 commits into
mainfrom
ai_main_a22984fd7a074c0f90c5

Conversation

@timmilazzo

@timmilazzo timmilazzo commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Signup verification and password reset emails are now branded per app instead of using a generic "Agent Native" sender, subject, and body.

Problem

Every agent-native app sent the same generic "Verify your email for Agent Native" email from noreply@builder.io, with no indication of which specific app the user had signed up for.

Solution

Resolve the app's slug and description from template metadata, then use them to customize the sender address, reply-to, subject, heading, and body copy for both the signup verification and password reset emails. Unknown apps still fall back to the generic "Agent Native" branding and configured EMAIL_FROM sender.

Key Changes

  • Added getAppSlug() and getAppDescription() to app-name.ts, resolving from first-party template metadata (with a slugified fallback for custom APP_NAME overrides).
  • RenderedEmailMessage now includes optional from and replyTo fields, and AGENT_NATIVE_REPLY_TO (hello@agent-native.com) is exported as the shared reply-to.
  • renderVerifySignupEmail builds an "Agent-Native <App>" brand, sets from to <slug>@agent-native.com, and appends a per-app one-line description (competitor names reframed as "replacement") to the email body.
  • renderResetPasswordEmail uses the same per-app branding and sender/reply-to but omits the value-pitch description since it's a security email.
  • better-auth-instance.ts passes the new from/replyTo values through to sendEmail for both verification and reset flows.
  • Added a changeset documenting the per-app email branding patch.

Edit in Builder  Preview


To clone this PR locally use the Github CLI with command gh pr checkout 2439

You can tag me at @BuilderIO for anything you want me to fix or change

@builder-io-integration builder-io-integration Bot changed the title Update from the Builder.io agent Brand transactional auth emails per app Jul 27, 2026
builder-io-integration[bot]

This comment was marked as outdated.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@steve8708

Copy link
Copy Markdown
Contributor

@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable)

…ents

Applying the branded sender unconditionally overrode a self-hosted
deployment's verified EMAIL_FROM, which providers can reject and would break
signup verification and password reset. Route branding through a new
appSender arg that sendEmail only honors when the configured sender is
already on agent-native.com; the same gate covers the reply-to so custom
deployments keep their own support mailbox.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Builder reviewed your changes and found 1 potential issue 🟡

Review Details

Incremental Code Review Summary

The latest commit addresses both previously reported custom-deployment issues: branding is now gated on the configured sender domain, and the branded reply-to is only applied alongside that first-party sender. I verified those fixes and resolved both stale review threads.

The new appSender plumbing and SendGrid coverage are sound, including explicit sender/reply-to precedence and preservation of self-hosted configuration. However, the first-party detection gate appears narrower than the deployment described by this PR: it recognizes only @agent-native.com, while the PR's stated existing first-party sender is noreply@builder.io. If that remains the actual production configuration, the feature is silently disabled for the main first-party deployment and auth emails retain generic branding.

Key Finding

  • 🟡 MEDIUM — Include the current verified first-party sender/domain (or use an explicit first-party deployment identity) in the gate and add a regression test for it, while continuing to exclude arbitrary self-hosted domains.

🧪 Browser testing: Skipped — PR only modifies backend authentication email rendering and delivery, with no user-facing browser UI impact.

Comment thread packages/core/src/server/email.ts Outdated
Comment on lines +147 to +148
const address = parseSendGridFrom(configuredFrom).email.toLowerCase();
if (!address.endsWith(`@${AGENT_NATIVE_SENDER_DOMAIN}`)) return undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Recognize the current first-party sender configuration

resolveAppSender only enables branding when EMAIL_FROM ends in @agent-native.com, but this PR identifies the existing first-party sender as noreply@builder.io. With that production configuration, branded remains undefined and both Better Auth callbacks continue sending the generic configured sender, so the feature is disabled for the first-party deployment it targets. Use an explicit first-party deployment identity or include the currently verified first-party domain, and add coverage for that configuration while still excluding arbitrary self-hosted domains.

Additional Info
New issue found by 1 of 3 incremental review agents; previous sender/reply-to issues were verified fixed and their threads resolved.

Fix in Builder

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not changing this — the premise does not hold up, though the failure mode you describe is a real one worth stating explicitly.

noreply@builder.io comes from the PR description, which is a paraphrase of the reported symptom, not a configured value. It is not the sender configuration anywhere in this repo:

  • grep -rn "noreply@builder\.io" across the repo returns nothing. The only @builder.io strings are unrelated fixtures in templates/mail/server/lib/sender-identity.spec.ts.
  • EMAIL_FROM is a per-deployment secret (synced via scripts/sync-template-netlify-env.ts), not a repo constant, so the PR cannot "identify" it.
  • The value actually configured for this workspace is Agent Native <noreply@agent-native.com>, which ends in @agent-native.com. Under that config resolveAppSender returns branding and both callbacks send as <slug>@agent-native.com — the intended behavior, not a no-op.

On the residual risk: you are right that if some deployment sets EMAIL_FROM to a non-agent-native.com address, branding silently does not apply there. I deliberately did not widen the allowlist on a guess. Adding builder.io would mean sending as <slug>@agent-native.com from an account that may only have verified builder.io — reintroducing exactly the unverified-sender rejection that the 🔴 comment on this PR just flagged. Trading a cosmetic no-op for a hard delivery failure is the wrong direction.

If someone confirms the production EMAIL_FROM really is on builder.io and that agent-native.com is a verified sending domain on that provider account, this becomes a one-line addition to AGENT_NATIVE_SENDER_DOMAIN. That is a config fact I cannot verify from the repo, so I have flagged it for @steve8708 / @tim rather than guessing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@steve8708 asked me to settle this rather than repeat myself, so here is a split verdict: I partly agree, and I fixed the part I agree with in 6f69e4d.

What I still reject: widening the domain gate.

The stated premise is that the first-party sender is noreply@builder.io. That is not a configured value — it is a paraphrase of the reported symptom from the PR description. grep -rn "noreply@builder\.io" returns nothing but unrelated fixtures in templates/mail/.../sender-identity.spec.ts, and EMAIL_FROM is a per-deployment secret, so no PR can "identify" it.

There is also positive evidence the gate is already correct: this workspace has SENDGRID_API_KEY set with EMAIL_FROM="Agent Native <noreply@agent-native.com>" and is sending today. SendGrid rejects unverified senders outright, so agent-native.com is demonstrably a verified sending domain here — exactly the condition resolveAppSender checks. Branding applies; it is not a no-op.

I also considered the obvious alternative of keeping the configured domain and swapping only the local-part (noreply@acme.comclips@acme.com). I rejected it: SendGrid single sender verification authorizes one exact address, not the domain, so that would break delivery for precisely the self-hosted deployments the 🔴 comment on this PR was raised to protect. Trading a cosmetic no-op for a hard delivery failure is the wrong direction.

What I agree with: the suppression should not be silent.

Strip away the incorrect premise and there is a real defect underneath. When the gate does not match, branding vanishes with no signal at all — an operator sees generic senders and has nothing pointing at why. That is the failure mode AGENTS.md calls out directly: a guard that returns a value callers cannot distinguish from success is a bug, and a loud failure beats a plausible-looking normal state.

Fixed by making it observable instead of invisible. resolveAppSender now warns once when appSender is supplied but suppressed, naming the configured address and the domain it needed:

[agent-native:email] Per-app sender branding is off because EMAIL_FROM
(noreply@acme.com) is not on agent-native.com. Transactional email keeps the
configured sender. Expected when self-hosting.

Warn-once so a high-volume signup flow cannot spam logs, and it fires for a missing EMAIL_FROM too. Behavior is otherwise unchanged — still no delivery risk. 13/13 email tests passing, tsc --noEmit clean.

If someone confirms a real deployment sets EMAIL_FROM on builder.io and that agent-native.com is verified on that provider account, the remaining change is one entry in the domain list. Until then that would be a guess, and the warning now makes the condition self-diagnosing rather than silent.

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Here's a visual recap of what changed:

Visual recap

Open the full interactive recap

@builder-io-integration

Copy link
Copy Markdown
Contributor

There was a problem with your request, please try again later. Error id: 8f5e4fae1d5243f296f9211ff5f30f5b

@builder-io-integration

Copy link
Copy Markdown
Contributor

@steve8708 Triage complete. Head 7039562, all CI terminal and green, mergeable: MERGEABLE, no conflicts.

Fixed (1 commit, 7039562)

The two original findings were both valid and shared one root cause: getFromAddress() resolves override || config.from, so the per-app from I added unconditionally beat a deployment-configured EMAIL_FROM.

  • 🔴 Sender override — a self-hosted deployment that verified its own domain would have sent verification and reset mail as an unverified <slug>@agent-native.com and been rejected by the provider, breaking signup outright.
  • 🟡 Reply-to — the hard-coded hello@agent-native.com routed replies away from the deploying teams support mailbox.

Templates no longer emit a raw from/replyTo. They emit appSender: { name, slug, replyTo }, and sendEmail only converts that to a real sender when the configured EMAIL_FROM is already on agent-native.com (resolveAppSender). Everything else is left untouched, and an explicit from/replyTo still wins. Two regression tests added in email.spec.ts (branded on agent-native.com / untouched on a self-hosted domain); file passes 6/6 and tsc --noEmit is clean.

Not fixed (1)

  • 🟡 "first-party sender is noreply@builder.io" — the premise does not hold. That string comes from the PR descriptions paraphrase of the reported symptom, not from any configuration: it appears nowhere in the repo, EMAIL_FROM is a per-deployment secret rather than a repo constant, and the value configured for this workspace is Agent Native <noreply@agent-native.com>, under which the branding does apply. Widening the allowlist to builder.io on a guess would mean sending as agent-native.com from an account that may only have verified builder.io — reintroducing the 🔴 delivery failure above. Full reasoning in the thread.

Needs a human

  • One config fact I cannot verify from the repo: if any production deployment really does set EMAIL_FROM on builder.io, branding silently no-ops there. That is a one-line change to AGENT_NATIVE_SENDER_DOMAIN once someone confirms the actual sender and that agent-native.com is verified on that provider account.
  • mergeStateStatus: BLOCKED is purely REVIEW_REQUIRED (branch protection wants 1 approval). Leaving that and the open thread for a human reviewer — not resolving or merging.

builder-io-integration[bot]

This comment was marked as outdated.

@steve8708

Copy link
Copy Markdown
Contributor

@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable)

APP_NAME flows into the branded From display name, and angle brackets there
made parseSendGridFrom read the address as part of the name, producing a
malformed sender the provider rejects. Strip header-significant characters
before interpolating, and fall back to the bare address when nothing is left.
builder-io-integration[bot]

This comment was marked as outdated.

@builder-io-integration

Copy link
Copy Markdown
Contributor

@steve8708 Second triage pass complete. Head e4dc584, all CI terminal and green (56 success / 27 neutral / 3 skipped, 0 failures), mergeable: MERGEABLE, no conflicts. Quiet 14:26Z → 15:01Z (35 min).

Fixed since last pass (e4dc584)

  • 🟡 Unencoded branded sender display name — valid, and I reproduced it before touching anything. APP_NAME feeds the From display name, and parseSendGridFroms lazy prefix matches the first <, so Agent-Native Acme <Support> <clips@agent-native.com> parsed to email: "Support> <clips@agent-native.com" — a malformed sender the provider rejects. stripCrlf only covered CR/LF. Added sanitizeSenderDisplayName at the single interpolation point in resolveAppSender (strips <>(),;:\" + CR/LF, collapses whitespace, falls back to the bare address if nothing survives), plus a regression test. 7/7 passing, tsc --noEmit clean.

I scoped that as a deliverability bug rather than a security one — APP_NAME is operator config, not attacker-controlled — and separately confirmed the same value cannot break the HTML body, since renderEmail/emailStrong run escapeHtml on every sink. The From header was the only unescaped one.

Also in this branch

AGENT_NATIVE_REPLY_TO is now agent-native@builder.io per @tims request. Note this is unrelated to the still-open sender-domain thread: the branding gate is keyed on EMAIL_FROM being on agent-native.com, and only the reply-to moved to builder.io. Senders remain <app-slug>@agent-native.com.

Still open, needs a human

  • The earlier 🟡 "first-party sender is noreply@builder.io" thread remains rejected on evidence (that string exists nowhere in the repo; EMAIL_FROM is a per-deployment secret, and this workspaces value is on agent-native.com). If someone confirms a production deployment actually sets EMAIL_FROM on builder.io and that agent-native.com is verified on that provider account, it is a one-line change to AGENT_NATIVE_SENDER_DOMAIN.
  • mergeStateStatus: BLOCKED is purely REVIEW_REQUIRED (1 approval). Not approving, merging, or resolving threads.

Full ledger: 4 review threads — 3 fixed and replied, 1 rejected with evidence and replied. No unanswered feedback.

@builder-io-integration

Copy link
Copy Markdown
Contributor

Babysitting complete on PR #2439.

Final state — head e4dc584, all CI terminal and green (56 success / 27 neutral / 3 skipped, 0 failures), mergeable: MERGEABLE, no conflicts. Quiet 14:26Z → 15:01Z (35 min, well past the 20-minute bar).

Fixed this pass (e4dc584)

One new finding, and it was real — I reproduced it before changing anything. APP_NAME flows into the sender display name, and parseSendGridFrom's lazy prefix matches the first <:

"Agent-Native Acme <Support> <clips@agent-native.com>"
  => email: "Support> <clips@agent-native.com"   ← malformed, provider rejects

Fixed with sanitizeSenderDisplayName at the single interpolation point in resolveAppSender, so it protects every appSender caller rather than just the two templates. Added a regression test — 7/7 passing, typecheck clean.

Two judgment calls worth surfacing:

  • I scoped it as a deliverability bug, not security — APP_NAME is operator config, not attacker-controlled input.
  • I checked whether the same value could break the HTML body (it also feeds brandName/heading/paragraphs). It can't — renderEmail and emailStrong escape every sink. The From header was the only unescaped one.

Cumulative ledger — 4 review threads: 3 fixed and replied, 1 rejected with evidence and replied. No feedback left unanswered.

Two things I did not do, deliberately:

  1. The earlier "production sender is noreply@builder.io" claim stays rejected — that string appears nowhere in the repo, and EMAIL_FROM is a per-deployment secret whose value here is on agent-native.com. Widening the domain allowlist on that guess would reintroduce the 🔴 delivery failure this PR just fixed. If someone confirms the real production sender, it's a one-line change.
  2. Merge is BLOCKED solely by REVIEW_REQUIRED (1 approval). I did not approve, merge, or resolve any threads — those are reviewer-owned.

Note the reply-to change you asked for (agent-native@builder.io) is independent of that open thread: only the reply-to moved to builder.io; senders remain <app-slug>@agent-native.com.



🟡 PR is green, mergeable, and fully triaged at e4dc584; blocked only on a human approval and one open thread awaiting a config confirmation.

@steve8708

Copy link
Copy Markdown
Contributor

@builderio-bot resolve hte merge conflict plz

…a22984fd7a074c0f90c5

# Conflicts:
#	packages/core/src/server/email.ts
@builder-io-integration

Copy link
Copy Markdown
Contributor

Conflict resolved and pushed as 241c8af. mergeable: MERGEABLE — no conflicts remain.

What the conflict actually was

Not a trivial textual clash. While this PR was open, main independently shipped a solution to the same problem I'd just fixed — the sender display name. Main added fromName + withDisplayName(), which swaps the display name while keeping the verified address, sanitizing and quoting it.

That's a better-scoped fix than mine, so I preserved main's version and deleted my sanitizeSenderDisplayName rather than keeping two competing sanitizers.

How I resolved it

Took main's side wholesale, then re-added only the piece main doesn't cover — the per-app address swap (clips@agent-native.com), which fromName alone can't do:

const branded = resolveAppSender(config.from, args.appSender);
const from =
  branded && !args.from
    ? withDisplayName(branded.address, args.fromName ?? branded.name)
    : getFromAddress(config, args.from, args.fromName);

resolveAppSender now returns a bare address and lets main's withDisplayName do all naming, so there's one sanitizer, not two. The args.fromName ?? branded.name ordering matters — routing the branded address through getFromAddress as an override would have silently discarded an explicit fromName, since main's function early-returns on override.

Verification: diff of the resolved file against main's version shows only my additions — nothing from main was lost (withDisplayName, defaultFromAddress, the 3-arg getFromAddress, and fromName all intact). Typecheck clean; 144 tests pass across email, credential-provider, and core-routes-plugin specs, including main's 8 new email tests.

One test expectation changed: my malformed-name regression test now asserts main's quoted output (Agent-Native Acme Support , Inc.) instead of my stripped version. The assertion that matters — email: "clips@agent-native.com" — still passes, confirming the address swap survived and the malformed-sender bug stays fixed.

CI is re-running on the new head. Merge is still BLOCKED solely by REVIEW_REQUIRED (1 approval) — I haven't approved or merged.



🟢 Merge conflict resolved preserving main's fromName work; branch is MERGEABLE at 241c8af with typecheck and 144 tests green.

builder-io-integration[bot]

This comment was marked as outdated.

@steve8708

Copy link
Copy Markdown
Contributor

@builderio-bot wdyt of the latest feedback? also resolve the merge conflict

…a22984fd7a074c0f90c5

# Conflicts:
#	packages/core/src/server/email.spec.ts
@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

builder-io-integration[bot]

This comment was marked as outdated.

@builder-io-integration

Copy link
Copy Markdown
Contributor

CI status on 99d071b: one failing job, and I do not believe it is this PR. I need a re-run from someone with permissions — my token gets Resource not accessible by integration on gh run rerun.

Failure: Fast tests lane-1templates/design/app/components/design/bridge/bridge.guard.spec.ts (123 tests, 1 failed, 63s).

Why I am treating it as infra flake rather than a regression:

  1. No code path. My change is confined to packages/core/src/server/email.ts. grep for sendEmail|appSender across templates/design/ returns nothing, and the failing spec contains no reference to email. The only callers of the changed code are the Better Auth verify/reset callbacks.
  2. The failure is a connection race, not an assertion. The log shows ECONNREFUSED 127.0.0.1:7331 and ECONNREFUSED ::1:3000, with the spec running 63s — a timeout, not a failed expectation.
  3. The spec launches a real browser. I ran it locally and confirmed it calls chromium.launch() at bridge.guard.spec.ts:9811, so it depends on Playwright plus a reachable local server. That is the class of test that fails on a startup race.
  4. It flakes elsewhere. Sampling the last 25 CI runs, Fast tests lane-1 also failed on the unrelated branch changes-536 and succeeded on many others — including twice on this branch with the identical email code already in place.

The rest of the run is green: 38 success / 3 neutral / 3 skipped, and Fast tests core — the lane that actually covers my change — passed. Locally packages/core is 14/14 on email.spec.ts with tsc --noEmit clean.

Could someone re-run that job? If it stays red on a re-run I will dig in properly rather than assume flake. I would rather not push a no-op commit purely to re-trigger CI.

Feedback since my last update — both accepted, both fixed in 99d071b:

  • 🟡 Unbounded warning cache and 🟡 tenant sender address in logs. Both were right, and both came from a claim I made without checking: I had asserted EMAIL_FROM was deployment config. resolveSecretDetailed in credential-provider.ts actually resolves it per user, org, and workspace from app_secrets before touching env, so it is tenant data. Logging it copied protected config into shared logs, and keying a Set by it grew without bound in a warm worker.

    Fixed by removing the address from the message entirely, which dissolves both problems and the earlier global-latch objection at once: with no tenant values the notice is byte-identical for every suppressed config, so there is nothing to key on and no information lost by emitting it once per process. Test now asserts two distinct tenant senders produce exactly one warning and that neither domain appears in it.

@builder-io-integration

Copy link
Copy Markdown
Contributor

There was a problem with your request, please try again later. Error id: 7a8c3a7b16d54b518568bab8effb9414

@steve8708

Copy link
Copy Markdown
Contributor

@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable)

builder-io-integration[bot]

This comment was marked as outdated.

@builder-io-integration

Copy link
Copy Markdown
Contributor

Update: Fast tests lane-1 passes on 4ca15343e. Flake confirmed — no code change was needed.

I could not re-run the job directly (both tokens return Resource not accessible by integration on the rerun endpoints), but main merged into this branch cleanly, which gave a legitimate reason to push and re-trigger CI rather than a no-op commit. Every fast-test lane is now green:

Fast tests core:   success
Fast tests lane-1: success   ← previously failed
Fast tests lane-2: success
Fast tests lane-3: success
Fast tests lane-4: success
Fast tests lane-5: success
Fast tests:        success

That matches the diagnosis: bridge.guard.spec.ts launches a real Chromium instance and failed on ECONNREFUSED to 127.0.0.1:7331 / ::1:3000 after 63s — a startup race in an unrelated package, not a regression from this PR.

Full CI on 4ca15343e: 41 success / 3 neutral / 3 skipped, 0 failures. mergeable: MERGEABLE, no conflicts.

Feedback status: nothing new. No new review threads or reviews since 17:25Z. Final ledger across all rounds — 7 threads:

  • 🔴 Sender override replacing a verified EMAIL_FROMfixed (appSender gated to agent-native.com)
  • 🟡 Reply-to routed away from a self-hosted support mailbox — fixed (same gate)
  • 🟡 Unencoded sender display name — fixed (reused mains withDisplayName after the merge, rather than keeping a duplicate sanitizer)
  • 🟡 Missing Resend appSender coverage — fixed (regression test)
  • 🟡 Permanently-latched suppression warning — fixed
  • 🟡 Unbounded warning key cache — fixed
  • 🟡 Tenant sender address written to shared logs — fixed
  • 🟡 "Recognize the current first-party sender configuration" — rejected, still open, with reasoning in the thread

That last one is the only unresolved thread and it needs a human call, not code: it assumes production sends from noreply@builder.io, which appears nowhere in the repo, while this workspace demonstrably sends from agent-native.com via SendGrid. Widening the gate on that assumption would risk sending from an unverified domain — the exact failure the 🔴 above was raised to prevent. If someone confirms the real production EMAIL_FROM, it is a one-line change.

Merge is BLOCKED solely on REVIEW_REQUIRED (1 approval). Not approving, merging, or resolving threads.

@builder-io-integration

Copy link
Copy Markdown
Contributor

There was a problem with your request, please try again later. Error id: 714e03e0e4be4f18afc01c4738fcf5c1

builder-io-integration[bot]

This comment was marked as outdated.

@steve8708

Copy link
Copy Markdown
Contributor

@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable)

builder-io-integration[bot]

This comment was marked as outdated.

The slug fell back to a slugified APP_NAME, so a custom app on an
agent-native.com deployment would send as <its-name>@agent-native.com and
route replies to the Builder mailbox. Only a recognized template name now
qualifies; anything else keeps the configured sender.
builder-io-integration[bot]

This comment was marked as outdated.

@builder-io-integration

Copy link
Copy Markdown
Contributor

There was a problem with your request, please try again later. Error id: cb7d7af8709b48fa86a3218c8a0a1453

@sanyamkamat

Copy link
Copy Markdown
Contributor

@builderio-bot any issues or bugfixes this PR needs before merging?

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@builder-io-integration

Copy link
Copy Markdown
Contributor

Reviewed the post-merge state. One real issue left, now fixed in f82f39fa6; everything else is clean.

Fixed: custom apps were still branded as Agent-Native in the copy.
The earlier fix gated the sender to recognized first-party templates, but the recipient-visible brand string was still Agent-Native ${APP_NAME} unconditionally. A self-hosted app named "Acme Portal" would send "Verify your email for Agent-Native Acme Portal" — telling that deployment's users they signed up for an Agent Native app. The branding decision now flows from the same getAppSlug() gate as the sender, so an unrecognized app renders as plain "Acme Portal". Added a test asserting the subject and that the HTML never contains Agent-Native Acme Portal.

Checked and found nothing to change:

  • Slug resolution actually works end to end — templates/clips/package.json name is clips, which matches the TEMPLATES entry, so branding resolves to clips@agent-native.com rather than silently no-oping.
  • Merge from main is resolved and pushed. main's withDisplayName/fromName work is preserved; app-sender resolution layers on top rather than duplicating the sanitizer.
  • packages/core: 16 tests pass, tsc --noEmit clean.

The one open thread remains the first-party sender-domain gate, which I'm still not widening — this workspace's configured EMAIL_FROM is Agent Native <noreply@agent-native.com>, so agent-native.com is the correct gate and builder.io would be an unverified guess.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Builder reviewed your changes and has a few items to flag 🟡

Review Details

Incremental Code Review Summary

The latest PR head matches the previously reviewed implementation, including the allow-listed first-party app slug resolution and the fixes for custom-app mailbox generation, warning-cache growth, and tenant sender logging. The existing first-party sender-domain issue remains open and unchanged, so it was intentionally not reposted.

The parallel review workers were interrupted before returning findings; no new actionable findings are available from this increment. A summary review is being recorded with no new comments.

🧪 Browser testing: Skipped — PR only modifies backend authentication email rendering and delivery, with no user-facing browser UI impact.

@builder-io-integration

Copy link
Copy Markdown
Contributor

CI update: Fast tests core is red, but not from this PR.

The single failure is src/client/guided-questions.flow.spec.tsx > useGuidedQuestionFlow scoped reads > keeps active questions visible while a DB-sync refresh is pending (1 failed | 741 passed). The second red check, Fast tests, is just the gate job reporting test-core did not succeed.

I verified it's pre-existing rather than a regression: I checked out origin/main in a clean worktree and ran that spec file on its own — it fails there identically (1 failed | 9 passed). There is also no path from the email changes to a guided-questions client hook; this PR touches email.ts, email-templates.ts, app-name.ts, better-auth-instance.ts, and their specs.

Email-specific verification on this branch's head: 16 tests pass across email.spec.ts and email-templates.spec.ts, and tsc --noEmit is clean for packages/core.

That failure needs a fix on main; it shouldn't block this PR beyond the required-check gate.

@builder-io-integration

Copy link
Copy Markdown
Contributor

I found and fixed one real issue, and confirmed the CI red is not from this PR.

Fixed (f82f39fa6): the Agent-Native prefix was applied to any APP_NAME. The earlier review round gated the sender address to recognized first-party templates, but the recipient-visible brand string wasn't — so a self-hosted app named "Acme Portal" would send "Verify your email for Agent-Native Acme Portal". Branding now flows from the same getAppSlug() gate as the sender, with a test asserting the subject stays "Verify your email for Acme Portal".

Also done: merged main into the branch (preserving main's withDisplayName/fromName work), pushed, and verified slug resolution actually works end to end (templates/clips/package.json name clips matches the TEMPLATES entry, so it resolves to clips@agent-native.com rather than silently no-oping). 16 email tests pass, tsc --noEmit clean.

CI: Fast tests core fails on guided-questions.flow.spec.tsx (1 failed / 741 passed). I reproduced that same failure on a clean origin/main worktree, so it's pre-existing and needs a fix on main, not here. The PR is MERGEABLE; the only open review thread is the sender-domain gate, which I'm still declining to widen since this workspace's EMAIL_FROM is noreply@agent-native.com.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants