Greptile Summary

This PR stabilizes the Area 2 Telegram test harness by completing dependency injection wiring (upsertChannelPairingRequest, buildModelsProviderData, readChannelAllowFromStore) through TelegramBotDeps, fixing module-mock hoisting issues (vi.doMock → vi.mock for undici, web-media, and media-runtime), and quarantining two flaky sticker e2e tests pending a deterministic rework. No production runtime logic changes were introduced.

Key changes:

• bot-deps.ts, bot-handlers.runtime.ts, bot-message-context.ts, dm-access.ts, bot/helpers.ts: properly route upsertChannelPairingRequest, buildModelsProviderData, and readChannelAllowFromStore through the injectable TelegramBotDeps object instead of direct module imports.
• bot.create-telegram-bot.test-harness.ts: switches vi.doMock → vi.mock for undici and web-media so mocks are hoisted before any module import; adds .js extension variants for ESM dual-resolution; extracts dispatchHarnessReplies to avoid duplicated harness logic; wires buildModelsProviderData mock backed by a local createModelsProviderDataFromConfig helper.
• bot.media.e2e-harness.ts / bot.media.test-utils.ts: exports undiciFetchSpy (reset in beforeEach) and threads it as the default proxyFetch, so sticker/media e2e paths are correctly intercepted at the HTTP layer.
• bot.media.stickers-and-fragments.e2e.test.ts: quarantines two sticker download/cache-hit tests with it.skip; migrates the active animated-sticker test to the proxyFetch-injection pattern.
• monitor.test.ts / fetch.test.ts: removes vi.resetModules() calls and drops computeBackoff/sleepWithAbort assertions that were causing flakiness — note this reduces coverage of the retry backoff mechanism (see inline comment).

Confidence Score: 4/5

• Safe to merge — all production logic is unchanged; changes are confined to test harness wiring and dependency injection plumbing.
• Production code changes are minimal and well-scoped: they only make existing direct imports injectable via TelegramBotDeps, all with correct fallbacks to the original imports. The test harness changes are more substantial but the author verified full suite passage. The one meaningful concern is that computeBackoff/sleepWithAbort assertions were dropped from monitor.test.ts, reducing verification of the retry-backoff mechanism. Two sticker e2e tests are intentionally quarantined with it.skip, leaving a noted coverage gap. These are acknowledged trade-offs rather than defects.
• extensions/telegram/src/monitor.test.ts — three test paths no longer assert that backoff and sleep were invoked during recoverable retry scenarios.

This is a comment left during a code review.
Path: extensions/telegram/src/monitor.test.ts
Line: 203-205

Comment:
**Weakened retry-backoff verification**

Removing the `computeBackoff` and `sleepWithAbort` assertions from `expectRecoverableRetryState` (and from the three inline test callsites at lines ~451, ~532, ~676) reduces confidence that the retry mechanism actually engages backoff before retrying. The remaining `runSpy.toHaveBeenCalledTimes(N)` only confirms *that* retries occurred, not *that* the system backed off between them — meaning a busy-loop regression would still pass these tests.

If the root cause of flakiness was that `computeBackoff`/`sleepWithAbort` weren't always called on the exact invocation being checked (e.g., race with timer mocks), a more targeted fix would be to scope the assertion to the correct call count rather than dropping it entirely:

```suggestion
function expectRecoverableRetryState(expectedRunCalls: number) {
  expect(computeBackoff).toHaveBeenCalled();
  expect(sleepWithAbort).toHaveBeenCalled();
  expect(runSpy).toHaveBeenCalledTimes(expectedRunCalls);
}
```

If the assertions genuinely cannot be made reliable, a brief comment explaining the known limitation would help future maintainers understand the reduced coverage is intentional.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: extensions/telegram/src/bot.create-telegram-bot.test-harness.ts
Line: 145-149

Comment:
**Inconsistent null-safety on `dispatcherOptions`**

In `dispatchHarnessReplies`, some accesses to `dispatcherOptions` use optional chaining (lines 137, 143, 151, 154) while the accesses to `responsePrefix`, `enableSlackInteractiveReplies`, `responsePrefixContextProvider`, `responsePrefixContext`, and `onHeartbeatStrip` here do not. Since TypeScript is satisfied (implying `dispatcherOptions` is a required field in `DispatchReplyHarnessParams`), the `?.` usages elsewhere are just overly defensive — but the inconsistency could mislead future readers into thinking `dispatcherOptions` can be nullish and make them wonder why these lines aren't equally guarded.

Consider removing the redundant `?.` on `dispatcherOptions` in the other spots so the code reads uniformly, e.g.:

```typescript
  await params.dispatcherOptions.typingCallbacks?.onReplyStart?.();
  // ...
  await params.dispatcherOptions.deliver?.(payload, info);
  // ...
  params.dispatcherOptions.onSkip?.(payload, info);
  params.dispatcherOptions.onError?.(err, info);
```

