🔒 Aisle Security Analysis

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

1. 🟡 Sensitive information leakage via unredacted direct-cron delivery error logging

Description

retryTransientDirectCronDelivery logs the raw error message/serialized error object for outbound delivery failures.

• summarizeDirectCronDeliveryError() returns error.message, a raw string, or JSON.stringify(error).
• retryTransientDirectCronDelivery() interpolates this summary directly into logWarn(...).
• Errors thrown by outbound adapters / HTTP clients commonly contain PII (recipient IDs, channel IDs, usernames) and can sometimes include secrets (request URLs embedding tokens, Authorization headers, etc.) or verbose stack traces / request objects.
• Because this path runs in cron, these logs may be retained centrally and accessible to operators/services that should not see these details.

Vulnerable code:

function summarizeDirectCronDeliveryError(error: unknown): string {
  if (error instanceof Error) {
    return error.message || "error";
  }
  if (typeof error === "string") {
    return error;
  }
  try {
    return JSON.stringify(error) || String(error);
  } catch {
    return String(error);
  }
}
...
logWarn(
  `[cron:${params.jobId}] transient direct announce delivery failure, retrying ...: ${summarizeDirectCronDeliveryError(err)}`,
);

Recommendation

Redact and minimize error details before logging.

• Prefer a centralized formatter that applies redactSensitiveText (e.g. formatErrorMessage / formatUncaughtError in src/infra/errors.ts).
• Avoid JSON.stringify(error) on arbitrary error objects; it can include nested request configs/headers.

Example fix:

import { formatErrorMessage } from "../../infra/errors.js";

function summarizeDirectCronDeliveryError(error: unknown): string {
  ​// Produces a short message and applies best-effort secret redaction.
  return formatErrorMessage(error);
}

​// Optionally: cap length to prevent huge logs
const detail = summarizeDirectCronDeliveryError(err).slice(0, 500);
logWarn(`[cron:${jobId}] transient direct delivery failure, retrying ...: ${detail}`);

Additionally, consider logging structured fields (error code, channel) separately and omitting recipient identifiers (to, thread IDs) unless explicitly needed and redacted.

2. 🟡 Non-idempotent transient retry in cron direct delivery can duplicate outbound messages

Description

dispatchCronDelivery now retries direct outbound delivery on a set of “transient” error message patterns. Because outbound sends are not idempotent (no idempotency key / de-dupe token is passed through deliverOutboundPayloads to channel adapters), a failure that occurs after the provider has accepted/delivered the message (e.g., network timeout/reset after send) will be retried and can produce duplicate outbound messages (spam, unintended disclosure, extra cost).

Why this is risky:

• The transient patterns include network errors such as ECONNRESET and ETIMEDOUT:
  • These errors can happen after a request is processed.
  • The codebase already documents this for Telegram: telegram/network-errors.ts explicitly warns that retrying ECONNRESET/ETIMEDOUT for non-idempotent sends can cause duplicates.
• retryTransientDirectCronDelivery blindly re-runs deliverOutboundPayloads(...) on these errors.
• deliverOutboundPayloads(...) does not accept/propagate an idempotency key for direct adapters, so retries are “send again”.

Impact:

• A single cron run can send the same text/payload up to 4 times (initial + 3 retries: 5s/10s/20s delays).
• Because each retry is a fresh deliverOutboundPayloads(...) call (which also write-ahead enqueues), earlier failed attempts may remain in the delivery queue and be replayed later by recovery, compounding duplication risk.

Vulnerable code (retry loop):

} catch (err) {
  const delayMs = retryDelaysMs[retryIndex];
  if (delayMs == null || !isTransientDirectCronDeliveryError(err) || params.signal?.aborted) {
    throw err;
  }
  retryIndex += 1;
  await sleepWithAbort(delayMs, params.signal);
}

Transient patterns include non-idempotent network failures:

​/\b(econnreset|econnrefused|etimedout|enotfound|ehostunreach|network error)\b/i

The retry is enabled for text delivery:

• finalizeTextDelivery → deliverViaDirect(delivery, { retryTransient: true })

Recommendation

Make retries safe for non-idempotent outbound sends.

Options (preferred first):

1. Only retry errors that are guaranteed “pre-send” (pre-connect / DNS / connection refused), similar to Telegram’s isSafeToRetrySendError.
   • Remove ambiguous post-send errors like ETIMEDOUT and ECONNRESET from retry matching.
2. Add an idempotency key generated once per cron delivery attempt and propagate it through outbound delivery into channel adapters that can support de-duplication (gateway mode already has idempotencyKey; direct mode currently does not).
3. If bestEffort is enabled, consider disabling sleeps/retries (or limiting to a single immediate retry) to avoid repeated sends and unnecessary resource use.

Concrete minimal change example (tighten transient matching):

