fix(fanout): partial-batch retry + github-comment defense-in-depth (krokoko review aws-samples#1, aws-samples#5, aws-samples#9, aws-samples#12)

Four related fixes on the fanout + github-comment surface, from the
code review on PR aws-samples#52. Grouped because they share the narrative
"defense-in-depth on the fanout dispatcher" — any one landing without
the others leaves a hole.

## Findings addressed

**aws-samples#1 — Fanout handler returns void despite reportBatchItemFailures: true**

The ``FanOutConsumer`` construct (``cdk/src/constructs/fanout-consumer.ts:146``)
has ``reportBatchItemFailures: true`` on its DDB Stream event-source
mapping. The handler returned ``void``, so Lambda retried the entire
batch on any unhandled throw instead of isolating the poisonous
record. Combined with aws-samples#5 this could cascade into retry storms and
violated the per-task ordering guarantee we rely on (§6.4, AD-9).

Fix: handler return type becomes ``Promise<DynamoDBBatchResponse>``.
Per-record processing is wrapped in try/catch; caught throws push
``{ itemIdentifier: record.eventID }`` to ``batchItemFailures`` and
emit ``fanout.record.failed`` warn. Final ``fanout.batch.complete``
log grows a ``failed`` count.

Note: ``DynamoDBStreamHandler`` constrains return to
``void | Promise<void>``, so the handler is typed as a plain 3-arg
async function. Lambda's runtime accepts either shape; existing
tests (passing ``event, context, cb``) work unchanged.

**aws-samples#5 — Unhandled exception in routeEvent crashes batch**

``routeEvent`` uses ``Promise.allSettled`` internally, but
``resolveTokenSecretArn`` can throw ``AccessDeniedException``
SYNCHRONOUSLY before the ``allSettled`` guard is reached. The new
per-record try/catch from aws-samples#1 catches these too.

**aws-samples#9 — renderCommentBody not self-defending against uncoerced DDB strings**

The ``.toFixed(4)`` call on ``costUsd`` is the same bug class as the
``toFixed is not a function`` crash we fixed at the fanout boundary
in commit 9fe704e. Today the sole call site coerces via the shared
helper; a future caller that forgets to would crash.

Fix: ``renderCommentBody`` coerces ``durationS`` and ``costUsd``
internally via the shared ``coerceNumericOrNull`` helper (second
line of defense; caller's coercion remains the first). Widened
``CommentBodyInput`` fields to ``number | string | null`` to
honestly model the DDB Document-client boundary.

**aws-samples#12 — Markdown injection possible via prUrl in GitHub comment body**

``prUrl`` was interpolated directly into a Markdown link target
(``[link](${input.prUrl})``). A crafted URL containing ``)`` / ``|``
/ ``\n`` could break the table layout or inject content, and a
``javascript:`` scheme could produce a click-to-execute link on some
Markdown renderers.

Fix: new exported ``sanitizeMarkdownLinkTarget`` helper in
``shared/github-comment.ts`` rejects URLs containing
``\r\n\t\s)|]"<>`` characters, validates via ``new URL()``, and
rejects non-http(s) schemes. Returns ``null`` on rejection so
``renderCommentBody`` omits the Pull-request row entirely rather
than emitting a broken or unsafe link.

## Tests

