Skip to content

fix(sdk): tag raising S3 PUT failures with the s3_upload step#432

Merged
mocha06 merged 1 commit into
devfrom
rc-dev/fix/attachment-s3-put-step-tag
Jul 18, 2026
Merged

fix(sdk): tag raising S3 PUT failures with the s3_upload step#432
mocha06 merged 1 commit into
devfrom
rc-dev/fix/attachment-s3-put-step-tag

Conversation

@mocha06

@mocha06 mocha06 commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Motivation

AttachmentService's docstring promises every step surfaces as AttachmentUploadError carrying step, but the S3 put call was not wrapped: an httpx transport error or the uploader's host-allowlist ValueError bubbled bare instead of being tagged step="s3_upload". Only HTTP >= 400 results from a completed PUT were tagged, so the step-aware error envelope silently degraded to a generic untagged error for exactly the network / tampered-URL failures where the stage tag is most useful.

Outcome

Transport errors and host-allowlist rejections from the S3 PUT now raise AttachmentUploadError(step="s3_upload") from the cause, keeping the attachment and knowledge-base document upload pipelines on one contract. Existing HTTP >= 400 tagging is unchanged.

Closes #430

AttachmentService promised every step surfaces as AttachmentUploadError
carrying step, but the S3 put call was not wrapped: an httpx transport error
or the uploader's host-allowlist ValueError bubbled bare, so the step-aware
envelope degraded to a generic untagged error for exactly the network /
tampered-URL failures where the stage tag matters most. The put call is now
wrapped and re-raised as AttachmentUploadError(step="s3_upload") from the
cause, keeping the attachment and knowledge-base document upload pipelines on
one contract. Existing HTTP >= 400 tagging is unchanged.

Closes #430
@mocha06 mocha06 self-assigned this Jul 18, 2026
@mocha06
mocha06 requested a review from adriannoes July 18, 2026 04:23

@adriannoes adriannoes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Surgical fix that closes #430: raising S3 PUT failures now surface as AttachmentUploadError with step="s3_upload", matching the KnowledgeBaseService wrap. Reviewed with checkout+tests; CI lint and test are green on the tip.

What worked well

  • Mirrors the KB put wrap from #428 so attachment and knowledge-base uploads stay on one contract.
  • Unit test asserts step, message fragment, and __cause__; it fails if the wrap is removed.
  • Leaves the existing HTTP >= 400 tagging path untouched; no MCP/CLI surface churn needed.

Review path

  • Checked out the PR tip, ran attachment_service unit tests (24 passed) and ruff on touched files; CI green.
  • Callers (facade, MCP tools, CLI) already catch AttachmentUploadError and benefit from the step tag.
  • Coverage polish (allowlist ValueError in the service unit test) matches the intentional KB twin gap and was verified manually with HttpxS3Uploader; not a merge gate.

@mocha06
mocha06 merged commit f2866b5 into dev Jul 18, 2026
2 checks passed
@mocha06
mocha06 deleted the rc-dev/fix/attachment-s3-put-step-tag branch July 18, 2026 06:42
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