const TRANSIENT_DIRECT_CRON_DELIVERY_ERROR_PATTERNS: readonly RegExp[] = [
  ​/no active .* listener/i,
  ​/gateway not connected/i,
  ​/gateway closed \(1006/i,
  ​/gateway timeout/i,
  ​// Only pre-connect style errors (avoid etimedout/econnreset):
  ​/\b(econnrefused|enotfound|ehostunreach)\b/i,
];

Or implement a channel-aware predicate:

function isSafeToRetryDirectSend(err: unknown, channel: string): boolean {
  if (channel === "telegram") return isSafeToRetrySendError(err);
  ​// others: implement similarly or default false
  return false;
}

3. 🟡 Cron text-only direct delivery bypasses announce-flow suppression (ANNOUNCE_SKIP / NO_REPLY decision layer)

Description

The cron delivery dispatcher was changed to send text-only cron output via direct outbound adapters (deliverOutboundPayloads) instead of routing through runSubagentAnnounceFlow.

This introduces a suppression/consent bypass risk:

• Previously, runSubagentAnnounceFlow executed an agent-side “announce step” that could intentionally suppress user-facing delivery by returning ANNOUNCE_SKIP or NO_REPLY (see runSubagentAnnounceFlow in src/agents/subagent-announce.ts, which treats isAnnounceSkip(reply) / isSilentReplyText(reply, SILENT_REPLY_TOKEN) as a terminal suppression path).
• After this change, finalizeTextDelivery() performs only a narrow suppression check for exact NO_REPLY in the cron agent’s synthesized text, then always sends via deliverViaDirect().
• As a result, any existing session/system-prompt/hook behavior that relied on the announce step to prevent outbound delivery (e.g., “remain silent” behavior expressed via ANNOUNCE_SKIP/NO_REPLY in the announce flow) no longer applies to cron text-only runs, potentially causing unintended outbound messages to be sent to end users/channels.

Vulnerable code (new behavior):

if (synthesizedText.toUpperCase() === SILENT_REPLY_TOKEN.toUpperCase()) {
  return params.withRunSession({ status: "ok", summary, outputText, delivered: true, ... });
}
...
return await deliverViaDirect(delivery, { retryTransient: true });

This is especially impactful because the change explicitly shifts text-only cron delivery away from internal announce routing (which could be suppressed) to unconditional direct sending.

Recommendation

Decide explicitly whether cron text delivery should honor the same suppression/consent decisions as the announce flow.

Options:

1. Reintroduce an explicit suppression decision layer before direct sending (e.g., a lightweight policy hook or a configurable “respect_suppression” mode that runs the same announce-step logic used by runSubagentAnnounceFlow).

2. If suppression is intended to be token-based, treat ANNOUNCE_SKIP as a silent token for cron output (in addition to NO_REPLY) before calling deliverOutboundPayloads:

import { isAnnounceSkip } from "../../agents/tools/sessions-send-helpers.js";
import { isSilentReplyText, SILENT_REPLY_TOKEN } from "../../auto-reply/tokens.js";

const text = synthesizedText?.trim() ?? "";
if (isAnnounceSkip(text) || isSilentReplyText(text, SILENT_REPLY_TOKEN)) {
  return params.withRunSession({ status: "ok", summary, outputText, delivered: true, ...params.telemetry });
}

3. If direct sending is required for correctness, add an explicit, non-LLM-based opt-out control (config/session flag) that is checked in the cron delivery dispatcher to prevent outbound sends when users/channels have disabled automated messages.

Analyzed PR: #40575 at commit 54b1513

_{Last updated on: 2026-03-09T06:05:04Z}

Greptile Summary

This PR fixes a silent correctness bug where cron text-only announce delivery jobs could mark themselves as delivered: true even when no outbound Telegram (or other channel) message was actually sent. The fix replaces the internal runSubagentAnnounceFlow routing path for text-only payloads with a direct call to deliverOutboundPayloads via deliverViaDirect, while preserving the descendant-output consolidation logic (subagent wait, fallback reply, stale interim suppression) as a pre-step named finalizeTextDelivery.

Core logic is sound. All early-return guard paths (activeSubagentRuns > 0, stale interim suppression, SILENT_REPLY_TOKEN) still set deliveryAttempted = true before returning, preserving double-deliver prevention. The resolveCronAnnounceSessionKey / ensureOutboundSessionEntry removals are safe—those helpers existed only for announce routing. Removed test assertions about announce childRunId and unique identifier handling are appropriate since that path no longer exists.

Test coverage is nearly complete, but one edge case is untested: text-only delivery with bestEffort: false when the direct send fails. The PR adds a regression for bestEffort: true failure (line 344) and bestEffort: false success (line 375), but not the bestEffort: false failure scenario. The new behavior in that case (status: "error", delivered: false) is correct—it reflects the new error semantics where failures propagate directly instead of falling back through announce routing—but lacks a direct test.

Confidence Score: 4/5

• Safe to merge; the logic change is well-scoped and the identified test gap does not block functionality but rather documents edge-case coverage.
• The core refactor in delivery-dispatch.ts is sound—the new finalizeTextDelivery path correctly preserves all guard conditions and consolidates descendant output before calling the real outbound adapter. Removed helpers (resolveCronAnnounceSessionKey, ensureOutboundSessionEntry) are demonstrably unnecessary for the new path. Tests cover happy paths, bestEffort: true failure, bestEffort: false success, double-deliver guards, descendant consolidation, forum-topic direct delivery, and Signal parity. The one minor deduction is a single untested scenario: text-only send failing with bestEffort: false. The expected behavior in that case (status error, delivered false) is correct per the code at lines 176-185, and this change represents intentional new semantics (failures propagate directly instead of via announce fallback)—it just lacks a test to document it.
• src/cron/isolated-agent.skips-delivery-without-whatsapp-recipient-besteffortdeliver-true.test.ts — missing test for text-only delivery failure with bestEffort=false (see inline comment).

_{Last reviewed commit: 305fa38}

Merged via squash.

• Prepared head SHA: 54b1513c78613bddd8cae16ab2d617788a0dacb6
• Merge commit: a40c29b11a0246271d49a33e142e742c7f0e23da

Thanks @obviyus!