Skip to content

feat(crews): per-tier avatar reactions -- ghost motions, static pictures, pack-carried sounds - #10087

Merged
iamwhatever merged 1 commit into
mainfrom
feat/avatar-reactions-backend
Sep 11, 2026
Merged

feat(crews): per-tier avatar reactions -- ghost motions, static pictures, pack-carried sounds#10087
iamwhatever merged 1 commit into
mainfrom
feat/avatar-reactions-backend

Conversation

@iamwhatever

@iamwhatever iamwhatever commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

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) and sounds (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: CrewStateAvatar reads soundsFrom(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 a done animation could not ship the done sound, 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 ghost motions is 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.

motions is 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. sounds and expressions stay 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; only motions is 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 sounds section of its manifest, naming an .mp3, .ogg or .wav file 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's warnings list, so our own export always re-imports and the person who picked the file still reads why it is silent. Structural problems — a sounds value 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.json is 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
Loading

🟩 added · 🟨 changed · 🟦 unchanged

The two legacy keys stay where they are on every tier. What is new is stored where it can be played: motions on the ghost alone, and a pack's own audio inside the pack.

What a user loses, stated plainly

Nothing. expressions and sounds both round-trip on every tier: CrewAvatar.tsx:326 still draws a ghost's eyes/mouth picks, CrewAvatarBuilder.tsx:538 still submits expressions on a picture or pack save, and CrewStateAvatar.tsx:48 plays 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. Only motions is 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 motions have no player and pack audio has no listener; nothing a user already has is affected. A motions pick set through the API inside the window is not erased by the shipped editor either: _carry_motions_through_motionless_save keeps a ghost's stored motions when 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_save exists — 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, none kept as explicit stillness, junk stripped without collapsing the avatar, empty maps omitted rather than stored, a traits-less ghost carrying only reactions valid, expressions round-tripping on every tier (eyes/mouth only, truncated) and motions stripped on picture and pack. Endpoint round-trips for each tier, including that a committed picture stores exactly kind/v/file/sounds and that keeping the current picture keeps the pin and stays silent. The motions carry: an editor-style ghost save (traits + expressions + sounds, no motions key) 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, idle not a cue state, a linked cue file refused), pack_detail reporting presence without inlining audio, bundle round-trip returning a playable cue, export reading one revision (pack_revision stable 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 in warnings while a structural problem (non-map sounds, 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_payload returns the carry or None), 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-shaped expressions axis 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_tier is 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 a TestServer, 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.py used 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 the PYTEST_CURRENT_TEST environment variable, whose ceiling is 32767 characters, so both items errored at setup with ValueError, 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 explicit ids=, and the root conftest.py refuses any node id over MAX_NODEID_CHARS (30000) at collection with the offending id named -- the message the shard log never got to show. test/test_nodeid_length_guard.py pins the guard from both sides and asserts nothing collected in the session is oversized.

test/fixtures/appearance_packs/cue-demo.bundle.json is 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:

kirocrew pod up avatar-reactions --provision --json   # prints the base URL and a token
BASE=<url from that output>; TOK=<token from that output>
AUTH="Authorization: Bearer $TOK"
  1. A ghost keeps its reactions.
    curl -sX PUT "$BASE/api/agents/nova" -H "$AUTH" -H 'content-type: application/json' \
      -d '{"avatar":{"kind":"ghost","motions":{"done":"bounce","error":"shake"},"sounds":{"done":"chime"}}}'
    curl -s "$BASE/api/agents" -H "$AUTH" | python3 -m json.tool | grep -A6 '"avatar"'
    The record round-trips with both keys.
  2. A cross-state motion is dropped, not refused. Repeat step 1 with "motions":{"done":"shake"} — HTTP 200, and motions is absent from the stored record.
  3. A picture is static but keeps its cue. Upload a PNG to POST /api/agents/nova/avatar, then PUT {"avatar":{"kind":"image","promote":true,"token":"<staging token>","motions":{"done":"bounce"},"sounds":{"done":"pop"}}}. The stored record is kind/v/file/soundsmotions is gone, sounds stays.
  4. A pack carries its own. Import the shipped fixture and fetch its cue:
    curl -sX POST "$BASE/api/appearances/import" -H "$AUTH" -H 'content-type: application/json' \
      -d "{\"bundle\": $(cat test/fixtures/appearance_packs/cue-demo.bundle.json)}"
    curl -s "$BASE/api/appearances/cue-demo" -H "$AUTH" | python3 -m json.tool | grep -A3 '"sounds"'
    curl -sD - -o /tmp/done.wav "$BASE/api/appearances/cue-demo/sound/done" -H "$AUTH" | head -6
    The detail payload reports {"done": true}, and the route answers 200 with Content-Type: audio/wav and Cache-Control: private, max-age=60. file /tmp/done.wav says RIFF/WAVE, and it plays.
  5. A state with no cue, and the built-in. GET /api/appearances/cue-demo/sound/working → 404 sound_not_found; GET /api/appearances/kiro-ghost/sound/done → 404 builtin_no_content.
  6. A crew wearing that pack keeps its record cue, loses only 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 own done.wav still answers step 4's route; the two cues live on different paths.
  7. An unplayable cue warns, a sound-only bundle refuses, both where the user can see it. Re-import the fixture with done.wav replaced 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 with idle.svg removed → 400 "That bundle has no art in it".
  8. Export reads one revision. With the Crew Companion app enabled, GET $BASE/api/apps/crew-companion/appearances/export?id=cue-demo in a loop while a second shell re-saves the pack through POST $BASE/api/apps/crew-companion/appearances/save alternating two idle.svg bodies 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 existing pack_not_found 400) rather than exported as a mix.
kirocrew pod down avatar-reactions

Related Issues

no linked issue: one work item of a larger avatar-reactions change; the frontend halves land separately.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@iamwhatever
iamwhatever requested a review from a team September 11, 2026 08:15
@iamwhatever
iamwhatever requested a review from a team as a code owner September 11, 2026 08:15
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 22df9491621ac2f353281a00f8f1f6a844b8d571 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 save_pack absent-key-preserves tri-state is the right layer (the gallery editor's read-modify-write cannot round-trip audio that pack_detail deliberately doesn't inline), and the pack_revision optimistic snapshot beats the lock alternative because it confines the cost to the one multi-read caller. The transitional window (stored motions with no player, a sound route with no listener) is process-sequenced rather than code-enforced, but the forgiving validators make every dormant value droppable, so the door stays two-way. Docs updated in the same commit; description and diff match on every claim I checked, including the expressions carry on the picture commit path.

[DESIGN-REVIEWED] 22df949

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ⏭️ skipped

Revision 22df9491621ac2f353281a00f8f1f6a844b8d571 touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 22df9491621ac2f353281a00f8f1f6a844b8d571 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 22df949

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 22df9491621ac2f353281a00f8f1f6a844b8d571: <one-sentence reason>

@iamwhatever
iamwhatever force-pushed the feat/avatar-reactions-backend branch from 61e777c to 2435758 Compare September 11, 2026 08:27
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 22df9491621ac2f353281a00f8f1f6a844b8d571 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: CONCERNS

Four zero-consumer surfaces (motions, the sound route, detail sounds, import warnings) rest entirely on the unverifiable claim "This PR is held to merge with" the frontend siblings.

Not justified as shipped

  • 1 — zero consumers: grep motions under website/src hits 9 files, all crew-companion fidget/keyboard code; nothing reads an avatar motions value. Declared transitional, but the closing of the window is a process claim, not a repo fact.
  • 4 — zero consumers: grep sound/ under website/src → 0 matches; no client fetches the route.
  • 5 — zero consumers: nothing in the frontend reads pack_detail's sounds map.
  • 8 (warnings half) — zero consumers: grep warnings in CrewAvatarLibraryTab.tsx and the crew-companion app → 0; no shipped client displays the list the description says "the person who picked the file reads".
  • 9 — rides along: a suite-wide collection gate on a feature PR; harm named (Windows 32767-char env-var limit, a real shard incident), so justified on merits but a separate logical change.
  • 10 — rides along: black-baseline removal of the two touched files, mechanical.

What this change ships

Inventory (10 items) — 4 justified

Intent: 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).

  1. A ghost crew can store per-state motions through the API — zero consumers
  2. A motions value on a picture or pack payload is silently stripped — justified
  3. An unrelated editor save no longer erases API-set ghost motions — justified
  4. A pack's manifest may name per-state audio, served at GET /api/appearances/{id}/sound/{state} by byte-sniffing — zero consumers
  5. Pack detail now advertises which states have cues — zero consumers
  6. Re-saving a pack without naming sounds keeps the cues; an unreadable declared cue refuses the save — justified (consumer: crew_companion/backend/routes.py:312)
  7. Export carries cues and refuses a bundle mixing two pack revisions (pack_revision, 3 retries) — justified (consumer: routes.py:325)
  8. Import installs unplayable audio and names it in a warnings list; structural sound problems refuse; "no art" counts art — zero consumers (the warnings field)
  9. Collection refuses any test node id over 30000 chars, suite-wide — rides along
  10. store.py/transfer.py reformatted and dropped from the black baseline — rides along

