feat(attachments): add multimodal task attachments support#176
Conversation
Resolve conflicts in attachment-screening and create-task-core, keeping URL hydration pending records, presigned upload instructions, and the shared image-tokens module. Co-authored-by: Cursor <cursoragent@cursor.com>
…etected during testing
isadeks
left a comment
There was a problem hiding this comment.
Blockers (must fix before merge)
-
Cleanup-on-failed-conditional-write race —
cdk/src/handlers/confirm-uploads.ts:1782-1796, 1819-1827, 2122-2131
failTaskOnScreening swallows ConditionalCheckFailedException and returns void,
so the caller can't tell whether we transitioned the task. It then
unconditionally calls cleanupAllAttachments, which can delete S3 objects
belonging to a task that another caller already moved past PENDING_UPLOADS.
Make failTaskOnScreening return a boolean and skip cleanup when it returns
false. -
Redundant S3 PUT in screenSingleAttachment —
cdk/src/handlers/confirm-uploads.ts:1906-1911
EXIF stripping was removed, so screenResult.content === content. The
unconditional PutObjectCommand rewrites the same bytes to the same key,
doubles S3 cost per presigned attachment, and creates an extra version that
confuses cleanup. Either skip the re-upload (reuse the existing
versionId/checksum) or only PUT when the buffer actually changed. -
ListObjectVersions pagination bug —
cdk/src/handlers/cleanup-pending-uploads.ts:1472
if (objects.length === 0) break; exits on any empty page, but S3 can return an
empty page with IsTruncated=true. Use NextKeyMarker/IsTruncated as the loop
terminator, otherwise stale versions can leak past cleanup.
isadeks
left a comment
There was a problem hiding this comment.
⏺ Re-reviewed at a9d4c6f. All 3 blockers from the previous pass are addressed:
- ✅
failTaskOnScreeningreturnsPromise<boolean>; both callers gate
cleanupAllAttachmentson the result. Race test added in
confirm-uploads.test.ts. - ✅ Redundant
PutObjectCommandremoved fromscreenSingleAttachment
passed-path; originalversionId/sizeBytesreused. - ✅
cleanupTaskAttachmentsnow drives the loop onIsTruncatedwith
continueon empty pages.
I also did a deep pass on the areas I hadn't audited the first time around —
CDK wiring, entry-point handlers, agent Python runtime.
CDK infrastructure — clean
- IAM scoping is tight: create-task gets
grantPut+grantDeleteonly;
confirm-uploads getsgrantReadWrite+grantDelete; cleanup gets
grantRead+grantDelete; agent runtime getsgrantReadonly. Bedrock
guardrail policy is scoped to the guardrail ARN, not wildcard. - Bucket:
BLOCK_ALLpublic access,enforceSSL: true, S3-managed
encryption, versioning on, 90-day current + 7-day noncurrent lifecycle
attached. - WAF
SizeRestrictions_BODYexclusion is properly scoped to/v1/tasks
(STARTS_WITH) and/v1/linear/webhook(EXACT) viascopeDownStatement— not
stack-wide. - EventBridge schedule uses CDK's auto-granted invoke; no over-scoped role.
- Presigned POST policy enforces
content-length-rangeand fixed
Content-Typeserver-side.
Entry-point handlers — clean
All four paths funnel through validateAttachments() →
createAttachmentRecord(). No direct AttachmentRecord construction bypasses
validation/screening.
- Linear webhook: extracts markdown image URLs, HTTPS-only, capped at 10,
routed through the URL-fetch path with full SSRF protection at hydration. - Slack: pre-validates size + MIME, uses
isSlackFileUrl()SSRF guard
before sending the bot token, then re-validates server-side. - CLI: routes inline (<500 KB) vs presigned vs URL correctly; presigned
path always goes throughconfirm-uploadsfor screening.
Agent Python runtime — one new finding
Partial-download workspace leak — agent/src/attachments.py:70-82,
agent/src/pipeline.py:294-319. If _download_single() raises mid-loop
(e.g., checksum mismatch on attachment #2), already-downloaded files from
earlier iterations are left in /workspace/.attachments/. Not a security
issue (workspace is per-task and disposable), but on retry the agent could see
stale files alongside fresh ones. Wrap the loop in try/finally with
shutil.rmtree(attachments_dir) on failure.
Otherwise clean: multimodal injection sources only from verified
PreparedAttachment objects; AttachmentConfig has extra="forbid" + a
thorough model_validator; chmod 0o444 after write.
Non-blocking follow-ups (file separately if useful)
isPrivateIpmissing 198.18.0.0/15, RFC 5737 TEST-NETs, IPv4/IPv6 multicast
(resolve-url-attachments.ts)- PDF text byte-cap uses
String.slice— can exceed cap with multi-byte UTF-8
(attachment-screening.ts) - Filename regex permits trailing
.(validation.ts) MAX_FETCH_SIZE_BYTESenforced twice — outer streaming loop is dead code
(resolve-url-attachments.ts)agent/src/attachments.pyrecreates the boto3 client per call — make it
module-levelMAX_TASK_DESCRIPTION_LENGTH2000→10_000 deserves a one-line rationale
comment- Test gaps: DNS-rebinding-via-redirect (302 public → hostname resolving
private); multi-attachment partial-failure cleanup
Overall
LGTM. The new partial-download leak is should-fix, not blocking. Original
blockers cleared, security-critical paths audited end to end.
ESLint --fix removes no-op suppression comments for rules not enabled in our config. These were introduced by the attachments PR (#176) merge and are cleaned up by the new ESLint 10 flat config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…samples#171) * chore(deps): upgrade eslint 9->10, replace import plugin with import-x - eslint ^9 -> ^10 (10.4.0) - @cdklabs/eslint-plugin ^1 -> ^2 (flat config support) - eslint-plugin-import removed (no ESLint 10 support) - eslint-plugin-import-x ^4 added (drop-in replacement) - eslint-import-resolver-typescript removed (import-x has built-in TS resolution) - Root resolution for eslint-plugin-import/minimatch removed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(eslint): migrate cdk/ to ESLint 10 flat config Convert .eslintrc.json to eslint.config.mjs (ESM flat config format). Replace import/ rule prefix with import-x/. Remove --ext flag and ESLINT_USE_FLAT_CONFIG env var from the eslint script. All existing rules preserved with equivalent flat config syntax. Four @cdklabs rules (no-core-construct, invalid-cfn-imports, no-literal-partition, no-invalid-path) are temporarily disabled because they use context.getFilename() which was removed in ESLint 10. Stale eslint-disable comments for disabled/absent rules removed by auto-fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(eslint): migrate cli/ to ESLint 10 flat config Convert .eslintrc.json to eslint.config.mjs. Replace import/ rule prefix with import-x/. Remove --ext flag and ESLINT_USE_FLAT_CONFIG. All existing rules preserved including no-console override for src/. Removed stale no-constant-condition disable comments (rule deprecated in ESLint 10). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(eslint): restore eslint-import-resolver-typescript for import-x The resolver is required by eslint-plugin-import-x for TypeScript path resolution. Was incorrectly removed in the package upgrade commit. Also disable license-header rule for shebang bin files (workaround). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(eslint): remove stale suppressions from merged attachments code ESLint --fix removes no-op suppression comments for rules not enabled in our config. These were introduced by the attachments PR (aws-samples#176) merge and are cleaned up by the new ESLint 10 flat config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: bgagent <345885+scottschreckengaust@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Alain Krok <alkrok@amazon.com>
Fixes #177
Summary
Supported file types
image/png,image/jpeg.png,.jpgtext/plain,text/csv,text/markdown,application/json,application/pdf,text/x-log.txt,.csv,.md,.json,.pdf,.logSecurity measures
ApplyGuardrailCommandimage content blocks; text files/PDFs screened via text content blocks; prompt attack detection atMEDIUM strength
redirect validation, HTTPS-only, 10s timeout
Key changes
CDK Infrastructure
AttachmentsBucketconstruct (versioned S3, 90-day lifecycle, block public access, enforce SSL)confirm-uploadsLambda (1024 MB, 180s timeout, parallel screening with concurrency 3)PendingUploadCleanupEventBridge rule (5-min schedule, auto-cancels stale uploads after 30 min with TTL)POST /v1/tasks/{task_id}/confirm-uploadsAPI endpointPENDING_UPLOADStask status withPRE_ACTIVEclassificationSizeRestrictions_BODYrule excluded for task creation endpoint (base64 payloads exceed 8 KB)Handlers
attachment-screening.ts— image + text screening with retry (3 attempts, exponential backoff), pure buffer dimension parsing (PNG IHDR / JPEG SOF markers), no native dependenciesresolve-url-attachments.ts— SSRF-safe fetch with DNS pinning via nativehttps.request+https.Agent({ servername })for TLS SNI, redirect validation, streaming sizeenforcement,
isPrivateIpcovering IPv4 and IPv6image-tokens.ts— synchronous Claude vision token estimation using buffer-parsed dimensions (resize-aware, 1568px cap, 28px tile padding)confirm-uploads.ts— presigned upload confirmation with concurrent-call safety (conditional DDB writes), TTL on failure paths, pre-check concurrency with proper errordiscrimination
cleanup-pending-uploads.ts— expired task auto-cancel with S3 prefix cleanup, TTL, race condition handling (ConditionalCheckFailedException)create-task-core.ts— inline screening/upload path with try-catch around presigned URL generationorchestrator.ts— URL attachment resolution during hydrationvalidation.ts— full attachment validation (magic bytes, MIME allowlist, filename, size limits); GIF/WebP rejected at admissionAgent runtime (Python)
models.py—AttachmentConfigandPreparedAttachmentPydantic models (frozen, no extras)pipeline.py— attachment preparation step, multimodal content block injection for imagesCLI
--attachmentflag (repeatable) onbgagent submitDocumentation
docs/design/ATTACHMENTS.md— comprehensive design doc reflecting simplified pipeline (no sharp, no EXIF stripping, no GIF/WebP conversion)API_CONTRACT.md— supported types (PNG/JPEG only), limits tables, confirm-uploads endpointSECURITY.md— attachment screening details for submission-time and hydration-timeUSER_GUIDE.md— attachments section with supported types, limits, CLI usage, security screening explanationROADMAP.md— marked attachments as implementedTest plan
cdk/test/handlers/shared/attachment-screening.test.ts— 18 tests: PNG/JPEG dimension parsing, magic bytes, GIF/WebP rejection, oversized rejection, Bedrock pass/block, rawpass-through
cdk/test/handlers/shared/resolve-url-attachments.test.ts— 17 tests:isPrivateIpcovering IPv4 RFC1918, loopback, CGN, link-local, IPv6 ULA, link-local, IPv4-mapped,all-zeros
cdk/test/handlers/shared/validation.test.ts— updated for GIF/WebP rejection at admissioncdk/test/handlers/confirm-uploads.test.ts— presigned upload confirmation, concurrent-call safety, screening failure handlingcdk/test/handlers/cleanup-pending-uploads.test.ts— 6 tests: TTL on cancel, race condition, S3 cleanup, partial success, all-error throwagent/tests/test_attachments.py— 10 tests: download/verify, checksum mismatch, size mismatch, read-only perms, VersionId passthroughcdk/test/constructs/task-status.test.ts— PENDING_UPLOADS status transitionstsc --noEmit)Design decisions
sharp/libvipsnative dependency that caused ARM64 Lambda decode failures and cross-platform build issues. Bedrock accepts PNG/JPEGpdf-parsekept as only bundled depArea
cdk— infrastructure, handlers, constructsagent— Python runtime / Docker imagecli—bgagentclientdocs— guides or design sources (docs/guides/,docs/design/)tooling— rootmise.toml, scripts, CI workflowsTip: AGENTS.md lists where to edit and which tests to extend.
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.