feat(brand-claims): name the requester in the on-demand claims Slack alert (LLMO-7263) - #3215
Conversation
…alert (LLMO-7263) The alert now includes who triggered the run - "by <Name> (<email>)" - resolved from the caller's IMS profile (trial_email, then preferred_username; profile.email is an IMS GUID last resort), mirroring user-details.js. The lookup is fail-safe: wrapped in try/catch returning 'unknown', and runs only in the best-effort Slack block after the run is already enqueued, so it can never block the run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… available (LLMO-7263) getRequesterLabel now returns null (instead of 'unknown') when no name/email can be resolved, and the Slack alert appends " by <requester>" only when present - so the message never reads "by unknown". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR will trigger a minor release when merged. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
MysticatBot
left a comment
There was a problem hiding this comment.
Hey @akshaymagapu,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - two code paths in the new helper lack test coverage.
Complexity: MEDIUM - small diff; controllers/API-surface signal.
Changes: Adds requester identity (name and email from IMS profile) to the on-demand Brand Claims Slack alert (2 files).
Must fix before merge
- [Important] Two untested code paths in
getRequesterLabel(name-only return and catch-block fallback) match the 4 lines Codecov flagged -src/controllers/llmo/brand-claims.js:192-196(details inline)
Non-blocking (3): minor issues and suggestions
- nit: JSDoc says "Returns 'unknown' when no profile is available" but the function returns
null- stale doc fragment -src/controllers/llmo/brand-claims.js:178 - suggestion: Split the compound test into two
itblocks (email-only fallback and missing-identity are independent scenarios; a failure in the first currently prevents the second from running) -test/controllers/llmo/brand-claims.test.js:414 - nit: IMS profile fields are interpolated into a
mrkdwn-typed Slack block without sanitization; trial users can edit their own display name, which could inject Slack formatting. Low-likelihood defense-in-depth concern -src/controllers/llmo/brand-claims.js:284
There was a problem hiding this comment.
Hey @akshaymagapu,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - two code paths in the new helper lack test coverage.
Complexity: MEDIUM - small diff; controllers/API-surface signal.
Changes: Adds requester identity (name and email from IMS profile) to the on-demand Brand Claims Slack alert (2 files).
Must fix before merge
- [Important] Two untested code paths in
getRequesterLabel(name-only return and catch-block fallback) match the 4 lines Codecov flagged -src/controllers/llmo/brand-claims.js:192-196(details inline)
Non-blocking (3): minor issues and suggestions
- nit: JSDoc says "Returns 'unknown' when no profile is available" but the function returns
null- stale doc fragment -src/controllers/llmo/brand-claims.js:178 - suggestion: Split the compound test into two
itblocks (email-only fallback and missing-identity are independent scenarios; a failure in the first currently prevents the second from running) -test/controllers/llmo/brand-claims.test.js:414 - nit: IMS profile fields are interpolated into a
mrkdwn-typed Slack block without sanitization; trial users can edit their own display name, which could inject Slack formatting. Low-likelihood defense-in-depth concern -src/controllers/llmo/brand-claims.js:284
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 5m 45s | Cost: $4.78 | Commit: 3c3bfe2671911d147b34c54e2852ea0456a7641d
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…tize the label (LLMO-7263) Address MysticatBot review on #3215: - Cover the two untested getRequesterLabel paths Codecov flagged: name-only (no email) and the catch-block fail-safe (getProfile throws) -> omit the "by" clause. - Sanitize the requester label: strip Slack mrkdwn control chars (< > ` |) since trial users control their display name, closing a low-risk formatting-injection vector into the ops channel. - Split the compound requester test into focused it blocks. - Fix the stale "Returns 'unknown'" JSDoc fragment (returns null now). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MysticatBot
left a comment
There was a problem hiding this comment.
Hey @akshaymagapu,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Approve - all prior findings addressed cleanly; no new blocking issues.
Complexity: MEDIUM - small diff; API-surface signal.
Changes: Adds requester identity (name and email from IMS profile) to the on-demand Brand Claims Slack alert, with mrkdwn sanitization and a three-layer fail-safe (2 files).
Non-blocking (2): minor issues and suggestions
- suggestion: The
authInfo.profileobject fallback path (whengetProfileis not a function) is not exercised by any test - a one-linercontext.attributes.authInfo = { profile: { trial_email: 'static@example.com' } }would close it -src/controllers/llmo/brand-claims.js:181 - suggestion: The
given_name/family_nameOIDC claim aliases are never exercised by tests (all tests usefirst_name/last_name) - worth a test to lock down the mapping -src/controllers/llmo/brand-claims.js:189
Previously flagged, now resolved
- Two untested code paths in
getRequesterLabel(name-only return and catch-block fallback) now covered by dedicated tests - Stale JSDoc ("Returns 'unknown'") corrected to document
nullreturn - Compound test split into separate
itblocks for independent failure reporting - Slack mrkdwn control characters now stripped via regex with dedicated test coverage
# [1.797.0](v1.796.1...v1.797.0) (2026-09-07) ### Bug Fixes * add diagnostic logging to edge-live-preview fetch (LLMO-7416) ([#3216](#3216)) ([4279a84](4279a84)) ### Features * **brand-claims:** name the requester in the on-demand claims Slack alert (LLMO-7263) ([#3215](#3215)) ([143d1c4](143d1c4)), closes [#3208](#3208) * **plg-onboard:** waitlist authenticated (login/SSO) sites during onboarding ([#3142](#3142)) ([be909a9](be909a9)), closes [Hi#level](https://github.com/Hi/issues/level) [hi#precision](https://github.com/hi/issues/precision) [hi#precision](https://github.com/hi/issues/precision)
|
🎉 This PR is included in version 1.797.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What
Name the person who triggered an on-demand Brand Claims run in the Slack alert (LLMO-7263). The alert now reads:
:rocket: On-demand Brand Claims requested for *<site>* (<id>) by <Name> (<email>).The requester is resolved from the caller's IMS profile, mirroring
user-details.js:trial_email->preferred_username->email(the trial user's real address istrial_email;profile.emailis an IMS user GUID, used only as a last resort).first_name/given_name+last_name/family_namewhen present.unknown.Fail-safe
The requester lookup can never block the run:
try/catch, a failure only logs a warning).getRequesterLabelitself is wrapped intry/catchreturningunknown.Testing
test/controllers/llmo/brand-claims.test.js- asserts the alert names the requester (name+email), thepreferred_usernamefallback, andunknownwhen no profile is present. Existing paths unchanged.Notes
Split out from #3208 (prerequisite audits) to keep that PR standalone; both are LLMO-7263. No dependency between them.
🤖 Generated with Claude Code
Change Management