Skip to content

fix(core): reject downloads that cannot be saved - #4308

Open
Sourish Chakraborty (sourrris) wants to merge 1 commit into
ComposioHQ:nextfrom
sourrris:fix/download-write-failure
Open

fix(core): reject downloads that cannot be saved#4308
Sourish Chakraborty (sourrris) wants to merge 1 commit into
ComposioHQ:nextfrom
sourrris:fix/download-write-failure

Conversation

@sourrris

Copy link
Copy Markdown

Summary

Automatic S3 downloads currently report success with filePath: null when the downloaded bytes cannot be written to disk. This happens because saveFile() preserves its nullable contract, but downloadFileFromS3() casts the nullable result to string.

This change makes the download operation reject when persistence fails, so callers never receive a false-success result.

Changes

  • Reject downloadFileFromS3() when saveFile() returns null
  • Remove the unsafe type assertion and stale nullable TODO
  • Add a regression test that simulates a filesystem write failure
  • Add a patch changeset for @composio/core

Type of change

  • Bug fix
  • New feature
  • Refactor/Chore
  • Documentation
  • Breaking change

How Has This Been Tested?

Environment: Node.js 24.19.0, pnpm 11.19.0.

  • pnpm --filter @composio/core exec vitest run test/utils/fileUtils.test.ts (25 tests passed)
  • pnpm --filter @composio/core typecheck
  • pnpm exec oxlint ts/packages/core/src/types/files.types.ts ts/packages/core/src/utils/fileUtils.node.ts ts/packages/core/test/utils/fileUtils.test.ts
  • pnpm exec prettier --check .changeset/fail-unsaved-downloads.md ts/packages/core/src/types/files.types.ts ts/packages/core/src/utils/fileUtils.node.ts ts/packages/core/test/utils/fileUtils.test.ts
  • pnpm validate:changesets
  • git diff --check

Screenshots (if applicable)

Not applicable.

Checklist

  • I have read the Code of Conduct and this PR adheres to it
  • I ran linters/tests locally and they passed
  • I updated documentation as needed
  • I added tests or explain why not applicable
  • I added a changeset if this change affects published packages

Additional context

saveFile() remains nullable for existing callers and runtimes without filesystem support. The stricter failure behavior is limited to downloadFileFromS3(), whose return type already promises a string path.

@vercel

vercel Bot commented Aug 30, 2026

Copy link
Copy Markdown

Sourish Chakraborty (@sourrris) is attempting to deploy a commit to the Composio Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Community review, not a merge gate.

Traced downloadFileFromS3 in ts/packages/core/src/utils/fileUtils.node.ts. The bug was explicitly marked as known technical debt in the code itself — the removed comments read @todo: actually, this can be null on the type definition and @todo: fix in follow-up PR at the return site, with filePath: filePath as string lying to the type system about a value that could genuinely be undefined/null if the on-disk write failed. That meant a failed save silently returned a "successful" FileDownloadData with a bogus file path, which is a worse failure mode than an exception: any caller trusting the type signature would proceed as if the file existed.

The fix is a straightforward, correct guard: if (!filePath) { throw new Error(...) } before constructing the return value, and the type now honestly declares filePath: string without the stale todo comment, since the function itself now guarantees a real string reaches that field. Also removes the as string cast that was previously papering over the type mismatch.

The test mocks fs.writeFileSync to throw and confirms the download call rejects with the expected message including the synthesized filename, which matches the actual thrown error's interpolation (Failed to save downloaded file: ${fileName}). I checked this against the neighboring existing test for a fetch-failure case (rejects.toThrow('Failed to download file: Not Found')) to confirm the new test follows the same pattern rather than being a one-off. Small, well-scoped fix that turns a silently wrong result into a clear error, which is the right direction for a file-download path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants