Skip to content

Phase A: add canonical avatar resolver#680

Open
lilyshen0722 wants to merge 1 commit into
mainfrom
codex/569-avatar-unification-phase-a
Open

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

Conversation

@lilyshen0722

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 resolve canonical relative upload paths through the configured API origin

Migration safety

  • no migration or backfill runs in this phase
  • no schema or existing-row changes
  • existing absolute, relative, data URI, and external 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

  • backend focused suites: 10 suites, 95 tests passed
  • backend full suite: 171 suites / 1,243 tests exercised; one stale URL expectation was corrected, then both affected service suites passed (23 tests)
  • backend TypeScript check: passed
  • backend production build: passed
  • frontend full suite: 51 suites, 218 tests passed
  • frontend focused avatar test: 2 tests passed
  • frontend production build: passed
  • changed frontend ESLint files: 0 errors (2 repository-rule JSX extension warnings)
  • git diff --check: passed

Existing baseline findings

  • frontend tsc --noEmit still reports the three pre-existing errors in V2GithubPrCard.tsx and V2MessageBubble.tsx
  • repository-wide lint remains red on pre-existing baseline errors; changed files introduce no lint errors

No visual or layout rules changed, so the v2 compatibility check is a DOM/render assertion plus a production build rather than a browser-layout check.

@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
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