fix(jira): address #619 review — orphan cleanup, real-byte cap, comment screening, idempotency (#577)

Addresses isadeks's review on PR #619:

1. Zero-byte attachment → JiraAttachmentError (fail-closed). A 0-byte body
   passed magic-byte + screening then tripped createAttachmentRecord's size
   guard with a plain Error outside the fail-closed conversion (no ❌ comment).
   Now rejected explicitly with a Jira comment.
2. 50 MB total-attachment cap enforced on REAL downloaded bytes, not the
   attacker-declared `size` metadata (which can under-report). Cumulative real
   bytes tracked in the download loop; throws once over the ceiling.
3. S3 orphan cleanup. downloadScreenAndStoreJiraAttachments now tracks uploaded
   keys and best-effort-deletes them if the batch throws mid-way; the processor
   deletes the returned pre-screened objects when createTaskCore returns non-201
   (and on a 200 idempotent replay, whose objects the replayed task doesn't
   reference). New exported cleanupPreScreenedAttachments helper.
4. Comment content screened SEPARATELY and dropped fail-open. Third-party
   comment text that trips the guardrail no longer fails the reporter's task —
   screenCommentsOrDrop screens the folded comment block and drops it (task
   proceeds) on GUARDRAIL_INTERVENED / screening outage / no guardrail.
5. Deterministic idempotency key (`jira-<issueKey>-<webhookTimestamp>`) so an
   async webhook re-delivery dedupes instead of minting a duplicate task and
   re-downloading every attachment. A 200 idempotent replay is handled as
   success (no ❌ comment), cleaning up the round's re-uploaded objects.
6. Webhook-processor timeout 60s → 300s. 10 serial 10s attachment fetches can
   sum past 60s; a mid-loop kill would orphan objects and force a retry.

Tests: +5 (zero-byte reject, real-byte total cap, mid-batch orphan cleanup,
comment-blocked fail-open drop, deterministic idempotency key + 200-replay).
mise run build green (cdk 2362).

Author: ayushtr-aws

cdk/src/constructs/jira-integration.ts

@@ -40,10 +40,13 @@ const DEFAULT_TASK_RETENTION_DAYS = 90;
  * Webhook-processor Lambda timeout (seconds). The processor is invoked
  * asynchronously (not behind the API Gateway 30s deadline), so it can run
  * longer than the receiver. #577 added serial authenticated download +
- * Bedrock screening of up to 10 Jira attachments, so 60s leaves headroom over
- * the per-attachment fetch (10s) + screening-retry budget.
+ * Bedrock screening of up to 10 Jira attachments; the per-attachment fetch
+ * timeout alone (10s) can sum past 60s across a full batch, and a mid-loop
+ * kill would orphan objects and force an idempotent retry. 300s covers the
+ * worst-case serial batch with headroom. (A future optimization could
+ * parallelize the download/screen loop and lower this again.)
  */
-const WEBHOOK_PROCESSOR_TIMEOUT_SECONDS = 60;
+const WEBHOOK_PROCESSOR_TIMEOUT_SECONDS = 300;
 
 /**
  * Marker key embedded in the auto-generated stack-wide webhook-secret

cdk/src/handlers/jira-webhook-processor.ts

@@ -18,7 +18,7 @@
  */
 
 import * as crypto from 'crypto';
-import { BedrockRuntimeClient } from '@aws-sdk/client-bedrock-runtime';
+import { BedrockRuntimeClient, ApplyGuardrailCommand } from '@aws-sdk/client-bedrock-runtime';
 import { DynamoDBClient } from '@aws-sdk/client-dynamodb';
 import { S3Client } from '@aws-sdk/client-s3';
 import { DynamoDBDocumentClient, GetCommand, ScanCommand } from '@aws-sdk/lib-dynamodb';
