Repository navigation
Add magpie digital pet plugin - #539
manjughosa wants to merge 4 commits into
Conversation
yetone
left a comment
There was a problem hiding this comment.
Thank you for this, @manjughosa. A pet that grows from your real usage is a lovely idea, and the port is clearly a lot of work. I read every file and ran the package on a clean macOS box at head 40068d6. I can't approve it yet. Below are the blockers, then the smaller bugs, each with how I checked it.
Blockers
1. The sprites can't be redistributed here (licensing / IP)
lib/monster-frames.json (2.2 MB, 646 sprites, 7,227 frames) is the MONSTER_FRAME_DATA array from @jcendal/opencode-digital-pet 0.18.0's dist/tui.js. Running tools/extract-frames.mjs on that tarball reproduces the file byte for byte. The upstream README says those sprites were "derived from sundeth/omnipet and Tortoiseshel's Full Color Digimon Dot Sprites post … then edited":
- sundeth/omnipet is licensed CC BY-NC 4.0. NonCommercial terms can't be relicensed as Apache-2.0, which is what
package.jsonand NOTICE claim here. - The characters, names and catalog are Bandai / Toei's Digimon IP. Neither upstream nor this PR has a licence for them.
NOTICE also says "this port ships no sprite assets", which isn't the case.
This alone blocks the merge: the org repo and the npm package can't carry these assets. Possible routes:
- original art, or art under a licence that allows redistribution, with the catalog renamed to match;
- or no bundled art: the user points the plugin at assets they supply or download themselves, and nothing Digimon-derived goes into this repo or the npm tarball.
2. The local server is open to any web page
lib/pet.server.mjs listens on 127.0.0.1:3571 with access-control-allow-origin: *. It has no token, no Host check and no Origin check. The plugins README asks a page service to "guard every route with a token only its own page gets, and check Host". packages/page-chat/server.mjs is the model: a per-process token in a header, a host allow-list and an origin check. What I reproduced with curl against a running instance:
GET /api/pingwithOrigin: https://evil.examplereturned 200 with ACAO*and the absolute path ofpet.db. The OPTIONS preflight returned 204 with*.POST /api/command(pet-rename) withOrigin: https://evil.exampleandHost: attacker.examplereturned 200 and renamed the pet topwned. Because the Host header is accepted, a DNS-rebinding page gets in too.digital-pet-setwithOrigin: null(a sandboxed frame) returned 200 and pinned the pet./api/stateexposes token and cost totals.spawnretires the current pet, so any site can wipe the user's progress.
3. It overwrites another plugin's config
On every start, ensureGrowthSettings() in lib/pet.adapter.mjs rewrites ~/.config/opencode-digital-pet.json. That is the real global config of the upstream OpenCode plugin, and the write replaces stageThresholds with doubled values. A user who set egg: 1000 came back with 10000000 after one start. Please keep all state under this plugin's own folder and pass the thresholds to the engine in memory.
4. A fresh install doesn't start
In a new HOME the first start fails with start failed: unable to open database file, and nothing listens on 3571. createCareStore opens pet.db synchronously before the engine's async mkdir of ~/.config/magpie-digital-pet has run. The second start works only because the first one made the folder. Creating the directory before opening the DB fixes it.
5. The test suite fails in a clean checkout
CI's run is HOME=$(mktemp -d) bun test. On mote (macOS, Bun 1.4.2) it gave 3808 pass, 8 skip, 3 fail, 3 errors, exit 1, and every failure is in this package:
tests/pet-frames.test.mjs: ENOENT on.upstream/dist/tui.js, which is gitignored.tests/e2e-page.test.mjs: "real database missing: …/data/pet.db". It needs the author's own DB.tests/pet-server-care.test.mjs:SQLITE_CANTOPENon the sameREAL_DB.
Tests should build their own fixtures in a temp dir. Two more things to fix while you're there: the tests use fixed ports (3641–3643, 3597–3599), which break when suites run in parallel, so listen on port 0 instead; and selftest.mjs finds ROOT with a Windows-only regex (/\\tests$/).
6. It doesn't fit the page-plugin contract yet
- The package name is
magpie-digital-pet. Page plugins are named@magpie-community/page-<name>, so this one would bepage-digital-pet. repository.urlis"local".- There is no
magpie.page.service.page/boot.jsjumps straight tohttp://127.0.0.1:<port>/, so the page breaks undermagpie webopened from another machine. page-chat shows how to use the declared service. - The package isn't added to the README list or the registry.
- NOTICE refers to a
pet.middleware.jsthat doesn't exist. - The README's magpie link is a placeholder (
https://github.com/), and the README says the server "只服务 magpie 窗口跳来的页面" (serves only pages opened from the magpie window), which point 2 shows isn't true.
Other bugs I found
- Toasts are empty. Upstream calls
showToast({ body: payload }), but the fake client readspayload.title, so every toast comes through as{}. I saw[{}]in a live run. enabled: falsedoesn't stop anything.startAllinindex.mjsreturns before it stops the old watcher and server. After reloading withenabled: false, the server still answered 200.- Two engines share one DB. The server makes its own adapter beside the watcher's, and the care store sets no
busy_timeout, so concurrent writes can fail withSQLITE_BUSY. - Renaming counts as care. Rename is logged as kind
care, so it counts toward the daily care goal and the care-10 achievement. getCareSnapshotwrites to the DB on every GET.- The settings panel shows a Windows path on every OS.
ui/panel.htmlhard-codes%APPDATA%for the DB location. - Chinese only. The page UI and the README are only in Chinese. Other page plugins follow magpie's language.
- Dead code.
debugUnknownis never called.
Size
npm pack gives 355 KB packed and 2.88 MB unpacked (21 files). That makes it the largest package in the repo; the next are agent-office at 1.7 MB and chat at 892 KB. Most of it is the sprite JSON, which has to go anyway (point 1), plus the vendored upstream bundle lib/upstream/index.js (510 KB). I checked that the bundle is the unmodified 0.18.0 dist/index.js (same sha256). Depending on @jcendal/opencode-digital-pet at a pinned version would be smaller and easier to update than vendoring it.
What I checked and found fine
- No child processes, no
eval, no telemetry and no postinstall script. - The only network call is the watcher polling
GET {gatewayUrl}/v1/magpie/usage/requestson the local gateway, and it dedupes on route_id. - The care tables (
magpie_pet_*,user_version2) don't collide with the engine'sschema_migrations. page/app.jsbuilds the DOM withoutinnerHTML.bun scripts/check.mjspasses, both for this package and for the whole repo.
What I ran
On mote (macOS), at head 40068d6 in a separate worktree:
HOME=$(mktemp -d) bun test: exit 1, with the 3 failures and 3 errors listed above.bun scripts/check.mjs: exit 0.npm pack --dry-runfor the size figures.- I re-extracted the frames from the npm tarball of 0.18.0 and compared sha256 sums.
- A live run against a fake gateway serving one usage row, in a sandbox HOME. The first start failed as in point 4. The second start spawned a pet and fed it one request (15M tokens, $0.50). The curl probes in point 2 were run against that instance, and so were the reload with
enabled: falseand the overwrite of an existingopencode-digital-pet.json.
If you'd like to keep going, I'd start with point 1, since it decides what the package can contain, and then point 2. Happy to look again once those are in.
yetone
left a comment
There was a problem hiding this comment.
Thanks for the update, @manjughosa. I looked again at head 34968a0. The new commit adds five habitat backgrounds (ocean, sunset, snow, sakura, void) and removes the external dex link. That part is fine. But it doesn't touch any file behind the six blockers from the last review, so all six are still open. I can't merge it yet.
The six blockers, at 34968a0
-
Licensing. Still open.
lib/monster-frames.jsonis unchanged (same blob, 2,213,354 bytes), so the CC BY-NC 4.0 / Digimon-derived sprites are still bundled.package.jsonstill saysApache-2.0and lists thedigimonandtamagotchikeywords.NOTICEstill says "this port ships no sprite assets". This decides what the package can contain, so please start here: use original art or art under a licence that allows redistribution, or bundle no art at all. -
Server open to any page. Still open.
lib/pet.server.mjsis unchanged:access-control-allow-origin: *, with no token, no Host check and no Origin check. Against a fresh instance on 127.0.0.1 in a sandbox HOME:GET /api/pingwithOrigin: https://evil.examplereturned 200 with ACAO*and the absolute path ofpet.db.OPTIONS /api/commandreturned 204 with ACAO*.POST /api/command {"name":"digital-pet-spawn"}withOrigin: https://evil.exampleandHost: attacker.examplereturned 200.POST /api/command {"name":"pet-rename","args":"pwned"}withOrigin: nullreturned 200, and/api/statethen read"petName":"pwned".
packages/page-chat/server.mjsshows how to do it: a per-process token, a Host allow-list and an Origin check. -
Overwrites the upstream plugin's config. Still open. I seeded
~/.config/opencode-digital-pet.jsonwith{"stageThresholds":{"egg":1000}}. After one start it read"egg": 10000000.ensureGrowthSettings()inlib/pet.adapter.mjsis unchanged. -
Fresh install. Still open. In a new HOME, the first start logs
start failed: unable to open database fileand nothing listens. The second start works. -
Tests need local files. Still open.
HOME=$(mktemp -d) bun testat the repo root gave 3808 pass, 8 skip, 3 fail, 3 errors, exit 1, the same three as before:pet-frames.test.mjshits ENOENT on.upstream/dist/tui.js.e2e-page.test.mjsfails with "real database missing: …/data/pet.db".pet-server-care.test.mjsfails withSQLITE_CANTOPENonREAL_DB.
The fixed ports and the Windows-only
ROOTregex inselftest.mjsare also unchanged. -
Page-plugin contract. Still open:
- The name is still
magpie-digital-pet, not@magpie-community/page-digital-pet. repository.urlis still"local".- There is still no
magpie.page.service, andpage/boot.jsstill jumps to a hard-codedhttp://127.0.0.1:<port>/. - Nothing outside the package is changed, so it's still not in the README list or the registry.
- NOTICE still names
pet.middleware.js.
- The name is still
Smaller bugs from the last review, also still there
- Toasts are still empty: the fake client reads
payload.title. enabled: falsestill returns before stopping the old watcher and server (index.mjs).- Rename is still counted as care.
ui/panel.htmlstill shows%APPDATA%on every OS.- The UI and README are still Chinese only.
debugUnknownis still never called.
What I ran
On a clean macOS box (mote, Bun 1.4.2), in a separate worktree at 34968a0:
HOME=$(mktemp -d) bun test: exit 1, results as above.bun scripts/check.mjs: exit 0.- A fresh install in a sandbox HOME, started through the plugin's
server()entry on a spare port with a dead gateway URL. This covered the first-start failure, the config overwrite and the curl probes above.
Please push again once points 1 and 2 are addressed, and I'll review it again.
yetone
left a comment
There was a problem hiding this comment.
Thanks, @manjughosa. This round is a big step. Most of what the last two reviews asked for is done, and I checked each fix myself at head bb8b91e. Three things still block the merge. The first is a security hole.
Still blocking
1. Any web page can still read the token and drive the API
lib/pet.server.mjs:461 serves app.html, which has window.PET_TOKEN in it, through send() with the default cors = true. For Origin: null, corsHeaders() answers access-control-allow-origin: null. A sandboxed iframe on any site sends Origin: null, so it can read the page, take the token and call every /api/* route.
With curl against a fresh instance:
curl -s -D - -H 'Origin: null' http://127.0.0.1:3671/
HTTP/1.1 200 OK
access-control-allow-origin: null
...<script>window.PET_TOKEN="61696d44...";</script>
With that token and Origin: null, POST /api/command {"name":"pet-rename","args":"pwned"} returned 200, and /api/state then read "petName":"pwned".
In a real browser (Playwright Chromium on macOS) I opened a page served from http://localhost:<other port>/. Inside it, an <iframe sandbox="allow-scripts"> ran fetch("http://127.0.0.1:3671/"), took the token out of the HTML, and posted pet-rename. The result: status 200 petName=stolen.
page-chat gets this right. Its HTML carries the token and goes out with no CORS headers (packages/chat/lib/server.mjs:739-740, send(res, 200, html, MIME[".html"], false)), so no other origin can read it. The fix here is the same: send app.html with cors = false. Please also add a test that GET / with Origin: null has no access-control-allow-origin, and that it fails on the current code.
2. The repo's test run is still red
CI runs HOME=$(mktemp -d) bun test at the repo root without installing anything. No other package has a dependencies entry, so nothing installs @jcendal/opencode-digital-pet. On mote (macOS, Bun 1.4.2) at bb8b91e the run gave 3808 pass, 8 skip, 3 fail, exit 1. All three are error: Cannot find module '@jcendal/opencode-digital-pet' from …/lib/pet.adapter.mjs, as unhandled errors in docs-contract.test.mjs, pet-progression-rules.test.mjs and pet-server-care.test.mjs.
After bun install --ignore-scripts, all eight test files pass, with temp DBs and port 0. So the tests are fine once the dependency is there. This package is the first in the repo with a runtime dependency, so either CI has to install it (a workflow change, which is ours to make, not yours), or the tests that need the engine have to run without it. Let's agree on which before merging. I'll raise the CI side with the maintainer.
Related: the test files use their own test() plus process.exit(1) instead of bun:test, so bun test reports "0 pass, 0 fail" for them. A failure exits the whole runner from inside one file, and the result is never counted as a test. Please switch them to import { test, expect } from "bun:test".
3. README.md rows are broken
README.md:120: the chat row is changed. Its author link text went fromMagiczerowxytoMagic_zero. That's not part of this PR, so please revert it.README.md:121: the new digital-pet row has five cells. The chat row's description and permission cells are pasted after the pet's own two. It should have three cells, like the other rows.
Smaller things (not blocking on their own)
- Chinese only.
page/boot.jspasseslang, butpage/app.jsnever reads it, andapp.htmlandindex.htmlarelang="zh"with Chinese strings. The settings panel and README are Chinese only too. Other page plugins follow magpie's language, so at least English and Chinese would be good. - Install size. The dependency is 6 MB unpacked, and it has peer dependencies (
@opentui/core,@opentui/solid,@opentui/keymap,@opencode-ai/plugin) that Bun installs too.bun installpulled 133 packages for this one page. Please say so in the README. If the engine can be imported without the TUI peers, that would be better. lib/pet.adapter.mjs:60.VACUUM INTO ${JSON.stringify(target)}quotes the path with double quotes. In SQLite those are identifier quotes, and a path that contains"breaks the statement. Use a?bound parameter (VACUUM INTO ?) instead.lib/pet.server.mjs:30.STAGE_THRESHOLDSmirrors the upstream defaults "for display only". If the engine picks up a user's~/.config/opencode-digital-pet.jsonthresholds, the progress bar will be wrong.- Browser test. The browser part of
tests/e2e-page.test.mjsruns only with Edge on Windows (PET_E2E_EDGE). On any other box the page is never loaded in a browser.
Fixed since the last review (checked at bb8b91e)
- Licensing:
lib/monster-frames.jsonandlib/upstream/are gone. The engine is now a pinned npm dependency, and the page draws a procedural blob, so no sprite art is in the repo or the tarball (316 KB on disk). The dex still lists the engine's 650 names at run time, from the dependency. Whether that's acceptable is the maintainer's call; it isn't bundled here. - Server guard: a Host allow-list (
Host: attacker.examplegets 403), an Origin check (https://evil.examplegets 403), and a per-process token on/api/*(no token gets 403). Apart from point 1, this is right. - Upstream config: I seeded
~/.config/opencode-digital-pet.jsonwith{"stageThresholds":{"egg":1000}}, and it was unchanged after a start. - Fresh install: the first start in a new HOME works.
~/.config/magpie-digital-pet/pet.dbis created, and the page is served. - Tests: they now build their own fixtures, use port 0, and the
selftest.mjsROOT works on every OS. - Contract: the package is
@magpie-community/page-digital-pet, with the realrepository.url,magpie.page.service(port option), a registry entry, and NOTICE fixed. - Toasts carry title and message.
enabled: falseon reload stops the server (ping refused afterwards).- Rename is logged as
system, notcare. - Panel gives per-OS paths, and
debugUnknownis removed. - Harm check: no child processes,
evalor telemetry. The only network call is still the watcher'sGET {gatewayUrl}/v1/magpie/usage/requests.bun scripts/check.mjspasses.
What I ran (mote, macOS, Bun 1.4.2, fresh clone at bb8b91e)
HOME=$(mktemp -d) bun testat the root: exit 1, as in point 2.bun scripts/check.mjs: exit 0.bun install --ignore-scripts, thenbun testin the package: all 8 files print "all tests passed".- A live run of the plugin entry (
server()) in a sandbox HOME, with a dead gateway URL and port 3671. It covered the first start, the config seed, the curl probes above, and a reload withenabled: falsein one process. - Playwright Chromium: the sandboxed-iframe probe in point 1. WebKit didn't launch on the box (timeout), so it isn't tried. I expect the same result, since WebKit honours
ACAO: nulltoo.
Once point 1 is fixed with a test, and the README rows are fixed, I'll look again.
Summary
Validation
All pass locally; selftest reports 26/26 checks.