This makes it clear the object itself is always present and only its callback sub-fields are optional.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "Telegram: stabilize ..."}

Addressed the review concerns in this branch:

1. Scope/claim tightening

• Updated the PR description to remove the broad "no-restart" user-visible claim and frame this as Telegram DI/routing/reply stabilization.

2. Explicit regression test for routing refresh behavior

• Added reloads DM routing bindings between messages without recreating the bot in extensions/telegram/src/bot.create-telegram-bot.test.ts.
• This verifies route/session binding selection updates across consecutive messages on the same bot instance when config bindings change.

3. Retry/backoff assertion coverage

• Added explicit helper assertions in extensions/telegram/src/monitor.test.ts for the setup-time recoverable retry path (computeBackoff + sleepWithAbort).

4. Skipped sticker tests follow-up tracked

• Opened Telegram: unquarantine skipped sticker e2e tests #50185 and linked it from PR + test TODOs.

Evidence snippets (local):

pnpm test -- extensions/telegram/src/bot.create-telegram-bot.test.ts -t "reloads DM routing bindings between messages without recreating the bot"
Test Files  1 passed (1)
Tests       1 passed | 46 skipped (47)

pnpm test -- extensions/telegram/src/monitor.test.ts -t "retries setup-time recoverable errors before starting polling"
Test Files  1 passed (1)
Tests       1 passed | 23 skipped (24)

Also included CI before/after boundary evidence in the PR description:

• before: run 23277677077 extension-src-outside-plugin-sdk-boundary failed
• after: run 23278040250 extension-src-outside-plugin-sdk-boundary passed

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🟡 Telegram native command auth uses mixed config snapshots (stale allowlists) despite per-invocation runtime config reload

Description

registerTelegramNativeCommands now loads a fresh runtimeCfg and runtimeTelegramCfg on every command invocation, but the authorization decision still depends on startup-captured allowlists/policy resolvers. This creates a TOCTOU-style inconsistency where access can be granted using stale authorization inputs even though other decisions (routing/session/delivery) use the fresh config.

Impact:

• DM / group sender allowlists can remain permissive after runtime config revocation. The handler refreshes runtimeTelegramCfg, but still passes allowFrom / groupAllowFrom variables that were captured when the bot started (and are not derived from runtimeTelegramCfg).
• Group allowlist policy can be evaluated using an older snapshot because resolveGroupPolicy is injected from bot.ts and (currently) closes over the startup cfg rather than runtimeCfg. As a result, removing a group from the allowlist in the live config may not take effect for native commands until restart.

Vulnerable code (mixed fresh+stale auth inputs):

const runtimeCfg = loadFreshRuntimeConfig();
const runtimeTelegramCfg = resolveFreshTelegramConfig(runtimeCfg);
const auth = await resolveTelegramCommandAuth({
  cfg: runtimeCfg,
  telegramCfg: runtimeTelegramCfg,
  allowFrom,        // startup snapshot
  groupAllowFrom,   // startup snapshot
  resolveGroupPolicy, // may use startup cfg
  ...
});

And inside auth, DM allowlist prefers these values:

const dmAllowFrom = groupAllowOverride ?? allowFrom;

This means a user/group that was removed from config allowlists can potentially continue executing native/plugin commands (until process restart), contrary to the intent of loading fresh runtime configuration per invocation.

Recommendation

Ensure all authorization inputs come from the same fresh snapshot used for the rest of the command execution.

Suggested fixes:

1. Derive allowFrom/groupAllowFrom from the freshly resolved runtimeTelegramCfg (and only fall back to startup values for CLI overrides if that is intentional):

const runtimeAllowFrom = runtimeTelegramCfg.allowFrom;
const runtimeGroupAllowFrom =
  runtimeTelegramCfg.groupAllowFrom ?? runtimeTelegramCfg.allowFrom ?? runtimeAllowFrom;

const auth = await resolveTelegramCommandAuth({
  ...,
  telegramCfg: runtimeTelegramCfg,
  allowFrom: runtimeAllowFrom,
  groupAllowFrom: runtimeGroupAllowFrom,
});

2. Make resolveGroupPolicy use runtimeCfg (not a startup closure). For example, change the function signature to accept cfg or construct a per-invocation resolver:

const resolveGroupPolicyRuntime = (chatId: string | number) =>
  resolveChannelGroupPolicy({ cfg: runtimeCfg, channel: "telegram", accountId, groupId: String(chatId) });

3. Consider failing closed when fresh account config cannot be resolved (instead of silently falling back to the startup snapshot) for auth-relevant settings.

These changes prevent stale allowlists/policies from granting access after runtime revocation.

Analyzed PR: #50155 at commit a1d84d9

_{Last updated on: 2026-03-19T06:17:02Z}