Watch

  • The whole transitional design hangs on one sentence: "This PR is held to merge with them, so no release is cut inside the window." Nothing in this repo enforces that; if the frontend half stalls, four public surfaces (a config key the carry helper actively preserves, an API route, two response fields) ship with no reader.
    Clears when: the sibling frontend PRs are linked from this one, or one counted consumer lands.
  • "the import response is the one place the person who picked the file reads the problem" — grep count above is 0: no shipped client renders warnings, so today the message reaches only a curl caller. Clears when: the frontend change renders the import warnings list.

[FIRST-PRINCIPLES-REVIEWED] 22df949

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 11, 2026
@iamwhatever
iamwhatever force-pushed the feat/avatar-reactions-backend branch from 2435758 to 2e89fc1 Compare September 11, 2026 08:43
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 11, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-26de828680e5.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-4040be09418a.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-fa2e016aed55.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-c02c7c389f3b.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-ae6d98306d4c.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-7688b6ce3d2d.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-6e0905aaa56d.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-1b36bf6e715c.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-47f65a7d482a.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-a3c9d40f32c1.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-944a245f001e.md

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

@/home/zejiangg/.kiro/crew/scratch/runtime-422299d6/disp-6b2821ff3949.md

@iamwhatever
iamwhatever force-pushed the feat/avatar-reactions-backend branch from d7becfa to 7e490fe Compare September 11, 2026 17:33
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 11, 2026
@iamwhatever
iamwhatever force-pushed the feat/avatar-reactions-backend branch from 7e490fe to 2285974 Compare September 11, 2026 17:50
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 11, 2026
@iamwhatever
iamwhatever force-pushed the feat/avatar-reactions-backend branch from 2285974 to 7a7459a Compare September 11, 2026 18:15
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 11, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Export can combine art and cues from different saves — span=ba1ebbb726ab — fixed

