[chazmaniandinkle]

Stacked on #154 (body channel). Agents author their own animations as data-only JSON clips, keep personal libraries, and trigger them with args.

• clip.py: clip format + compiler. Tracks per semantic channel (head.x, eyes.open, mouth.smile, ...) summed from const/keys/wave/jitter/follow, optional smoothing/fade/base state. Args with ranges; numeric fields can be arithmetic over args, resolved server-side by a whitelisted AST walk, so the body never runs agent code.
• POST /v1/bodies/{id}/play, /stop, POST /v1/clips/check. Tracks a body can't drive are dropped and reported, not fatal.
• dashboard/clip-player.js: layered over procedural idle, under held params. Receipts carry semantic channel values + running clips.
• clips/: shared starter library. Vroku's states rebuilt as clips (credited), plus nod/shake/think.
• scripts/mod3-body: stdlib CLI: list/show/play/try/stop/check/save/fork/rm/act. Personal library at .cog/body/clips or ~/.mod3/body/clips.
• skills/body/SKILL.md: the shared skill every agent inherits.

Verified: 24 tests (compiler, endpoints, code-injection refusal). E2E with tests/e2e_clip_body.mjs (real player code in Node against a live server): nod oscillates and returns to rest, refusing-to-look settles at the requested pose and fades to 0 on stop, dancing counter-phases head/body, and a personal clip saved/forked/played with an unsupported channel dropped. Not yet verified: a rendered Live2D model in a browser.

[github-actions]

🤖 cog-review — request_changes (head f9adb65)

This PR adds a well-designed, data-only agent clip system (clip.py compiler, play/stop/check endpoints, JS player, starter library, CLI, skill doc) with a genuinely safe expression evaluator for track/duration/smooth fields (a whitelisted AST walk with no Pow/subscript/attribute/call-to-arbitrary-name support, verified by test_expressions_are_arithmetic_only). However, resolve_args() in clip.py breaks that safety contract for one specific sub-path: the args-spec's min/max bounds (lines 141-142, 152) are read with a bare float(s.get("min"/"max")) instead of going through the same evaluate()/ClipError machinery used for every other numeric field in the file. A non-numeric min/max (a string that isn't a valid float literal, or a list/dict) raises a plain ValueError or TypeError, which is not a ClipError subclass and is therefore not caught by the except ClipError handlers in body.py's play() or http_api.py's clips_check/bodies_play — producing an unhandled 500 instead of the clean, field-named 400 that this feature explicitly promises (SKILL.md: "errors name the field"). This is easily reachable by a normal, non-malicious authoring mistake (e.g. an agent assuming, per the module's own docstring claim that 'numeric fields may be strings: arithmetic over args', that min/max also accept expressions) and is untested — no test in tests/test_clips.py exercises a malformed args.min/max. The rest of the diff (body.py's play/stop/_send_and_wait refactor, the FastAPI endpoints, the JS ClipPlayer, the manifest/receipt channel-field schema change, and the shipped clips) checks out logically and is internally consistent; I could not execute the test suite in this sandbox to confirm the claimed '24 tests' pass, so that claim is noted as unverified rather than confirmed.

Confirmed findings (2):

• clip.py:141 — args-spec min/max bounds are parsed with a bare float() instead of the evaluate() whitelist used for every other numeric field, so a non-numeric min/max raises an uncaught ValueError/TypeError rather than ClipError.
  • Failure scenario: POST /v1/clips/check (or /v1/bodies/{id}/play) with clip.args = {"foo": {"min": "bad", "default": 1}} (or min given as a list/dict) causes float(s.get("min")) to raise, which is not caught by except ClipError in http_api.py's clips_check/bodies_play or body.py's play(), so FastAPI returns an unhandled 500 instead of the advertised field-named 400.
• clip.py:152 — The identical unguarded float(s.get("min"/"max")) pattern is repeated for argument overrides on the /v1/bodies/{id}/play args path, so the same crash class is reachable a second way.
  • Failure scenario: A clip's args spec has a min/max value that isn't a clean float literal; any override via the play endpoint's args dict re-triggers float(s.get("min"...)) / float(s.get("max"...)) at this line, raising an uncaught exception on every play() call that supplies an override for that arg, not just at clip-check time.

Unverified notes:

