Skip to content

fix: skip file download errors in search instead of aborting#69

Merged
AmethystLiang merged 9 commits intostablyai:mainfrom
shhac:paul/fix-download-errors
Mar 22, 2026
Merged

fix: skip file download errors in search instead of aborting#69
AmethystLiang merged 9 commits intostablyai:mainfrom
shhac:paul/fix-download-errors

Conversation

@shhac
Copy link
Contributor

@shhac shhac commented Mar 20, 2026

Problem

When a Slack file attachment returns a 401 (no read permission) or 404 (deleted), downloadSlackFile throws.

  • The message-read path (message get, message list) handles this gracefully — downloadMessageFiles wraps each download in a try/catch, logs a warning, and continues.
  • The search paths (search-messages, search-files) call downloadSlackFile directly with no error handling, so a single inaccessible attachment kills the entire search operation.

This commonly happens with:

  • Deleted attachments
  • Files forwarded from a workspace/channel the caller doesn't have access to

Fix

Wrap each downloadSlackFile call in search-messages and search-files with try { … } catch { continue }

  • Matches the existing bare-catch convention used throughout src/slack/ for tolerable errors (e.g. channels.ts, messages.ts, search-messages.ts message enrichment).
  • Failed downloads are silently skipped; remaining results still return normally.

Notes

  • Canvas fetching (canvas.ts) is intentionally left unchanged — there, the file download IS the primary operation, so a failure should still surface as an error.
  • IMO it might be a nicer fix to offer a sibling to downloadSlackFile, downloadOptionalSlackFile which handles the try-catch on that side, but this doesn't look like the pattern in this repo

A 401/404 on a single file attachment (deleted file, no read
permission) would throw through downloadSlackFile and kill the
entire search operation. The message-read path already handled
this gracefully via downloadMessageFiles' per-file try/catch,
but search-messages and search-files called downloadSlackFile
directly with no error handling.

Wrap each downloadSlackFile call in try/catch { continue } so
failed downloads are skipped and remaining results still return.
@AmethystLiang
Copy link
Contributor

This is the Codex review. Once addressed, will merge the PR for you.

High

  • src/slack/search-messages.ts:268 causes search messages --content-type file|image|snippet to drop valid matches whenever the attachment download fails. The new catch { continue; } means downloadedPaths[f.id] is never set, and later src/slack/search-messages.ts:222 derives hasFiles from compact.files, which only includes successfully downloaded files. So a message that does contain an image/snippet/file is now treated as file-less and filtered out. That changes search semantics, not just error handling.

Medium

  • src/slack/search-files.ts:49, src/slack/search-files.ts:141, and src/slack/search-messages.ts:268 swallow every exception from src/slack/files.ts:34, not just the intended 401/404 cases. That also hides network failures, malformed responses, temp-dir/write failures, and auth regressions as silent partial success. The existing message-read path logs a warning instead of failing hard; it does not suppress the failure completely (src/cli/message-file-downloads.ts:98). This PR should at least narrow the skipped cases or log them.

No tests cover these search download/error paths, so the regression risk here is higher than usual.

Introduce SlackDownloadError and tryDownloadSlackFile wrapper that
distinguishes network/HTTP errors (caught) from disk/write errors
(propagated). Failed downloads now produce DownloadResult objects
that flow through toCompactMessage, preserving file metadata so
content-type filtering works correctly even when downloads fail.
@shhac
Copy link
Contributor Author

shhac commented Mar 21, 2026

Addressed both issues (note: this increases scope):

High — failed downloads dropping messages from content-type–filtered search results

The root cause was that downloadedPaths had no entry for failed files, so toCompactMessage excluded them from compact.files, making passesContentTypeFilter treat the message as file-less. Fix: downloadedPaths now stores DownloadResult objects (success or failure). Failed downloads produce a compact file entry with error instead of path, preserving the file metadata (mimetype, mode) so content-type filtering still works.

Medium — swallowing all exceptions silently

Introduced SlackDownloadError to distinguish network/HTTP errors from disk/write errors. tryDownloadSlackFile catches only SlackDownloadError and returns a structured failure result; disk errors propagate. All call sites (including canvas downloads) now log warnings on failure. The boundary: we defend against unavailability from the remote, not a broken local system.

Added 18 tests covering the download wrapper, toCompactMessage with error entries, and the content-type filter integration.

Example search output with a failed download
{
  "messages": [
    {
      "ts": "1710000000.000001",
      "author": { "user_id": "U12345" },
      "content": "Here's the screenshot",
      "files": [
        {
          "mimetype": "image/png",
          "path": "/tmp/agent-slack-downloads/F0001.png"
        }
      ]
    },
    {
      "ts": "1710000000.000002",
      "author": { "user_id": "U67890" },
      "content": "Check this attachment",
      "files": [
        {
          "mimetype": "image/jpeg",
          "error": "Failed to download file (401)"
        }
      ]
    }
  ]
}

Copy link
Contributor

@AmethystLiang AmethystLiang left a comment

Choose a reason for hiding this comment

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

Reviewed latest branch state locally after the follow-up fixes. Full test suite and typecheck pass, and the download-error contract/docs are now aligned.

@AmethystLiang AmethystLiang merged commit bc7f26f into stablyai:main Mar 22, 2026
1 check passed
@shhac shhac deleted the paul/fix-download-errors branch March 23, 2026 10:44
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