Repository navigation
Conversation
`hey contact photo set <contact-id> <file>` uploads a local image as the contact's avatar (Attachments.Upload, then Contacts.UpdateAvatar with the blob's signed ID); `hey contact photo remove <contact-id>` takes it off (Contacts.DeleteAvatar). `avatar` is an alias for `photo`. The file is checked before anything is sent, because HEY stores any blob it is handed and only fails later, when it resizes the photo to show it: - PNG or JPEG only, the formats HEY's photo picker accepts, judged by the bytes rather than the extension. A HEIC from a phone would otherwise leave the contact with a broken avatar. - The header must decode with real dimensions, which catches a truncated or mislabelled file. - At most 40 MiB, the most HEY's web form can post through its proxy. The direct upload has no cap of its own. - A regular, non-empty file. The contact is read before the upload, so a mistyped ID costs a read rather than an upload. HEY answers 404 for a contact you cannot edit (hidden, an alias, another HEY user), so a not_found from either write says so in its hint.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
🟡 Changes recommended
The new API operations and upload caller must be recorded in API-COVERAGE.md.
1 open finding
What changed in this PR
Adds CLI support for setting and removing contact photos with validation and SDK-backed uploads.
Changes:
- Adds
hey contact photo set/removeandavataralias. - Validates PNG/JPEG files and provides comprehensive tests.
- Updates CLI documentation, skill guidance, smoke coverage, and command surface.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
internal/cmd/contact_photo.go |
Implements photo validation, upload, set, and removal. |
internal/cmd/contact_photo_test.go |
Tests validation, requests, errors, and output. |
internal/cmd/contacts.go |
Registers the photo subcommand. |
tests/smoke/contacts_test.go |
Adds end-to-end photo lifecycle coverage. |
docs/cli.md |
Documents contact photo behavior. |
skills/hey/SKILL.md |
Adds agent guidance and examples. |
.surface |
Records the new command surface. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
HEY resizes a contact photo with ImageMagick under hotcell/policy.xml, which refuses an image wider or taller than 16KP and spills one over 128MP to a disk cache that its disk ceiling refuses (haystack's imagemagick_policy_test proves 15000x15000). HEY would store such a photo and then fail to draw it, as with a HEIC, so the CLI refuses it from the dimensions it already decodes. A 200MP phone photo is past both limits. The help also repeats HEY's own advice from the profile photo page: a square image without transparency works best.
The 128MP cap came from policy.xml, but the cell that resizes a photo runs under tighter limits of its own (hotcell/Dockerfile: MAGICK_MEMORY_LIMIT 896MiB, and a 256MB file_size that kills a worker whose cache spills to disk). Running HEY's variant command under that policy and those limits in the hotcell image (ImageMagick 6.9.12 Q16) puts the line at 896MiB of pixel cache: - 8 bytes a pixel, so an upright photo tops out near 117MP (10800x10800 resizes, 11000x11000 does not, grayscale too); - 10 bytes a pixel for a CMYK JPEG, near 94MP (9600x9600 / 9800x9600); - twice that for a JPEG whose EXIF orientation is not upright, because auto-orienting it holds a second copy: near 58.7MP (7600x7600 / 7700x7700, for a flip, a 180 and a 90 alike) and 47MP for CMYK. Past it the worker is killed, a verdict HEY records as permanent. A PNG's orientation is not applied, so a PNG never pays twice. A 108MP phone photo taken in portrait is past the budget; 48MP ones are not. The CLI reads the JPEG's EXIF orientation itself (jpegOrientation, which reads only the markers before the scan) and the colour model from the header it already decodes. All 28 measured files get the verdict HEY gave them.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
HEY shows a contact photo at 300×300 at most and does the resizing itself, so the CLI only needs a sane ceiling, not a model of ImageMagick's memory. 58MP admits a full-resolution photo from a modern phone (an iPhone's 48MP, 8064×6048; 50MP Android cameras) and sits under the tightest ceiling measured against HEY's own variant command: about 58.7MP for a JPEG whose EXIF orientation has to be applied (7600² resizes, 7700² kills the worker). The 16000-pixel side limit from policy.xml stays. That retires the per-colour-model budget and the EXIF orientation reader. The one case left uncovered is a rotated CMYK JPEG over 47MP, which no camera produces.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…e photo - openContactPhoto stats the path before opening it, as prepareAttachments does: opening a named pipe blocks until something writes to it, so a FIFO hung instead of being refused. A Unix test proves it returns. - The smoke test fetches the avatar after setting it and requires a raster image. avatar_url changes with the blob whether or not HEY can resize it, and AvatarsController answers a photo it cannot resize with the contact's initials as SVG, so the URL alone proved nothing. Its cleanup also removes the photo before hiding the contact. - The upload-failure test checks that the failing request was reached. - A photo HEY cannot resize is never shown, and the contact keeps their initials; it is not a broken avatar. The comments and docs say so, the docs name the one case past the 58MP cap's cover (a rotated CMYK JPEG over 47MP), and the format hint says PNG or JPEG is this command's rule rather than claiming HEY can show nothing else. - photo remove mentions the 30-minute avatar cache, as set does. - API-COVERAGE.md lists the avatar endpoints and the new callers.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…e docs The avatar request used http.Get, so a stalled response held the smoke suite until its global timeout; it now uses a 10s client like the other direct requests. docs/cli.md wrote `hey contact photo remove` without the contact ID it requires.
Review round summary (head
|
|
Smoke tests now run against a local dev server (haystack
|
What
hey contact photo set <contact-id> <file>uploads a local image as a contact's photo. It callsAttachments.Upload, thenContacts.UpdateAvatarwith the blob's signed ID, and answers the contact with its newavatar_url.hey contact photo remove <contact-id>callsContacts.DeleteAvatar, which puts back HEY's default avatar. Removing from a contact with no photo still succeeds.avataris an alias forphoto.This is the CLI half of basecamp/haystack#8895 (the JSON contract) and basecamp/hey-sdk#229 (the operations, shipped in
go/v0.32.0; we already pin v0.32.1).Edge cases, from reading haystack
HEY's avatar endpoint (
Contacts::Avatars::UploadsController#update) attaches whatever blob it's handed. Nothing goes wrong until the avatar is drawn:Avatar#variant_forresizes it withblob.variant(resize_to_fill: [300, 300], …). HEIC and AVIF are removed fromvariable_content_types, so a photo straight off an iPhone, or a file that isn't an image, gets stored and is never shown.AvatarsController#showfalls back to the contact's initials, so the upload silently does nothing. The CLI therefore checks the file before sending anything:accept: "image/png, image/jpeg"(contacts/avatars/_edit.html.erb)image.DecodeConfig)max_request_body: 41943040(config/deploy.yml), so nothing bigger reaches a contact from the web. The direct upload to storage has no cap of its ownSize limits, measured
policy.xml's 128 MP area ceiling isn't the one that matters. The cell that resizes the photo runs under its own, tighter limits (hotcell/Dockerfile):MAGICK_MEMORY_LIMIT=896MiB, and a 256 MBfile_size(hotcell/config.rb) that kills a worker whose cache spills to disk. haystack notes that this verdict is recorded as permanent.I ran HEY's variant command (
-auto-orient -resize 300x300^ … -extent 300x300 -quality 40 -strip) in the hotcell image (ImageMagick 6.9.12 Q16), under HEY'spolicy.xmland the cell's env and rlimits, on 28 generated photos:A PNG's orientation isn't applied (a 9000² PNG with orientation 6 works), so PNGs never pay double.
The CLI doesn't model any of that. HEY does the resizing, and shows a contact photo at 300×300 at most, so the CLI caps at 58 MP. That admits a modern phone's full-resolution photo and stays under the tightest measured ceiling (58.7 MP, rotated JPEG), so a photo the CLI accepts always resizes. The only exception is a rotated CMYK JPEG over 47 MP, which no camera produces. The 16000 px side limit from
policy.xmlstays.The 40 MiB byte cap is the one limit that isn't about something failing. The cell reads its input through a file descriptor, and direct uploads bypass the proxy, so HEY would take bigger files. It matches what the web form can send.
The filename doesn't matter. Rails picks the variant format from the extension only when it agrees with the content type (
Representable#format, at haystack's pinned revision), and the CLI declares the type it sniffed. So a PNG named.jpg, or a JPEG with no extension, renders fine. The help also repeats HEY's advice from the profile photo page (contacts/users/edit.html.erb): a square image without transparency works best.Other behaviour:
ensure_contact_editable): hidden, alias, tombstone, system contact, or another HEY user. Anot_foundfrom either write therefore carries a hint saying so. Your own user contact is allowed.Not done, on purpose
photo remove.Tests
internal/cmd/contact_photo_test.go:.jpg, a JPEG with no extension).png, text named.jpg, broken PNG header, PNG cut off at its signature, JPEG cut off before its frame header, over the byte limitcontact_photo_unix_test.go, with a 5s deadline; it hangs without the fix)tests/smoke/contacts_test.go:TestContactPhotocreates a contact, sets a generated PNG, checks thatavatar_urlchanged and that fetching it returns a raster image rather than the SVG initials fallback, removes it, checks thatavatar_urlis back to the default, then removes again. Validation cases were added too. I haven't run it: the dev server wasn't up.make testpasses and golangci-lint v2.14.0 reports 0 issues..surface,docs/cli.mdand the skill are updated.