🔒 Aisle Security Analysis

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

1. 🟠 Client-controlled idempotencyKey can suppress mirrored transcript entries (audit/integrity bypass)

Description

appendAssistantMessageToSessionTranscript now deduplicates mirrored assistant messages by idempotencyKey and skips appending if the key already exists in the transcript.

Trust boundary problem:

• idempotencyKey originates from gateway request parameters (request.idempotencyKey) and is therefore client-controlled.
• That client-controlled key is propagated into mirror.idempotencyKey and ultimately into transcript appends.
• Because transcript dedupe checks the entire persisted transcript file, a client can reuse an old key (e.g., after a gateway restart / in-memory dedupe eviction) to send a new outbound message while preventing the mirror entry from being recorded.

Impact:

• Enables stealth outbound sends in sessions that rely on mirroring for audit/history.
• Can desync “what was sent” vs “what was logged” (especially if delivery still occurs but transcript append is skipped).

Vulnerable code:

if (
  params.idempotencyKey &&
  (await transcriptHasIdempotencyKey(sessionFile, params.idempotencyKey))
) {
  return { ok: true, sessionFile };
}

Recommendation

Do not use a client-controlled idempotency key as the authority for transcript de-duplication.

Mitigations (pick one or combine):

1. Generate a server-side transcript idempotency key that is scoped to the concrete delivery event (e.g., messageId + sessionKey), and keep the client idempotency key only for request retry semantics.

2. If a client-supplied key must be accepted, make dedupe conditional on the content matching as well (do not suppress when the same key is reused with different mirrored content).

3. Add strict validation and limits for idempotency keys (format + max length) at the protocol/schema boundary.

Example (server-side key):

​// When you have a stable delivery identifier:
const transcriptIdempotencyKey = `mirror:v1:${sessionKey}:${result.messageId}`;

await appendAssistantMessageToSessionTranscript({
  sessionKey,
  text: mirrorText,
  idempotencyKey: transcriptIdempotencyKey,
});

And enforce bounds in the gateway schema, e.g.:

const IdempotencyKey = Type.String({ minLength: 1, maxLength: 128, pattern: "^[a-zA-Z0-9:_-]+$" });

2. 🟠 Unbounded transcript scan for idempotency key causes O(n) per append (DoS risk)

Description

appendAssistantMessageToSessionTranscript() now supports idempotencyKey de-duplication by scanning the entire session transcript JSONL file on every append.

Impact:

• Unbounded memory usage: fs.promises.readFile(transcriptPath, "utf-8") loads the entire transcript into memory.
• Unbounded CPU usage / algorithmic complexity: it then split()s into lines and JSON.parse()s each line, making each append O(file_size). Over time, repeated appends to the same session become O(n²) total work.
• Malformed line handling does not mitigate resource use: malformed JSON is silently ignored, but still requires scanning/splitting the full file contents.

This is reachable in normal operation because outbound delivery mirroring passes an idempotency key through to transcript appends, and gateway callers can supply idempotency keys as arbitrary non-empty strings.

Vulnerable code:

const raw = await fs.promises.readFile(transcriptPath, "utf-8");
for (const line of raw.split(/\r?\n/)) {
  ...
  const parsed = JSON.parse(line);
  ...
}

Recommendation

Avoid full-file reads/parses on the hot path.

Recommended mitigations (choose one or combine):

1. Stream the file line-by-line (bounded memory) and short-circuit on match:

import readline from "node:readline";

async function transcriptHasIdempotencyKey(transcriptPath: string, key: string): Promise<boolean> {
  const stream = fs.createReadStream(transcriptPath, { encoding: "utf-8" });
  const rl = readline.createInterface({ input: stream, crlfDelay: Infinity });
  for await (const line of rl) {
    if (!line.trim()) continue;
    try {
      const parsed = JSON.parse(line) as { message?: { idempotencyKey?: unknown } };
      if (parsed.message?.idempotencyKey === key) return true;
    } catch {
      ​// optionally count/log bad lines
    }
  }
  return false;
}

2. Bound the scan (e.g., only last N bytes/lines) since duplicates are typically recent retries.

3. Cap and validate inputs: enforce a reasonable maximum length/charset for idempotencyKey before storing or scanning.

4. Introduce transcript rotation/compaction guarantees (or enable disk budgets by default) so individual transcript files cannot grow without bound.

