chore(review): implement fixes from automated review

Author: bgagent

agent/src/pipeline.py

@@ -473,12 +473,27 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
             if config.attachments:
                 from attachments import download_attachments
 
-                with task_span("task.attachment_download"):
-                    prepared_attachments = download_attachments(config.attachments, setup.repo_dir)
-                progress.write_agent_milestone(
-                    "attachments_downloaded",
-                    f"count={len(prepared_attachments)}",
-                )
+                try:
+                    with task_span("task.attachment_download"):
+                        prepared_attachments = download_attachments(
+                            config.attachments, setup.repo_dir
+                        )
+                    progress.write_agent_milestone(
+                        "attachments_downloaded",
+                        f"count={len(prepared_attachments)}",
+                    )
+                except RuntimeError as e:
+                    log("ERROR", f"Attachment integrity check failed: {e}")
+                    raise RuntimeError(
+                        f"Attachment download/verification failed: {e}. "
+                        "The task cannot proceed without valid attachments."
+                    ) from e
+                except Exception as e:
+                    err_type = type(e).__name__
+                    log("ERROR", f"Attachment download failed: {err_type}: {e}")
+                    raise RuntimeError(
+                        f"Failed to download task attachments from S3: {err_type}: {e}"
+                    ) from e
 
             # Log discovered repo-level project configuration
             # (all files loaded by setting_sources=["project"])

cdk/src/handlers/cleanup-pending-uploads.ts

@@ -40,7 +40,7 @@ import {
   UpdateItemCommand,
   PutItemCommand,
 } from '@aws-sdk/client-dynamodb';
-import { DeleteObjectsCommand, ListObjectsV2Command, S3Client } from '@aws-sdk/client-s3';
+import { DeleteObjectsCommand, ListObjectVersionsCommand, S3Client } from '@aws-sdk/client-s3';
 import { ulid } from 'ulid';
 import { ATTACHMENT_OBJECT_KEY_PREFIX } from '../constructs/attachments-bucket';
 import { logger } from './shared/logger';