Right. export_bundle read the art through pack_detail and the cues through pack_sound_payload, two unlocked manifest reads, while every save_pack is a whole-directory swap -- so a save landing between them handed back revision A's art with revision B's cue, a pack the user never had that imports cleanly. I had made the SAME class of mistake twice already on this PR (the unreadable-cue export, the art-only save), so this one is on me for not asking "what else reads twice".
Fixed by snapshot identity rather than a lock: AppearanceStore.pack_revision returns the device/inode/mtime of the pack directory and its manifest, which a swap changes even for byte-identical content (staging is a fresh inode created before the old directory is removed, so it cannot be reused). export_bundle records it before the first read and compares after the last; a mismatch discards and re-reads, up to _EXPORT_SNAPSHOT_ATTEMPTS (3), and a pack that changes on every attempt is refused rather than exported as a mix -- the same answer a declared-but-unreadable cue gets. A lock across every store read would have been the wider change for one multi-read caller, and would not have covered a second process.
Five tests pin it: pack_revision stable across reads and new on every save (including an identical-content save), a save injected between the art read and the cue read yielding the NEW revision's art AND cue together with nothing of the old, a pack that changes on every read refused after exactly the bounded number of attempts, a still pack exported on the first read, and None for the built-in and a malformed id.

self-added: yes

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • The spec, the manual verification, and the code disagree about pack-tier cues — span=6030afa186bd — fixed