Analyzed PR: #44666 at commit 8e6d254

_{Last updated on: 2026-03-13T09:16:00Z}

Greptile Summary

This PR addresses two follow-up issues from a delivery deduplication review: it fixes cache pruning for completed direct cron deliveries so the 2 000-entry cap is reliably enforced, and it makes the transcript mirror idempotency scan async and tolerant of malformed JSONL lines. It also consolidates previously duplicated mirror-payload type definitions into a single src/infra/outbound/mirror.ts module and propagates idempotencyKey through all mirror paths (plugin-handled sends, gateway send, and the core deliver layer).

Key changes:

• delivery-dispatch.ts: New in-memory COMPLETED_DIRECT_CRON_DELIVERIES map with TTL + size-based pruning (pruneCompletedDirectCronDeliveries). Pruning is invoked both on write (rememberCompletedDirectCronDelivery) and read (getCompletedDirectCronDelivery), correctly keeping the map at ≤ 2 000 entries.
• transcript.ts: appendAssistantMessageToSessionTranscript now accepts an optional idempotencyKey; before appending it calls transcriptHasIdempotencyKey which scans the transcript JSONL file line-by-line, skipping malformed lines, so a replay of the same message is a no-op.
• subagent-registry.ts: finalizeAnnounceCleanup wrapper resets cleanupHandled = false and re-persists the registry when finalizeSubagentCleanup itself throws, making the cleanup retryable on the next registry restart.
• mirror.ts (new): Single canonical home for OutboundMirror and DeliveryMirror types, eliminating four inline duplicate definitions across message.ts, outbound-send-service.ts, delivery-queue.ts, and deliver.ts.

Confidence Score: 4/5

• Safe to merge; changes are well-tested, logically sound, and limited in scope to deduplication and type consolidation.
• The pruning logic, idempotency scan, and cleanup retry path are all covered by new regression tests. The two flagged points are minor: the cloneDeliveryResults allocation that is immediately discarded on a cache hit, and the inherent TOCTOU window in the file-based idempotency check (acceptable for best-effort mirror deduplication). No missing tests, no risky behavioral changes in the hot delivery path.
• No files require special attention; src/cron/isolated-agent/delivery-dispatch.ts and src/config/sessions/transcript.ts carry the two minor style notes above but are otherwise correct.

This is a comment left during a code review.
Path: src/cron/isolated-agent/delivery-dispatch.ts
Line: 333-337

Comment:
**Cloned results allocated but immediately discarded**

`getCompletedDirectCronDelivery` deep-clones the cached `OutboundDeliveryResult[]` via `cloneDeliveryResults`, but the returned value is only inspected for truthiness at the call site — the actual array is never read. This allocates a fresh array + object spread on every cache hit for no benefit.

Consider returning a simple boolean from a helper, or refactoring `getCompletedDirectCronDelivery` to return `boolean` when the caller only needs existence:

```suggestion
      const isCachedDelivery = (() => {
        pruneCompletedDirectCronDeliveries(Date.now());
        return COMPLETED_DIRECT_CRON_DELIVERIES.has(deliveryIdempotencyKey);
      })();
      if (isCachedDelivery) {
        // Cached entries are only recorded after a successful non-empty delivery.
        delivered = true;
        return null;
      }
```

Or, more minimally, just inline:

```ts
if (getCompletedDirectCronDelivery(deliveryIdempotencyKey)) {
  delivered = true;
  return null;
}
```
and have `getCompletedDirectCronDelivery` return `boolean` instead of `OutboundDeliveryResult[] | undefined`.

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

---

This is a comment left during a code review.
Path: src/config/sessions/transcript.ts
Line: 183-188

Comment:
**TOCTOU window between idempotency check and append**

The `transcriptHasIdempotencyKey` read and the `sessionManager.appendMessage` call are not atomic. Two concurrent invocations with the same `idempotencyKey` could both read the file, both find no match, and both append — defeating the deduplication. This is an inherent limitation of the file-based approach without a write lock.

If the transcript write path already holds an exclusive advisory lock (e.g. in `SessionManager.open`), this is fine and can be documented with a comment. If it does not, a note explaining that this is best-effort deduplication would make the intent clearer for future contributors.

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

_{Last reviewed commit: 5baa548}