• Could not execute tests/test_clips.py or the e2e script in this sandbox (python invocation required approval that wasn't available); the PR's claim of '24 tests' and all-passing e2e behavior is not independently confirmed here, only read for plausibility.
• dashboard/live2d-body.js repurposes the pre-existing manifest channels field (sent in the body's hello/manifest and exposed via GET /v1/bodies) from a flat {name: paramId} shape to a nested {name: {polarity, params:[...]}} shape, and renames the receipt's old flat channels output to raw while introducing a new normalised channels. Nothing else in this repo currently consumes the old shapes, so it isn't a break today, but the PR description doesn't call out that this is a schema change to a field that already existed before this PR, only that 'receipts carry semantic channel values.'

---

This review was generated by an AI reviewer with review-only authority — it can approve or block, it cannot merge or close. Verdict basis: pr-review-rubric.md. The merge decision belongs to a human or their operator workflow.

[github-actions]

🤖 cog-review — request_changes (head be89c10)

The clip compiler, body.py play/stop wiring, and dashboard ClipPlayer are well-structured and the AST-whitelist expression evaluator genuinely blocks code execution (confirmed by reading clip.py's _walk and the accompanying refusal tests); the previously-fixed replaced-socket-close path is inherited correctly by the new play/stop commands via the shared _send_and_wait, so the stacked PR doesn't regress #154's fix. However, scripts/mod3-body's fork command builds the destination file path directly from the clip's self-declared name field (or --as) with no filename sanitization, unlike save which routes through the server's NAME_RE-enforcing /v1/clips/check first — this is a path-traversal/arbitrary-file-write gap that a maintainer would want closed before merge. There is also a real (if currently unexercised by shipped clips) state-restoration bug in clip-player.js: base state capture/restore isn't stack-based, so two overlapping clips that both set a base state can have the earlier-finishing one clobber the state out from under the still-running one. Neither issue is covered by the 24 added tests, which exercise the Python compiler and HTTP endpoints but not scripts/mod3-body's filesystem commands or ClipPlayer's multi-clip base interaction.

Confirmed findings (2):

• scripts/mod3-body:217 — fork writes d / f"{clip['name']}.json" using the clip JSON's own name field (or --as) with no NAME_RE/slash check, unlike save which validates via POST /v1/clips/check before writing.
  • Failure scenario: A clip file present in the shared or personal library (filename irrelevant) has its JSON name field set to /tmp/pwned or ../../../../.ssh/authorized_keys. Running mod3-body fork <that-clip> (no --as needed) executes p = d / f"{clip['name']}.json"; since joining a pathlib Path with a string that starts with / discards the base directory entirely (and .. segments escape it for relative traversal), p.write_text(...) writes the forked JSON to the attacker-chosen path instead of inside the library, i.e. an arbitrary file write/overwrite outside .cog/body/clips or ~/.mod3/body/clips. The same unsanitized-name pattern also lets rm delete arbitrary *.json paths and show/find read arbitrary files, since find() (used by both) performs no name validation either.
• dashboard/clip-player.js:67 — base state capture/restore in ClipPlayer.play()/tick() is not stack-based: a clip's prevState is captured as 'whatever setState() returns right now', so if two different clips both declare base and overlap in time, whichever finishes first restores its own captured prevState even though the other, still-running, base clip's state is live at that moment.
  • Failure scenario: Body is in state IDLE. Agent plays clip A (loop:false, duration_ms:2000, base:'VIBING') → A.prevState='IDLE', state becomes VIBING. At t=500ms, while A is still running, agent plays clip B (loop:true, base:'EXCITED') → B.prevState=setState('EXCITED') returns the then-current state 'VIBING', state becomes EXCITED. At t=2000ms A finishes (tick() line ~160) and runs this.setState(inst.prevState) = setState('IDLE'), forcing the body back to IDLE even though B is still actively looping and expects 'EXCITED' to remain underneath it — B's base state is silently clobbered until B itself stops.

Unverified notes:

• None of the 24 added tests exercise scripts/mod3-body at all (fork/rm/save/show/list), so the path-traversal gap above would not be caught by the described test suite.
• clip-player.js's multi-clip base interaction (overlapping base clips) is not covered by any test in tests/test_clips.py (which only checks the Python-side base not in body_states validation) or by tests/e2e_clip_body.mjs as included in the diff, so I could not confirm via a passing/failing test run whether this is already known/accepted behavior — flagging from code reading alone.
• Did not execute the test suite in this sandbox (pytest invocation was not permitted), so 'tests currently pass' is based on static reading of tests/test_clips.py against clip.py/body.py/http_api.py rather than an actual run.

---

This review was generated by an AI reviewer with review-only authority — it can approve or block, it cannot merge or close. Verdict basis: pr-review-rubric.md. The merge decision belongs to a human or their operator workflow.