Correct on all three, and the lane named the cause exactly: those sentences describe the design the user overruled (strip on picture/pack) while the code and tests pin the one that shipped (keep on every tier). config.md's carve-out paragraph now says _carry_pack_through_faceless_save rides the save's expressions and sounds onto the kept pack and leaves only motions behind, with the reason (both legal on every tier; a pack's own cue answers a different route, so they do not collide). Manual step 6 now shows the stored record {"kind":"pack","id":"cue-demo","sounds":{"done":"chime"}} with motions gone -- what test_agent_avatar.py pins. sounds.py's module comment and read_sound docstring now say the importer WARNS and installs rather than refuses, which is what import_bundle does since the previous head.
Also corrected while in there: manual step 7 (oversize → 200 with warnings, not a refusal) and a new step 8 for the one-revision export the GPT lane's finding on this head produced.

self-added: yes

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Surface the "declared sound cue cannot be read" refusal in the save/export API error — span=f9e0b8a00447 — deferred

Agreed it is the right next step and deliberately not in this PR. The refusal today surfaces as the store's False / None, which the Companion routes (crew_companion/backend/routes.py, untouched here) map to a generic 400; giving it a code and a message means changing the store's return shape (save_pack → bool, export_bundle → dict|None) and every caller of both, in an app this PR does not modify. That is its own change with its own tests, and this PR already grew two rounds past its dispatch to close data-loss findings. The escape hatch ("sounds": {}) is documented in learn-cron-dashboard.md's library paragraph so the remedy is at least written down.

self-added: no

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Dashboard saves erase stored ghost motions — span=c2b062f89868 — fixed

Right, and it is the mirror of the argument I made for keeping expressions: a save from a client that cannot see a value is not a decision about it. The shipped builder rebuilds a ghost draft from traits/expressions/sounds and submits exactly those, so a motions pick set through the API was erased by the next unrelated save.
handlers/agents._carry_motions_through_motionless_save now keeps a ghost's stored motions when a ghost save has NO motions key. Naming the key -- {} included -- is the caller's statement and replaces them, so a client that knows the key can still clear it. Narrow on purpose: both the stored record and the validated save must be ghosts (a tier change is a real face replacing the old one; a reset means reset). Same tri-state save_pack gives a pack's cues, same shape as _carry_pack_through_faceless_save, and it retires with the frontend change that submits motions itself.
Six endpoint tests: editor-style save keeps them, a traits change keeps them, {} clears, a named map replaces, a reset does not carry, a tier change does not carry. Spec'd in config.md beside the pack carve-out.
Not taken: deferring motions persistence. The dispatch names the vocabulary as this PR's contract, and the sibling frontend PR is what plays it.

self-added: yes

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Split cue reads can silently lose audio — span=cb279efbd8d2 — fixed

Right. _declared_sound_states (the refuse set) and pack_sound_payload (the carry) were two passes over the same files, and a read that failed only on the second pass produced a carry missing a cue and a refuse set that did not mention it -- the save went through, the export succeeded, the cue was gone. Exactly the class of split-read bug the previous head's export fix was about, one level down.
Collapsed into ONE traversal: pack_sound_payload now returns the carry (states, files) or None when a present cue's read failed, reading each file exactly once. _declared_sound_states is deleted. save_pack and export_bundle both consume the single answer.
Two tests: a read that fails once INSIDE the carry traversal (armed only while pack_sound_payload runs, because pack_detail's presence scan reads the same files first on the export path and would otherwise absorb the failure) refuses both save and export and leaves both cues on disk; and each cue file is read exactly once by the carry.

self-added: yes

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Overloaded sounds key, two shapes, one data-loss guard — span=002a30fdc599 — rebutted

The two shapes cannot meet on the wire. pack_detail reports "sounds": {state: true}; save_pack's carry condition is "sounds" not in manifest. The gallery editor sends a MANIFEST built from its own draft (meta/states/moods/random), not the detail payload echoed back -- pack_detail is the RENDERER's shape, with art inlined per slot and a sounds presence map, and save_pack refuses a manifest without a meta dict and ignores animations, so a detail payload posted as a manifest was never a valid save. A client that did copy the presence map into a manifest would send {"done": true}, which _safe_filename(True) drops as an unusable name -- the cue would be REMOVED, which is what an explicit map means. That is the contract the tri-state states and the tests pin (test_an_editor_style_resave_keeps_the_cue sends no key; test_an_explicit_empty_sounds_map_removes_the_cue sends {}).
Renaming the detail field is a frontend contract change (the Companion gallery reads sounds) and out of this PR. Treating a map with no string values as absent would make {"done": true} and {} mean different things, which is a worse overload than the one flagged.

self-added: no

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • The all-or-nothing refusal treats the condition as transient, but a link-refused cue is permanent — span=86fd0926b20b — rebutted

A symlinked cue file IS permanent, and refusing it is the point: the alternative is carrying forward a pack minus a file the user put there, on a save that reported success. The escape is not only "sounds": {} -- the user can replace the link with a file, or save with a sounds map naming a new file, both from the gallery. Naming the remedy in the API error is the previous head's f9e0b8a00447 suggestion, deferred there for the same reason: it changes the store's return shape and the Companion routes this PR does not touch.

self-added: no

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Item 1 — zero consumers for motions — span=d08c050dff67 — rebutted

Correct that nothing in website/src reads or writes motions at this head, and stated as such in the PR body's transitional-window paragraph. It is the backend half of one change whose frontend half (the picker that writes it, the player that draws it) is the sibling work item; the dispatch split them so the frontend can build against a merged contract rather than a branch. A vocabulary with no consumer costs nothing at load (_safe_motions drops junk silently) and, since this head, is not erased by the shipped editor either.

self-added: no

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Item 3 — zero consumers for /sound/{state} — span=15a8a3489c70 — rebutted

Same sequencing as item 1, with the provenance the lane itself found: the base feature-map recorded pack-carried sound cues as returning in a later change, and this is that change. The route is owner-gated, serves only bytes the store already holds, and answers 404 for every pack that carries none, so a release with no listener ships an endpoint nobody hits, not a behavior change.

self-added: no

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Item 4 — nothing reads pack_detail["sounds"] presence — span=453fd0247d63 — rebutted

The Companion gallery's detail view already renders pack_detail, and the presence map is what it will branch on to show a cue indicator without fetching audio -- the reason it is presence rather than inlined bytes is spelled out in learn-cron-dashboard.md. It is one line of the payload, additive, and the built-in answers {} so no client branches on a missing key.

self-added: no

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Item 10 — undeclared rider: the credential-shaped expressions mask pin was deleted — span=5f2109041f85 — fixed

Fair. The pin was removed in an earlier head when expressions was going to retire, and survived its un-retirement. agents.py still masks a credential-shaped expressions axis value in the roster, and the assertion is restored as test_a_credential_shaped_expression_value_is_masked -- widened to check the mask on a pack record too, since the key is now legal on every tier. Same gap as 4e6805312b82.

self-added: yes

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Items 1/3/4 rest on a hold-to-merge no linked PR verifies — span=a9aa94f13c5f — rebutted

The hold is the conductor's, not this PR's to enforce: the sibling frontend item is dispatched from the same goal and will link here when it opens. Until then the PR body says the window exists and what is inside it; a release cut inside it ships an unused config key and an owner-gated route that 404s, which is the whole exposure.

self-added: no

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • The deleted pin recorded that expressions axes carry user text and mask like traits — span=4e6805312b82 — fixed

Same gap as 5f2109041f85; one restoration covers both -- test_a_credential_shaped_expression_value_is_masked is back, on a ghost and on a pack record.

self-added: yes

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • Defer the motions half into the frontend PR that plays it — span=d6f3e3395867 — rebutted

Separable, yes; deferred, no. The dispatch names _AVATAR_MOTIONS as part of this PR's contract so the frontend picker can target a merged vocabulary, and since this head the key is protected against the one data-loss path the window opened (the shipped editor's saves). What is left is a zero-option key nobody writes, which is the cost the lane itself calls nothing.

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 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

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