Skip to content

Add magpie digital pet plugin - #539

Open
manjughosa wants to merge 4 commits into
magpie-community:mainfrom
manjughosa:add-magpie-digital-pet
Open

manjughosa wants to merge 4 commits into
magpie-community:mainfrom
manjughosa:add-magpie-digital-pet

Conversation

@manjughosa

Copy link
Copy Markdown
Contributor

Summary

  • Adds packages/magpie-digital-pet, a Magpie port of @jcendal/opencode-digital-pet 0.18.0.
  • Feeds the active Digimon partner from Magpie usage receipts, with local SQLite persistence outside the plugin directory.
  • Adds a local sidebar pet page with pixel habitat, care actions, expeditions, daily goals, achievements, decorations, naming, dex selection, and pinned display management.
  • Includes the upstream 650-Digimon catalog/evolution data and extracted monster frames.

Validation

  • bun tests/pet-frames.test.mjs
  • bun tests/pet-care-rules.test.mjs
  • bun tests/pet-progression-rules.test.mjs
  • bun tests/pet-care-store.test.mjs
  • bun tests/pet-server-care.test.mjs
  • bun tests/page-contract.test.mjs
  • bun tests/docs-contract.test.mjs
  • bun tests/e2e-page.test.mjs
  • bun tests/selftest.mjs

All pass locally; selftest reports 26/26 checks.

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

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.json and 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/ping with Origin: https://evil.example returned 200 with ACAO * and the absolute path of pet.db. The OPTIONS preflight returned 204 with *.
  • POST /api/command (pet-rename) with Origin: https://evil.example and Host: attacker.example returned 200 and renamed the pet to pwned. Because the Host header is accepted, a DNS-rebinding page gets in too.
  • digital-pet-set with Origin: null (a sandboxed frame) returned 200 and pinned the pet.
  • /api/state exposes token and cost totals. spawn retires 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_CANTOPEN on the same REAL_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 be page-digital-pet.
  • repository.url is "local".
  • There is no magpie.page.service. page/boot.js jumps straight to http://127.0.0.1:<port>/, so the page breaks under magpie web opened 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.js that 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 reads payload.title, so every toast comes through as {}. I saw [{}] in a live run.
  • enabled: false doesn't stop anything. startAll in index.mjs returns before it stops the old watcher and server. After reloading with enabled: 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 with SQLITE_BUSY.
  • Renaming counts as care. Rename is logged as kind care, so it counts toward the daily care goal and the care-10 achievement.
  • getCareSnapshot writes to the DB on every GET.
  • The settings panel shows a Windows path on every OS. ui/panel.html hard-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. debugUnknown is 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/requests on the local gateway, and it dedupes on route_id.
  • The care tables (magpie_pet_*, user_version 2) don't collide with the engine's schema_migrations.
  • page/app.js builds the DOM without innerHTML.
  • bun scripts/check.mjs passes, 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-run for 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: false and the overwrite of an existing opencode-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 yetone 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.

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

  1. Licensing. Still open. lib/monster-frames.json is unchanged (same blob, 2,213,354 bytes), so the CC BY-NC 4.0 / Digimon-derived sprites are still bundled. package.json still says Apache-2.0 and lists the digimon and tamagotchi keywords. NOTICE still 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.

  2. Server open to any page. Still open. lib/pet.server.mjs is 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/ping with Origin: https://evil.example returned 200 with ACAO * and the absolute path of pet.db.
    • OPTIONS /api/command returned 204 with ACAO *.
    • POST /api/command {"name":"digital-pet-spawn"} with Origin: https://evil.example and Host: attacker.example returned 200.
    • POST /api/command {"name":"pet-rename","args":"pwned"} with Origin: null returned 200, and /api/state then read "petName":"pwned".

    packages/page-chat/server.mjs shows how to do it: a per-process token, a Host allow-list and an Origin check.

  3. Overwrites the upstream plugin's config. Still open. I seeded ~/.config/opencode-digital-pet.json with {"stageThresholds":{"egg":1000}}. After one start it read "egg": 10000000. ensureGrowthSettings() in lib/pet.adapter.mjs is unchanged.

  4. Fresh install. Still open. In a new HOME, the first start logs start failed: unable to open database file and nothing listens. The second start works.

  5. Tests need local files. Still open. HOME=$(mktemp -d) bun test at the repo root gave 3808 pass, 8 skip, 3 fail, 3 errors, exit 1, the same three as before:

    • pet-frames.test.mjs hits ENOENT on .upstream/dist/tui.js.
    • e2e-page.test.mjs fails with "real database missing: …/data/pet.db".
    • pet-server-care.test.mjs fails with SQLITE_CANTOPEN on REAL_DB.

    The fixed ports and the Windows-only ROOT regex in selftest.mjs are also unchanged.

  6. Page-plugin contract. Still open:

    • The name is still magpie-digital-pet, not @magpie-community/page-digital-pet.
    • repository.url is still "local".
    • There is still no magpie.page.service, and page/boot.js still jumps to a hard-coded http://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.

Smaller bugs from the last review, also still there

  • Toasts are still empty: the fake client reads payload.title.
  • enabled: false still returns before stopping the old watcher and server (index.mjs).
  • Rename is still counted as care.
  • ui/panel.html still shows %APPDATA% on every OS.
  • The UI and README are still Chinese only.
  • debugUnknown is 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 yetone 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.

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 from Magiczerowxy to Magic_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.js passes lang, but page/app.js never reads it, and app.html and index.html are lang="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 install pulled 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_THRESHOLDS mirrors the upstream defaults "for display only". If the engine picks up a user's ~/.config/opencode-digital-pet.json thresholds, the progress bar will be wrong.
  • Browser test. The browser part of tests/e2e-page.test.mjs runs 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.json and lib/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.example gets 403), an Origin check (https://evil.example gets 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.json with {"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.db is created, and the page is served.
  • Tests: they now build their own fixtures, use port 0, and the selftest.mjs ROOT works on every OS.
  • Contract: the package is @magpie-community/page-digital-pet, with the real repository.url, magpie.page.service (port option), a registry entry, and NOTICE fixed.
  • Toasts carry title and message.
  • enabled: false on reload stops the server (ping refused afterwards).
  • Rename is logged as system, not care.
  • Panel gives per-OS paths, and debugUnknown is removed.
  • Harm check: no child processes, eval or telemetry. The only network call is still the watcher's GET {gatewayUrl}/v1/magpie/usage/requests. bun scripts/check.mjs passes.

What I ran (mote, macOS, Bun 1.4.2, fresh clone at bb8b91e)

  • HOME=$(mktemp -d) bun test at the root: exit 1, as in point 2.
  • bun scripts/check.mjs: exit 0.
  • bun install --ignore-scripts, then bun test in 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 with enabled: false in 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: null too.

Once point 1 is fixed with a test, and the README rows are fixed, I'll look again.

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