Skip to content

Phase A: add canonical avatar resolver#680

Merged
lilyshen0722 merged 1 commit into
mainfrom
codex/569-avatar-unification-phase-a
Jul 25, 2026
Merged

Phase A: add canonical avatar resolver#680
lilyshen0722 merged 1 commit into
mainfrom
codex/569-avatar-unification-phase-a

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Phase A of #569. This PR adds the canonical avatar API and normalizes new writes without migrating or mutating existing data.

  • add normalizeAvatarUrl, resolveAvatarUrl, and batched resolveAvatarUrls
  • store Commonly upload references as relative /api/uploads/... paths at profile, upload, template, registry seed, and agent identity creation boundaries
  • keep package iconUrl separate from participant identity: package icons seed only newly created agent users, while reinstall preserves customized User.profilePicture
  • stop template edits from overwriting existing agent-user identity
  • make v2 avatars and relative image-message uploads resolve through the configured API origin
  • preserve legacy color-avatar identifiers instead of treating them as upload object keys

Rebase audit (2026-07-24)

  • rebased directly onto current main (5d1bf59c)
  • resolved the authController conflict by retaining both the current community-pod wiring and the avatar normalizer
  • confirmed this is not superseded: main still has the Unify agent-avatar source — 5 divergent stores broke independently after domain migration #569 Mongo/PG/object/package avatar-store divergence and no canonical backend resolver
  • fixed two stale-branch regressions found during the audit:
    • blue and other legacy color choices no longer become /api/uploads/blue
    • v2 image messages resolve the new relative upload response against the API origin, not the frontend host

Migration safety

  • no migration or backfill runs in this phase
  • no schema or existing-row changes
  • existing absolute, relative, data URI, external, and legacy color-avatar reads remain supported
  • Phase B read-through, Phase C dry-run-first backfill, and Phase D cache removal remain follow-ups after this is verified live

Verification (rebased head 69973c56)

  • backend changed-path suites under Node 20: 10 suites / 99 tests passed
  • backend full suite under Node 20: 188 suites / 1,332 tests passed (then emitted the repository's existing open-handle warning)
  • backend TypeScript production build: passed
  • frontend full suite: 68 suites / 308 tests passed
  • frontend production build: passed
  • changed frontend ESLint files: 0 errors (repository-rule JSX extension warnings only)
  • git diff --check: passed

No CSS or layout rules changed. The frontend compatibility checks are DOM/render assertions plus the production build; a browser-layout pass is not applicable.

@samxu01 samxu01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

APPROVE — code is right. One product consequence needs an explicit decision before merge (not a code defect).

Verified against the Phase A spec. avatarService.ts is exactly the contract asked for: absolute Commonly origins collapse to /api/uploads/<id>, already-relative and data: pass through, 'default' and empty → null, external CDN URLs are preserved (right call — we can't invent a local object key for them), and resolveAvatarUrls batches to one query with missing ids as null. The test table covers all of it. No migration, no schema change — reversible by revert, as specified.

The two behavioral fixes are the heart of it and both are correct per CLAUDE.md rule 8:

  • agentIdentityService.ts:395 seeds profilePicture from the package icon only when creating a new identity — reinstall preserves a customized avatar (tested).
  • templates.ts drops both User.updateMany calls, so editing package metadata no longer reaches into live identity rows.

I also chased the one thing that looked like a Critical and cleared it: handleUpload now returns a relative URL for every upload, not just avatars (post images, chat attachments). That's a contract change wider than "avatar write path" — but it's safe, because every render path already normalizes: normalizeUploadUrl (apiBaseUrl.ts:37) resolves /api/uploads/... against the API origin and is already called by PostFeed, ChatRoom, Thread, AgentCard, and avatarUtils; v2 attachments already build relative paths and sign them (V2MessageBubble.tsx:250). I found no server-side path that ships an upload URL to an external platform (Discord's att.url values are inbound). Old absolute URLs still render, so both shapes coexist. The new uploads.post.test.js assertion pinning the response to relative-and-not-https:// is the right guard. Deleting the cf-visitor scheme-sniffing block is a real win — that whole class of Mixed-Content bug is now unreachable.

Important — for the merger, not the author

Removing the templates.tsUser.updateMany sync is correct, and it removes the only path that currently exists for changing a live agent's avatar. Editing a template icon was the (accidental) mechanism people were using. After this merges, a deployed agent's avatar can only be changed by authenticating as that agent against updateProfile, or by a direct DB write — there is no UI. That's exactly the gap #570 is meant to fill, and this PR is its foundation, so the sequencing is intentional. But it is a real, user-visible capability regression in the window between this merge and #570.

@xcjsam — your call, and it's a genuine fork: (a) merge as-is and treat #570 as the immediate next item (my recommendation — the old path was a bug that overwrote identity, and preserving it would preserve the thing we're fixing), or (b) ask for a small admin-only PATCH /api/agents/:id/avatar in this PR to bridge the window. Don't merge without picking one.

Nits (non-blocking)

  • normalizeAvatarUrl maps 'default'null, but agentIdentityService still writes the literal string 'default' as its fallback (:395) and the User schema defaults to it. Two sentinels for "no avatar" now coexist. Readers are consistent (resolveAvatarUrl maps 'default'null), so nothing breaks — but Phase C is the moment to collapse them to one.
  • authController / userController changed if (profilePicture)if (profilePicture !== undefined), so an empty string now clears an avatar instead of being ignored. That's almost certainly intended (it's how #570 will clear one) — just naming it so it's a decision, not an accident.

@lilyshen0722
lilyshen0722 force-pushed the codex/569-avatar-unification-phase-a branch from b08e315 to e98baaf Compare July 19, 2026 11:18
@lilyshen0722
lilyshen0722 force-pushed the codex/569-avatar-unification-phase-a branch from e98baaf to 69973c5 Compare July 24, 2026 17:06
@lilyshen0722
lilyshen0722 merged commit e368465 into main Jul 25, 2026
11 checks passed
@lilyshen0722
lilyshen0722 deleted the codex/569-avatar-unification-phase-a branch July 25, 2026 02:18
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