Skip to content

feat: serve attachment content via GET /api/v1/attachments/{hash}#463

Merged
wesm merged 6 commits into
kenn-io:mainfrom
robelkin:feat/api-attachments-endpoint
Jul 11, 2026
Merged

feat: serve attachment content via GET /api/v1/attachments/{hash}#463
wesm merged 6 commits into
kenn-io:mainfrom
robelkin:feat/api-attachments-endpoint

Conversation

@robelkin

@robelkin robelkin commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What

Adds GET /api/v1/attachments/{hash} — an authenticated endpoint that streams a stored attachment's raw bytes, addressed by its SHA-256 content hash, with the attachment's own Content-Type and a download Content-Disposition filename.

Why

The HTTP API had no way to retrieve file-attachment content. /messages/{id}/inline serves only inline images (it requires a cid and an image/* content type), and attachment bytes were otherwise reachable only from the co-located CLI/MCP paths that read the on-disk store directly. A networked client — a remote TUI, or any integration without filesystem access to the attachments directory — had no route to fetch them.

Usage

GET /api/v1/attachments/{sha256-hex}
Authorization: Bearer <api-key>
  • 200 with the file bytes and Content-Type / Content-Disposition / Content-Length set.
  • 400 for a malformed hash (not 64 hex chars; also guards path traversal).
  • 404 when the attachment metadata row or its on-disk file is absent (e.g. an account removed with its attachments, or an archive built with --no-attachments).

Auth is inherited from the existing /api/v1 API-key middleware. content_hash is not unique (content-addressed dedup), so any matching row supplies the header metadata.

@roborev-ci

roborev-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown

roborev: Combined Review (8d2ebc4)

Summary verdict: one Medium issue needs attention before merge.

Medium

  • internal/api/handlers.go:103 - The new download route is keyed by content hash, but normal message attachment API responses still expose only filename, MIME type, and size. Clients using /api/v1/messages/{id} cannot discover the hash needed to call /api/v1/attachments/{hash}.
    • Fix: Add content_hash to the API attachment DTO and populate it from both query-engine and store-backed message detail paths.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 5m11s

@robelkin

robelkin commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the Medium finding in 5bdb2fc: content_hash is now included in the message-detail attachment payload (/api/v1/messages/{id}), populated from both the query-engine and store-backed paths, so clients can discover the hash to pass to GET /api/v1/attachments/{hash}. Added a test asserting the field is present.

@roborev-ci

roborev-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown

roborev: Combined Review (5bdb2fc)

PR has one medium issue to address before merge.

Medium

  • internal/remote/store.go:187: The server emits content_hash and the new download endpoint requires that hash, but the in-repo remote client still decodes only filename, MIME type, and size. Remote GetMessage callers get empty attachment hashes and cannot discover GET /api/v1/attachments/{hash} from message details.

    Fix: Add ContentHash string with json:"content_hash,omitempty" to the remote attachment response, copy it through remote.Store.GetMessage and remote.Engine.GetMessage, and cover it with a remote message-detail test.


Reviewers: 2 done | Synthesis: codex, 6s | Total: 8m26s

@wesm

wesm commented Jul 10, 2026

Copy link
Copy Markdown
Member

looking

@wesm wesm force-pushed the feat/api-attachments-endpoint branch from 5bdb2fc to 70a9a13 Compare July 10, 2026 13:35
@roborev-ci

roborev-ci Bot commented Jul 10, 2026

Copy link
Copy Markdown

roborev: Combined Review (70a9a13)

The attachment-content endpoint has two medium-severity compatibility issues affecting PostgreSQL and namespaced attachment storage.

Medium

  • internal/query/sqlite.go:1052GetAttachmentByHash bypasses the dialect-aware query helper, leaving SQLite’s ? placeholder in PostgreSQL queries and causing attachment downloads to fail with an internal error. Use e.queryRowContext(...) and add backend-compatible query coverage.

  • internal/api/handlers.go:2531 — The handler reconstructs a canonical <hash-prefix>/<hash> path instead of using the attachment’s recorded storage_path. Importers such as SyncTech SMS use namespaced paths like synctech-sms/<prefix>/<hash>, so valid attachments return 404. Retrieve the stored local path with the metadata and safely resolve it beneath AttachmentsDir.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 3m20s

@wesm

wesm commented Jul 10, 2026

Copy link
Copy Markdown
Member

I will rebase this once #464 is merged and then I can get this ready to merge

Add an authenticated endpoint that streams stored attachments by SHA-256 hash with their own content type and download filename. This gives networked clients access to attachment bytes that were previously available only to co-located CLI/MCP paths.

The endpoint validates content-addressed paths, returns not found for missing metadata or files, and supports SQLite, Postgres, DuckDB, mocks, and the remote engine compatibility boundary.

Squashed follow-up:
- feat: expose attachment content_hash in message detail responses

Generated with Codex

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@wesm wesm force-pushed the feat/api-attachments-endpoint branch from 70a9a13 to 4e31bef Compare July 10, 2026 22:19
@roborev-ci

roborev-ci Bot commented Jul 10, 2026

Copy link
Copy Markdown

roborev: Combined Review (4e31bef)

Verdict: Changes requested — the attachment download path has one high-severity and two medium-severity functional defects.

High

  • Packed attachments return 404internal/api/handlers.go:2567
    The handler opens only the legacy loose-file path. Attachments moved into packed CAS storage remain valid but cannot be downloaded through this endpoint. Read through s.blobStore.Open(hash) when available, retain the loose-file fallback when it is nil, and add coverage for packed attachments.

Medium

  • PostgreSQL metadata lookup uses an incompatible placeholderinternal/query/sqlite.go:1051
    GetAttachmentByHash sends a ? placeholder directly to the database, bypassing dialect-aware rebinding. PostgreSQL attachment-content requests therefore fail during lookup. Use e.queryRowContext(ctx, query, contentHash) and add PostgreSQL coverage.

  • Generated client JSON-decodes binary contentpkg/client/generated/client.go:668
    GetAttachmentContent attempts to unmarshal a successful binary response as JSON, causing ordinary attachment downloads to return a decoding error. Return the raw response bytes and add a client test with non-JSON content.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 1m34s

wesm and others added 3 commits July 10, 2026 17:25
The public download endpoint still derived loose-file paths after packed attachment storage became authoritative, so sealed blobs were unreachable even though every other attachment reader could open them. Route content reads through the shared blob store while retaining the existing fallback for embedded servers.

Attachment metadata lookup also bypassed dialect rebinding, which left SQLite placeholders in PostgreSQL queries. Use the engine query helper so both backends execute the same lookup contract.

VALID (fixed): #1, #2

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
Legacy imports may record a valid local blob beneath a namespaced path rather than the canonical hash shard. Packed reads remain authoritative, but a not-found result must fall back to that recorded location until maintenance canonicalizes it.

Resolve the recorded path through the attachments root, reject traversal, absolute paths, URLs, and symlink escapes, and expose no storage-path detail in API responses.

VALID (fixed): #2 legacy namespaced-path case

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
The packed-attachment endpoint regression test adopts PostgreSQL in CI through NewTestStore, so hard-coding the SQLite query dialect leaves placeholders unrebound and makes the test fail before it can exercise the endpoint. Use the backend-aware engine factory so the same coverage validates both supported stores.\n\nValidation: reproduced the failing job against PostgreSQL 16 and reran the focused endpoint test successfully.\n\nGenerated with Codex\nCo-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Jul 10, 2026

Copy link
Copy Markdown

roborev: Combined Review (ced8023)

The attachment download endpoint is secure, but the generated client mishandles successful binary responses.

Medium

  • pkg/client/generated/client.go:668GetAttachmentContent passes raw attachment bytes to json.Unmarshal, causing normal binary or plain-text attachments to return a ResponseDecodeError after a successful 200 response.
    • Fix: Return the response bytes directly as runtime.File, or route the method through the binary-aware GetAttachmentContentWithResponse path.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 2m22s

@roborev-ci

roborev-ci Bot commented Jul 10, 2026

Copy link
Copy Markdown

roborev: Combined Review (3410234)

Attachment downloads have one medium-severity client bug; otherwise the implementation looks sound.

Medium

  • pkg/client/generated/client.go:663GetAttachmentContent JSON-decodes a successful binary response into runtime.File, causing ordinary attachment bytes to fail with a response-decoding error. The WithResponse variant correctly retains resp.Content.

    Fix: Return the raw response bytes for HTTP 200, following GetCLIMessageRaw, and add a generated-client test covering non-JSON binary content.


Reviewers: 2 done | Synthesis: codex, 6s | Total: 1m56s

Attachment downloads can carry arbitrary MIME types and non-JSON bytes, but describing the success response as application/octet-stream makes the client generator select its file-upload type and JSON decoder. Describe the binary response with the wildcard media type the endpoint actually serves so regeneration selects its raw-byte response path.

VALID (fixed): #1

Validation: reproduced the decode failure with non-JSON binary content, then verified the generated convenience client returns the exact bytes.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Jul 10, 2026

Copy link
Copy Markdown

roborev: Combined Review (fd5d8c8)

Potentially incorrect attachment metadata/path selection needs attention before merge.

Medium

  • internal/query/sqlite.go:1054LIMIT 1 arbitrarily selects among attachments sharing a content hash, even though filenames, MIME types, and storage paths may differ. Downloads can receive incorrect metadata or fail by selecting a missing or URL-backed path when another matching row has valid local content. Retrieve matching rows and try usable local paths deterministically, or decouple blob resolution from attachment-specific metadata unless an attachment ID is supplied.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 2m45s

A content hash can belong to attachment rows with different metadata and legacy storage paths. Selecting one arbitrary row can hide a valid loose copy behind a stale or non-local path and can send headers unrelated to the bytes that were opened. Preserve stable row order for packed and canonical content, while trying every distinct recorded path and selecting metadata from the row whose loose content succeeds.

VALID (fixed): #1

Validation: reproduced the failure with duplicate rows whose first path was missing, then served the later namespaced path with matching headers on SQLite and PostgreSQL 16.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Jul 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (afef30f)

Code review verdict: no Medium, High, or Critical findings.


Reviewers: 2 done | Synthesis: codex, 5s | Total: 2m40s

@wesm wesm merged commit c262ace into kenn-io:main Jul 11, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants