Skip to content

fix(#1924): report attachment download failures instead of hiding them - #1925

Open
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/1924-attachment-download-failures
Open

fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/1924-attachment-download-failures

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

Updates skills/code-implementation/references/attachments.md so failed issue-attachment downloads show up in the agent's output and are not silently lost.

Changes to the download block:

  • Listing errors are visible. set -o pipefail plus && between the two gh api calls means a 403/404/rate-limit ends in INCOMPLETE: listing or download failed. When grep finds no matches (exit 1), that still counts as a success, so "no attachments" stays clean.
  • Bounded, safe downloads. curl gets --proto '=https' and --max-time 60. A failed, empty or non-image/* download is deleted and reported as FAILED <n>-<name>. The signed URL is never printed.
  • Streaming. Removed sort -u. Duplicates are skipped by file name inside the loop. grep --line-buffered and decoding &amp; in the loop (no sed stage) mean each download starts as soon as its URL is read.
  • Tighter matching. The pattern is now [^"<>[:space:]]+, so a plain-text link (<a href="U">U</a> and more) gives U once. .mp4/.mov are skipped.
  • Summary. Prints downloaded X/Y.

The prose now says: on INCOMPLETE/FAILED, run the block once more (this fetches fresh signed URLs). If a needed image is still missing, mark the work PARTIAL in the commit message and pr_body, name the missing <uuid>s, and never use a placeholder. Existing rules (no Authorization header, no signed URLs in files, commits or pr_body) are unchanged.

Testing

  • Ran the block against stub gh/curl:
    • gh 403 on the issue: downloaded 0/0 followed by INCOMPLETE
    • 403 on comments only: INCOMPLETE
    • Mixed curl results: a 403 that left a 0-byte file and a text/html response were both deleted and reported, giving downloaded 2/4
    • The duplicated <a href><img src> link was fetched once
    • A plain-text link followed by HTML gave a clean URL with &amp; decoded
    • .mp4 was skipped
    • No attachments gave downloaded 0/0 with no error
    • A slow comment page: the first download started before the page returned
  • shellcheck (repo args) passes on the extracted block, before and after the second commit.
  • skillsaw 0.18.0 --strict passes.
  • pre-commit could not fetch hook repos in the sandbox (TLS error), so I ran the applicable pre-commit-hooks v6.0.0 hooks and lint-release-cases directly, and they passed. scan-secrets passes.
  • Not tested: a real stalled transfer hitting --max-time 60, and real GitHub downloads (code: verify issue-attachment download in the sandbox on a private repository #1923 covers sandbox verification).

Caveat (pre-existing, not changed here)

In this sandbox, Tirith blocks the download block when it is pasted inline as one Bash command, because it cannot parse the brace-group/while read pipeline. The original block on main is blocked the same way. Writing the block to a file and running bash <file> works, and that is how it was tested above. The second commit replaces the case alternation and [[ ]] with plain test checks to match the accepted dialect, but that alone does not unblock the inline paste. This may be worth handling with #1923.

Related

#1922 and #1923 also touch this reference doc. Whichever lands second may need a rebase.

🤖 Generated with Claude Code


Closes #1924

Post-script verification

  • Branch is not main/master (agent/1924-attachment-download-failures)
  • Secret scan passed (gitleaks — 88e5770175ac0cab3a3624255556362ff1e73e63..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Final PR description secret scan passed (gitleaks — no-git)

fullsend-ai-coder Bot and others added 2 commits November 9, 2026 14:09
The download loop in the code-implementation attachments reference
could lose images without the agent noticing: a failed gh api call
looked like "no attachments", curl had no time limit, failed downloads
left 0-byte files and no summary, sort -u held every URL until all
comment pages were fetched (signed URLs expire after 300 s), the URL
pattern ran on into trailing HTML, and videos were downloaded too.

Changes to the bash block:
- set -o pipefail and chain the gh calls with &&, so a listing error
  ends in an INCOMPLETE message; grep's "no match" stays a success.
- curl gets --proto '=https' and --max-time 60; a failed, empty or
  non-image download is deleted and reported as FAILED.
- Drop sort -u. De-duplicate by file name inside the loop and use
  grep --line-buffered (with &amp; decoded in the loop) so each
  download starts as soon as its URL is read.
- Tighten the pattern to [^"<>[:space:]]+ and skip .mp4/.mov.
- Print "downloaded X/Y".

The prose now says to re-run once on failure and otherwise mark the
work PARTIAL in the commit message and pr_body, never substituting a
placeholder.

Tested with stubbed gh/curl: gh 403, mixed curl failures, a non-image
response, duplicate and plain-text links, a video, no attachments, and
a slow comment page (first download starts before it returns).
shellcheck passes on the block. pre-commit could not fetch hook repos
(TLS error), so the applicable pre-commit-hooks v6.0.0 hooks and
lint-release-cases were run directly and passed; skillsaw 0.18.0
--strict passes. A real stalled transfer was not exercised.

Closes #1924

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the multi-pattern case arm (*.mp4 | *.mov) and the [[ ]] image
check with test comparisons. This matches the shell dialect the
sandbox's Tirith hook accepts. Behavior is unchanged; the stubbed
gh/curl scenarios and shellcheck still pass.

Tirith still blocks the whole block when it is pasted inline as one
command (the original block on main is blocked the same way). Saving
it to a file and running it with bash works. Not changed here.

Related to #1924

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner November 9, 2026 14:12
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Nov 9, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:14 PM UTC · Completed 2:22 PM UTC

Commit: 3ff4f0e · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.11

@fullsend-ai-review fullsend-ai-review Bot added the risk/low PR risk: low label Nov 9, 2026
@fullsend-ai-review

Copy link
Copy Markdown

Risk Assessment: low (1/5)

Details

Single skill reference doc updated by a bot with low churn and fully aligned issue scope, presenting low risk overall despite touching a protected path.

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] skills/code-implementation/references/attachments.md — This PR modifies a file under the protected skills/ path. The PR links issue code: attachment download failures are silent #1924 and explains the rationale, but human approval is always required for protected-path changes, regardless of context. No other findings were raised by the correctness, security, style-conventions, intent-coherence, or docs-currency reviewers.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Nov 9, 2026

This branch was previously deployed

1 inactive deployment
dev — 3ff4f0e3 Deployed Nov 9, 2026 by fullsend-ai-coder[bot] via behaviour #1831
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review Triggers review agent dispatch requires-manual-review Review requires human judgment risk/low PR risk: low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

code: attachment download failures are silent

1 participant