feat(crews): per-tier avatar reactions -- ghost motions, static pictures, pack-carried sounds - #10087
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Per-tier gating, pack-carried audio, and the no-silent-data-loss carries are each derived from a named renderer/editor behavior — sound, proportionate design. The riskiest pieces hold up under the strongest alternatives: the [DESIGN-REVIEWED] 22df949 |
UX Review (Fable 5) — ⏭️ skippedRevision |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
61e777c to
2435758
Compare
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS Four zero-consumer surfaces ( Not justified as shipped
What this change shipsInventory (10 items) — 4 justifiedIntent: let each avatar tier carry only the reaction it can play, and let a pack supply its own per-state audio — an ADDITION (backend half of a two-part change).
Watch
[FIRST-PRINCIPLES-REVIEWED] 22df949 |
2435758 to
2e89fc1
Compare
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-26de828680e5.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-4040be09418a.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-fa2e016aed55.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-c02c7c389f3b.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-ae6d98306d4c.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-7688b6ce3d2d.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-6e0905aaa56d.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-1b36bf6e715c.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-47f65a7d482a.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-a3c9d40f32c1.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-944a245f001e.md |
|
@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-6b2821ff3949.md |
d7becfa to
7e490fe
Compare
7e490fe to
2285974
Compare
2285974 to
7a7459a
Compare
self-added: yes |
self-added: yes |
self-added: no |
self-added: yes |
self-added: yes |
self-added: no |
self-added: no |
self-added: no |
self-added: no |
self-added: no |
self-added: yes |
self-added: no |
self-added: yes |
self-added: no |
…res, pack-carried sounds
A crew's reactions now live on the tier that can play them. A ghost picks a
built-in motion and a synthesized preset per state; a picture is static and
silent; a pack ships its own per-state art and its own per-state audio, so its
record is just the pack id.
The pack tier gains a sound: an optional manifest `sounds` section names an
mp3/ogg/wav inside the pack, `GET /api/appearances/{id}/sound/{state}` serves one
state's bytes typed by sniffing them, and a bundle carries the cues with the art.
A cue is capped at 512 KB decoded and read through the ordinary pack-file read,
so it inherits the link refusal, the containment re-check and the size ceiling
every other pack file gets. One unusable entry is dropped with a warning and the
pack's art still loads.
Reactions stored on a tier that cannot play them are stripped silently, not
refused: config.json is hand-editable and agent-writable, so a version-skewed
record must cost the crew that key and never its whole face. The retired
`expressions` key loads the same way, on every tier.
bolichen97
left a comment
There was a problem hiding this comment.
Backend design is sound (tier-gated motions, sniffed pack audio, revision-guarded export, silent-drop on stale keys). Approving the code, but per your own note this PR is held to merge together with the sibling frontend PRs -- not merging it standalone.
Problem / Motivation
A crew's reactions are stored in one shape for all three avatar tiers: any record may carry
expressions(a per-state eyes/mouth pick) andsounds(a preset cue). The eyes/mouth half is dead weight on two of them — an uploaded picture is a still image with no eyes axis to move, and an appearance pack draws its per-state art from its own files, so a stored eyes/mouth pick has nothing to move either.The sound half is not dead:
CrewStateAvatarreadssoundsFrom(avatar)kind-agnostically, so a picture crew and a pack crew do play their preset cue today. That key therefore stays exactly where it is. What was missing is a pack's own audio: a pack is the one tier whose art the user supplies, and the pack that ships adoneanimation could not ship thedonesound, so a user who wanted their own audio had no way in and had to reach for one of six synthesized presets instead.Why it matters
For packs the cost is what a user cannot do at all: a pack is the one tier where art comes from the user, so it is the one tier where audio should come from the user too. Without it, a crew's sound is six built-in presets forever, whatever art it wears.
The eyes/mouth half (
expressions) is stored on tiers that cannot draw it, and on the ghostmotionsis the named vocabulary that will replace it. It still round-trips on every tier here, because the shipped builder submits it on every tier and the shipped ghost renderer draws it: a ghost → picture → ghost round-trip must not lose the picks.What changed (motivation → approach → change)
Each tier now carries the reaction it can play, and no more.
motionsis the GHOST's alone: it names a built-in animation of a trait-composed face (done:none/bounce/nod/sparkle;error:none/shake/cross-eyes/droop), so a picture has nothing to move and a pack animates from its own files.soundsandexpressionsstay legal on every tier, because that is what the shipped renderer plays and draws today and what the shipped builder submits. Both retire in the frontend change that stops reading them; onlymotionsis tier-gated. A pack gains its own per-state audio on top.Each state has its own motion list, and a value from the other state's list is dropped.
{"done": "shake"}would play a failure animation on success, which is not what its author wrote.A pack carries audio in an optional
soundssection of its manifest, naming an.mp3,.oggor.wavfile inside the pack.GET /api/appearances/{id}/sound/{state}serves one state's bytes, typed by SNIFFING the decoded bytes rather than by trusting the filename: a manifest is hand-editable, so a name is a claim and not evidence. A cue is capped at 512 KB decoded and read through the ordinary pack-file read, so it inherits the link refusal, the containment re-check and the per-file ceiling every other pack file gets. One unusable entry is dropped with a warning and the pack's art still loads.Bundles carry cues alongside art, so export → delete → import returns the pack the user had rather than a silent one — and they carry ONE revision of it. Export composes a bundle from several unlocked store reads while every save is a whole-directory swap, so a save landing between the art read and the cue read would hand back old art with new cues, a pack the user never had that imports cleanly.
AppearanceStore.pack_revision(the swapped-in directory's identity) is recorded before the first read and compared after the last; a mismatch re-reads, and a pack that changes on every attempt is refused rather than exported as a mix. Import judges the audio at the boundary: a cue that decodes but will not play is installed and named in the response'swarningslist, so our own export always re-imports and the person who picked the file still reads why it is silent. Structural problems — asoundsvalue that is not a map, a state naming a file the bundle does not carry, a name the reader cannot serve — still refuse the bundle, because they install a pack whose sound route then 404s. The art allowlist stayed narrow, and the "a bundle must contain art" refusal now counts art rather than files: a sound-only bundle installs a pack with nothing to draw.Every stripped key is dropped silently, never refused.
config.jsonis hand-editable and agent-writable, so a version-skewed record must cost the crew that key and never its whole face.flowchart LR subgraph Before R1[crew record]:::ctx --> E1[expressions + sounds<br/>on any kind]:::ctx E1 --> G1[ghost<br/>no named motion]:::ctx E1 --> P1[pack<br/>no audio of its own]:::ctx end subgraph After R2[crew record]:::ctx --> K2[expressions + sounds<br/>still on any kind]:::ctx K2 --> G2[ghost<br/>+ motions]:::added K2 --> I2[picture<br/>motions stripped]:::changed K2 --> P2[pack id<br/>motions stripped]:::changed P2 --> S2[pack files<br/>slot art + sound cue]:::added end classDef added fill:#DCFCE7,stroke:#16A34A,color:#14532D,stroke-width:2px classDef changed fill:#FEF3C7,stroke:#D97706,color:#78350F,stroke-width:2px classDef ctx fill:#E0F2FE,stroke:#0284C7,color:#0C4A6E linkStyle 3,4,5,6 stroke:#16A34A,stroke-width:2px🟩 added · 🟨 changed · 🟦 unchanged
The two legacy keys stay where they are on every tier. What is new is stored where it can be played:
motionson the ghost alone, and a pack's own audio inside the pack.What a user loses, stated plainly
Nothing.
expressionsandsoundsboth round-trip on every tier:CrewAvatar.tsx:326still draws a ghost's eyes/mouth picks,CrewAvatarBuilder.tsx:538still submitsexpressionson a picture or pack save, andCrewStateAvatar.tsx:48plays a crew-record cue whatever face it draws, so dropping either key on any tier would erase a value on an unrelated save — or on a ghost → picture → ghost round-trip — with no way back. Onlymotionsis tier-gated, and it is new, so nothing stored today is lost. Both legacy keys retire in the frontend change that stops reading them.Transitional window, and how it closes. This is the backend half of one change. Until the frontend half lands, stored
motionshave no player and pack audio has no listener; nothing a user already has is affected. Amotionspick set through the API inside the window is not erased by the shipped editor either:_carry_motions_through_motionless_savekeeps a ghost's storedmotionswhen a ghost save does not name the key (naming it,{}included, replaces them; a tier change or a reset does not carry), the same tri-state a pack's cues already get and the same reason_carry_pack_through_faceless_saveexists — a save from a client that cannot see a value is not a decision about it. The window is bounded by sequencing, not by hope — the frontend picker and the reaction player are the sibling work items of this same change, in flight now. This PR is held to merge with them, so no release is cut inside the window. Nothing else regresses: every stored cue keeps playing on every tier, and no record loses a trait, a picture or a pack.Tests
test/test_agent_avatar.py— motions accepted per state from that state's own vocabulary, a cross-state value dropped,nonekept as explicit stillness, junk stripped without collapsing the avatar, empty maps omitted rather than stored, a traits-less ghost carrying only reactions valid,expressionsround-tripping on every tier (eyes/mouth only, truncated) andmotionsstripped on picture and pack. Endpoint round-trips for each tier, including that a committed picture stores exactlykind/v/file/soundsand that keeping the current picture keeps the pin and stays silent. Themotionscarry: an editor-style ghost save (traits + expressions + sounds, nomotionskey) keeps the stored motions, a traits change keeps them, an explicit{}clears them, a named map replaces them, a reset does not carry them, a tier change does not carry them.test/test_appearance_packs_sounds.py— the cue reader (each container recognised, type from the bytes and not the filename, oversize dropped, at-the-ceiling kept, unusable filename dropped, absent file dropped, one bad cue not costing the good one, junk section non-fatal with the art still loading,idlenot a cue state, a linked cue file refused),pack_detailreporting presence without inlining audio, bundle round-trip returning a playable cue, export reading one revision (pack_revisionstable across reads and new on every save, a save injected between the art read and the cue read yielding the NEW revision's art and cue together and nothing of the old, a pack that changes on every read refused after a bounded number of attempts, a still pack exported on the first read), a sound-only bundle refused, a cue that will not play installed and named inwarningswhile a structural problem (non-mapsounds, a state naming a file the bundle does not carry, an unusable name) still refuses, and the preserve-on-overwrite rule with its refuse-on-unreadable half — decided by ONE traversal (pack_sound_payloadreturns the carry orNone), pinned by a read that fails once inside that traversal refusing both save and export while leaving both cues on disk, and by each cue file being read exactly once.test/test_appearance_library.py— the sound route: present, absent state, pack with no cues, missing pack, built-in, bytes that are not audio, oversize, ETag revalidation, no CSP on audio, the owner gate, the 503 on an unreadable library, and the accepted-read audit.test/test_agents_roster_contract.py— a reaction name is pinned rather than masked, a credential-shaped one never reaches the roster because validation drops it, and a credential-shapedexpressionsaxis value IS masked, on a ghost and on a pack record alike, because those two leaves are free text.The importer and the reader now share one predicate (
sounds.read_sound), and a test pins that a refusal reason exists exactly when the cue will not play, plus that the importer only formats that reason instead of decoding a second time — the drift that would otherwise install cues the reader silently drops.test_the_cue_survives_a_config_load_on_every_tieris the pin for the paragraph above: a ghost, a picture and a pack record each keep their stored cue through a real config load.The new tests start no HTTP server. The sound-route tests and the reaction round-trips call their handlers through
aiohttp.test_utils.make_mocked_request, so nothing binds a socket or runs a per-test event-loop server. Only the two multipart staging tests keep aTestServer, because a multipart POST needs a genuine request stream.Why the Windows shard 1 timed out, and the guard that stops it recurring. Two parametrized cases in
test_appearance_packs_sounds.pyused the oversize cue -- a 700 KB base64 string -- as the parameter VALUE, so pytest baked it into the node id. On Windows pytest exports the running node id as thePYTEST_CURRENT_TESTenvironment variable, whose ceiling is 32767 characters, so both items errored at setup withValueError, every report line for them carried 700 KB, and the shard ran to its 40-minute cap with its log dropped. Linux has no such ceiling, so nothing reproduced locally. Both cases now have explicitids=, and the rootconftest.pyrefuses any node id overMAX_NODEID_CHARS(30000) at collection with the offending id named -- the message the shard log never got to show.test/test_nodeid_length_guard.pypins the guard from both sides and asserts nothing collected in the session is oversized.test/fixtures/appearance_packs/cue-demo.bundle.jsonis a real bundle (one SVG frame, one ~1 KB 8-bit mono WAV) with a test that imports it, so the manual steps below stay runnable.Manual verification
An isolated instance, so nothing here touches a live install:
"motions":{"done":"shake"}— HTTP 200, andmotionsis absent from the stored record.POST /api/agents/nova/avatar, thenPUT{"avatar":{"kind":"image","promote":true,"token":"<staging token>","motions":{"done":"bounce"},"sounds":{"done":"pop"}}}. The stored record iskind/v/file/sounds—motionsis gone,soundsstays.{"done": true}, and the route answers200withContent-Type: audio/wavandCache-Control: private, max-age=60.file /tmp/done.wavsays RIFF/WAVE, and it plays.GET /api/appearances/cue-demo/sound/working→ 404sound_not_found;GET /api/appearances/kiro-ghost/sound/done→ 404builtin_no_content.motions.PUT{"avatar":{"kind":"pack","id":"cue-demo","motions":{"done":"bounce"},"sounds":{"done":"chime"}}}— the stored record is{"kind":"pack","id":"cue-demo","sounds":{"done":"chime"}}. The pack's owndone.wavstill answers step 4's route; the two cues live on different paths.done.wavreplaced by 600 KB of base64 WAV → HTTP 200 with"warnings": ["… is a longer sound than a pack may carry"]and the pack installed. Re-import it withidle.svgremoved → 400"That bundle has no art in it".GET $BASE/api/apps/crew-companion/appearances/export?id=cue-demoin a loop while a second shell re-saves the pack throughPOST $BASE/api/apps/crew-companion/appearances/savealternating twoidle.svgbodies and two cue files — every bundle that comes back pairs the art with the cue from the SAME save, and a pack that changes on every read is refused (the route's existingpack_not_found400) rather than exported as a mix.Related Issues
no linked issue: one work item of a larger avatar-reactions change; the frontend halves land separately.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)