+22 regression tests net (fanout 7 for aws-samples#1+aws-samples#5 + 3 for aws-samples#9; github-comment
12 for aws-samples#12):

- Fanout partial-batch: poison-pill isolation, mixed-batch (good
  record NOT in failures), observability warn, empty-failures
  regression guard, baseline pin that today's ``Promise.allSettled``
  containment still works.
- renderCommentBody numeric self-defense: string-typed values render
  correctly; non-finite strings collapse to null with warn; null
  does NOT warn.
- sanitizeMarkdownLinkTarget unit tests: accept clean http/https,
  reject 9 injection patterns, reject 4 non-http schemes
  (``javascript:``, ``data:``, ``file:``, ``ftp:``), reject
  malformed, handle null/undefined. Plus end-to-end assertions on
  ``renderCommentBody`` proving the PR row is omitted on rejection.

CDK suite: 1029 passing (was 1001).

Refs: krokoko code review on PR aws-samples#52 (findings 1, 5, 9, 12)

Author: bgagent

cdk/src/handlers/fanout-task-events.ts

@@ -46,7 +46,12 @@
 
 import { DynamoDBClient } from '@aws-sdk/client-dynamodb';
 import { DynamoDBDocumentClient, GetCommand, UpdateCommand } from '@aws-sdk/lib-dynamodb';
-import type { DynamoDBStreamEvent, DynamoDBStreamHandler, DynamoDBRecord } from 'aws-lambda';
+import type {
+  DynamoDBBatchItemFailure,
+  DynamoDBBatchResponse,
+  DynamoDBRecord,
+  DynamoDBStreamEvent,
+} from 'aws-lambda';
 import { clearTokenCache, resolveGitHubToken } from './shared/context-hydration';
 import { renderCommentBody, upsertTaskComment } from './shared/github-comment';
 import { logger } from './shared/logger';
@@ -693,9 +698,53 @@ export async function routeEvent(
 /**
  * Lambda entry point. Invoked by the DynamoDB Streams event source
  * mapping with batches of NEW_IMAGE records from `TaskEventsTable`.
+ *
+ * Returns a ``DynamoDBBatchResponse`` so the event-source-mapping's
+ * ``reportBatchItemFailures: true`` setting (see
+ * ``constructs/fanout-consumer.ts``) can honor partial-batch semantics.
+ * Without a structured return, a single poisonous record would cause
+ * Lambda to retry the **entire batch** from the stream checkpoint,
+ * replaying every sibling event and defeating the per-task ordering
+ * guarantee promised by ``ParallelizationFactor: 1`` upstream.
+ *
+ * Partial-failure surface (per-record try/catch below):
+ *   - ``routeEvent`` wraps each dispatcher in ``Promise.allSettled``, so
+ *     dispatcher rejections are already caught at the channel granularity
+ *     and do not reach here. What DOES reach here is a throw BEFORE the
+ *     ``allSettled`` — e.g. ``resolveTokenSecretArn`` throwing
+ *     ``AccessDeniedException`` on an IAM misconfig (deliberate hard fail
+ *     inside ``dispatchToGitHubComment``), a synchronous throw in
+ *     ``loadTaskForComment`` on a broken DDB env, or any future writer
+ *     that opens a non-``allSettled`` code path.
+ *   - Parse / filter / rate-limit errors are defensive — today they
+ *     cannot throw, but catching them keeps one stray ``throw`` in a
+ *     future refactor (e.g. a stricter ``parseStreamRecord``) from
+ *     crashing the whole batch.
+ *
+ * On any caught throw we push ``{ itemIdentifier: record.eventID }`` so
+ * Lambda retries ONLY that record, isolating the poison pill per
+ * design §6 + §8.9 expectations. Successful records are NOT in
+ * ``batchItemFailures`` and advance the stream checkpoint normally.
+ *
+ * Refs: PR #52 krokoko code review findings #1 and #5 (the fanout
+ * handler returned ``void`` despite ``reportBatchItemFailures: true``,
+ * and a ``routeEvent`` throw from ``resolveTokenSecretArn`` could crash
+ * the whole batch).
  */
-export const handler: DynamoDBStreamHandler = async (event: DynamoDBStreamEvent) => {
+// ``DynamoDBStreamHandler`` constrains the return to ``void | Promise<void>``,
+// which blocks the ``DynamoDBBatchResponse`` we must return for
+// ``reportBatchItemFailures: true`` to work (finding #1). Typing the
+// handler as a plain async function preserves the 3-arg Lambda
+// signature for existing call sites (tests pass ``event, context, cb``)
+// while allowing a structured return. Lambda's runtime accepts either
+// shape.
+export const handler = async (
+  event: DynamoDBStreamEvent,
+  _context?: unknown,
+  _callback?: unknown,
+): Promise<DynamoDBBatchResponse> => {
   const perTaskCounts = new Map<string, number>();
+  const batchItemFailures: DynamoDBBatchItemFailure[] = [];
   let processed = 0;
   let dispatched = 0;
   let dropped = 0;
@@ -706,33 +755,57 @@ export const handler: DynamoDBStreamHandler = async (event: DynamoDBStreamEvent)
 
   for (const record of event.Records) {
     processed++;
-    const ev = parseStreamRecord(record);
-    if (!ev) {
-      dropped++;
-      continue;
-    }
-    if (!shouldFanOut(ev, overrides)) {
-      dropped++;
-      continue;
-    }
+    try {
+      const ev = parseStreamRecord(record);
+      if (!ev) {
+        dropped++;
+        continue;
+      }
+      if (!shouldFanOut(ev, overrides)) {
+        dropped++;
+        continue;
+      }
 
-    const seen = perTaskCounts.get(ev.task_id) ?? 0;
-    if (seen >= MAX_EVENTS_PER_TASK_PER_INVOCATION) {
-      logger.warn('[fanout] per-task cap hit — dropping event', {
-        event: 'fanout.rate_limit.hit',
-        task_id: ev.task_id,
-        event_id: ev.event_id,
-        event_type: ev.event_type,
-        effective_event_type: effectiveEventType(ev),
-        cap: MAX_EVENTS_PER_TASK_PER_INVOCATION,
+      const seen = perTaskCounts.get(ev.task_id) ?? 0;
+      if (seen >= MAX_EVENTS_PER_TASK_PER_INVOCATION) {
+        logger.warn('[fanout] per-task cap hit — dropping event', {
+          event: 'fanout.rate_limit.hit',
+          task_id: ev.task_id,
+          event_id: ev.event_id,
+          event_type: ev.event_type,
+          effective_event_type: effectiveEventType(ev),
+          cap: MAX_EVENTS_PER_TASK_PER_INVOCATION,
+        });
+        dropped++;
+        continue;
+      }
+      perTaskCounts.set(ev.task_id, seen + 1);
+
+      const channels = await routeEvent(ev, overrides);
+      if (channels.length > 0) dispatched++;
+    } catch (err) {
+      // Poison-pill isolation: one record's unhandled throw must not
+      // crash the batch. See the handler doc block for the full list of
+      // paths that can reach here (notably AccessDeniedException from
+      // ``resolveTokenSecretArn``, finding #5).
+      //
+      // ``eventID`` is the stream-record identifier Lambda uses for the
+      // retry cursor; on Kinesis-style event-source-mappings with
+      // ``reportBatchItemFailures: true`` the service retries all
+      // records at-or-after the lowest-sequence failure. Returning even
+      // one failed itemIdentifier is enough to preserve ordering across
+      // the whole batch for that task.
+      const eventID = record.eventID;
+      logger.warn('[fanout] record threw — flagging for partial-batch retry', {
+        event: 'fanout.record.failed',
+        event_id: eventID,
+        error: err instanceof Error ? err.message : String(err),
+        error_name: err instanceof Error ? err.name : undefined,
       });
-      dropped++;
-      continue;
+      if (eventID !== undefined) {
+        batchItemFailures.push({ itemIdentifier: eventID });
+      }
     }
-    perTaskCounts.set(ev.task_id, seen + 1);
-
-    const channels = await routeEvent(ev, overrides);
-    if (channels.length > 0) dispatched++;
   }
 
   logger.info('[fanout] batch complete', {
@@ -741,6 +814,9 @@ export const handler: DynamoDBStreamHandler = async (event: DynamoDBStreamEvent)
     processed,
     dispatched,
     dropped,
+    failed: batchItemFailures.length,
     unique_tasks: perTaskCounts.size,
   });
+
+  return { batchItemFailures };
 };

cdk/src/handlers/shared/github-comment.ts

@@ -47,6 +47,7 @@
  */
 
 import { logger } from './logger';
+import { coerceNumericOrNull } from './numeric';
 
 /** GitHub REST v3 media type — required on writes for stable behavior. */
 const GITHUB_ACCEPT = 'application/vnd.github.v3+json';
@@ -284,31 +285,122 @@ function sanitizeEventType(eventType: string): string {
   return eventType.replace(/[`|\r\n]/g, '');
 }
 
+/**
+ * Sanitize a ``prUrl`` before interpolating it into a Markdown link
+ * target (``[link](<here>)``). Without this guard a crafted URL could
+ * break the table layout or inject trailing Markdown: ``)`` closes the
+ * link prematurely, ``|`` starts a new table column, and CR / LF / ``]``
+ * / ``"`` each let an attacker extend the comment body past the link.
+ *
+ * Strategy: strip the six characters that are meaningful inside
+ * ``[text](target)`` Markdown AND reject anything that does not parse
+ * as an ``http://`` or ``https://`` URL after the strip. The strip is
+ * deliberately conservative — a legitimate GitHub PR URL
+ * (``https://github.com/owner/repo/pull/42``) never contains any of the
+ * listed characters, so false positives are effectively zero. Any URL
+ * that fails validation returns ``null`` and the caller must omit the
+ * Pull-request row entirely rather than risk a broken-layout comment.
+ *
+ * Refs: PR #52 krokoko code review finding #12 (Markdown injection
+ * possible via ``prUrl`` in GitHub comment body).
+ */
+export function sanitizeMarkdownLinkTarget(url: string | null | undefined): string | null {
+  if (url === null || url === undefined) return null;
+  // Reject anything carrying a Markdown-significant character. We do
+  // NOT attempt to URL-encode these — encoded ``)`` (``%29``) would
+  // render correctly, but encoding opens a harder surface to reason
+  // about (e.g. an attacker who gets ``%0A`` past this function could
+  // still break the table on some Markdown renderers). A flat reject
+  // keeps the contract simple and the comment row trustworthy.
+  if (/[\r\n\t\s)|\]"<>`]/.test(url)) return null;
+  // Validate the parsed URL is http(s). A ``javascript:`` link target
+  // is also attacker-controlled content and has no place in a status
+  // comment. ``new URL`` throws on malformed input.
+  let parsed: URL;
+  try {
+    parsed = new URL(url);
+  } catch {
+    return null;
+  }
+  if (parsed.protocol !== 'https:' && parsed.protocol !== 'http:') return null;
+  return url;
+}
+
 function bgagentMarker(taskId: string): string {
   return `<!-- ${BGAGENT_COMMENT_MARKER_PREFIX}${taskId} -->`;
 }
 
-/** A compact terminal-friendly summary the GitHub comment displays as
- *  the task progresses. Kept small on purpose — GitHub truncates long
- *  comments in mobile / email notifications and the PR activity log
- *  accumulates the full history anyway. */
+/**
+ * A compact terminal-friendly summary the GitHub comment displays as
+ * the task progresses. Kept small on purpose — GitHub truncates long
+ * comments in mobile / email notifications and the PR activity log
+ * accumulates the full history anyway.
+ *
+ * The numeric fields accept ``string`` alongside ``number | null`` to
+ * honestly model the DynamoDB Document-client boundary: ``Number``
+ * attributes deserialize as JS ``string`` in some code paths (see
+ * ``shared/numeric.ts``). ``renderCommentBody`` coerces defensively so
+ * a future caller that forgets to run the shared coercion helper at
+ * its own boundary does not crash the way the fanout dispatcher did in
+ * Scenario 7-ext (``costUsd.toFixed is not a function``, commit
+ * ``9fe704e``).
+ */
 export interface CommentBodyInput {
   readonly taskId: string;
   readonly status: string;
   readonly repo: string;
   readonly latestEventType: string;
   readonly latestEventAt: string;
   readonly prUrl: string | null;
-  readonly durationS: number | null;
-  readonly costUsd: number | null;
+  readonly durationS: number | string | null;
+  readonly costUsd: number | string | null;
 }
 
 /**
- * Render the Markdown body for the in-place comment. Pure: no logger,
- * no timing, no side effects — callers can snapshot-test the exact
- * bytes without monkey-patching anything.
+ * Render the Markdown body for the in-place comment. Pure: no timing,
+ * no network — callers can snapshot-test the exact bytes. The only
+ * side effect is a ``logger.warn`` via ``coerceNumericOrNull`` if a
+ * numeric field arrives with a non-finite value (e.g. ``'NaN'``), which
+ * surfaces upstream writer bugs instead of silently dropping the row.
+ *
+ * Defense-in-depth vs. the caller's coercion (finding #9):
+ *   - Callers SHOULD coerce DDB numerics at their own boundary using
+ *     ``coerceNumericOrNull`` so the warn log carries their context
+ *     (task_id, event_id). The fanout dispatcher does this today.
+ *   - ``renderCommentBody`` coerces again internally so a future caller
+ *     that forgets the boundary step (e.g. a Chunk K reconciler
+ *     reading raw DDB items) still degrades to a null-omitted row
+ *     instead of throwing ``TypeError: .toFixed is not a function``.
+ *     Non-finite values (``NaN``, ``Infinity``) collapse to null and
+ *     omit the row; finite values (including parseable strings) render
+ *     normally.
+ *
+ * Markdown-link target sanitization (finding #12):
+ *   - ``prUrl`` is interpolated into a Markdown link (``[link](<here>)``).
+ *     Without sanitization a crafted URL containing ``)`` / ``|`` / CR
+ *     / LF could break the table layout or inject trailing content.
+ *     ``sanitizeMarkdownLinkTarget`` strips the injection surface and
+ *     validates the URL is http(s); a rejected URL omits the row
+ *     rather than rendering a broken or misleading link.
  */
 export function renderCommentBody(input: CommentBodyInput): string {
+  // Coerce DDB-string numerics defensively — see doc block above. The
+  // context object gives the ``numeric.coercion_failed`` warn enough
+  // breadcrumbs (field + task_id) to trace back to the upstream writer.
+  const durationS = coerceNumericOrNull(
+    input.durationS,
+    { field: 'duration_s', task_id: input.taskId },
+    logger,
+  );
+  const costUsd = coerceNumericOrNull(
+    input.costUsd,
+    { field: 'cost_usd', task_id: input.taskId },
+    logger,
+  );
+  // Sanitize the PR link target before interpolation. A rejected URL
+  // returns null and the row is omitted.
+  const safePrUrl = sanitizeMarkdownLinkTarget(input.prUrl);
+
   const lines: string[] = [];
   lines.push(bgagentMarker(input.taskId));
   lines.push(`### Background agent — ${input.status}`);
@@ -319,14 +411,14 @@ export function renderCommentBody(input: CommentBodyInput): string {
   lines.push(`| Repo  | \`${input.repo}\` |`);
   lines.push(`| Status | **${input.status}** |`);
   lines.push(`| Last event | \`${sanitizeEventType(input.latestEventType)}\` @ ${input.latestEventAt} |`);
-  if (input.prUrl) {
-    lines.push(`| Pull request | [link](${input.prUrl}) |`);
+  if (safePrUrl) {
+    lines.push(`| Pull request | [link](${safePrUrl}) |`);
   }
-  if (input.durationS !== null) {
-    lines.push(`| Duration | ${input.durationS}s |`);
+  if (durationS !== null) {
+    lines.push(`| Duration | ${durationS}s |`);
   }
-  if (input.costUsd !== null) {
-    lines.push(`| Cost | $${input.costUsd.toFixed(4)} |`);
+  if (costUsd !== null) {
+    lines.push(`| Cost | $${costUsd.toFixed(4)} |`);
   }
   const rendered = lines.join('\n');
   if (rendered.length <= MAX_COMMENT_BODY_CHARS) return rendered;