@@ -187,41 +187,43 @@ async function cancelExpiredTask(task: ExpiredTask): Promise<boolean> {
 async function cleanupTaskAttachments(task: ExpiredTask): Promise<void> {
   const prefix = `${ATTACHMENT_OBJECT_KEY_PREFIX}${task.user_id}/${task.task_id}/`;
 
-  let continuationToken: string | undefined;
+  let keyMarker: string | undefined;
+  let versionIdMarker: string | undefined;
   let totalDeleted = 0;
 
   do {
-    const listResp = await s3.send(new ListObjectsV2Command({
+    const listResp = await s3.send(new ListObjectVersionsCommand({
       Bucket: ATTACHMENTS_BUCKET,
       Prefix: prefix,
-      ContinuationToken: continuationToken,
+      KeyMarker: keyMarker,
+      VersionIdMarker: versionIdMarker,
     }));
 
-    const objects = listResp.Contents;
-    if (!objects || objects.length === 0) break;
+    // Collect all versions and delete markers for deletion
+    const objects = [
+      ...(listResp.Versions ?? []).map(v => ({ Key: v.Key!, VersionId: v.VersionId })),
+      ...(listResp.DeleteMarkers ?? []).map(d => ({ Key: d.Key!, VersionId: d.VersionId })),
+    ].filter(obj => obj.Key !== undefined);
 
-    const keys = objects
-      .map(obj => obj.Key)
-      .filter((key): key is string => key !== undefined);
+    if (objects.length === 0) break;
 
-    if (keys.length > 0) {
-      const deleteResp = await s3.send(new DeleteObjectsCommand({
-        Bucket: ATTACHMENTS_BUCKET,
-        Delete: { Objects: keys.map(Key => ({ Key })) },
-      }));
-
-      if (deleteResp.Errors && deleteResp.Errors.length > 0) {
-        logger.error('Partial S3 cleanup failure for expired pending-upload task', {
-          task_id: task.task_id,
-          failedKeys: deleteResp.Errors.map(e => e.Key),
-        });
-      }
+    const deleteResp = await s3.send(new DeleteObjectsCommand({
+      Bucket: ATTACHMENTS_BUCKET,
+      Delete: { Objects: objects.map(({ Key, VersionId }) => ({ Key, VersionId })) },
+    }));
 
-      totalDeleted += keys.length - (deleteResp.Errors?.length ?? 0);
+    if (deleteResp.Errors && deleteResp.Errors.length > 0) {
+      logger.error('Partial S3 cleanup failure for expired pending-upload task', {
+        task_id: task.task_id,
+        failedKeys: deleteResp.Errors.map(e => e.Key),
+      });
     }
 
-    continuationToken = listResp.NextContinuationToken;
-  } while (continuationToken);
+    totalDeleted += objects.length - (deleteResp.Errors?.length ?? 0);
+
+    keyMarker = listResp.NextKeyMarker;
+    versionIdMarker = listResp.NextVersionIdMarker;
+  } while (keyMarker);
 
   if (totalDeleted > 0) {
     logger.info('Cleaned up S3 objects for expired pending-upload task', {

cdk/src/handlers/confirm-uploads.ts

@@ -799,6 +799,10 @@ async function decrementConcurrency(userId: string): Promise<void> {
         error: err instanceof Error ? err.message : String(err),
         metric_type: 'concurrency_counter_leak',
       });
+      throw new Error(
+        `Concurrency counter decrement failed for user ${userId} after ${maxAttempts} attempts. ` +
+        'Manual intervention may be required to reset the counter.',
+      );
     }
   }
 }

cdk/src/handlers/shared/attachment-screening.ts

@@ -404,7 +404,18 @@ function assertImageDimensionsWithinLimits(
   } else if (contentType === 'image/jpeg') {
     dims = readJpegDimensions(content);
     if (!dims) {
-      // Non-fatal: if we can't parse dimensions, let Bedrock handle rejection
+      // Fail-closed for large JPEGs where dimensions cannot be verified (> 5 MB).
+      // Smaller files are allowed through to Bedrock which will reject if oversized.
+      if (content.length > 5 * 1024 * 1024) {
+        throw new AttachmentScreeningError(
+          `Image "${filename}" is ${(content.length / (1024 * 1024)).toFixed(1)} MB and its dimensions ` +
+          'could not be verified. Please use a standard JPEG encoder or convert to PNG.',
+        );
+      }
+      logger.warn('Could not parse JPEG dimensions — relying on Bedrock validation', {
+        filename,
+        size_bytes: content.length,
+      });
       return;
     }
   } else {

cdk/src/handlers/shared/create-task-core.ts

@@ -25,7 +25,7 @@ import { BedrockRuntimeClient, ApplyGuardrailCommand } from '@aws-sdk/client-bed
 import { DynamoDBClient } from '@aws-sdk/client-dynamodb';
 import { InvokeCommand, LambdaClient } from '@aws-sdk/client-lambda';
 import { PutObjectCommand, DeleteObjectsCommand, S3Client } from '@aws-sdk/client-s3';
-import { DynamoDBDocumentClient, PutCommand, QueryCommand, GetCommand } from '@aws-sdk/lib-dynamodb';
+import { DynamoDBDocumentClient, PutCommand, QueryCommand, GetCommand, UpdateCommand } from '@aws-sdk/lib-dynamodb';
 import { createPresignedPost } from '@aws-sdk/s3-presigned-post';
 import type { APIGatewayProxyResult } from 'aws-lambda';
 import { ulid } from 'ulid';
@@ -666,12 +666,31 @@ export async function createTaskCore(
         taskRecord, validatedAttachments, context.userId, taskId, s3Client,
       );
     } catch (presignErr) {
-      logger.error('Failed to generate presigned upload instructions — task orphaned in PENDING_UPLOADS', {
+      logger.error('Failed to generate presigned upload instructions — transitioning task to FAILED', {
         task_id: taskId,
         error: presignErr instanceof Error ? presignErr.message : String(presignErr),
         request_id: requestId,
         metric_type: 'presigned_post_generation_failure',
       });
+      // Transition task to FAILED so it doesn't remain orphaned in PENDING_UPLOADS
+      try {
+        await ddb.send(new UpdateCommand({
+          TableName: TABLE_NAME,
+          Key: { task_id: taskId },
+          UpdateExpression: 'SET #s = :failed, error_message = :err, updated_at = :now',
+          ExpressionAttributeNames: { '#s': 'status' },
+          ExpressionAttributeValues: {
+            ':failed': TaskStatus.FAILED,
+            ':err': 'Failed to generate upload instructions. Please try again.',
+            ':now': new Date().toISOString(),
+          },
+        }));
+      } catch (cleanupErr) {
+        logger.error('Failed to transition orphaned task to FAILED', {
+          task_id: taskId,
+          error: cleanupErr instanceof Error ? cleanupErr.message : String(cleanupErr),
+        });
+      }
       return errorResponse(500, ErrorCode.INTERNAL_ERROR,
         'Failed to generate upload instructions. Please try again.', requestId);
     }

cdk/src/handlers/shared/orchestrator.ts

@@ -483,6 +483,7 @@ export async function hydrateAndTransition(task: TaskRecord, blueprintConfig?: B
         bucketName: ATTACHMENTS_BUCKET_NAME,
         screeningConfig,
         githubToken,
+        githubInstallationDomain: process.env.GITHUB_INSTALLATION_DOMAIN,
       },
     );
   }

cdk/src/handlers/shared/resolve-url-attachments.ts

@@ -67,7 +67,13 @@ const PRIVATE_IP_RANGES = [
   { prefix: '169.254.', mask: null },
   { prefix: '127.', mask: null },
   { prefix: '0.', mask: null },
-  { prefix: '100.64.', mask: null }, // CGN / Shared Address Space (RFC 6598)
+  {
+    prefix: '100.',
+    mask: (ip: string) => {
+      const second = parseInt(ip.split('.')[1], 10);
+      return second >= 64 && second <= 127; // 100.64.0.0/10 (RFC 6598)
+    },
+  },
   // IPv6
   { prefix: '::1', mask: null },
   { prefix: '::', mask: (ip: string) => ip === '::' }, // Unspecified address (could route to localhost)
@@ -131,14 +137,29 @@ async function resolveAndValidate(hostname: string): Promise<string> {
   try {
     // Try IPv4 first (more common for HTTP endpoints)
     addresses = await dns.resolve4(hostname);
-  } catch (ipv4Err) {
-    try {
-      addresses = await dns.resolve6(hostname);
-    } catch (ipv6Err) {
-      throw new AttachmentResolutionError(
-        `DNS resolution failed for '${hostname}'. Check that the URL is correct and the server is reachable.`,
-        { cause: new AggregateError([ipv4Err, ipv6Err], `Both IPv4 and IPv6 resolution failed for '${hostname}'`) },
-      );
+  } catch (ipv4Err: any) {
+    // Only fall through to IPv6 for NODATA/NXDOMAIN — system errors should propagate
+    const dnsNoRecordCodes = ['ENODATA', 'ENOTFOUND', 'NODATA'];
+    if (!dnsNoRecordCodes.includes(ipv4Err?.code)) {
+      // System-level DNS failure (ENOMEM, ESERVFAIL, etc.) — do not mask
+      try {
+        addresses = await dns.resolve6(hostname);
+      } catch (ipv6Err) {
+        throw new AttachmentResolutionError(
+          `DNS resolution failed for '${hostname}': ${ipv4Err?.code ?? ipv4Err?.message ?? 'unknown error'}`,
+          { cause: new AggregateError([ipv4Err, ipv6Err], `Both IPv4 and IPv6 resolution failed for '${hostname}'`) },
+        );
+      }
+    } else {
+      // No IPv4 records — try IPv6
+      try {
+        addresses = await dns.resolve6(hostname);
+      } catch (ipv6Err) {
+        throw new AttachmentResolutionError(
+          `DNS resolution failed for '${hostname}'. Check that the URL is correct and the server is reachable.`,
+          { cause: new AggregateError([ipv4Err, ipv6Err], `Both IPv4 and IPv6 resolution failed for '${hostname}'`) },
+        );
+      }
     }
   }
 
@@ -207,7 +228,17 @@ async function pinnedHttpsRequest(
       },
       (res) => {
         const chunks: Buffer[] = [];
-        res.on('data', (chunk: Buffer) => chunks.push(chunk));
+        let totalBytes = 0;
+        res.on('data', (chunk: Buffer) => {
+          totalBytes += chunk.length;
+          if (totalBytes > MAX_FETCH_SIZE_BYTES) {
+            res.destroy();
+            agent.destroy();
+            reject(new Error(`Response exceeds ${MAX_FETCH_SIZE_BYTES} byte size limit`));
+            return;
+          }
+          chunks.push(chunk);
+        });
         res.on('end', () => {
           const body = Buffer.concat(chunks);
           const responseHeaders = new Headers();

cdk/src/handlers/shared/types.ts

@@ -418,10 +418,47 @@ export type ScreeningResult =
   | { readonly status: 'blocked'; readonly screened_at: string; readonly categories: [string, ...string[]] };
 
 // ---------------------------------------------------------------------------
-// Attachment record (persisted metadata in TaskRecord)
+// Attachment record (persisted metadata in TaskRecord) — discriminated union
+// keyed on screening.status ensures that passed records always have storage fields.
 // ---------------------------------------------------------------------------
 
-export interface AttachmentRecord {
+interface BaseAttachmentRecord {
+  readonly attachment_id: string;
+  readonly type: AttachmentType;
+  readonly content_type: string;
+  readonly filename: string;
+  readonly source_url?: string;
+  readonly token_estimate?: number;
+}
+
+export interface PendingAttachmentRecord extends BaseAttachmentRecord {
+  readonly screening: { readonly status: 'pending' };
+  readonly s3_key?: string;
+  readonly s3_version_id?: string;
+  readonly size_bytes?: number;
+  readonly checksum_sha256?: string;
+}
+
+export interface PassedAttachmentRecord extends BaseAttachmentRecord {
+  readonly screening: { readonly status: 'passed'; readonly screened_at: string };
+  readonly s3_key: string;
+  readonly s3_version_id: string;
+  readonly size_bytes: number;
+  readonly checksum_sha256: string;
+}
+
+export interface BlockedAttachmentRecord extends BaseAttachmentRecord {
+  readonly screening: { readonly status: 'blocked'; readonly screened_at: string; readonly categories: [string, ...string[]] };
+  readonly s3_key?: string;
+  readonly s3_version_id?: string;
+  readonly size_bytes?: number;
+  readonly checksum_sha256?: string;
+}
+
+export type AttachmentRecord = PendingAttachmentRecord | PassedAttachmentRecord | BlockedAttachmentRecord;
+
+/** Parameters for creating an AttachmentRecord — accepts the union of all fields. */
+export type CreateAttachmentRecordParams = {
   readonly attachment_id: string;
   readonly type: AttachmentType;
   readonly content_type: string;
@@ -433,22 +470,23 @@ export interface AttachmentRecord {
   readonly source_url?: string;
   readonly checksum_sha256?: string;
   readonly token_estimate?: number;
-}
-
-/** Parameters for creating an AttachmentRecord with cross-field invariant validation. */
-export type CreateAttachmentRecordParams = AttachmentRecord;
+};
 
 /**
  * Factory function enforcing cross-field invariants on AttachmentRecord construction.
- * Validates that required fields are present based on screening status and type.
+ * Returns the appropriate discriminated union variant based on screening status.
  */
 export function createAttachmentRecord(params: CreateAttachmentRecordParams): AttachmentRecord {
   if (params.screening.status === 'passed') {
     if (!params.s3_key || !params.s3_version_id || !params.checksum_sha256 || !params.size_bytes) {
       throw new Error('Passed screening requires s3_key, s3_version_id, checksum_sha256, and size_bytes');
     }
+    return params as PassedAttachmentRecord;
+  }
+  if (params.screening.status === 'blocked') {
+    return params as BlockedAttachmentRecord;
   }
-  return params;
+  return params as PendingAttachmentRecord;
 }
 
 // ---------------------------------------------------------------------------

cdk/src/handlers/slack-command-processor.ts

@@ -375,8 +375,8 @@ const SLACK_FILE_MAX_SIZE_BYTES = 10 * 1024 * 1024;
 /** Max number of file attachments per Slack message. */
 const SLACK_FILE_MAX_COUNT = 10;
 
-/** MIME types supported for attachments (must match validation.ts). */
-const SUPPORTED_IMAGE_MIMES = new Set(['image/png', 'image/jpeg', 'image/gif', 'image/webp']);
+/** MIME types supported for attachments (must match validation.ts — PNG/JPEG only). */
+const SUPPORTED_IMAGE_MIMES = new Set(['image/png', 'image/jpeg']);
 const SUPPORTED_FILE_MIMES = new Set([
   'text/plain', 'text/csv', 'text/markdown', 'application/json',
   'application/pdf', 'text/x-log',
@@ -428,6 +428,13 @@ async function extractSlackFileAttachments(
       continue;
     }
 
+    // Validate the download URL points to a legitimate Slack domain before
+    // sending the bot token — prevents SSRF and token exfiltration via crafted events.
+    if (!isSlackFileUrl(file.url_private_download)) {
+      errors.push(`\`${file.name}\` (invalid download URL — not a Slack domain)`);
+      continue;
+    }
+
     // Download the file from Slack CDN using the bot token
     try {
       const response = await fetch(file.url_private_download, {
@@ -476,6 +483,17 @@ async function extractSlackFileAttachments(
   return attachments;
 }
 
+/** Validate that a URL points to a legitimate Slack file domain. */
+function isSlackFileUrl(url: string): boolean {
+  try {
+    const parsed = new URL(url);
+    return parsed.protocol === 'https:'
+      && (parsed.hostname === 'files.slack.com' || parsed.hostname.endsWith('.slack.com'));
+  } catch {
+    return false;
+  }
+}
+
 // ─── Helpers ──────────────────────────────────────────────────────────────────
 
 async function lookupPlatformUser(teamId: string, userId: string): Promise<string | null> {