@@ -27,6 +27,7 @@ import type { ScreeningConfig } from './shared/attachment-screening';
 import { createTaskCore } from './shared/create-task-core';
 import { extractDescriptionMarkdown } from './shared/jira-adf';
 import {
+  cleanupPreScreenedAttachments,
   downloadScreenAndStoreJiraAttachments,
   fetchRecentHumanComments,
   JiraAttachmentError,
@@ -46,6 +47,9 @@ const USER_MAPPING_TABLE = process.env.JIRA_USER_MAPPING_TABLE_NAME!;
 const WORKSPACE_REGISTRY_TABLE = process.env.JIRA_WORKSPACE_REGISTRY_TABLE_NAME;
 const DEFAULT_LABEL_FILTER = 'bgagent';
 
+/** Max length of the idempotency key (matches validation's IDEMPOTENCY_KEY_PATTERN). */
+const MAX_IDEMPOTENCY_KEY_LENGTH = 128;
+
 // Attachment enrichment (#577). The processor downloads Jira `media` file
 // attachments, screens them through the Bedrock Guardrail, and uploads the
 // cleaned bytes to S3 before creating the task. All three must be configured;
@@ -415,7 +419,13 @@ export async function handler(event: ProcessorEvent): Promise<void> {
     const tenantCtx = { cloudId, registryTableName: WORKSPACE_REGISTRY_TABLE };
 
     // Recent human comments — advisory context, never gate task creation.
-    comments = await fetchRecentHumanComments(tenantCtx, issue.key);
+    const fetchedComments = await fetchRecentHumanComments(tenantCtx, issue.key);
+    // Fail-OPEN on comment content policy: comments are third-party text the
+    // reporter didn't write, so a policy-tripping comment must not fail the
+    // reporter's task. Screen the rendered comment section on its own and drop
+    // it (not the task) if the guardrail intervenes. (createTaskCore separately
+    // screens the description, which the reporter authored.)
+    comments = await screenCommentsOrDrop(fetchedComments, issue.key, cloudId);
 
     const rawAttachments = issue.fields?.attachment;
     if (Array.isArray(rawAttachments) && rawAttachments.length > 0) {
@@ -481,17 +491,43 @@ export async function handler(event: ProcessorEvent): Promise<void> {
       channelSource: 'jira',
       channelMetadata,
       taskId,
+      // Deterministic key so an async re-delivery of the same trigger event
+      // dedupes instead of minting a second task (and re-downloading every
+      // attachment). Keyed on issue + webhook timestamp, matching the
+      // receiver's dedup key shape.
+      idempotencyKey: buildIdempotencyKey(issue.key, payload.timestamp),
       ...(preScreenedAttachments.length > 0 && { preScreenedAttachments }),
     },
     requestId,
   );
 
+  if (result.statusCode === 200) {
+    // Idempotent replay: this is a duplicate delivery of the same trigger event
+    // (createTaskCore matched the deterministic idempotency key to an existing
+    // task). Not a failure — but the attachments we re-downloaded and uploaded
+    // this round are keyed on a fresh taskId the replayed task doesn't
+    // reference, so delete them rather than orphan them. No ❌ comment.
+    logger.info('Jira-triggered task was an idempotent replay (duplicate delivery)', {
+      issue_key: issue.key,
+      request_id: requestId,
+    });
+    if (preScreenedAttachments.length > 0 && s3Client && ATTACHMENTS_BUCKET) {
+      await cleanupPreScreenedAttachments(s3Client, ATTACHMENTS_BUCKET, preScreenedAttachments);
+    }
+    return;
+  }
+
   if (result.statusCode !== 201) {
     logger.warn('Jira-triggered task creation returned non-201', {
       status: result.statusCode,
       body: result.body,
       issue_key: issue.key,
     });
+    // Don't orphan the attachment objects we uploaded before this call failed —
+    // createTaskCore only rolls back its own inline uploads, not ours.
+    if (preScreenedAttachments.length > 0 && s3Client && ATTACHMENTS_BUCKET) {
+      await cleanupPreScreenedAttachments(s3Client, ATTACHMENTS_BUCKET, preScreenedAttachments);
+    }
     await safeReportIssueFailure(
       issue.key,
       cloudId,
@@ -648,6 +684,71 @@ function truncateCommentSection(section: string, budget: number): string {
   return section.slice(0, room) + COMMENT_TRUNCATION_NOTICE;
 }
 
+/**
+ * Screen the rendered comment block through the Bedrock Guardrail on its own,
+ * so third-party comment content that trips the policy is DROPPED (fail-open)
+ * rather than gating the reporter's task. Returns the comments unchanged when
+ * they pass, and `[]` when the guardrail intervenes or is unavailable — the
+ * task still proceeds with the reporter-authored summary/description (which
+ * createTaskCore screens separately). This keeps the comment-enrichment
+ * contract fail-open end to end (issue #577 review, item 4).
+ */
+async function screenCommentsOrDrop(
+  comments: RenderedComment[],
+  issueKey: string,
+  cloudId: string,
+): Promise<RenderedComment[]> {
+  if (comments.length === 0) return comments;
+  if (!bedrockClient || !GUARDRAIL_ID || !GUARDRAIL_VERSION) {
+    // No guardrail configured — drop unscreened third-party text rather than
+    // route it, unscreened, into the agent context.
+    logger.warn('Dropping Jira comments: guardrail not configured to screen them', {
+      issue_key: issueKey,
+      jira_cloud_id: cloudId,
+    });
+    return [];
+  }
+  const text = renderCommentSection(comments);
+  try {
+    const result = await bedrockClient.send(new ApplyGuardrailCommand({
+      guardrailIdentifier: GUARDRAIL_ID,
+      guardrailVersion: GUARDRAIL_VERSION,
+      source: 'INPUT',
+      content: [{ text: { text } }],
+    }));
+    if (result.action === 'GUARDRAIL_INTERVENED') {
+      logger.warn('Dropping Jira comments: blocked by content policy (task still proceeds)', {
+        issue_key: issueKey,
+        jira_cloud_id: cloudId,
+      });
+      return [];
+    }
+    return comments;
+  } catch (err) {
+    // Fail-open on a screening outage too — comments are advisory.
+    logger.warn('Dropping Jira comments: screening unavailable (task still proceeds)', {
+      issue_key: issueKey,
+      jira_cloud_id: cloudId,
+      error: err instanceof Error ? err.message : String(err),
+    });
+    return [];
+  }
+}
+
+/**
+ * Deterministic idempotency key for a trigger event: `<issueKey>#<timestamp>`,
+ * sanitized to the allowed key charset (`[A-Za-z0-9_-]{1,128}`). A webhook
+ * re-delivery of the same event yields the same key so createTaskCore dedupes
+ * instead of creating a duplicate task (and re-downloading attachments). Falls
+ * back to undefined if we can't form a stable key, preserving prior behavior.
+ */
+function buildIdempotencyKey(issueKey: string, timestamp: number | undefined): string | undefined {
+  if (typeof timestamp !== 'number' || !Number.isFinite(timestamp)) return undefined;
+  const raw = `jira-${issueKey}-${timestamp}`;
+  const sanitized = raw.replace(/[^a-zA-Z0-9_-]/g, '-').slice(0, MAX_IDEMPOTENCY_KEY_LENGTH);
+  return sanitized || undefined;
+}
+
 /**
  * Extract image URLs from the rendered description markdown. Same limits
  * as the Linear processor: HTTPS only, capped at 10.