fix(api-service): add Redis-backed action tokens for chat card buttons fixes NV-7956#11430
Merged
Merged
Conversation
Short-lived at: tokens fix Telegram callback_data limits and keep long action ids out of platform payloads while preserving raw ids in history.
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a complete action tokenization system for agent-driven button actions. It introduces AgentActionTokenService to mint opaque, cache-backed tokens prefixed with at:, integrates tokenization into outbound message delivery pipelines, and adds token resolution to inbound action handling with binding validation. ChangesAgent Action Token Minting, Delivery, and Resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| if (!resolved) { | ||
| this.logger.warn( | ||
| { agentId, integrationIdentifier: config.integrationIdentifier, actionId }, |
Contributor
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
actionId is logged on token-resolution failures, and for tokenized actions this value is the raw at: bearer token. Because these tokens remain valid for up to 3 days, anyone with log access can replay a leaked token within its binding scope.
Consider redacting actionId here (or logging a one-way hash/short fingerprint) so failed resolutions cannot leak replayable token material.
Reviewed by Cursor Security Reviewer for commit 8fef358. Configure here.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Share opaque-token utilities with email, add typed card walker and resolveForDispatch with full env binding, all-or-nothing mint, and replyOnThread tokenization hook.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.ts (1)
13-32: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
interfacefor these backend object contracts.
AgentActionTokenClaims,AgentActionTokenBinding, andMintedButtonTokenare plain object shapes inapps/api, so they should be declared as interfaces to follow the repo rule. As per coding guidelines,**/*.{ts,tsx}: On the backend: useinterfacefor type definitions; on the frontend: usetypefor type definitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.ts` around lines 13 - 32, Replace the three plain type aliases with interface declarations: change AgentActionTokenClaims, AgentActionTokenBinding, and MintedButtonToken to interfaces (keeping exported status for the ones currently exported), preserve all fields and optional modifiers (e.g., value?: string) and the referenced ButtonElement type on MintedButtonToken; ensure the names remain identical so existing usages of AgentActionTokenClaims, AgentActionTokenBinding, and MintedButtonToken continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.ts`:
- Around line 46-47: isActionToken currently treats any id starting with
AGENT_ACTION_TOKEN_PREFIX as an opaque token; change it to only return true when
the id matches the full minted agent-action token format (i.e., the exact token
shape used to create tokens in this module) so raw action ids like "at:approve"
pass through; update isActionToken (and the same logic block referenced around
lines 115–130) to validate against the canonical token pattern used by
AgentActionToken/AGENT_ACTION_TOKEN_PREFIX and not just a prefix; additionally,
add a validation/rejection at card-construction time for ids that would collide
with the reserved minted token shape to prevent accidental reservation of the
prefix.
---
Outside diff comments:
In
`@apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.ts`:
- Around line 13-32: Replace the three plain type aliases with interface
declarations: change AgentActionTokenClaims, AgentActionTokenBinding, and
MintedButtonToken to interfaces (keeping exported status for the ones currently
exported), preserve all fields and optional modifiers (e.g., value?: string) and
the referenced ButtonElement type on MintedButtonToken; ensure the names remain
identical so existing usages of AgentActionTokenClaims, AgentActionTokenBinding,
and MintedButtonToken continue to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03d34a23-60d2-4f38-9eaa-f507f5aa7b68
📒 Files selected for processing (9)
apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.spec.tsapps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.tsapps/api/src/app/agents/conversation-runtime/action-token/card-callback-button.walker.tsapps/api/src/app/agents/conversation-runtime/action-token/opaque-token.util.spec.tsapps/api/src/app/agents/conversation-runtime/action-token/opaque-token.util.tsapps/api/src/app/agents/conversation-runtime/egress/outbound.gateway.action-token.spec.tsapps/api/src/app/agents/conversation-runtime/egress/outbound.gateway.tsapps/api/src/app/agents/conversation-runtime/ingress/chat-instance.registry.tsapps/api/src/app/agents/email/agent-email-action-token.service.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/app/agents/conversation-runtime/action-token/opaque-token.util.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/src/app/agents/conversation-runtime/ingress/chat-instance.registry.ts
- apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.spec.ts
- apps/api/src/app/agents/conversation-runtime/egress/outbound.gateway.action-token.spec.ts
Contributor
Author
|
@cursoragent review comments and fix the e2e test |
Contributor
|
Addressed the Greptile/CodeRabbit review comments and fixed the failing e2e test. Pushed to E2E fix (
|
| Issue | Fix |
|---|---|
| Misleading test name | Renamed to propagates error when mint fails mid-card after an earlier button was tokenized; uses two long action ids that actually need tokenization |
Binding log missing organizationId |
Added organizationId / tokenOrganizationId to the mismatch warning |
at: prefix collision |
isActionToken now requires full minted shape at: + 22-char base64url body; at:approve passes through on ingress |
| Security (token in logs) | Inbound warnings redact minted tokens as at:[redacted] |
| CodeRabbit interfaces | Switched backend types to interface |
Not changed (already handled or out of scope)
replyOnThreadtokenization — already opt-in viaactionTokenBinding; bridge cards without callback buttons don’t need it- Section-only recursion — left as-is (P2; no failing test)
Unit tests (19) and the Slack e2e case above pass on this branch.
Only mint action tokens when Telegram callback_data would exceed 64 bytes, so Slack keeps raw action_ids. Match the full at:<22-char> minted shape on ingress so ids like at:approve pass through. Add organizationId to binding-mismatch logs, redact tokens in warnings, and rename the mid-card mint failure spec. Co-authored-by: Dima Grossman <dima@grossman.io>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/app/agents/conversation-runtime/ingress/chat-instance.registry.ts (1)
451-458:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact action tokens in the error path too.
The warning branch masks opaque
at:tokens, but this catch block still logs and reports the rawevent.actionId. Those tokens stay valid until TTL expiry, so leaking them to logs/Sentry weakens the protection this PR adds.Proposed fix
} catch (err) { - this.logger.error(err, `[agent:${agentId}] Error handling action ${event.actionId}`); + const actionIdForLogs = this.agentActionTokenService.isActionToken(event.actionId) + ? `${event.actionId.slice(0, 3)}[redacted]` + : event.actionId; + + this.logger.error(err, `[agent:${agentId}] Error handling action ${actionIdForLogs}`); captureAgentException(err, { component: 'chat-instance-registry', operation: 'on-action', agentId, - extra: { actionId: event.actionId }, + extra: { actionId: actionIdForLogs }, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/agents/conversation-runtime/ingress/chat-instance.registry.ts` around lines 451 - 458, The catch block currently logs and reports the raw event.actionId, leaking opaque "at:" tokens; update the catch to mask/redact the action id using the same masking utility used in the warning branch before calling this.logger.error and before passing extra.actionId to captureAgentException (i.e., replace direct event.actionId with the masked/redacted value when invoking this.logger.error and captureAgentException in the chat-instance-registry on-action error path). Ensure you reference the same masking function the warning branch uses so both logs and Sentry receive the redacted token.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@apps/api/src/app/agents/conversation-runtime/ingress/chat-instance.registry.ts`:
- Around line 451-458: The catch block currently logs and reports the raw
event.actionId, leaking opaque "at:" tokens; update the catch to mask/redact the
action id using the same masking utility used in the warning branch before
calling this.logger.error and before passing extra.actionId to
captureAgentException (i.e., replace direct event.actionId with the
masked/redacted value when invoking this.logger.error and captureAgentException
in the chat-instance-registry on-action error path). Ensure you reference the
same masking function the warning branch uses so both logs and Sentry receive
the redacted token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e44782d-d15e-43b6-82d5-60448c621a9d
📒 Files selected for processing (6)
apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.spec.tsapps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.tsapps/api/src/app/agents/conversation-runtime/action-token/card-callback-button.walker.tsapps/api/src/app/agents/conversation-runtime/action-token/opaque-token.util.spec.tsapps/api/src/app/agents/conversation-runtime/action-token/opaque-token.util.tsapps/api/src/app/agents/conversation-runtime/ingress/chat-instance.registry.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/app/agents/conversation-runtime/action-token/opaque-token.util.spec.ts
Reorder usecase options to surface Inbox first and change the default selection to 'inbox'. Add a new TelemetryEvent (USECASE_SELECTED) and call telemetry when the user continues from the selector. Adjust handleContinue navigation flow to ensure correct early return after navigating to inbox.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.ts`:
- Around line 131-140: The tokenization predicate
callbackPayloadNeedsTokenization currently only checks button.id, so update the
check used by forEachCallbackButton (and the predicate in
card-callback-button.walker.ts) to evaluate the combined/encoded callback
payload derived from both button.id and button.value (e.g., treat the button as
CallbackPayloadButton with 'id' and 'value'), and return true when the
serialized/encoded payload would exceed the platform callback-data budget; then
use that predicate before calling this.mintActionToken so long button.value is
tokenized/removed when required (refer to forEachCallbackButton,
callbackPayloadNeedsTokenization, and mintActionToken).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67959bf3-812b-4ac8-af9d-032e050b0ad6
📒 Files selected for processing (8)
apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.spec.tsapps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.tsapps/api/src/app/agents/conversation-runtime/action-token/card-callback-button.walker.tsapps/api/src/app/agents/conversation-runtime/egress/outbound.gateway.action-token.spec.tsapps/api/src/app/agents/conversation-runtime/egress/outbound.gateway.tsapps/api/src/app/agents/conversation-runtime/ingress/chat-instance.registry.tsapps/dashboard/src/pages/usecase-select-page.tsxapps/dashboard/src/utils/telemetry.ts
💤 Files with no reviewable changes (1)
- apps/api/src/app/agents/conversation-runtime/ingress/chat-instance.registry.ts
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/src/utils/telemetry.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/app/agents/conversation-runtime/action-token/card-callback-button.walker.ts
- apps/api/src/app/agents/conversation-runtime/egress/outbound.gateway.ts
- apps/api/src/app/agents/conversation-runtime/egress/outbound.gateway.action-token.spec.ts
- apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.spec.ts
Comment on lines
+131
to
+140
| await forEachCallbackButton(clone, async (button) => { | ||
| if (!callbackPayloadNeedsTokenization(button.id)) { | ||
| return; | ||
| } | ||
|
|
||
| const token = await this.mintActionToken({ | ||
| ...binding, | ||
| id: button.id, | ||
| value: typeof button.value === 'string' ? button.value : undefined, | ||
| }); |
Contributor
There was a problem hiding this comment.
Tokenization check ignores oversized button.value.
This gate only evaluates button.id, but this method also persists button.value into the token claims and strips it from the delivered button once tokenized. That means a button with a short id and a long value can still exceed the platform callback-data budget and slip through un-tokenized, which defeats the Telegram fix on that path.
Suggested direction
- if (!callbackPayloadNeedsTokenization(button.id)) {
+ if (!callbackPayloadNeedsTokenization(button)) {
return;
}// In apps/api/src/app/agents/conversation-runtime/action-token/card-callback-button.walker.ts,
// make the predicate evaluate the encoded callback payload from both fields.
type CallbackPayloadButton = Pick<ButtonElement, 'id' | 'value'>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/api/src/app/agents/conversation-runtime/action-token/agent-action-token.service.ts`
around lines 131 - 140, The tokenization predicate
callbackPayloadNeedsTokenization currently only checks button.id, so update the
check used by forEachCallbackButton (and the predicate in
card-callback-button.walker.ts) to evaluate the combined/encoded callback
payload derived from both button.id and button.value (e.g., treat the button as
CallbackPayloadButton with 'id' and 'value'), and return true when the
serialized/encoded payload would exceed the platform callback-data budget; then
use that predicate before calling this.mintActionToken so long button.value is
tokenized/removed when required (refer to forEachCallbackButton,
callbackPayloadNeedsTokenization, and mintActionToken).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AgentActionTokenServiceto mint shortat:tokens (16-byte entropy) in Redis with 3-day TTL (AGENT_ACTION_TOKEN_TTL).OutboundGatewaytokenizes cardbuttonactions on a delivery-only clone before posting/editing; history persists the original card with raw action IDs. Slack-native blocks are unchanged.ChatInstanceRegistryresolvesat:tokens back to real{ id, value }before dispatch; missing/expired/mismatched tokens no-op with a warning.Fixes Telegram tool-approval cards (and any long custom action IDs) failing silently when
callback_dataexceeds 64 bytes.Linear: https://linear.app/novu/issue/NV-7956
Architecture
flowchart TD subgraph egress [OutboundGateway] card[Card with real button ids] --> clone[Clone card] clone --> mint["mint at: token per button -> Redis"] mint --> deliver[Deliver tokenized card] card --> persist[Persist original card] end deliver --> platform[Telegram / WhatsApp / Teams / Slack portable] platform --> click[User clicks button] click --> onAction[ChatInstanceRegistry.onAction] subgraph ingress [Ingress] onAction --> check{starts with at:?} check -- no --> passthru[Pass through raw id] check -- yes --> lookup[Redis GET + binding check] lookup --> restore[Restore id + value] end passthru --> dispatch[managed / bridge onAction] restore --> dispatchBreaking changes
None. Non-prefixed action IDs continue to pass through unchanged.
Test plan
AgentActionTokenServicemint/resolve/binding/tokenize cardOutboundGatewaydelivery clone + fallback on mint failureChatInstanceRegistrytoken resolution + raw passthroughrequires_action), confirm card delivers and Approve/Deny round-tripsMade with Cursor
What changed
Introduces Redis-backed opaque action tokens (prefix "at:") for agent chat-card buttons to avoid platform payload limits (notably Telegram’s 64‑byte callback_data). Buttons that would produce oversized callback payloads are replaced with short, random tokens (16 bytes entropy, URL-safe) stored in Redis with a configurable TTL (default 3 days); the original IDs/values are preserved in persisted history. Outbound delivery tokenizes cards; inbound handling resolves tokens and validates binding claims (agent, integration, environment, organization). Token resolution failures fall back to safe behaviors and log warnings.
Affected areas
Key technical decisions
Testing
New unit tests cover AgentActionTokenService (mint/resolve/tokenize), opaque-token utilities, OutboundGateway tokenization and fallback, and ChatInstanceRegistry token resolution; manual Telegram/Slack checks and full e2e verification remain pending.
Greptile Summary
This PR introduces
AgentActionTokenService, which mints short Redis-backed opaque tokens (16-byte entropy,at:prefix, 3-day TTL) to replace long button action IDs in chat cards before delivery, solving Telegram's 64-bytecallback_datalimit. Outbound paths deep-clone cards and tokenize only buttons whose encoded payload would exceed the limit; history persists the original IDs; inbound resolution validates a four-field binding and restores the original{ id, value }before dispatch.OutboundGatewaynow appliesapplyActionTokensForDeliveryinpostToConversation,sendDirectMessage,editInConversation, and optionallyreplyOnThread(via newactionTokenBindingopt-in param); Slack-native block paths are untouched.ChatInstanceRegistry.onActionresolvesat:tokens viaresolveForDispatch; missing/expired/binding-mismatched tokens no-op with a warning; raw (non-prefixed) action IDs pass through unchanged.mintRandomToken,isMintedOpaqueActionId,buildOpaqueStorageKey,parseTtlFromEnv) are extracted and reused by the pre-existingAgentEmailActionTokenService.Confidence Score: 5/5
Safe to merge — token mint, binding validation, and card-clone logic are all correct; existing callers of replyOnThread use link-buttons or markdown only so no live path bypasses tokenization.
The implementation correctly deep-clones cards before mutating, enforces four-field binding equality on resolution, and passes through raw action IDs unchanged. All new Redis operations are wrapped with proper error handling and fallbacks. Unit tests cover mint/resolve round-trips, binding mismatches, cache failures, partial mint failures, and the raw-passthrough path.
No files require special attention. The one style note is in agent-action-token.service.ts (redundant ternary in the warning log).
Important Files Changed
Sequence Diagram
sequenceDiagram participant Agent as Agent Runtime participant OG as OutboundGateway participant ATS as AgentActionTokenService participant Redis as Redis (CacheService) participant Platform as Telegram / WhatsApp / Teams participant CIR as ChatInstanceRegistry (onAction) Note over Agent,Platform: Egress — card delivery Agent->>OG: deliver(target, msg, persist) OG->>OG: applyActionTokensForDelivery(card, binding) OG->>ATS: tokenizeCardForDelivery(card, binding) ATS->>ATS: structuredClone(card) loop for each callback button needing tokenization ATS->>Redis: "SET agent:action:{token} claims TTL=3d" end ATS-->>OG: tokenized card clone OG->>Platform: "thread.post({ card: tokenizedCard })" OG->>OG: persistDelivered(originalMsg) [original IDs kept] Note over Platform,CIR: Ingress — user clicks button Platform->>CIR: "onAction(event.actionId = at:xxx...)" CIR->>ATS: resolveForDispatch(actionId, value, binding) alt actionId is a minted token ATS->>Redis: "GET agent:action:{token}" Redis-->>ATS: claims JSON ATS->>ATS: claimsMatchBinding? ATS-->>CIR: "{ id: originalId, value: originalValue }" CIR->>Agent: callbacks.onAction(resolvedAction) else actionId is a raw (non-token) id ATS-->>CIR: "{ id: actionId, value }" CIR->>Agent: callbacks.onAction(rawAction) else token missing / expired / binding mismatch ATS-->>CIR: null CIR->>CIR: no-op + warn log endComments Outside Diff (1)
apps/api/src/app/agents/conversation-runtime/egress/outbound.gateway.ts, line 157-189 (link)replyOnThreadbypasses action-token tokenizationreplyOnThreadpasses its payload directly totoThreadPostArgwithout going throughapplyActionTokensForDelivery. Inbridge.runtime.tsline 133–135 this method is already called with{ card: reply }, and the signature accepts anyOutboundMessageincluding ones withbuttonchildren. Any card with long action IDs posted through this path — on Telegram in particular — would still exceed the 64-bytecallback_datalimit. The three delivery methods fixed by this PR (postToConversation,sendDirectMessage,editInConversation) won't help if this path is used.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "api(agents): fix CardElement casts for n..." | Re-trigger Greptile