feat(issue-163): unify Telegram into single channel with ephemeral setup#164
feat(issue-163): unify Telegram into single channel with ephemeral setup#164JonasBaeumer wants to merge 4 commits into
Conversation
The bot UX previously created a dual-channel feeling: setup/test pings mingled with live notifications, and the seeded demo user was auto-linked to a dev chat. The recently added TELEGRAM_TEST_CHANNEL_ID addressed this by routing test traffic to a separate group, but the goal of #163 is the opposite — keep one chat per user and make setup messages disappear after onboarding completes. Behaviour changes: - Track every bot-sent and user-sent message_id in the signup session. - On successful signup, after the API-key message, bulk-delete all tracked messages and land the user on the persistent main menu. - Same cleanup runs when a stale session is replaced by a new /start. - Emit TELEGRAM_SETUP_CLEANED audit event with the deleted count. Other changes: - Drop TELEGRAM_TEST_CHAT_ID from env.ts/seed.ts; demo user is no longer auto-linked. Drop TELEGRAM_TEST_CHANNEL_ID and its docs. - Live tests still read process.env.TELEGRAM_TEST_CHAT_ID as a local-dev escape hatch (skips when unset). - Fix live-test bug: callback_query filter compared from.id (user) against the configured chat ID, dropping every tap in a group chat. Now compares message.chat.id. Same fix in menuHandler.live. - Wrong-button alert in live tests no longer leaks raw callback_data. - Add deleteMessage to mockBot; tests for cleanup, stale-session cleanup, and best-effort deletion. - docs/telegram-setup.md rewritten around a single onboarding path.
📝 WalkthroughWalkthroughConsolidates Telegram onboarding into a single /start pairing flow, tracks ephemeral signup message IDs in Redis, bulk-deletes them on completion, removes dual test env variables, updates seed/docs, extends mock Bot API (deleteMessage), and adapts unit/integration/e2e tests to assert cleanup and audit emission. ChangesTelegram Single-Channel Ephemeral Setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/integration/e2e/telegramApprovalCheckout.test.ts`:
- Around line 44-47: Prettier formatting violations are causing CI failure in
this test file; run Prettier to reformat the file or manually fix formatting
around the hasTelegram const declaration and the code block at lines referenced
(the boolean expression combining isMockMode, process.env.TELEGRAM_BOT_TOKEN,
and process.env.TELEGRAM_TEST_CHAT_ID, and the subsequent block at 64-67) so the
file matches the project's Prettier rules—easiest fix: run prettier --write
"tests/integration/e2e/telegramApprovalCheckout.test.ts" and commit the
resulting changes.
In `@tests/integration/telegram/connectionFlow.test.ts`:
- Line 24: Prettier formatting in the Telegram integration test is failing;
reformat the test file that declares TELEGRAM_TEST_CHAT_ID and the later
connection-flow assertion block using Prettier (run your project's prettier
--write on that test file) so the constant declaration and the connection
assertions are properly formatted and CI passes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 63fb4892-d76e-4b3a-8ff1-2737a5a410c2
📒 Files selected for processing (20)
.env.exampleCLAUDE.mdREADME.mddocs/telegram-setup.mdsrc/config/env.tssrc/contracts/audit.tssrc/db/seed.tssrc/telegram/mockBot.tssrc/telegram/sessionStore.tssrc/telegram/signupHandler.tstests/integration/e2e/onboarding.test.tstests/integration/e2e/telegramApprovalCheckout.test.tstests/integration/telegram/connectionFlow.test.tstests/integration/telegram/menuHandler.live.test.tstests/integration/telegram/preferences.live.test.tstests/unit/api/rateLimit.test.tstests/unit/api/telegram.test.tstests/unit/telegram/callbackHandler.test.tstests/unit/telegram/notificationService.test.tstests/unit/telegram/signupHandler.test.ts
💤 Files with no reviewable changes (7)
- README.md
- tests/unit/api/rateLimit.test.ts
- tests/unit/telegram/notificationService.test.ts
- src/config/env.ts
- tests/unit/api/telegram.test.ts
- .env.example
- tests/unit/telegram/callbackHandler.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/e2e/telegramApprovalCheckout.test.ts (1)
84-90:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard cleanup deletes when IDs are unset to prevent broad Prisma deletes.
At Line 84-90,
intentId/userIdcan beundefinedif setup fails early, anddeleteMany({ where: { ...: undefined } })is unsafe. Add runtime guards before building thesewherefilters.🔧 Suggested fix
afterAll(async () => { - await prisma.virtualCard.deleteMany({ where: { intentId } }); - await prisma.ledgerEntry.deleteMany({ where: { intentId } }); - await prisma.pot.deleteMany({ where: { intentId } }); - await prisma.approvalDecision.deleteMany({ where: { intentId } }); - await prisma.auditEvent.deleteMany({ where: { intentId } }); - await prisma.purchaseIntent.deleteMany({ where: { id: intentId } }); - await prisma.user.deleteMany({ where: { id: userId } }); + if (intentId) { + await prisma.virtualCard.deleteMany({ where: { intentId } }); + await prisma.ledgerEntry.deleteMany({ where: { intentId } }); + await prisma.pot.deleteMany({ where: { intentId } }); + await prisma.approvalDecision.deleteMany({ where: { intentId } }); + await prisma.auditEvent.deleteMany({ where: { intentId } }); + await prisma.purchaseIntent.deleteMany({ where: { id: intentId } }); + } + if (userId) { + await prisma.user.deleteMany({ where: { id: userId } }); + } });Based on learnings: In Prisma queries, do not pass
undefinedinsidewhereclauses because it can trigger unintended full-table operations.🤖 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 `@tests/integration/e2e/telegramApprovalCheckout.test.ts` around lines 84 - 90, The cleanup calls call prisma.deleteMany with potentially undefined intentId/userId (variables intentId and userId), which can produce unsafe broad deletes; add runtime guards so each deleteMany is only executed when the corresponding id is truthy (e.g., check intentId before calling prisma.virtualCard.deleteMany, prisma.ledgerEntry.deleteMany, prisma.pot.deleteMany, prisma.approvalDecision.deleteMany, prisma.auditEvent.deleteMany, prisma.purchaseIntent.deleteMany and check userId before prisma.user.deleteMany), skipping or logging if the id is not set to ensure no where filter contains undefined.src/telegram/signupHandler.ts (1)
269-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCleanup is skipped on the "code already claimed" abort path.
This branch clears the signup session before deleting the tracked setup message ids, so
/start, the confirmation prompt, and the email reply remain in chat forever when the claim race is hit. Cleanup needs to run beforeclearSignupSession(chatId)here as well.Suggested direction
} catch (err: any) { if (err.name === 'PairingCodeAlreadyClaimedError') { + const finalSession = await getSignupSession(chatId); + await cleanupSignupMessages(chatId, finalSession); await clearSignupSession(chatId); await bot.api.sendMessage( chatId, '⚠️ This pairing code was already claimed by another account. Please ask your OpenClaw assistant for a new code.', );🤖 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 `@src/telegram/signupHandler.ts` around lines 269 - 275, The PairingCodeAlreadyClaimedError branch clears the signup session before removing tracked setup messages, leaving /start, confirmation prompt, and email reply in chat; change the order so the cleanup that removes tracked message ids runs before clearSignupSession(chatId). In the catch block that checks err.name === 'PairingCodeAlreadyClaimedError' (in signupHandler.ts), call the same cleanup function used elsewhere to delete tracked setup message ids (e.g., deleteTrackedSetupMessageIds or equivalent) prior to awaiting clearSignupSession(chatId), and preserve the existing bot.api.sendMessage behavior.
🤖 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 `@src/telegram/signupHandler.ts`:
- Around line 194-196: Replace the regex-based check using isValidEmail(email)
with a Zod string email parse: import z from "zod" (or reuse an existing schema)
and validate via a zod schema like z.string().email().safeParse(email); in the
signup flow in signupHandler.ts where sendEphemeral(chatId, "...") is currently
called, branch on the safeParse result (if not success, send the same ephemeral
message and return). Ensure you reference the existing sendEphemeral and chatId
variables and remove the isValidEmail dependency for this branch so validation
conforms to the src/ Zod rule.
- Around line 42-49: appendSignupMessageId currently does a read/modify/write
via getSignupSession and setSignupSession which can race; change it to use an
atomic update on the session store (instead of getSignupSession +
setSignupSession) so the append to messageIds is done inside a single atomic
operation. Replace the body of appendSignupMessageId with a call to the
session-store's atomic update method (or implement a new
updateSignupSessionAtomic) that receives chatId and a transformer callback which
reads the existing session, ensures session/messageIds exist, returns a new
session with messageIds = [...existingMessageIds, messageId], and resolves the
promise; keep the function name appendSignupMessageId and operate only on the
messageIds field to avoid clobbering other session data.
- Around line 243-255: The API key message sent by sendEphemeral is being
included in finalSession and removed by cleanupSignupMessages, so users can't
copy result.rawKey; change the flow to avoid deleting that message immediately
by either (A) send the API key message after calling
clearSignupSession/getSignupSession so its id is not appended to finalSession,
or (B) modify cleanupSignupMessages to accept an exclude list and pass the API
key message id to preserve it; update the code paths around sendEphemeral,
getSignupSession, clearSignupSession, and cleanupSignupMessages (and references
to session.agentId/result.rawKey) accordingly so the key message remains until
explicit acknowledgement.
---
Outside diff comments:
In `@src/telegram/signupHandler.ts`:
- Around line 269-275: The PairingCodeAlreadyClaimedError branch clears the
signup session before removing tracked setup messages, leaving /start,
confirmation prompt, and email reply in chat; change the order so the cleanup
that removes tracked message ids runs before clearSignupSession(chatId). In the
catch block that checks err.name === 'PairingCodeAlreadyClaimedError' (in
signupHandler.ts), call the same cleanup function used elsewhere to delete
tracked setup message ids (e.g., deleteTrackedSetupMessageIds or equivalent)
prior to awaiting clearSignupSession(chatId), and preserve the existing
bot.api.sendMessage behavior.
In `@tests/integration/e2e/telegramApprovalCheckout.test.ts`:
- Around line 84-90: The cleanup calls call prisma.deleteMany with potentially
undefined intentId/userId (variables intentId and userId), which can produce
unsafe broad deletes; add runtime guards so each deleteMany is only executed
when the corresponding id is truthy (e.g., check intentId before calling
prisma.virtualCard.deleteMany, prisma.ledgerEntry.deleteMany,
prisma.pot.deleteMany, prisma.approvalDecision.deleteMany,
prisma.auditEvent.deleteMany, prisma.purchaseIntent.deleteMany and check userId
before prisma.user.deleteMany), skipping or logging if the id is not set to
ensure no where filter contains undefined.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f390b3fb-fa84-4b73-89a7-189dea3a7ac0
📒 Files selected for processing (5)
src/telegram/signupHandler.tstests/integration/e2e/onboarding.test.tstests/integration/e2e/telegramApprovalCheckout.test.tstests/integration/telegram/connectionFlow.test.tstests/unit/telegram/signupHandler.test.ts
- sessionStore: split signup messageIds out of the JSON session into a
Redis list with atomic RPUSH appends, eliminating the read/modify/write
race in concurrent webhook handlers. clearSignupSession now drops both
keys.
- signupHandler:
- replace local isValidEmail regex with z.string().email() to match the
project rule on Zod validation in src/.
- send the API-key success message via bot.api.sendMessage (not tracked)
so the cleanup pass cannot delete it before the user copies the key.
- run cleanupSignupMessages before clearSignupSession in the
PairingCodeAlreadyClaimedError branch so abandoned signup messages
are removed when a claim race aborts the flow.
- telegramApprovalCheckout.test: guard intentId/userId truthiness in
afterAll so a beforeAll failure cannot trigger unfiltered Prisma
deleteMany operations.
- unit tests updated to mock the new sessionStore surface and assert
message-id tracking via the dedicated list rather than via the session
blob.
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 `@src/telegram/sessionStore.ts`:
- Around line 36-45: appendSignupMessageId currently calls rpush and expire
separately which can leave the list without TTL if expire fails; change it to
use Redis pipeline/multi so rpush and expire are sent together atomically (e.g.,
getRedisClient().pipeline() or .multi()), push the RPUSH and EXPIRE commands
into the pipeline for key `${MSGS_KEY_PREFIX}${chatId}`, execute the pipeline
and await its result, and handle any pipeline errors (log/throw) instead of
calling rpush/expire separately.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 89ef16da-ba00-45e8-b8c0-4c5e30e7f6a4
📒 Files selected for processing (4)
src/telegram/sessionStore.tssrc/telegram/signupHandler.tstests/integration/e2e/telegramApprovalCheckout.test.tstests/unit/telegram/signupHandler.test.ts
Avoids an orphaned list (RPUSH succeeds but EXPIRE fails on a network blip) that would otherwise live forever without a TTL.
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 `@src/telegram/sessionStore.ts`:
- Around line 43-46: The pipeline exec result from
getRedisClient().pipeline().rpush(...).expire(...).exec() must be checked for
per-command errors because ioredis returns [err,result] tuples that don't
reject; update the code in sessionStore (where MSGS_KEY_PREFIX and
getRedisClient are used) to capture the array returned by pipeline().exec(),
iterate the tuples for errors, and handle them (e.g., throw or log and bail) so
a failed RPUSH or EXPIRE doesn't silently succeed—ensure you map errors to the
correct command (rpush then expire) when reporting.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6ae817e8-5f3f-485a-93ab-e17ff499197c
📒 Files selected for processing (1)
src/telegram/sessionStore.ts
| const redis = getRedisClient(); | ||
| const key = `${MSGS_KEY_PREFIX}${chatId}`; | ||
| await redis.pipeline().rpush(key, String(messageId)).expire(key, ttlSeconds).exec(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For ioredis v5.10.1, does pipeline().exec()reject on command-level errors, or does it resolve with[err, result] tuples that must be checked manually?
💡 Result:
In ioredis 5.10.x (including 5.10.1), pipeline.exec does not reject the Promise for per-command errors (e.g., a command that returns an error reply). Instead, it resolves and returns an array where each entry is a tuple in the form [err, result]; you must check each tuple/err manually. The docs show that the callback err is “always null” and that results contain “[err, result]” responses for the queued commands. [1][2] Practical implication: treat pipeline.exec as “batch execution succeeded at the transport/batch level”, and inspect every element of the results array to detect command-level failures. [1][2] Note: pipeline.exec can still reject for non-command-level failures (e.g., connection issues). The cited docs specifically cover command-level errors: “err is always null” and “Each response follows the format [err, result]”. [1][2]
Citations:
- 1: https://ioredis.readthedocs.io/en/stable/README/
- 2: https://ioredis.readthedocs.io/en/latest/README/
🏁 Script executed:
# First, let's look at the file mentioned in the review
cat -n src/telegram/sessionStore.ts | head -60Repository: JonasBaeumer/AgentWallet
Length of output: 2585
🏁 Script executed:
# Search for other pipeline.exec() calls in the codebase to see if this is a widespread pattern
rg "pipeline\(\)" --type ts -A 2Repository: JonasBaeumer/AgentWallet
Length of output: 245
🏁 Script executed:
# Check if the function needs TypeScript strict mode and type annotations
head -50 src/telegram/sessionStore.ts | tail -20Repository: JonasBaeumer/AgentWallet
Length of output: 909
🏁 Script executed:
# Verify the error handling pattern - check if Promise<void> functions throw or handle errors elsewhere
rg "throw|catch" src/telegram/sessionStore.tsRepository: JonasBaeumer/AgentWallet
Length of output: 50
Check for command-level errors in pipeline execution.
At line 45, pipeline().exec() is awaited but its tuple results are not checked. With ioredis 5.10.1, command-level failures are returned per entry in the results array as [err, result] tuples and do not cause rejection. Without checking these results, a failed RPUSH or EXPIRE command will silently skip message-ID tracking or TTL refresh, potentially leaving orphaned data.
Suggested fix
export async function appendSignupMessageId(
chatId: number | string,
messageId: number,
ttlSeconds = DEFAULT_TTL_SECONDS,
): Promise<void> {
const redis = getRedisClient();
const key = `${MSGS_KEY_PREFIX}${chatId}`;
- await redis.pipeline().rpush(key, String(messageId)).expire(key, ttlSeconds).exec();
+ const results = await redis.pipeline().rpush(key, String(messageId)).expire(key, ttlSeconds).exec();
+ for (const [err] of results ?? []) {
+ if (err) throw err;
+ }
}🤖 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 `@src/telegram/sessionStore.ts` around lines 43 - 46, The pipeline exec result
from getRedisClient().pipeline().rpush(...).expire(...).exec() must be checked
for per-command errors because ioredis returns [err,result] tuples that don't
reject; update the code in sessionStore (where MSGS_KEY_PREFIX and
getRedisClient are used) to capture the array returned by pipeline().exec(),
iterate the tuples for errors, and handle them (e.g., throw or log and bail) so
a failed RPUSH or EXPIRE doesn't silently succeed—ensure you map errors to the
correct command (rpush then expire) when reporting.
Summary
Closes #163
The bot UX previously created a dual-channel feeling: setup/test pings mingled with live notifications, and the seeded demo user was auto-linked to a dev chat via
TELEGRAM_TEST_CHAT_ID. A more recent change addedTELEGRAM_TEST_CHANNEL_IDto route test traffic to a separate Telegram group — but the goal of #163 is the opposite: keep one chat per user and make setup messages disappear after onboarding completes.This PR delivers:
/startand email reply) on the Redis signup session. On successful signup the bot sends the API-key message, then bulk-deletes the entire signup conversation and lands the user on the persistent main menu. Same cleanup runs when a stale session is replaced by a new/start. ATELEGRAM_SETUP_CLEANEDaudit event records the deleted count for observability after the chat is wiped.TELEGRAM_TEST_CHAT_IDis removed fromenv.ts/seed.ts(demo user is no longer auto-linked);TELEGRAM_TEST_CHANNEL_IDis removed entirely. Live integration tests still readprocess.env.TELEGRAM_TEST_CHAT_IDas a local-dev escape hatch and gracefully skip when unset.waitForCallbackfilter comparedcb.from.idagainst the configured chat ID — wrong in a group chat, wherefrom.idis the tapping user's personal ID. Now comparescb.message.chat.id. Same fix inmenuHandler.live.callback_dataidentifiers; points to the instruction message instead.deleteMessageadded tomockBot. New unit and e2e tests cover messageIds tracking, success-path cleanup, stale-session cleanup, audit event, and best-effort deletion.docs/telegram-setup.mdrewritten around a single canonical/start <code>path with an ephemeral-messages note up top.Type of change
Module(s) affected
src/contracts/)prisma/,src/db/)src/telegram/)Checklist
npm testpasses locally (376 unit tests; 2 pre-existing module-load failures on main unaffected)tests/unit/telegram/signupHandler.test.ts— 5 new cases)tests/integration/e2e/onboarding.test.ts— asserts cleanup + audit event)src/contracts/(AgentAuditEventgainsTELEGRAM_SETUP_CLEANED).env.exampleupdated (removedTELEGRAM_TEST_CHAT_ID)How to test
docker compose up -d(Postgres + Redis)npm run db:migrate && npm run seed(createsdemo@agentpay.dev, no chat auto-link)npm run dev, register webhook perdocs/telegram-setup.mdPOST /v1/agent/register→ grab pairing code →/start <code>in Telegram → Confirm → reply emailPOST /v1/intents+npm run worker) — approval request stays (NOT deleted, that's intentional)npx jest --config jest.integration.live.js --testPathPattern=connectionFlow --forceExit→ 5/5 passnpx jest --config jest.integration.live.js --testPathPattern=preferences.live --forceExit --testTimeout=180000→ 12/12 pass interactivelyNotes for reviewer
TELEGRAM_TEST_CHANNEL_IDis intentional and reverses recent work; per the discussion on Remove demo mode dual-channel Telegram setup; unify into single bot channel #163 the dual-channel split was the UX problem we're solving, not a feature to keep.connectionFlow.test.tsstep 3.1 still skips gracefully ifTELEGRAM_TEST_CHAT_IDis unset — no CI impact.Summary by CodeRabbit
New Features
Documentation
; removed optional test chat/channel env entries.Chores
Tests