The user does not need to know whether their limit is set via a Group or a User Override, that is something the Admin needs to know. I think we can drop this sentence and the user_override label.

In fact, I'm not sure whether users even need to know what their effective group is 🤔

@ssncferreira We discussed this before, and Agent raised the same concern.

I think there are two reasonable options:

• Keep it as is. I think it's fine—the notification still gives the user useful information about why they were blocked and what action their manager or admin needs to take to unblock them.
• Remove both limit_source and group_name. Showing only the group name can be misleading because it implies the request was blocked by the group budget, which isn't necessarily true.

I'd prefer the first option, but up to you.

Showing only the group name can be misleading because it implies the request was blocked by the group budget, which isn't necessarily true.

I don't quite agree with this. A budget is always associated with a group, either via a group budget or a user override, so a request is always blocked by a group (the only difference is how it is configured).

IMO, I think we can keep the group, but I would prefer removing the user_override label. For me, the sentence This limit is set specifically for your account. is confusing and don't adds value.

A budget is always associated with a group, either via a group budget or a user override, so a request is always blocked by a group (the only difference is how it is configured).

That was my initial approach, but Agent raised this concern: #27346 (comment), and I agreed with it. "Effective group" felt out of place in this notification template.

This limit is set specifically for your account. is confusing and doesn't add value.

I think it still provides value. Imagine the Engineering group has a limit of $2,000, and everyone (users and admins) is aware of it. A user suddenly gets blocked at 1000$ but doesn't know why. This message provides a useful hint that they're likely hitting an individual limit rather than the group limit.

That said, I'm also okay with removing both limit_source and group_name.

This message provides a useful hint that they're likely hitting an individual limit rather than the group limit.

But this is not relevant for the regular user, and the user doesn't need to know whether the budget is set via the group or via the user override (it doesn't even need to know about this difference). For the user, it sees that it is for group X, notifies the admin, and the admin goes to group X and sees the user information (which shows if it is via group budget or user override).

I say, keep the group_name and remove limit_source.

I still agree with Agent's comment:

Effective group: {{.Labels.group_name}} leaks internal resolver jargon and is actively misleading when the limit comes from a per-user override.

That said, I'll change it.

@ssncferreira I reverted to previous version.

Maybe?

• To be honest, I don't like "Effective". It feels like an internal term. Do we expose it anywhere in the UI?
• "AI Governance" is better, but it still implies there are AI Governance-specific groups, which isn't true.

@ssncferreira I reverted the template to its previous version, so this comment is no longer relevant unless you want to change the template text.

We should probably also add the AI period like we do in the UI, for consistency.

I thought about that too, but it could be a bit tricky given the different budget periods.

Do you want to hardcode monthly for this?

Done

nit: for clarity, I would suggest using integer values and the associated USD cost in the comments.

I don't think "associated USD cost" would be clearer. For example:

// pre  = 849_500 ($0.8495, < $0.85)
// post = 850_500 ($0.8505, >= $0.85) -> crosses

I can scale it to something like:

// pre  = 84_950_000 ($84.95, < $85)
// post = 85_050_000 ($85.05, >= $85) -> crosses

But I'm not sure it's worth it.

Actually, I changed my mind—I refactored the test, and I think it looks cleaner now. Let me know what you think.

I think this can probably be merged with the previous test: TestRecordTokenUsageBudgetWarningNotification
For cleaner tests and to avoid bigger tests

make sense, done

This one can also probably be merged with the previous TestRecordTokenUsageBudgetWarningNotification and TestRecordTokenUsageBudgetLimitReachedNotification. This test file is getting quite big...

This test is no longer relevant, so I removed it.

I would prefer to do this on the caller side, just to have all database operations in the same place. Instead of passing tx we would just pass the spend.

Good idea, but my concern is that it doesn't fit well with our best-effort semantics.

One option would be to move this logic into the transaction:

period, err := budget.CurrentPeriod(createdAt, s.budgetPeriod)
if err != nil {
	s.logger.Error(ctx, "compute AI budget period for threshold detection", slog.Error(err))
	return nil
}

spend, err := tx.GetUserAISpendSince(ctx, database.GetUserAISpendSinceParams{
	UserID:           intc.InitiatorID,
	EffectiveGroupID: cost.effectiveGroupID.UUID,
	PeriodStart:      period.Start,
})
if err != nil {
	s.logger.Error(
		ctx,
		"get user AI spend for threshold detection",
		slog.F("initiator_id", intc.InitiatorID),
		slog.Error(err),
	)
	return nil
}

However, these early returns would make it harder to extend the transaction with additional operations in the future.

Alternatively, I could log the errors without returning early, but that could leave period and spend undefined or invalid later in the scope, which also doesn't seem ideal.

I can move:

period, err := budget.CurrentPeriod(createdAt, s.budgetPeriod)

outside the transaction, but the issue remains.

detectBudgetThresholdCrossings nicely encapsulates these concerns for us.

I don't think we are testing this 🤔 this would be nice to have in the tests

I wasn't sure this needed a test, but I added one anyway.

It still uses mockdb, but I think that's sufficient.

Obligatory reminder to check migration number before merge!

I just synced with main

This would be simple enough to generalize and move to a util function:

if p[len(p)-1] == 'y' {
  p[len(p)-1] = 'i'
}
return p + 'ly'

I looked in github.com/dustin/go-humanize and it has some English-specific functions, but not what we ware looking for.

To be honest, I don't like this approach:

if p[len(p)-1] == 'y' {
	p[len(p)-1] = 'i'
}
return p + "ly"

It feels a bit hard to read.

I prefer the current approach. If you'd like, I can simplify it to something like this instead:

// Adjective renders the period as the adjective used in user-facing text (e.g. "monthly").
func (p AIBudgetPeriod) Adjective() string {
	switch p {
	case "day":
		return "daily"
	case "week", AIBudgetPeriodMonth, "year":
		return string(p) + "ly"
	default:
		return string(p)
	}
}