From edb1385d18de97f8d954dc3af6c3e48924603c5a Mon Sep 17 00:00:00 2001 From: Died Broke Date: Tue, 11 Aug 2026 18:13:43 +0000 Subject: [PATCH 01/15] identity: let a joining slot say who it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A live_pvp battle is created before its players arrive: the creator POSTs p1_name/p2_name, both trainer FKs bind then, and the slots fill later over the join WebSocket. So the names on a battle were always the creator's guesses about who would show up -- "Opponent", "AI", "Agent 2" -- and there was no moment in the protocol at which a slot's actual occupant could say otherwise. pokearena-agent had no name flag, MCP join_battle had no name argument, and the SPA's share-link join hardcoded 'Trainer'. Elo has been real since the beginning and has been accruing to placeholders. This adds the missing moment. A joiner may declare a name on the play URL (?name=), the gateway sanitizes it and rebinds that slot's trainer on the battle row, and the name rides the attach message so the session labels the slot in room frames and in the engine state it builds. Omitting it keeps the creator's name, so every existing flow is unchanged. The same missing field was blocking the benchmark from the other side: PR 109's handoff records that Postgres carries no model identity, so per-model attribution had to be reconstructed from bid= mappings in /tmp -- and a full attributed batch was re-run after those were wiped. One cause, two symptoms. Scope, stated plainly: this makes identity expressible and attributable, not verified. Any holder of a slot token may claim any name, including one already on the board; claim-a-handle + secret is separate work, and shipping half of it would produce a board that looks verified. README and live-pvp.md §3 say so in those words, with impersonation listed under "not designing against" rather than left unmentioned. Two constraints that would be real bugs rather than known limitations: a name is only accepted after the slot claim succeeds, so guessing a battle id cannot rewrite its trainers; and RebindBattleTrainer refuses a battle that already has a winner, so a late or replayed join cannot move a rating that was computed against the previous trainer. Accepting a name at attach turned Match.trainerName into a field written by the action-pump goroutine and read by the coordinator -- benign in behavior, a data race in the memory model. It gets its own mutex-guarded type following the slotConns precedent. set("") is a no-op, or an anonymous re-attach after a blip would blank a name the slot declared on its first one. pokearena-agent --name defaults to the model id: an unnamed agent inherits the placeholder, which is the behavior that caused all of this, so staying anonymous is the wrong default for a bot. The SPA remembers its name in localStorage so a returning player keeps their row. Also corrects two stale rows in the README status table -- the leaderboard has been visible in the SPA since the field-state work. The rebind SQL is verified against a real Postgres (integration tag): the column mapping and the settled-battle guard are both invisible to a unit test of the surrounding Go. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UZ6bbo8VkaDrs47Yz8v84U --- README.md | 5 +- backlog/2026-08-11T18-30-identity-on-join.md | 106 ++++++++++++++ cmd/pokearena-agent/main.go | 16 ++- docs/live-pvp.md | 26 +++- docs/mcp-protocol.md | 17 ++- internal/agentloop/loop.go | 9 +- internal/gwclient/gwclient.go | 13 +- internal/gwclient/gwclient_test.go | 88 +++++++++++- internal/httpapi/server.go | 6 +- internal/httpapi/ws.go | 58 +++++++- internal/livebattle/bridge.go | 4 + internal/livebattle/coordinator.go | 8 +- internal/livebattle/livebattle.go | 9 +- internal/livebattle/trainers.go | 39 ++++++ internal/livebattle/trainers_test.go | 124 +++++++++++++++++ internal/mcpserver/session.go | 6 +- internal/mcpserver/session_test.go | 20 +-- internal/mcpserver/tools.go | 8 +- internal/messages/messages.go | 12 +- internal/protocol/pvp.go | 64 ++++++++- internal/protocol/pvp_test.go | 131 ++++++++++++++++++ internal/session/trainerrebind_test.go | 137 +++++++++++++++++++ internal/store/repo.go | 23 ++++ web/app.js | 49 ++++++- 24 files changed, 923 insertions(+), 55 deletions(-) create mode 100644 backlog/2026-08-11T18-30-identity-on-join.md create mode 100644 internal/livebattle/trainers.go create mode 100644 internal/livebattle/trainers_test.go create mode 100644 internal/protocol/pvp_test.go create mode 100644 internal/session/trainerrebind_test.go diff --git a/README.md b/README.md index 7f518ded..31421093 100644 --- a/README.md +++ b/README.md @@ -186,6 +186,7 @@ export ANTHROPIC_API_KEY=sk-ant-… | `--model` | `claude-haiku-4-5-20251001` | Anthropic model id. Use opus for stronger play at higher cost. | | `--turn-timeout` | `12s` | Per-turn LLM budget. The gateway default-actions the slot if exceeded. | | `--data-version` | `gen1-v1` | Must match the gateway's `DATA_VERSION` env. | +| `--name` | the `--model` id | Trainer name this slot's results post under on the leaderboard. | --- @@ -196,8 +197,8 @@ between "an arena where bots compete on a real leaderboard" and what runs today. | Area | Today | To make the headline true | |---|---|---| -| **Leaderboard identity** | Free-text name, no ownership; clients barely prompt | Prompt for a trainer/agent name everywhere a battle starts; surface the board in the SPA. (Optional later: claim-a-handle + secret to stop impersonation.) | -| **Leaderboard visibility** | Rating computed + stored, but not shown in the UI | A real standings page — wins/losses/Elo, sortable | +| **Leaderboard identity** | A joining slot declares its own name (`--name`, MCP `trainer_name`, `?name=` on the WS), and the battle's trainer is rebound to it — so a result posts under whoever actually played, not the placeholder the battle's creator typed. Names are **self-reported and unverified** | Claim-a-handle + secret, so a name on the board can't be taken by anyone who wants it | +| **Leaderboard visibility** | Standings page in the SPA (name + Elo), fed by `/api/leaderboard` | Sortable, with wins/losses alongside the rating — the store already returns them | | **Bot onboarding** | Two reference clients, MCP + CLI | A 5-minute "write your own bot" quickstart against a documented protocol | | **Provider coverage** | Benchmark (`cmd/bench`) runs Anthropic, OpenAI, Gemini, and local Ollama behind one `Client` interface, in `raw`/`cot` conditions; the live harness (`pokearena-agent`) is still Anthropic-only | Bring the remaining vendors to the live harness too | diff --git a/backlog/2026-08-11T18-30-identity-on-join.md b/backlog/2026-08-11T18-30-identity-on-join.md new file mode 100644 index 00000000..626d2f25 --- /dev/null +++ b/backlog/2026-08-11T18-30-identity-on-join.md @@ -0,0 +1,106 @@ +# Identity belongs to the joiner, not the creator + +The leaderboard has had working Elo since the beginning and has never +meant anything. Today's change is small — a name on the join — but it is +the one that makes a result attributable to whoever produced it. + +## The bug was in *when*, not *whether*, we asked for a name + +The README has said "free-text name, no ownership; clients barely prompt" +for months, and I read that as a UI gap: add a name field to more screens +and the board improves. That is not what was wrong. + +A `live_pvp` battle is created before its players arrive. The creator +POSTs `p1_name` / `p2_name`, both trainer FKs are bound *then*, and the +slots are filled later over the join WebSocket. So the names on a battle +are always the creator's guesses about who will show up — `"Opponent"`, +`"AI"`, `"Agent 2"`. There was no moment in the protocol at which the +actual occupant of a slot could say who it was. `pokearena-agent` had no +name flag, MCP `join_battle` had no name argument, and the SPA's +share-link join hardcoded `'Trainer'`. Not one of them was under-prompting; +there was nothing to prompt *into*. + +That reframes the fix. It isn't "collect the name in more places," it's +"make join a point at which identity can be declared," which needs a wire +field (`?name=` on the play URL), a message field (`LiveAction.Trainer`), +and a write (`RebindBattleTrainer`). Once those exist the three clients +are one line each. + +## The second track was blocked on the same field + +I went looking at PR #109 (decision-quality eval) in the same session and +its handoff doc says, in the gotchas: + +> **Postgres has NO model identity** — `p1_name` is always "Agent", +> `p2_name` "AI". Model attribution ONLY comes from the `bid=`→model +> mapping in the run dirs. The previous mapping (`/tmp/pk-agentic`) was +> wiped, which is why we re-ran. + +That is the same missing field, discovered independently, and it cost a +re-run of a full attributed batch. The benchmark had been reconstructing +in `/tmp` a fact the database should have carried. Worth naming because +the two symptoms look unrelated — "the leaderboard is meaningless" and +"we lost the model attribution for a batch" — and have one cause. + +## What I deliberately did not build + +The name is **self-reported**. Any holder of a slot token can claim any +name, including one already on the board. I considered gating it — a +secret per handle, first-writer-wins on a name — and stopped, because +that is a different feature (accounts) wearing this one's clothes, and +shipping half of it would produce a board that *looks* verified. + +So the honest split is: this change makes identity **expressible and +attributable**; it does not make it **verified**. Both the README status +row and `live-pvp.md` §3 now say that in those words, and §3 lists +impersonation under "not designing against" rather than leaving it +unmentioned. The README's "for fun, unverified" disclaimer stays exactly +as it was — it is still true, just for a narrower reason. + +Two things I did constrain, because they are cheap and the absence would +be a real bug rather than a known limitation: + +- **A name is only accepted after the slot claim succeeds.** Rebinding + before the claim would let anyone who can guess a battle id rewrite its + trainers without ever playing. +- **`RebindBattleTrainer` refuses a battle with a winner.** Elo is applied + from `p1_trainer` / `p2_trainer` at completion; a late or replayed join + that renamed a settled battle would move a rating that was already + computed against the old trainer. The `winner < 0` guard is the whole + defense and it is why that method has an integration test. + +## A concurrency seam I didn't expect + +`trainerName` was a plain `[2]string` on `Match`, written once at +construction and read by the coordinator goroutine. Accepting a name at +attach makes it a field written by the *action-pump* goroutine at an +arbitrary moment — a data race, even though nothing about a display name +can affect a turn's outcome. + +I gave it its own tiny mutex-guarded type (`trainerNames`) rather than +routing the name through the coordinator's channel, following the +`slotConns` precedent already in the package: a self-contained concurrency +unit, documented as touched from one goroutine and read from another. The +comment on it says out loud that a concurrent name arrival is benign in +behavior and a race only in the memory-model sense, because that's the +thing a future reader will otherwise have to re-derive before touching it. + +`set("")` is a no-op rather than a store. Without that, an anonymous +re-attach — which is exactly what a reconnect after a blip is — would +blank a name the slot declared on its first attach. + +## Where the default came from + +`pokearena-agent --name` defaults to the model id rather than to empty. +An unnamed agent inherits the creator's placeholder, which is the +behavior that caused all of this, so "stay anonymous" is the wrong +default for a bot: the useful thing a harness can do with no +configuration is say which model played. The SPA remembers its name in +`localStorage` so a returning player keeps their row instead of minting +"Challenger" every session. + +The one path still weaker than I'd like: a first-time share-link joiner +in the browser gets `"Challenger"`, because they never passed through the +setup form and I didn't want to put a modal in front of a battle +invitation. A name field on the picker screen is the obvious follow-up +and is UI work, not protocol work — the protocol side is done. diff --git a/cmd/pokearena-agent/main.go b/cmd/pokearena-agent/main.go index 5cef5359..c7ed82ae 100644 --- a/cmd/pokearena-agent/main.go +++ b/cmd/pokearena-agent/main.go @@ -29,6 +29,7 @@ import ( "pokearena/internal/agentloop" "pokearena/internal/domain" "pokearena/internal/llm" + "pokearena/internal/protocol" ) func main() { @@ -43,6 +44,8 @@ func main() { "Per-turn LLM call budget; the gateway will default-action the slot if a turn takes longer") dataVersion := flag.String("data-version", "gen1-v1", "Dataset version label — must match the gateway's DATA_VERSION") + name := flag.String("name", "", + "Trainer name to record this slot under on the leaderboard (default: the model id)") flag.Usage = usage flag.Parse() @@ -72,13 +75,24 @@ func main() { ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() - log.Printf("model=%s, gateway=%s, battle=%s, slot=%s", *model, gatewayURL, battleID, slot) + // Default the leaderboard name to the model id. An unnamed agent inherits + // the placeholder the battle's creator picked ("Opponent"), which is how + // every agent's games used to end up indistinguishable on the board — so + // the useful default is "say which model played", not "stay anonymous". + trainer := protocol.SanitizeTrainerName(*name) + if trainer == "" { + trainer = protocol.SanitizeTrainerName(*model) + } + + log.Printf("model=%s, gateway=%s, battle=%s, slot=%s, trainer=%q", + *model, gatewayURL, battleID, slot, trainer) cfg := agentloop.Config{ GatewayURL: gatewayURL, BattleID: battleID, Slot: slot, Token: token, + TrainerName: trainer, Dex: dex, LLM: llm.NewAnthropic(key, *model), PerTurnTimeout: *turnTimeout, diff --git a/docs/live-pvp.md b/docs/live-pvp.md index 51a6275f..e33becee 100644 --- a/docs/live-pvp.md +++ b/docs/live-pvp.md @@ -63,7 +63,7 @@ adapter on top of the contract described here. ## 2. URL shape ``` -/api/battles/{battle_id}/play?slot={p1|p2}&token={join_token} +/api/battles/{battle_id}/play?slot={p1|p2}&token={join_token}[&name={trainer}] ``` One route, one validator. The same path serves the single-player @@ -72,6 +72,18 @@ distinguishes them. Path-style slots (`/play/p1`) were tempting but keeping the slot as a query param lets a future third client type slot in without growing the route table. +`name` is optional and is the joiner's **self-declared trainer name** +— the leaderboard key this slot's result posts under. It is a query +param on the join rather than a field in the create body because the +battle is created before its players arrive: the creator names both +slots, and an agent joining `p2` would otherwise be recorded forever as +whatever placeholder they typed. Declaring it rebinds the battle row's +trainer for that slot (`store.RebindBattleTrainer`) and relabels the +slot in room frames and engine state. Omit it to keep the creator's +name. Unlike the other three params it can contain spaces and `&`, so +`PlayPath` query-escapes it; the gateway re-sanitizes on arrival +(trim, drop control characters, cap at 24 runes). + The URL builder lives in [`internal/protocol/pvp.go`](../internal/protocol/pvp.go) as `PlayPath`. Both the gateway (when issuing URLs to the creator) and any client constructing its own connect URL must use it — so @@ -110,12 +122,24 @@ We're **not** designing against: opponent. The slot goes to whoever connects first; this is acceptable given the threat above is the gating concern. +- **Impersonation on the leaderboard.** The `name` on a join is + self-reported: holding the slot token is the whole permission, and any + name may be claimed, including one already on the board. This is a + deliberate v1 position — attribution first, verification later — and + the reason the README calls the board "for fun, unverified." The fix + is claim-a-handle + secret, not a patch to this protocol. Note the + name is only accepted *after* the slot claim succeeds, so it is at + least no easier to forge than playing the battle would be. + We **are** designing against: - Probing the failure modes (token vs battle vs slot validity). The opaque-error rule blocks this. - A claimed slot being hijacked by a second client with the same token. The atomic claim (§4) blocks this. +- Renaming a settled result. `RebindBattleTrainer` refuses a battle + that already has a winner, so a late or replayed join cannot move a + rating that was computed against the previous trainer. ### Explicitly out of scope for v1 diff --git a/docs/mcp-protocol.md b/docs/mcp-protocol.md index 94cffaf0..b15de88d 100644 --- a/docs/mcp-protocol.md +++ b/docs/mcp-protocol.md @@ -55,12 +55,27 @@ All five tools return structured JSON. The SDK turns Go errors into MCP error responses with `isError: true`; the agent should switch on the message content to distinguish cases. -### `join_battle(battle_id, slot, join_token) → JoinResult` +### `join_battle(battle_id, slot, join_token, trainer_name?) → JoinResult` Binds the MCP session to a battle slot. Opens the underlying WebSocket to `/api/battles/{battle_id}/play?slot={slot}&token={join_token}` and blocks until the gateway sends the first `state` frame. +**`trainer_name` (optional)** is the name this slot's result posts under +on the leaderboard. It matters because a live_pvp battle is created +*before* its players arrive: whoever pressed "Start" named both slots, so +an agent that joins without declaring itself is recorded under a +placeholder like `"Opponent"` — which is why agent results used to be +indistinguishable on the board. Declaring it appends `&name=` to the join +URL; the gateway sanitizes it (trims, strips control characters, caps at +24 characters) and rebinds the battle's trainer for that slot. + +The name is **self-reported, not authenticated**. Holding the slot token +is the only permission required, and any name may be claimed — including +one already on the leaderboard. Verified handles are separate, later +work; until then, treat a leaderboard name as an assertion by whoever +played, not as an identity the server vouches for. + **Returns** on successful claim: ```json diff --git a/internal/agentloop/loop.go b/internal/agentloop/loop.go index 87bbbb5e..5d09a9e9 100644 --- a/internal/agentloop/loop.go +++ b/internal/agentloop/loop.go @@ -24,6 +24,13 @@ type Config struct { // sees in the SPA share URL. Slot is "p1" or "p2". BattleID, Slot, Token string + // TrainerName is the name recorded for this slot on the leaderboard. + // Empty inherits the placeholder the battle's creator supplied, which + // is how every agent result used to land as "Opponent" — set it to + // something identifying the controller (model, version) if the result + // is meant to be attributable. + TrainerName string + // Dex is needed to render move metadata in the prompt and to log // decisions in human-readable form. Dex *domain.Dex @@ -56,7 +63,7 @@ func Run(ctx context.Context, cfg Config) error { logger = log.Default() } - gc, err := gwclient.Dial(ctx, cfg.GatewayURL, cfg.BattleID, cfg.Slot, cfg.Token) + gc, err := gwclient.Dial(ctx, cfg.GatewayURL, cfg.BattleID, cfg.Slot, cfg.Token, cfg.TrainerName) if err != nil { return fmt.Errorf("dial gateway: %w", err) } diff --git a/internal/gwclient/gwclient.go b/internal/gwclient/gwclient.go index c3104da3..4c0cc11b 100644 --- a/internal/gwclient/gwclient.go +++ b/internal/gwclient/gwclient.go @@ -41,8 +41,13 @@ type Client struct { // starts the read pump, and returns the client ready for use. The // handshake itself respects ctx; the read pump runs in its own goroutine // and outlives ctx. -func Dial(ctx context.Context, baseURL, battleID, slot, token string) (*Client, error) { - return dialPath(ctx, baseURL, protocol.PlayPath(battleID, slot, token)) +// +// trainer is the name this client wants recorded for the slot on the +// leaderboard. Pass "" to inherit whatever the battle's creator named it — +// which for an agent joining someone else's battle is a placeholder like +// "Opponent", so a bot that wants its results attributed should set it. +func Dial(ctx context.Context, baseURL, battleID, slot, token, trainer string) (*Client, error) { + return dialPath(ctx, baseURL, protocol.PlayPath(battleID, slot, token, trainer)) } // DialLive opens a WS to a single-player live-mode battle, where the opponent @@ -50,8 +55,8 @@ func Dial(ctx context.Context, baseURL, battleID, slot, token string) (*Client, // slotless — the human is hardcoded to p1 — so this is the join path an MCP or // agent client uses to face the Heuristic/Expectimax opponent, the same one the // SPA's single-player mode plays against. -func DialLive(ctx context.Context, baseURL, battleID string) (*Client, error) { - return dialPath(ctx, baseURL, protocol.LivePlayPath(battleID)) +func DialLive(ctx context.Context, baseURL, battleID, trainer string) (*Client, error) { + return dialPath(ctx, baseURL, protocol.LivePlayPath(battleID, trainer)) } // dialPath is the shared connect: resolve the URL, open the socket, start the diff --git a/internal/gwclient/gwclient_test.go b/internal/gwclient/gwclient_test.go index 1d355029..8a5d91e4 100644 --- a/internal/gwclient/gwclient_test.go +++ b/internal/gwclient/gwclient_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/http/httptest" + "net/url" "strings" "testing" "time" @@ -59,7 +60,7 @@ func TestDialAndReceive(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() - gc, err := Dial(ctx, base, "battle-x", "p1", "tok") + gc, err := Dial(ctx, base, "battle-x", "p1", "tok", "") must(t, "dial", err) defer gc.Close() @@ -84,7 +85,7 @@ func TestSendAction(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() - gc, err := Dial(ctx, base, "b", "p2", "t") + gc, err := Dial(ctx, base, "b", "p2", "t", "") must(t, "dial", err) defer gc.Close() @@ -121,7 +122,7 @@ func TestDialLive_UsesTokenlessPath(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() - gc, err := DialLive(ctx, base, "battle-live-1") + gc, err := DialLive(ctx, base, "battle-live-1", "") must(t, "dial live", err) defer gc.Close() @@ -143,7 +144,7 @@ func TestCloseIsCleanAndIdempotent(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() - gc, err := Dial(ctx, base, "b", "p1", "t") + gc, err := Dial(ctx, base, "b", "p1", "t", "") must(t, "dial", err) must(t, "close 1", gc.Close()) @@ -172,7 +173,7 @@ func TestServerCloseReportsError(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() - gc, err := Dial(ctx, base, "b", "p1", "t") + gc, err := Dial(ctx, base, "b", "p1", "t", "") must(t, "dial", err) defer gc.Close() @@ -223,3 +224,80 @@ func drain(t *testing.T, gc *Client, n int) []protocol.MatchUpdate { } return out } + +// captureJoin runs a gateway that records the query of the join request, so a +// test can assert what a client actually sent rather than what it meant to. +func captureJoin(t *testing.T) (base string, got *url.Values, cleanup func()) { + t.Helper() + var seen url.Values + up := websocket.Upgrader{CheckOrigin: func(*http.Request) bool { return true }} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + seen = r.URL.Query() + c, err := up.Upgrade(w, r, nil) + if err != nil { + return + } + defer c.Close() + blockUntilPeerClose(c) + })) + return "ws" + strings.TrimPrefix(srv.URL, "http"), &seen, srv.Close +} + +// The trainer name is how a bot's results get attributed on the leaderboard; +// if it never leaves the client, every agent's games post under the placeholder +// the battle's creator chose. +func TestDial_SendsTrainerName(t *testing.T) { + base, seen, cleanup := captureJoin(t) + defer cleanup() + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + gc, err := Dial(ctx, base, "battle-x", "p2", "tok", "claude-haiku") + must(t, "dial", err) + defer gc.Close() + + if got := seen.Get("name"); got != "claude-haiku" { + t.Errorf("name query = %q, want %q", got, "claude-haiku") + } + if got := seen.Get("slot"); got != "p2" { + t.Errorf("slot query = %q, want p2", got) + } +} + +func TestDial_OmitsTrainerNameWhenUndeclared(t *testing.T) { + base, seen, cleanup := captureJoin(t) + defer cleanup() + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + gc, err := Dial(ctx, base, "battle-x", "p2", "tok", "") + must(t, "dial", err) + defer gc.Close() + + // Absent, not empty: the gateway reads a missing name as "keep the + // creator's", and an empty one would sanitize to the same thing — but + // sending it at all would misrepresent an anonymous join as a declaration. + if seen.Has("name") { + t.Errorf("name query present (%q), want it omitted entirely", seen.Get("name")) + } +} + +// Live mode is routed by the *absence* of a slot param, so adding a name must +// not turn a vs-AI join into a pvp one. +func TestDialLive_SendsTrainerNameWithoutASlot(t *testing.T) { + base, seen, cleanup := captureJoin(t) + defer cleanup() + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + gc, err := DialLive(ctx, base, "battle-x", "claude-opus") + must(t, "dial", err) + defer gc.Close() + + if got := seen.Get("name"); got != "claude-opus" { + t.Errorf("name query = %q, want %q", got, "claude-opus") + } + if seen.Has("slot") { + t.Errorf("slot query present (%q) — this would route to the pvp handler", seen.Get("slot")) + } +} diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 1ed925b8..33597665 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -338,8 +338,10 @@ func (s *Server) handleCreateBattle(w http.ResponseWriter, r *http.Request) { } writeJSON(w, http.StatusCreated, map[string]any{ "battle_id": battleID, "mode": "live_pvp", - "p1_url": protocol.PlayPath(battleID, string(cache.SlotP1), p1Token), - "p2_url": protocol.PlayPath(battleID, string(cache.SlotP2), p2Token), + // No name in the issued URLs: the slot's name is the joiner's to + // declare at connect time, not the creator's to assign in advance. + "p1_url": protocol.PlayPath(battleID, string(cache.SlotP1), p1Token, ""), + "p2_url": protocol.PlayPath(battleID, string(cache.SlotP2), p2Token, ""), }) return } diff --git a/internal/httpapi/ws.go b/internal/httpapi/ws.go index 46569334..dfe8b93f 100644 --- a/internal/httpapi/ws.go +++ b/internal/httpapi/ws.go @@ -50,13 +50,18 @@ func (s *Server) handleLiveWS(w http.ResponseWriter, r *http.Request) { return } + // Live mode seats the joiner at p1, so it names p1. An agent that dialed + // here (rather than being the SPA that created the battle) otherwise has no + // way to say who it is — the create call was made by someone else. + trainer := s.rebindTrainer(ctx, battleID, cache.SlotP1, r.URL.Query().Get("name")) + conn, err := upgrader.Upgrade(w, r, nil) if err != nil { return } defer conn.Close() - s.bridgeSlot(ctx, conn, battleID, cache.SlotP1) + s.bridgeSlot(ctx, conn, battleID, cache.SlotP1, trainer) } // joinableStatus reports whether a battle in the given lifecycle status still @@ -118,13 +123,55 @@ func (s *Server) handlePvPWS(w http.ResponseWriter, r *http.Request) { } defer s.releaseSlotBest(battleID, slot) + // The joiner may declare who it is. Do this after the claim so an + // unauthenticated caller cannot rewrite a battle's trainer by guessing + // battle ids — holding the slot token is the permission to name the slot. + trainer := s.rebindTrainer(ctx, battleID, slot, r.URL.Query().Get("name")) + conn, err := upgrader.Upgrade(w, r, nil) if err != nil { return } defer conn.Close() - s.bridgeSlot(ctx, conn, battleID, slot) + s.bridgeSlot(ctx, conn, battleID, slot, trainer) +} + +// rebindTrainer records the joiner's self-declared name as the occupant of +// this slot, and returns the sanitized name for the session to display ("" if +// none was declared or nothing survived sanitizing). +// +// Why this exists: a live_pvp battle is created before its players arrive, so +// both slots are named by whoever pressed "Start" — an agent joining p2 lands +// on the board as "Opponent". Declaring a name at join is what lets a result +// be attributed to the controller that actually played it. +// +// This is attribution, not authentication. Any holder of a slot token may +// claim any name, including one already on the leaderboard; verified handles +// (claim-a-handle + secret) are separate, later work. Treat a name here as +// self-reported. +// +// A failure to persist is logged and swallowed: the battle is playable and the +// player is already connected, so refusing the join over a leaderboard-label +// write would trade a working game for a cosmetic field. +func (s *Server) rebindTrainer(ctx context.Context, battleID string, slot cache.PvPSlot, raw string) string { + name := protocol.SanitizeTrainerName(raw) + if name == "" { + return "" // nothing declared — keep the creator's placeholder + } + trainerID, err := s.store.UpsertTrainer(ctx, name) + if err != nil { + log.Printf("pvp trainer upsert failed battle=%s slot=%s name=%q: %v", battleID, slot, name, err) + return name + } + idx := 0 + if slot == cache.SlotP2 { + idx = 1 + } + if err := s.store.RebindBattleTrainer(ctx, battleID, idx, trainerID, name); err != nil { + log.Printf("pvp trainer rebind failed battle=%s slot=%s name=%q: %v", battleID, slot, name, err) + } + return name } // bridgeSlot is the gateway's whole live-battle job: shuttle bytes between one @@ -137,7 +184,10 @@ func (s *Server) handlePvPWS(w http.ResponseWriter, r *http.Request) { // function's reader loop (socket → actions). On reader exit the bridge tells the // session the slot disconnected; the writer's deferred read-deadline unblocks a // half-open reader so the slot is never leaked. -func (s *Server) bridgeSlot(ctx context.Context, conn *websocket.Conn, battleID string, slot cache.PvPSlot) { +// trainer, when non-empty, is the slot's self-declared name; it rides the +// attach message so the session owner can label the slot in room frames and in +// the engine state it builds. +func (s *Server) bridgeSlot(ctx context.Context, conn *websocket.Conn, battleID string, slot cache.PvPSlot, trainer string) { slotName := string(slot) subID, frames, err := s.hub.SubscribeFrames(battleID, slotName) @@ -158,7 +208,7 @@ func (s *Server) bridgeSlot(ctx context.Context, conn *websocket.Conn, battleID // Announce attachment so the session shows this slot connected; announce // disconnect on exit so it can wind the match down (after its grace window). - s.sendLiveAction(messages.LiveAction{BattleID: battleID, Slot: slotName, Conn: connID, Phase: messages.LivePhaseAttach}) + s.sendLiveAction(messages.LiveAction{BattleID: battleID, Slot: slotName, Conn: connID, Phase: messages.LivePhaseAttach, Trainer: trainer}) defer s.sendLiveAction(messages.LiveAction{BattleID: battleID, Slot: slotName, Conn: connID, Phase: messages.LivePhaseDisconnect}) bridgeCtx, cancelBridge := context.WithCancel(ctx) diff --git a/internal/livebattle/bridge.go b/internal/livebattle/bridge.go index 97340770..c9b85f09 100644 --- a/internal/livebattle/bridge.go +++ b/internal/livebattle/bridge.go @@ -56,6 +56,10 @@ func (p *Pump) Route(a messages.LiveAction) { } switch a.Phase { case messages.LivePhaseAttach: + // Adopt the joiner's declared name before announcing the connection, so + // the room frame that the attach triggers already carries it and the + // opponent never sees the placeholder flash past. + p.m.SetTrainerName(slot, a.Trainer) // Record the connection before attaching the producer: a re-attach under a // new id must cancel any reconnect-grace timer from the prior connection's // disconnect, even though the producer is already registered. diff --git a/internal/livebattle/coordinator.go b/internal/livebattle/coordinator.go index e8b2cd1f..2780870d 100644 --- a/internal/livebattle/coordinator.go +++ b/internal/livebattle/coordinator.go @@ -277,8 +277,8 @@ func (m *Match) runOpenPhase(ctx context.Context) error { } st, err := engine.NewBattleFromPicks(m.deps.Dex, m.battleID, - m.trainerName[0], m.submitted[0], - m.trainerName[1], m.submitted[1], + m.trainerName.get(0), m.submitted[0], + m.trainerName.get(1), m.submitted[1], m.seed) if err != nil { return fmt.Errorf("engine init: %w", err) @@ -561,12 +561,12 @@ func (m *Match) broadcastRoom(phase protocol.RoomPhase, attached [2]bool) { you := protocol.RoomSlot{ Attached: attached[i], Submitted: m.submitted[i] != nil, - Trainer: m.trainerName[i], + Trainer: m.trainerName.get(i), } them := protocol.RoomSlot{ Attached: attached[1-i], Submitted: m.submitted[1-i] != nil, - Trainer: m.trainerName[1-i], + Trainer: m.trainerName.get(1 - i), } m.send(i, protocol.MatchUpdate{ Type: protocol.FrameRoom, diff --git a/internal/livebattle/livebattle.go b/internal/livebattle/livebattle.go index eae118de..5dce9977 100644 --- a/internal/livebattle/livebattle.go +++ b/internal/livebattle/livebattle.go @@ -186,7 +186,7 @@ type Match struct { battleID string createdAt time.Time seed uint64 - trainerName [2]string + trainerName trainerNames kind [2]SideKind aiTeam [2][]engine.TeamPick roomDeadline time.Duration @@ -257,7 +257,7 @@ func NewMatch(cfg Config) *Match { battleID: cfg.BattleID, createdAt: time.Now(), seed: cfg.Seed, - trainerName: [2]string{cfg.P1Name, cfg.P2Name}, + trainerName: trainerNames{name: [2]string{cfg.P1Name, cfg.P2Name}}, kind: cfg.Kinds, aiTeam: cfg.AITeams, roomDeadline: deadline, @@ -340,6 +340,11 @@ func (m *Match) Disconnect(slot int) { m.closeOnce[slot].Do(func() { close(m.closed[slot]) }) } +// SetTrainerName records a slot's self-declared display name, replacing the +// one the battle's creator supplied. Ignored when name is empty (the joiner +// declared nothing). Safe to call from the action-pump goroutine. +func (m *Match) SetTrainerName(slot int, name string) { m.trainerName.set(slot, name) } + // SlotConnected records that connID is now the live connection for slot, // canceling any reconnect-grace timer in flight. See slotConns.connected. func (m *Match) SlotConnected(slot int, connID string) { diff --git a/internal/livebattle/trainers.go b/internal/livebattle/trainers.go new file mode 100644 index 00000000..89e74667 --- /dev/null +++ b/internal/livebattle/trainers.go @@ -0,0 +1,39 @@ +package livebattle + +import "sync" + +// trainerNames holds the display name of each slot's occupant. +// +// It needs its own mutex because, unlike everything else the coordinator +// reads, these are written from a different goroutine at an arbitrary moment: +// the names start as whatever the battle's creator supplied, and a slot may +// replace its own when it attaches (see messages.LiveAction.Trainer). The +// write lands on the action-pump goroutine; the reads are on the coordinator +// goroutine, building room frames and the initial engine state. +// +// Names are display-only — no turn, action, or validation decision reads +// them — so a name that arrives concurrently with a room broadcast is a +// benign race in behavior, and only a data race in the memory-model sense. +// That is exactly what this guards. +type trainerNames struct { + mu sync.RWMutex + name [2]string +} + +func (t *trainerNames) get(slot int) string { + t.mu.RLock() + defer t.mu.RUnlock() + return t.name[slot] +} + +// set replaces the slot's name. An empty name is ignored rather than stored: +// a joiner that declares nothing keeps the creator-supplied placeholder, and +// treating "" as a value would blank the label on every anonymous attach. +func (t *trainerNames) set(slot int, name string) { + if name == "" { + return + } + t.mu.Lock() + defer t.mu.Unlock() + t.name[slot] = name +} diff --git a/internal/livebattle/trainers_test.go b/internal/livebattle/trainers_test.go new file mode 100644 index 00000000..d7670aed --- /dev/null +++ b/internal/livebattle/trainers_test.go @@ -0,0 +1,124 @@ +package livebattle + +import ( + "context" + "testing" + "time" + + "pokearena/internal/messages" + "pokearena/internal/protocol" +) + +// A live_pvp battle is created before its players arrive, so both slots carry +// names chosen by whoever pressed "Start" — an agent joining p2 was recorded as +// "Opponent". A slot that declares a name on attach must replace it, because +// that name is the leaderboard key the battle's result posts to. +func TestAttach_DeclaredTrainerNameReplacesCreatorPlaceholder(t *testing.T) { + dex := loadDex(t) + t1, t2 := twoTeams(t, dex) + + sink := newChanSink() + m := NewMatch(Config{ + BattleID: "B-name", P1Name: "Red", P2Name: "Opponent", Seed: 7, + Kinds: [2]SideKind{SideWS, SideWS}, + Sink: sink, + Deps: Deps{ + Dex: dex, Cache: &fakeCache{}, Store: &fakeStore{}, Publish: (&eventRecorder{}).publish, + }, + }) + pump := NewPump(m, [2]SideKind{SideWS, SideWS}) + + done := make(chan struct{}) + go func() { m.Run(context.Background()); close(done) }() + defer func() { + go func() { + for range sink.ch[0] { + } + }() + go func() { + for range sink.ch[1] { + } + }() + pump.Route(messages.LiveAction{BattleID: "B-name", Slot: "p1", Phase: messages.LivePhaseDisconnect}) + <-done + }() + + // p1 attaches anonymously (keeps "Red"); p2 declares itself. + pump.Route(messages.LiveAction{BattleID: "B-name", Slot: "p1", Phase: messages.LivePhaseAttach}) + pump.Route(messages.LiveAction{ + BattleID: "B-name", Slot: "p2", Phase: messages.LivePhaseAttach, Trainer: "claude-haiku", + }) + + // The room frame is the first place the opponent sees who they're facing. + // p1 gets a room frame at its own attach too, before p2 exists — wait for + // the broadcast that actually reflects p2 being in the room. + room := readRoomUntilThemAttached(t, sink.ch[0], 5*time.Second) + if got := room.You.Trainer; got != "Red" { + t.Errorf("p1 (anonymous attach) = %q, want the creator's name %q", got, "Red") + } + if got := room.Them.Trainer; got != "claude-haiku" { + t.Errorf("p2 as seen by p1 = %q, want the declared name %q", got, "claude-haiku") + } + + // And it must reach the engine state, which is what gets persisted and + // replayed — a name that only lived in a room frame would be cosmetic. + pump.Route(messages.LiveAction{BattleID: "B-name", Slot: "p1", Phase: messages.LivePhaseSubmit, Picks: t1}) + pump.Route(messages.LiveAction{BattleID: "B-name", Slot: "p2", Phase: messages.LivePhaseSubmit, Picks: t2}) + + // Each side sees its own Side in full; the fog projection reduces the foe + // to their active Pokémon, so p2's name is asserted from p2's own view. + s0 := readUntil(t, sink.ch[0], protocol.FrameState, 5*time.Second) + s1 := readUntil(t, sink.ch[1], protocol.FrameState, 5*time.Second) + if s0.View == nil || s1.View == nil { + t.Fatal("FrameState carried no view") + } + if got := s0.View.Self.Trainer; got != "Red" { + t.Errorf("engine state p1 trainer = %q, want %q", got, "Red") + } + if got := s1.View.Self.Trainer; got != "claude-haiku" { + t.Errorf("engine state p2 trainer = %q, want the declared name %q", got, "claude-haiku") + } +} + +// readRoomUntilThemAttached returns the first FrameRoom whose opponent slot is +// attached. A slot receives a room broadcast on its own attach as well, and +// that one necessarily predates the other side joining. +func readRoomUntilThemAttached(t *testing.T, ch <-chan protocol.MatchUpdate, timeout time.Duration) *protocol.RoomUpdate { + t.Helper() + deadline := time.After(timeout) + for { + select { + case u, ok := <-ch: + if !ok { + t.Fatal("frame channel closed before the opponent attached") + } + if u.Type == protocol.FrameRoom && u.Room != nil && u.Room.Them.Attached { + return u.Room + } + case <-deadline: + t.Fatal("timed out waiting for a room frame with the opponent attached") + } + } +} + +// set is the write half of the concurrency unit; an empty declaration must be +// a no-op rather than a store, or every anonymous attach would blank the label. +func TestTrainerNames_EmptyDeclarationKeepsExisting(t *testing.T) { + names := trainerNames{name: [2]string{"Red", "Blue"}} + + names.set(1, "") + if got := names.get(1); got != "Blue" { + t.Errorf("after empty set, name = %q, want the existing %q", got, "Blue") + } + + names.set(1, "claude-opus") + if got := names.get(1); got != "claude-opus" { + t.Errorf("after set, name = %q, want %q", got, "claude-opus") + } + + // A re-attach that declares nothing must not undo the name from the first. + names.set(1, "") + if got := names.get(1); got != "claude-opus" { + t.Errorf("re-attach blanked the name: got %q, want %q", got, "claude-opus") + } +} diff --git a/internal/mcpserver/session.go b/internal/mcpserver/session.go index 2a1ac90d..d9af4db1 100644 --- a/internal/mcpserver/session.go +++ b/internal/mcpserver/session.go @@ -159,7 +159,7 @@ func newSession(cfg Config) *session { // Join opens the gateway WS, starts the dispatcher, and blocks until // the first state frame arrives. Returns the initial view + identity // info the agent needs to play. -func (s *session) Join(ctx context.Context, battleID, slot, token string) (joinBattleOut, error) { +func (s *session) Join(ctx context.Context, battleID, slot, token, trainer string) (joinBattleOut, error) { s.mu.Lock() if s.client != nil { s.mu.Unlock() @@ -176,9 +176,9 @@ func (s *session) Join(ctx context.Context, battleID, slot, token string) (joinB var err error if token == "" { slot = "p1" - gc, err = gwclient.DialLive(ctx, s.cfg.GatewayURL, battleID) + gc, err = gwclient.DialLive(ctx, s.cfg.GatewayURL, battleID, trainer) } else { - gc, err = gwclient.Dial(ctx, s.cfg.GatewayURL, battleID, slot, token) + gc, err = gwclient.Dial(ctx, s.cfg.GatewayURL, battleID, slot, token, trainer) } if err != nil { return joinBattleOut{}, fmt.Errorf("connect to gateway: %w", err) diff --git a/internal/mcpserver/session_test.go b/internal/mcpserver/session_test.go index 24967533..92551037 100644 --- a/internal/mcpserver/session_test.go +++ b/internal/mcpserver/session_test.go @@ -90,7 +90,7 @@ func TestWaitPreservesFoeHPPercent(t *testing.T) { sess := newTestSession(base) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - if _, err := sess.Join(ctx, "b", "p1", "tok"); err != nil { + if _, err := sess.Join(ctx, "b", "p1", "tok", ""); err != nil { t.Fatalf("Join: %v", err) } w, err := sess.Wait(ctx, 5) @@ -124,7 +124,7 @@ func TestJoinReturnsFirstView(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - out, err := sess.Join(ctx, "battle-x", "p1", "tok") + out, err := sess.Join(ctx, "battle-x", "p1", "tok", "") must(t, "Join", err) if out.YourTrainer != "Red" { t.Errorf("YourTrainer=%q, want Red", out.YourTrainer) @@ -144,10 +144,10 @@ func TestJoinTwiceFails(t *testing.T) { sess := newTestSession(base) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - _, err := sess.Join(ctx, "b", "p1", "t") + _, err := sess.Join(ctx, "b", "p1", "t", "") must(t, "Join", err) - _, err = sess.Join(ctx, "b2", "p2", "t2") + _, err = sess.Join(ctx, "b2", "p2", "t2", "") if !errors.Is(err, errAlreadyJoined) { t.Errorf("second Join: got %v, want errAlreadyJoined", err) } @@ -183,7 +183,7 @@ func TestWaitReturnsImmediatelyAfterJoin(t *testing.T) { sess := newTestSession(base) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - _, err := sess.Join(ctx, "b", "p1", "t") + _, err := sess.Join(ctx, "b", "p1", "t", "") must(t, "Join", err) start := time.Now() @@ -228,7 +228,7 @@ func TestActThenWaitForOpponent(t *testing.T) { sess := newTestSession(base) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - _, err := sess.Join(ctx, "b", "p1", "t") + _, err := sess.Join(ctx, "b", "p1", "t", "") must(t, "Join", err) // First Wait → ready for turn 0. @@ -278,7 +278,7 @@ func TestWaitTimesOut(t *testing.T) { sess := newTestSession(base) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - _, err := sess.Join(ctx, "b", "p1", "t") + _, err := sess.Join(ctx, "b", "p1", "t", "") must(t, "Join", err) _, _ = sess.Wait(ctx, 1) _, _ = sess.Act(protocol.ActionKindMove, 0) @@ -314,7 +314,7 @@ func TestEndFrameTerminatesSession(t *testing.T) { sess := newTestSession(base) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - _, err := sess.Join(ctx, "b", "p1", "t") + _, err := sess.Join(ctx, "b", "p1", "t", "") must(t, "Join", err) _, _ = sess.Wait(ctx, 1) // returns immediately with state from Join _, err = sess.Act(protocol.ActionKindMove, 0) @@ -349,7 +349,7 @@ func TestLeaveAllowsRejoin(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - _, err := sess.Join(ctx, "b1", "p1", "t1") + _, err := sess.Join(ctx, "b1", "p1", "t1", "") must(t, "Join 1", err) must(t, "Leave", sess.Leave()) @@ -357,7 +357,7 @@ func TestLeaveAllowsRejoin(t *testing.T) { defer cleanup2() sess.cfg.GatewayURL = base2 - out, err := sess.Join(ctx, "b2", "p2", "t2") + out, err := sess.Join(ctx, "b2", "p2", "t2", "") must(t, "Join 2", err) if out.YourTrainer != "Blue" || out.BattleID != "b2" { t.Errorf("rejoin lost identity: %+v", out) diff --git a/internal/mcpserver/tools.go b/internal/mcpserver/tools.go index c5ad2aff..e9cb0a1c 100644 --- a/internal/mcpserver/tools.go +++ b/internal/mcpserver/tools.go @@ -20,6 +20,7 @@ type joinBattleIn struct { BattleID string `json:"battle_id" jsonschema:"the battle's UUID, as printed by the gateway when the battle was created"` Slot string `json:"slot" jsonschema:"which trainer slot to claim: 'p1' or 'p2'. Ignored for live (vs-AI) battles, which always seat you as p1"` Token string `json:"join_token" jsonschema:"the per-slot join token for a pvp battle; treat as a password — never log it. Omit (empty) to join a live vs-AI battle, which is tokenless"` + Trainer string `json:"trainer_name,omitempty" jsonschema:"the name to record for this slot on the leaderboard - identify yourself (e.g. your model name). Max 24 characters. Omit to inherit the placeholder the battle's creator chose, which means the result is recorded against a name like 'Opponent' rather than against you"` } type joinBattleOut struct { @@ -150,7 +151,10 @@ func (s *Server) registerTools() { Description: "Bind this MCP session to a battle. Opens a WebSocket to the gateway " + "and returns the initial fog-of-war view. Call this first; every other tool requires it. " + "For a live vs-AI battle, pass only battle_id (no slot, no join_token) — you are seated as " + - "p1 against the programmatic opponent. For a pvp battle, pass slot and join_token.", + "p1 against the programmatic opponent. For a pvp battle, pass slot and join_token. " + + "Pass trainer_name to say who you are: it is the name this battle's result is recorded " + + "under on the leaderboard, and without it your games land under the placeholder the " + + "battle's creator chose.", }, s.joinBattle) mcp.AddTool(s.mcp, &mcp.Tool{ @@ -233,7 +237,7 @@ func (s *Server) registerTools() { // MCP result automatically. func (s *Server) joinBattle(ctx context.Context, _ *mcp.CallToolRequest, in joinBattleIn) (*mcp.CallToolResult, joinBattleOut, error) { - out, err := s.session.Join(ctx, in.BattleID, in.Slot, in.Token) + out, err := s.session.Join(ctx, in.BattleID, in.Slot, in.Token, in.Trainer) return nil, out, err } diff --git a/internal/messages/messages.go b/internal/messages/messages.go index 9b8d9015..774b82f8 100644 --- a/internal/messages/messages.go +++ b/internal/messages/messages.go @@ -104,9 +104,15 @@ type LiveAction struct { // is no longer the live one, and (b) cancel a disconnect's reconnect-grace // timer when the same slot re-attaches under a new id. Empty on legacy/test // messages, which are always honored (no identity check). - Conn string `json:"conn,omitempty"` - Picks []engine.TeamPick `json:"picks,omitempty"` // Phase == "submit" - Action engine.Action `json:"action,omitempty"` // Phase == "action" + Conn string `json:"conn,omitempty"` + // Trainer is the slot's self-declared name, carried on Phase == "attach" + // only. The gateway has already sanitized it and rebound the battle row's + // trainer, so this copy exists purely so the owner's in-memory match can + // label the slot in room frames and in the engine state it builds. Empty + // means the joiner declared nothing — keep the creator-supplied name. + Trainer string `json:"trainer,omitempty"` + Picks []engine.TeamPick `json:"picks,omitempty"` // Phase == "submit" + Action engine.Action `json:"action,omitempty"` // Phase == "action" } // Live action phases. diff --git a/internal/protocol/pvp.go b/internal/protocol/pvp.go index 5a59bcbc..e85a91a5 100644 --- a/internal/protocol/pvp.go +++ b/internal/protocol/pvp.go @@ -10,6 +10,9 @@ package protocol import ( "encoding/json" + "net/url" + "strings" + "unicode" "pokearena/internal/ai" "pokearena/internal/engine" @@ -124,15 +127,59 @@ type WsClientMsg struct { Picks []engine.TeamPick `json:"picks,omitempty"` } +// MaxTrainerNameLen bounds a self-declared trainer name, matching the SPA's +// input maxlength. The name is a leaderboard key, so it is stored and shown +// verbatim; the cap keeps one from crowding a standings row or a room frame. +const MaxTrainerNameLen = 24 + +// SanitizeTrainerName normalizes a self-declared trainer name to what the +// gateway is willing to store. Both sides call it: a client so it can tell +// the user what will actually be recorded, and the gateway because a name +// arriving over the wire is untrusted input. +// +// The rules are deliberately narrow — trim surrounding space, drop control +// characters (a newline in a name corrupts every log line that prints it), +// and cap the length. Everything else is allowed through unchanged: this +// normalizes, it does not police. Returns "" when nothing survives, which +// callers read as "no name declared" rather than as a name of empty string. +func SanitizeTrainerName(name string) string { + cleaned := strings.Map(func(r rune) rune { + if r == '\t' || r == '\n' || r == '\r' { + return ' ' // fold whitespace-ish controls so words stay separated + } + if unicode.IsControl(r) { + return -1 + } + return r + }, name) + cleaned = strings.TrimSpace(cleaned) + // Count runes, not bytes: a 24-byte cap would truncate a non-ASCII name + // mid-codepoint and store invalid UTF-8. + if runes := []rune(cleaned); len(runes) > MaxTrainerNameLen { + cleaned = strings.TrimSpace(string(runes[:MaxTrainerNameLen])) + } + return cleaned +} + // PlayPath builds the WebSocket join path for a live_pvp slot. Both the // gateway (issuing URLs to the battle creator) and any client that // constructs its own connect URL (the MCP server building from a // battle_id + token pair) call this so the shape stays in lockstep. // +// trainer is the joiner's self-declared name for the leaderboard; pass "" +// to leave the slot with whatever name the battle's creator gave it. It is +// sanitized here so a client cannot smuggle a name past the shared rule by +// building its own query, and query-escaped because unlike battleID/slot/ +// token (UUID, "p1"/"p2", base64url) a name may contain spaces and '&'. +// // Returns the path only, not the scheme/host — callers prepend their // origin or gateway base URL. -func PlayPath(battleID, slot, token string) string { - return "/api/battles/" + battleID + "/play?slot=" + slot + "&token=" + token +func PlayPath(battleID, slot, token, trainer string) string { + p := "/api/battles/" + battleID + "/play?slot=" + slot + "&token=" + token + if t := SanitizeTrainerName(trainer); t != "" { + p += "&name=" + url.QueryEscape(t) + } + return p } // LivePlayPath builds the WebSocket join path for a single-player live-mode @@ -141,6 +188,15 @@ func PlayPath(battleID, slot, token string) string { // ID is the whole auth model (see httpapi handleLiveWS). The absence of a slot // param is precisely what routes the gateway to the live handler instead of the // pvp one, so this must not append one. -func LivePlayPath(battleID string) string { - return "/api/battles/" + battleID + "/play" +// +// trainer is the joiner's self-declared name for the leaderboard, same +// contract as PlayPath; pass "" to keep the creator's name. A "name" query is +// safe to add here because the handler routes on "slot", not on the query +// being empty. +func LivePlayPath(battleID, trainer string) string { + p := "/api/battles/" + battleID + "/play" + if t := SanitizeTrainerName(trainer); t != "" { + p += "?name=" + url.QueryEscape(t) + } + return p } diff --git a/internal/protocol/pvp_test.go b/internal/protocol/pvp_test.go new file mode 100644 index 00000000..83a7f6fe --- /dev/null +++ b/internal/protocol/pvp_test.go @@ -0,0 +1,131 @@ +package protocol + +import ( + "net/url" + "strings" + "testing" +) + +func TestSanitizeTrainerName(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"plain", "Red", "Red"}, + {"trims surrounding space", " Red ", "Red"}, + {"keeps interior space", "Trainer Red", "Trainer Red"}, + {"empty stays empty", "", ""}, + {"whitespace only collapses to empty", " \t ", ""}, + // A newline in a name would break every log line and room frame that + // prints it, so controls are folded to a space rather than kept. + {"folds newline to space", "Trainer\nRed", "Trainer Red"}, + {"drops other controls", "Red\x00\x07", "Red"}, + {"caps at the max", strings.Repeat("a", MaxTrainerNameLen+10), strings.Repeat("a", MaxTrainerNameLen)}, + {"trims after capping", strings.Repeat("a", MaxTrainerNameLen-1) + " tail", strings.Repeat("a", MaxTrainerNameLen-1)}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := SanitizeTrainerName(c.in); got != c.want { + t.Errorf("SanitizeTrainerName(%q) = %q, want %q", c.in, got, c.want) + } + }) + } +} + +// A byte-based cap would slice a multi-byte rune in half and store invalid +// UTF-8 under a name that is then a leaderboard key. Count runes. +func TestSanitizeTrainerName_CapsRunesNotBytes(t *testing.T) { + in := strings.Repeat("é", MaxTrainerNameLen+5) // 2 bytes per rune + got := SanitizeTrainerName(in) + if n := len([]rune(got)); n != MaxTrainerNameLen { + t.Errorf("kept %d runes, want %d", n, MaxTrainerNameLen) + } + if !isValidUTF8(got) { + t.Errorf("truncation produced invalid UTF-8: %q", got) + } +} + +func isValidUTF8(s string) bool { + for _, r := range s { + if r == '�' { + return false + } + } + return true +} + +func TestPlayPath(t *testing.T) { + t.Run("omits the name query when none is declared", func(t *testing.T) { + got := PlayPath("b1", "p2", "tok", "") + want := "/api/battles/b1/play?slot=p2&token=tok" + if got != want { + t.Errorf("got %q, want %q", got, want) + } + }) + + t.Run("appends a declared name", func(t *testing.T) { + got := PlayPath("b1", "p2", "tok", "Red") + want := "/api/battles/b1/play?slot=p2&token=tok&name=Red" + if got != want { + t.Errorf("got %q, want %q", got, want) + } + }) + + // A name is the only join field that can contain a space or '&' — the + // others are a UUID, "p1"/"p2", and base64url. Unescaped, "A&slot=p1" + // would smuggle a second slot param into the query. + t.Run("escapes a name that could forge a query param", func(t *testing.T) { + got := PlayPath("b1", "p2", "tok", "A&slot=p1") + q, err := url.Parse(got) + if err != nil { + t.Fatalf("parse: %v", err) + } + if slot := q.Query().Get("slot"); slot != "p2" { + t.Errorf("slot = %q, want p2 — the name forged a query param", slot) + } + if name := q.Query().Get("name"); name != "A&slot=p1" { + t.Errorf("name = %q, want it round-tripped intact", name) + } + }) + + // A client building its own path must not be able to bypass the shared + // rule by passing something the gateway would have rejected. + t.Run("sanitizes before appending", func(t *testing.T) { + got := PlayPath("b1", "p2", "tok", " Red\n ") + if name := mustQuery(t, got).Get("name"); name != "Red" { + t.Errorf("name = %q, want %q", name, "Red") + } + }) +} + +func TestLivePlayPath(t *testing.T) { + t.Run("stays slotless and queryless without a name", func(t *testing.T) { + got := LivePlayPath("b1", "") + if got != "/api/battles/b1/play" { + t.Errorf("got %q, want the bare path", got) + } + }) + + // The gateway routes to the live handler on the *absence of a slot param*, + // not on an empty query — so adding "name" must not reroute the join. + t.Run("a name does not introduce a slot param", func(t *testing.T) { + got := LivePlayPath("b1", "Red") + q := mustQuery(t, got) + if q.Has("slot") { + t.Errorf("got %q, which would route to the pvp handler", got) + } + if q.Get("name") != "Red" { + t.Errorf("name = %q, want Red", q.Get("name")) + } + }) +} + +func mustQuery(t *testing.T, rawPath string) url.Values { + t.Helper() + u, err := url.Parse(rawPath) + if err != nil { + t.Fatalf("parse %q: %v", rawPath, err) + } + return u.Query() +} diff --git a/internal/session/trainerrebind_test.go b/internal/session/trainerrebind_test.go new file mode 100644 index 00000000..a04a07aa --- /dev/null +++ b/internal/session/trainerrebind_test.go @@ -0,0 +1,137 @@ +//go:build integration + +package session_test + +// RebindBattleTrainer is the write that makes the leaderboard describe who +// actually played. A live_pvp battle is created before its players arrive, so +// both trainer FKs are bound to names the creator invented ("Opponent"); the +// slot's real occupant only shows up later over the join WebSocket, and it is +// that occupant whose rating the result should move. +// +// The SQL is the whole mechanism, so it is verified against a real Postgres +// rather than mocked: the two failure modes that matter — writing the wrong +// slot's columns, and rewriting a battle whose rating is already settled — are +// both invisible to a unit test of the surrounding Go. + +import ( + "context" + "testing" + "time" + + "pokearena/internal/store" + + "github.com/google/uuid" +) + +// dialStore connects to Postgres alone. The shared dialInfra also dials Redis +// and RabbitMQ, which this test never touches — depending on them would make a +// pure SQL assertion fail for reasons that have nothing to do with the SQL. +func dialStore(t *testing.T) *store.Store { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + st, err := store.New(ctx, env("DATABASE_URL", "postgres://pokearena:pokearena@localhost:5432/pokearena?sslmode=disable")) + if err != nil { + t.Fatalf("no Postgres: %v", err) + } + if err := st.Migrate(ctx); err != nil { + t.Fatalf("migrate: %v", err) + } + return st +} + +func TestRebindBattleTrainer(t *testing.T) { + st := dialStore(t) + defer st.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + // Distinct per-run names so repeated runs against a persistent database + // don't collide on the trainers table's unique name constraint. + run := uuid.NewString()[:8] + creatorP1, creatorP2 := "Red-"+run, "Opponent-"+run + joiner := "claude-haiku-" + run + + newBattle := func(t *testing.T) (string, string, string) { + t.Helper() + t1, err := st.UpsertTrainer(ctx, creatorP1) + if err != nil { + t.Fatalf("upsert p1: %v", err) + } + t2, err := st.UpsertTrainer(ctx, creatorP2) + if err != nil { + t.Fatalf("upsert p2: %v", err) + } + id := uuid.NewString() + err = st.CreateBattle(ctx, store.Battle{ + ID: id, Mode: "live_pvp", Status: "open", Seed: 1, + P1Trainer: t1, P2Trainer: t2, P1Name: creatorP1, P2Name: creatorP2, + Winner: -1, + }) + if err != nil { + t.Fatalf("create battle: %v", err) + } + return id, t1, t2 + } + + t.Run("rebinds only the named slot", func(t *testing.T) { + id, t1, _ := newBattle(t) + + joinerID, err := st.UpsertTrainer(ctx, joiner) + if err != nil { + t.Fatalf("upsert joiner: %v", err) + } + if err := st.RebindBattleTrainer(ctx, id, 1, joinerID, joiner); err != nil { + t.Fatalf("rebind: %v", err) + } + + b, err := st.GetBattle(ctx, id) + if err != nil { + t.Fatalf("get battle: %v", err) + } + if b.P2Trainer != joinerID { + t.Errorf("p2_trainer = %q, want the joiner %q", b.P2Trainer, joinerID) + } + if b.P2Name != joiner { + t.Errorf("p2_name = %q, want %q", b.P2Name, joiner) + } + // The opposite slot is the one a column-mapping slip would corrupt, + // silently reassigning the *other* player's result. + if b.P1Trainer != t1 { + t.Errorf("p1_trainer = %q, want it untouched at %q", b.P1Trainer, t1) + } + if b.P1Name != creatorP1 { + t.Errorf("p1_name = %q, want it untouched at %q", b.P1Name, creatorP1) + } + }) + + t.Run("refuses to rebind a finished battle", func(t *testing.T) { + id, _, t2 := newBattle(t) + if err := st.CompleteBattle(ctx, id, 0, 12); err != nil { + t.Fatalf("complete battle: %v", err) + } + + joinerID, err := st.UpsertTrainer(ctx, joiner) + if err != nil { + t.Fatalf("upsert joiner: %v", err) + } + // A late or replayed join must not move a rating that was already + // computed against the old trainer. The call is a silent no-op, not an + // error — the join itself is still legitimate, it just cannot rename. + if err := st.RebindBattleTrainer(ctx, id, 1, joinerID, joiner); err != nil { + t.Fatalf("rebind after completion returned an error: %v", err) + } + + b, err := st.GetBattle(ctx, id) + if err != nil { + t.Fatalf("get battle: %v", err) + } + if b.P2Trainer != t2 { + t.Errorf("p2_trainer = %q, want the settled %q", b.P2Trainer, t2) + } + if b.P2Name != creatorP2 { + t.Errorf("p2_name = %q, want the settled %q", b.P2Name, creatorP2) + } + }) +} diff --git a/internal/store/repo.go b/internal/store/repo.go index d323e240..dc406812 100644 --- a/internal/store/repo.go +++ b/internal/store/repo.go @@ -101,6 +101,29 @@ func (s *Store) CreateBattle(ctx context.Context, b Battle) error { return err } +// RebindBattleTrainer points one slot of a battle at a different trainer. +// +// A live_pvp battle is created before its players arrive: the creator names +// both slots ("Opponent", "Agent 2") and both trainer FKs are bound then. The +// slot's actual occupant only shows up later, over the join WebSocket, and it +// is that occupant — not the creator's placeholder — whose rating the result +// should move. This is the write that makes the leaderboard describe who +// actually played. +// +// slot is 0 (p1) or 1 (p2). The update is confined to a battle that has not +// finished: rebinding after a result is applied would move a rating that was +// already computed against the old trainer, and a late/replayed join must not +// be able to rewrite settled history. +func (s *Store) RebindBattleTrainer(ctx context.Context, battleID string, slot int, trainerID, name string) error { + col := "p1_trainer=$2, p1_name=$3" + if slot == 1 { + col = "p2_trainer=$2, p2_name=$3" + } + _, err := s.pool.Exec(ctx, + `UPDATE battles SET `+col+` WHERE id=$1 AND winner < 0`, battleID, trainerID, name) + return err +} + // GetBattle fetches one battle. Returns pgx.ErrNoRows if absent. func (s *Store) GetBattle(ctx context.Context, id string) (Battle, error) { return scanBattle(s.pool.QueryRow(ctx, `SELECT `+battleColumns+` FROM battles WHERE id=$1`, id)) diff --git a/web/app.js b/web/app.js index c9a8ad54..0a97fa40 100644 --- a/web/app.js +++ b/web/app.js @@ -959,6 +959,36 @@ function renderPokedex() { }).join(''); } +// ---- trainer identity ---- +// The leaderboard is keyed on the name a slot is recorded under, and that name +// is now declared by whoever occupies the slot rather than by whoever created +// the battle. TRAINER_KEY remembers it across visits so a returning player +// keeps their row instead of starting a new one every session. +const TRAINER_KEY = 'pokearena.trainer'; +const TRAINER_NAME_MAX = 24; // mirrors protocol.MaxTrainerNameLen + +// trainerName resolves this browser's name: what the setup form currently +// says, else what we remembered from last time, else the default. The form +// wins so that editing the field and hitting Start does what it looks like. +function trainerName() { + const field = document.getElementById('player-name'); + const typed = field ? field.value.trim() : ''; + const name = typed || localStorage.getItem(TRAINER_KEY) || 'Challenger'; + return name.slice(0, TRAINER_NAME_MAX); +} + +function rememberTrainerName(name) { + try { localStorage.setItem(TRAINER_KEY, name); } catch (_) { /* private mode */ } +} + +// withTrainerName appends the leaderboard name to a gateway play URL. The +// gateway sanitizes it again on arrival — this is a convenience, not a +// trust boundary. +function withTrainerName(wsUrl, name) { + if (!name) return wsUrl; + return wsUrl + (wsUrl.includes('?') ? '&' : '?') + 'name=' + encodeURIComponent(name); +} + // ---- leaderboard ---- async function loadLeaderboard() { const tbody = document.querySelector('#lb-table tbody'); @@ -984,7 +1014,8 @@ async function startBattle() { if (!App.opp.team.length) { toast('Pick at least one Pokémon for the opponent'); return; } } - const name = document.getElementById('player-name').value.trim() || 'Challenger'; + const name = trainerName(); + rememberTrainerName(name); // so a share-link join later reuses it // agent_vs_agent is a UI framing on top of live_pvp — backend has no separate // mode because the protocol is identical (two external joiners, no AI). We // just present the URLs differently and drop the user into spectate. @@ -1186,9 +1217,9 @@ function enterPicker(res, mode, name) { if (mode === 'live_pvp') { showPickerShareBanner(res.battle_id, res.p2_url); - connectPvPWS(res.p1_url); + connectPvPWS(withTrainerName(res.p1_url, name)); } else { - connectWS(res.ws_url); + connectWS(withTrainerName(res.ws_url, name)); } } @@ -2177,18 +2208,24 @@ function autoJoinPvP(battleId, slot, token) { App.pick = newBuilderState(); App.pickerSubmitted = false; if (App.pickerDeadlineTimer) { clearInterval(App.pickerDeadlineTimer); App.pickerDeadlineTimer = null; } + // A share-link joiner never passed through the setup form, so the battle's + // creator named this slot ("Opponent") and the result would post to the + // board under that. Declare our own name on the join instead. + const name = trainerName(); App.battle = { - id: battleId, mode: 'live_pvp', name: 'Trainer', view: 'picker', + id: battleId, mode: 'live_pvp', name, view: 'picker', queue: [], playing: false, ended: false, state: null, ws: null, es: null, slot, }; showView('picker'); - document.getElementById('picker-label').textContent = `Pv-Player · joining slot ${slot} · drafting team`; + document.getElementById('picker-label').textContent = + `Pv-Player · ${name} · joining slot ${slot} · drafting team`; document.querySelector('.picker-main').classList.remove('picker-locked'); document.getElementById('picker-share-banner').classList.add('hidden'); document.getElementById('picker-opp').innerHTML = '

Opponent

Connecting…
'; renderPicker(); - const wsUrl = `/api/battles/${battleId}/play?slot=${slot}&token=${encodeURIComponent(token)}`; + const wsUrl = withTrainerName( + `/api/battles/${battleId}/play?slot=${slot}&token=${encodeURIComponent(token)}`, name); connectPvPWS(wsUrl); } From eac3bd67da31b88cd3a2f97029b1224be932e361 Mon Sep 17 00:00:00 2001 From: Shaumik Mondal Date: Tue, 14 Jul 2026 21:37:28 -0700 Subject: [PATCH 02/15] =?UTF-8?q?eval:=20decision-quality=20scoring=20?= =?UTF-8?q?=E2=80=94=20recover=20actions,=20measure=20regret=20vs=20oracle?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Score how well a side chose, not just whether it won. A live battle stored every turn's engine state, and the engine is a pure function of (state, actions), so each turn re-simulates to recover the exact action played (engine.LegalActions x ResolveTurn until the stored next state reproduces byte-for-byte). Then a depth-limited expectimax oracle scores every legal action from the identical fog-of-war view (ai.MakeView), and the value gap between the played action and the best is the decision's regret. - ai.ExpectimaxAgent.ScoreActions exposes the per-action maximin values searchRoot already computes (additive; Decide unchanged), so callers can measure regret rather than only agree/disagree. - eval.ScoreDecisions walks stored turns, recovers each free choice on a side, and returns per-decision {chosen, best, agree, regret, blunder} plus a count of skipped faint/replacement turns (v1 scores clean turns; coverage reported, not hidden). - cmd/decision-eval is a spike over a battle JSON export (db-replay's shape). Validated on real stored battles: recovery re-simulates exactly, and regret correctly separates real mistakes from picking an equal-value alternative (binary agreement counts equal moves as misses; regret scores them 0). --- cmd/decision-eval/main.go | 117 ++++++++++++++++++++ internal/ai/expectimax.go | 39 +++++++ internal/eval/decisionquality.go | 147 ++++++++++++++++++++++++++ internal/eval/decisionquality_test.go | 80 ++++++++++++++ 4 files changed, 383 insertions(+) create mode 100644 cmd/decision-eval/main.go create mode 100644 internal/eval/decisionquality.go create mode 100644 internal/eval/decisionquality_test.go diff --git a/cmd/decision-eval/main.go b/cmd/decision-eval/main.go new file mode 100644 index 00000000..e3280364 --- /dev/null +++ b/cmd/decision-eval/main.go @@ -0,0 +1,117 @@ +// Command decision-eval scores how well a side chose in a stored live battle, +// not just whether it won. It reads a JSON export of one battle — +// {seed, winner, turns:[{state, log}]} — the same shape db-replay consumes, +// re-simulates each turn to recover the action played, and compares it to a +// depth-limited expectimax oracle deciding from the identical fog-of-war view. +// +// This is a spike: it prints per-battle coverage and oracle-agreement so the +// approach can be validated on real data before it grows a report section. +// +// Usage: +// +// docker exec pg psql ... -c "" | decision-eval -side 0 -depth 3 +package main + +import ( + "encoding/json" + "flag" + "fmt" + "io" + "log" + "os" + + "pokearena/internal/ai" + "pokearena/internal/domain" + "pokearena/internal/engine" + "pokearena/internal/eval" +) + +type export struct { + Seed int64 `json:"seed"` + Winner int `json:"winner"` + Turns []struct { + State json.RawMessage `json:"state"` + Log json.RawMessage `json:"log"` + } `json:"turns"` +} + +func main() { + log.SetFlags(0) + log.SetPrefix("[decision-eval] ") + + in := flag.String("in", "-", "battle JSON export path (\"-\" for stdin)") + dataDir := flag.String("data", "data", "dex data directory") + side := flag.Int("side", 0, "which side to score (0 = the model seat)") + depth := flag.Int("depth", 3, "oracle expectimax search depth") + quiet := flag.Bool("quiet", false, "print only the summary line") + flag.Parse() + + raw, err := readAll(*in) + if err != nil { + log.Fatalf("read export: %v", err) + } + var ex export + if err := json.Unmarshal(raw, &ex); err != nil { + log.Fatalf("parse export: %v", err) + } + + dex, err := domain.LoadDex(*dataDir, "bench") + if err != nil { + log.Fatalf("load dex: %v", err) + } + oracle := ai.NewExpectimaxAgentFixed(dex, *depth) + + turns := make([]eval.StoredTurn, len(ex.Turns)) + for i, t := range ex.Turns { + turns[i] = eval.StoredTurn{State: t.State, Log: t.Log} + } + + scores, skipped, err := eval.ScoreDecisions(dex, oracle, *side, turns) + if err != nil { + log.Fatalf("score: %v", err) + } + + agree, blunders := 0, 0 + var sumRegret, maxRegret float64 + for _, s := range scores { + if s.Agree { + agree++ + } + if s.Blunder { + blunders++ + } + sumRegret += s.Regret + if s.Regret > maxRegret { + maxRegret = s.Regret + } + } + agreePct, blunderPct, meanRegret := 0.0, 0.0, 0.0 + if n := float64(len(scores)); n > 0 { + agreePct = 100 * float64(agree) / n + blunderPct = 100 * float64(blunders) / n + meanRegret = sumRegret / n + } + fmt.Printf("seed=%d winner=%d turns=%d recovered=%d skipped=%d agree=%.0f%% blunder=%.0f%% meanRegret=%.0f maxRegret=%.0f depth=%d\n", + ex.Seed, ex.Winner, len(ex.Turns), len(scores), skipped, agreePct, blunderPct, meanRegret, maxRegret, *depth) + if *quiet { + return + } + for _, s := range scores { + mark := " ok " + if s.Blunder { + mark = "BLUND" + } else if !s.Agree { + mark = "diff " + } + fmt.Printf("turn %2d %s chose %s oracle %s regret %.0f\n", s.Turn, mark, actStr(s.Chosen), actStr(s.Best), s.Regret) + } +} + +func actStr(a engine.Action) string { return fmt.Sprintf("%s#%d", a.Kind, a.Index) } + +func readAll(path string) ([]byte, error) { + if path == "-" { + return io.ReadAll(os.Stdin) + } + return os.ReadFile(path) +} diff --git a/internal/ai/expectimax.go b/internal/ai/expectimax.go index 4d944697..6df3b482 100644 --- a/internal/ai/expectimax.go +++ b/internal/ai/expectimax.go @@ -147,6 +147,45 @@ func (a *ExpectimaxAgent) searchRoot(v View, depth int, deadline time.Time, hasD return best, true } +// ActionValue is a root action paired with its maximin search value, in the +// eval points value() uses (materialValue = one whole Pokémon). +type ActionValue struct { + Action engine.Action + Value float64 +} + +// ScoreActions returns the maximin value of every legal action for the deciding +// side at the agent's max depth — the same per-action scores searchRoot ranks to +// pick its move, exposed so a caller can measure the value gap (regret) between a +// policy's actual choice and the best one. Deadline-free and reproducible, like +// fixed-depth Decide, and it breaks ties toward the first legal action exactly as +// Decide does, so the top-valued action here equals Decide's choice. Returns nil +// where regret is undefined: a forced replacement (a one-ply decision expectimax +// defers to the heuristic) or a turn with a single legal action. +func (a *ExpectimaxAgent) ScoreActions(v View) []ActionValue { + if v.Replace { + return nil + } + myActs := LegalActions(v) + if len(myActs) <= 1 { + return nil + } + sim := a.reconstruct(v) + sc := searchCtx{me: v.Me, foeBench: v.FoeBenchAlive} + foeActs := a.foeActions(sim, v.Me) + out := make([]ActionValue, 0, len(myActs)) + for _, my := range myActs { + worst := math.Inf(1) + for _, fo := range foeActs { + if val := a.evalPair(sc, sim, my, fo, a.maxDepth); val < worst { + worst = val + } + } + out = append(out, ActionValue{Action: my, Value: worst}) + } + return out +} + // evalPair simulates one (my, foe) action pair K times over the chance space // and averages the resulting position values. func (a *ExpectimaxAgent) evalPair(sc searchCtx, sim *engine.BattleState, my, fo engine.Action, depth int) float64 { diff --git a/internal/eval/decisionquality.go b/internal/eval/decisionquality.go new file mode 100644 index 00000000..b7d24fa8 --- /dev/null +++ b/internal/eval/decisionquality.go @@ -0,0 +1,147 @@ +package eval + +import ( + "bytes" + "encoding/json" + "math" + + "pokearena/internal/ai" + "pokearena/internal/domain" + "pokearena/internal/engine" +) + +// BlunderThreshold is the regret, in the oracle's eval points, above which a +// choice counts as a blunder. The oracle scores one whole Pokémon at ~1000 +// points, so 300 ≈ giving up a third of a Pokémon's worth of position in a +// single move — enough to separate a genuine mistake from picking the +// second-best of two near-equal options. Tunable as the metric is calibrated. +const BlunderThreshold = 300.0 + +// Decision-quality eval scores how well a policy chose, not just whether it won. +// A live battle stored every turn's engine state, and the engine is a pure +// function of (state, actions) — so we can re-simulate each turn to recover the +// exact action a side played, then ask a stronger reference agent (the oracle) +// what it would have played from the *identical* fog-of-war view. The gap +// between the two is a per-decision quality signal that a win/loss record hides. +// +// The oracle sees exactly what the policy saw: ai.MakeView projects the same +// fog every agent decides from, so a deeper-searching expectimax is a fair +// yardstick rather than an omniscient one. + +// Oracle is the reference policy a decision is scored against. It must expose +// per-action values (not just its top pick) so we can measure regret — how much +// value a choice gave up, not merely whether it matched. *ai.ExpectimaxAgent +// satisfies it; the interface keeps this package testable with a stub. +type Oracle interface { + ScoreActions(v ai.View) []ai.ActionValue +} + +// DecisionScore is one recovered free choice: the action a side actually played +// on a turn, the oracle's best from the identical view, and the value gap +// between them. +type DecisionScore struct { + Turn int `json:"turn"` + Side int `json:"side"` + Chosen engine.Action `json:"chosen"` + Best engine.Action `json:"best"` + Agree bool `json:"agree"` // Chosen == the oracle's top choice + Regret float64 `json:"regret"` // oracle value of Best minus value of Chosen (>= 0) + Blunder bool `json:"blunder"` // Regret > BlunderThreshold +} + +// recoverActions finds the [2]Action that, applied to prev, reproduces want +// byte-for-byte (both marshaled through engine.BattleState, so jsonb key-order +// differences wash out). It returns ok=false when no single action-pair +// reproduces want — most often a turn where a Pokémon fainted and a replacement +// was chosen mid-turn, which v1 does not yet score. Because the engine is +// deterministic from the stored RNGState, the matching pair is exactly what was +// played, and the match doubles as a validity check on the recorded state. +func recoverActions(dex *domain.Dex, prev json.RawMessage, want *engine.BattleState) ([2]engine.Action, bool) { + wantJSON, err := json.Marshal(want) + if err != nil { + return [2]engine.Action{}, false + } + var base engine.BattleState + if err := json.Unmarshal(prev, &base); err != nil { + return [2]engine.Action{}, false + } + a0s := engine.LegalActionsDex(dex, &base, 0) + a1s := engine.LegalActionsDex(dex, &base, 1) + for _, a0 := range a0s { + for _, a1 := range a1s { + var trial engine.BattleState + if err := json.Unmarshal(prev, &trial); err != nil { + continue + } + engine.ResolveTurn(dex, &trial, [2]engine.Action{a0, a1}) + if got, err := json.Marshal(&trial); err == nil && bytes.Equal(got, wantJSON) { + return [2]engine.Action{a0, a1}, true + } + } + } + return [2]engine.Action{}, false +} + +// ScoreDecisions replays a live battle's stored turns and, for every clean free +// choice on modelSide it can recover, compares the model's action to the +// oracle's from the identical fog-of-war view. It returns one score per +// recovered decision plus the count of choosing-turns it could not recover +// (the faint/replacement turns v1 skips), so callers can report coverage rather +// than silently dropping them. +func ScoreDecisions(dex *domain.Dex, orc Oracle, modelSide int, turns []StoredTurn) (scores []DecisionScore, skipped int, err error) { + for i := 1; i < len(turns); i++ { + var prevState engine.BattleState + if err := json.Unmarshal(turns[i-1].State, &prevState); err != nil { + return nil, skipped, err + } + // Only a turn chosen from a choosing state is a free decision; the last + // stored state (phase "ended") is a terminal snapshot, not a choice. + if prevState.Phase != engine.PhaseChoosing { + continue + } + var next engine.BattleState + if err := json.Unmarshal(turns[i].State, &next); err != nil { + return nil, skipped, err + } + acts, ok := recoverActions(dex, turns[i-1].State, &next) + if !ok { + skipped++ + continue + } + v := ai.MakeView(&prevState, modelSide) + vals := orc.ScoreActions(v) + if len(vals) == 0 { + // Forced or single-option turn — no free choice to score. + skipped++ + continue + } + chosen := acts[modelSide] + best := vals[0] + chosenVal, found := math.Inf(-1), false + for _, av := range vals { + if av.Value > best.Value { + best = av + } + if av.Action == chosen { + chosenVal, found = av.Value, true + } + } + if !found { + // The recovered action isn't in the oracle's legal set — a fog or + // version mismatch; skip rather than report a bogus regret. + skipped++ + continue + } + regret := best.Value - chosenVal + scores = append(scores, DecisionScore{ + Turn: next.Turn, + Side: modelSide, + Chosen: chosen, + Best: best.Action, + Agree: chosen == best.Action, + Regret: regret, + Blunder: regret > BlunderThreshold, + }) + } + return scores, skipped, nil +} diff --git a/internal/eval/decisionquality_test.go b/internal/eval/decisionquality_test.go new file mode 100644 index 00000000..8f263a8c --- /dev/null +++ b/internal/eval/decisionquality_test.go @@ -0,0 +1,80 @@ +package eval + +import ( + "encoding/json" + "testing" + + "pokearena/internal/ai" + "pokearena/internal/engine" +) + +// stubOracle scores the first legal action worst (0) and the last one best +// (1000), so a policy that always plays move#0 disagrees with a known, +// fixed regret of 1000 — letting the test assert regret/blunder arithmetic +// without depending on real expectimax values. +type stubOracle struct{} + +func (stubOracle) ScoreActions(v ai.View) []ai.ActionValue { + acts := ai.LegalActions(v) + if len(acts) <= 1 { + return nil // no free choice — mirror expectimax's contract + } + out := make([]ai.ActionValue, len(acts)) + for i, a := range acts { + out[i] = ai.ActionValue{Action: a} + } + out[len(out)-1].Value = 1000 + return out +} + +// TestScoreDecisions_RecoversActionsAndRegret plays a fixed line (both sides +// spam move#0), stores each turn's state the way the live coordinator does, and +// checks that ScoreDecisions re-simulates back to the exact action played and +// computes regret against the oracle. A Snorlax mirror keeps anyone from +// fainting in the first few turns, so every turn is a clean, recoverable choice. +func TestScoreDecisions_RecoversActionsAndRegret(t *testing.T) { + d := loadDex(t) + s, err := engine.NewBattle(d, "b", "P0", []int{143, 131}, "P1", []int{143, 131}, 42) + if err != nil { + t.Fatalf("new battle: %v", err) + } + + move0 := [2]engine.Action{{Kind: engine.ActionMove, Index: 0}, {Kind: engine.ActionMove, Index: 0}} + var turns []StoredTurn + for i := 0; i < 4 && !s.Ended(); i++ { + if s.Phase != engine.PhaseChoosing { + break + } + engine.ResolveTurn(d, s, move0) + b, err := json.Marshal(s) + if err != nil { + t.Fatalf("marshal state: %v", err) + } + turns = append(turns, StoredTurn{State: append(json.RawMessage(nil), b...)}) + } + if len(turns) < 2 { + t.Fatalf("need >= 2 stored turns to score a transition, got %d", len(turns)) + } + + scores, skipped, err := ScoreDecisions(d, stubOracle{}, 0, turns) + if err != nil { + t.Fatalf("score: %v", err) + } + if len(scores) == 0 { + t.Fatalf("no decisions scored (skipped=%d)", skipped) + } + for _, sc := range scores { + if sc.Chosen.Kind != engine.ActionMove || sc.Chosen.Index != 0 { + t.Errorf("turn %d: recovered %+v, want move#0", sc.Turn, sc.Chosen) + } + if sc.Regret != 1000 { + t.Errorf("turn %d: regret %.0f, want 1000", sc.Turn, sc.Regret) + } + if !sc.Blunder { + t.Errorf("turn %d: expected a blunder at regret 1000 (threshold %.0f)", sc.Turn, BlunderThreshold) + } + if sc.Agree { + t.Errorf("turn %d: move#0 is the oracle's worst here, should not agree", sc.Turn) + } + } +} From e6f244bccf9006f0e78651229d4e2902b509046d Mon Sep 17 00:00:00 2001 From: Shaumik Mondal Date: Wed, 15 Jul 2026 20:34:29 -0700 Subject: [PATCH 03/15] =?UTF-8?q?eval:=20recover=20faint=20turns=20too=20?= =?UTF-8?q?=E2=80=94=20decision=20coverage=20to=20~full?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The naive recovery replayed one ResolveTurn and compared to the stored next state, so any turn that fainted a Pokémon (resolution stops in the replacement phase) couldn't be matched and was skipped — about half of all turns. settle now drives the resolved state through its forced replacements, searching the legal replacement picks (a cascade recurses), so faint turns reproduce the stored next state and are recovered like any other. On real battles this takes coverage from ~40-65% to ~100% of choosing turns. Test plays a full heuristic mirror that KOs and replaces, then asserts every choosing turn — faint turns included — re-derives its actions exactly. --- internal/eval/decisionquality.go | 61 +++++++++++++++---- internal/eval/decisionquality_test.go | 86 +++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 11 deletions(-) diff --git a/internal/eval/decisionquality.go b/internal/eval/decisionquality.go index b7d24fa8..b64ee505 100644 --- a/internal/eval/decisionquality.go +++ b/internal/eval/decisionquality.go @@ -51,11 +51,12 @@ type DecisionScore struct { // recoverActions finds the [2]Action that, applied to prev, reproduces want // byte-for-byte (both marshaled through engine.BattleState, so jsonb key-order -// differences wash out). It returns ok=false when no single action-pair -// reproduces want — most often a turn where a Pokémon fainted and a replacement -// was chosen mid-turn, which v1 does not yet score. Because the engine is -// deterministic from the stored RNGState, the matching pair is exactly what was -// played, and the match doubles as a validity check on the recorded state. +// differences wash out). When the turn faints a Pokémon, resolving lands in the +// replacement phase; settle then searches the replacement picks that carry the +// state the rest of the way to want, so faint turns are recovered too. Because +// the engine is deterministic from the stored RNGState, the matching pair is +// exactly what was played, and the match doubles as a validity check on the +// recorded state. ok=false means no legal line reproduces want. func recoverActions(dex *domain.Dex, prev json.RawMessage, want *engine.BattleState) ([2]engine.Action, bool) { wantJSON, err := json.Marshal(want) if err != nil { @@ -69,12 +70,9 @@ func recoverActions(dex *domain.Dex, prev json.RawMessage, want *engine.BattleSt a1s := engine.LegalActionsDex(dex, &base, 1) for _, a0 := range a0s { for _, a1 := range a1s { - var trial engine.BattleState - if err := json.Unmarshal(prev, &trial); err != nil { - continue - } - engine.ResolveTurn(dex, &trial, [2]engine.Action{a0, a1}) - if got, err := json.Marshal(&trial); err == nil && bytes.Equal(got, wantJSON) { + trial := base.Clone() + engine.ResolveTurn(dex, trial, [2]engine.Action{a0, a1}) + if settle(dex, trial, wantJSON) { return [2]engine.Action{a0, a1}, true } } @@ -82,6 +80,47 @@ func recoverActions(dex *domain.Dex, prev json.RawMessage, want *engine.BattleSt return [2]engine.Action{}, false } +// settle drives st through any forced replacements until it leaves the +// replacement phase, then reports whether it marshals equal to wantJSON. A +// replacement is itself a choice, so it searches the legal picks for each side +// that must replace (a cascade — an incoming mon that faints on entry — recurses +// the same way). The branching is tiny: usually a single side replacing with one +// of at most five benched Pokémon. +func settle(dex *domain.Dex, st *engine.BattleState, wantJSON []byte) bool { + if st.Phase != engine.PhaseReplace { + got, err := json.Marshal(st) + return err == nil && bytes.Equal(got, wantJSON) + } + opts := [2][]engine.Action{{{}}, {{}}} // a non-replacing side stays put (nil pick) + for side := 0; side < 2; side++ { + if st.Replace[side] { + opts[side] = engine.LegalActionsDex(dex, st, side) + if len(opts[side]) == 0 { + return false + } + } + } + for _, r0 := range opts[0] { + for _, r1 := range opts[1] { + trial := st.Clone() + var sw [2]*engine.Action + if st.Replace[0] { + a := r0 + sw[0] = &a + } + if st.Replace[1] { + a := r1 + sw[1] = &a + } + engine.ResolveReplace(trial, sw) + if settle(dex, trial, wantJSON) { + return true + } + } + } + return false +} + // ScoreDecisions replays a live battle's stored turns and, for every clean free // choice on modelSide it can recover, compares the model's action to the // oracle's from the identical fog-of-war view. It returns one score per diff --git a/internal/eval/decisionquality_test.go b/internal/eval/decisionquality_test.go index 8f263a8c..dc2c87e0 100644 --- a/internal/eval/decisionquality_test.go +++ b/internal/eval/decisionquality_test.go @@ -1,6 +1,7 @@ package eval import ( + "context" "encoding/json" "testing" @@ -8,6 +9,18 @@ import ( "pokearena/internal/engine" ) +func faintedCount(s *engine.BattleState) int { + n := 0 + for _, side := range s.Sides { + for _, p := range side.Team { + if p.Fainted { + n++ + } + } + } + return n +} + // stubOracle scores the first legal action worst (0) and the last one best // (1000), so a policy that always plays move#0 disagrees with a known, // fixed regret of 1000 — letting the test assert regret/blunder arithmetic @@ -78,3 +91,76 @@ func TestScoreDecisions_RecoversActionsAndRegret(t *testing.T) { } } } + +// TestRecoverActions_FaintTurns plays a full heuristic-vs-heuristic game (which +// KOs Pokémon and forces mid-turn replacements), stores each settled turn the +// way the live coordinator does, and checks that recoverActions re-derives the +// exact actions on every turn — including the faint turns, where resolving must +// also settle the replacement pick. This is the coverage the naive single- +// ResolveTurn recovery misses. +func TestRecoverActions_FaintTurns(t *testing.T) { + d := loadDex(t) + // Frail, hard-hitting mirror so KOs (and replacements) happen quickly. + s, err := engine.NewBattle(d, "b", "P0", []int{65, 94, 101}, "P1", []int{65, 94, 101}, 7) + if err != nil { + t.Fatalf("new battle: %v", err) + } + h := ai.NewHeuristicAgent(d) + ctx := context.Background() + + var turns []StoredTurn + sawFaint := false + for guard := 0; !s.Ended(); guard++ { + if guard > 500 { + t.Fatal("battle did not terminate") + } + a0, _ := h.Decide(ctx, ai.MakeView(s, 0)) + a1, _ := h.Decide(ctx, ai.MakeView(s, 1)) + engine.ResolveTurn(d, s, [2]engine.Action{a0, a1}) + for s.Phase == engine.PhaseReplace { + sawFaint = true + var sw [2]*engine.Action + for side := 0; side < 2; side++ { + if s.Replace[side] { + ra, _ := h.Decide(ctx, ai.MakeView(s, side)) + sw[side] = &ra + } + } + engine.ResolveReplace(s, sw) + } + b, err := json.Marshal(s) + if err != nil { + t.Fatalf("marshal state: %v", err) + } + turns = append(turns, StoredTurn{State: append(json.RawMessage(nil), b...)}) + } + if !sawFaint { + t.Fatal("matchup produced no faint — test would not exercise replacement recovery") + } + + faintTurns := 0 + for i := 1; i < len(turns); i++ { + var prev, next engine.BattleState + if err := json.Unmarshal(turns[i-1].State, &prev); err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(turns[i].State, &next); err != nil { + t.Fatal(err) + } + if prev.Phase != engine.PhaseChoosing { + continue + } + if _, ok := recoverActions(d, turns[i-1].State, &next); !ok { + t.Errorf("turn %d: failed to recover actions (fainted prev=%d next=%d)", + next.Turn, faintedCount(&prev), faintedCount(&next)) + continue + } + if faintedCount(&next) > faintedCount(&prev) { + faintTurns++ + } + } + if faintTurns == 0 { + t.Fatal("no recovered turn involved a faint — coverage claim unverified") + } + t.Logf("recovered every choosing turn across %d stored turns, %d with a faint", len(turns), faintTurns) +} From 374fd15d4c9338785f2d003ee34f050612cc7b86 Mon Sep 17 00:00:00 2001 From: Shaumik Mondal Date: Fri, 17 Jul 2026 12:03:43 -0700 Subject: [PATCH 04/15] docs: handoff for decision-quality eval (state, remaining work, gotchas) --- docs/decision-quality-eval-handoff.md | 185 ++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 docs/decision-quality-eval-handoff.md diff --git a/docs/decision-quality-eval-handoff.md b/docs/decision-quality-eval-handoff.md new file mode 100644 index 00000000..cc226812 --- /dev/null +++ b/docs/decision-quality-eval-handoff.md @@ -0,0 +1,185 @@ +# Handoff — Decision-Quality Eval (per-model reasoning scoring) + +Status as of 2026-07-17. Branch: `feat/decision-quality-eval` (based on the +now-merged `feat/replay-samples`; PR #108 already merged to `main`). + +## Goal + +Score how well each model *chose*, not just whether it won. For every free +decision in a recorded live battle, compare the action the model played to what +a stronger expectimax "oracle" would have played **from the identical +fog-of-war view**, and measure the **regret** (value the choice gave up). Roll +up into a per-model table: blunder rate, median regret, match rate — the +reasoning-quality view that win/loss hides. Zero new API spend was the original +constraint; we ended up re-running a fresh attributed batch (see below) because +the old attribution was wiped. + +## What is DONE (committed on this branch) + +Two commits, both green (build + lint + tests). **They may be unpushed — run +`git push -u origin feat/decision-quality-eval` first thing.** + +- `4bccd5e eval: decision-quality scoring — recover actions, measure regret vs oracle` +- `3dde37b eval: recover faint turns too — decision coverage to ~full` + +Files: +- **`internal/eval/decisionquality.go`** — the core. + - `recoverActions(dex, prevRaw, want)` — re-simulates a turn (enumerate + `engine.LegalActionsDex` for both sides × `engine.ResolveTurn`) and returns + the action pair that reproduces the stored next state **byte-for-byte** (both + marshaled through `engine.BattleState`, so jsonb key-order washes out). The + engine is deterministic from the stored `RNGState`, so the match is exact and + doubles as a data-validity check. + - `settle(dex, st, wantJSON)` — drives a resolved state through forced + replacements (searching the replacement picks, cascades recurse) so **faint + turns are recovered too**. This took coverage from ~40–65% to ~100%. + - `ScoreDecisions(dex, oracle, modelSide, turns) ([]DecisionScore, skipped, err)` + — walks stored turns, recovers each free choice on `modelSide`, and scores it. + - `DecisionScore{Turn, Side, Chosen, Best, Agree, Regret, Blunder}`. + - `Oracle` interface (`ScoreActions(v ai.View) []ai.ActionValue`). + - `BlunderThreshold = 300.0` (regret in oracle eval points; ~one Pokémon = + 1000, so 300 ≈ giving up ~0.3 of a Pokémon). Tunable. +- **`internal/ai/expectimax.go`** — added `ActionValue` + `ScoreActions(v View) + []ActionValue` (additive; exposes the per-action maximin values `searchRoot` + already computes; `Decide` unchanged; ties break toward the first legal action + exactly like `Decide`, so the top-valued action equals `Decide`'s pick). +- **`cmd/decision-eval/main.go`** — spike CLI. Reads a battle JSON export + (`{seed, winner, turns:[{state, log}]}`, the same shape `db-replay` consumes) + on stdin/`-in`, prints per-decision + a summary line. Flags: `-side` (0 = the + model seat), `-depth` (oracle depth, default 3), `-quiet`. +- **`internal/eval/decisionquality_test.go`** — deterministic tests: + `TestScoreDecisions_RecoversActionsAndRegret` (Snorlax mirror, stub oracle) and + `TestRecoverActions_FaintTurns` (heuristic mirror that KOs → exercises the + replacement recovery). + +### Validated + +- Recovery re-simulates exactly on real battles. +- Regret >> binary agreement: equal-value alternatives score regret 0 even when + they "disagree" with the oracle (that's why we don't use match rate as the + headline). Missed-lethal shows up as regret ≈ 1e6 (winValue) — real but + off-scale. +- End-to-end on a freshly-played battle on the redeployed engine: play → store → + `bid=` attribution → export → `decision-eval` scores it. Example: a Haiku game + that WON but missed a lethal (turn 15 regret ≈ 998k). + +## What is IN FLIGHT + +**A fresh attributed batch is running in the background** (started 2026-07-16 +20:57, my bg task id `b1q49nvj7`, driver `/tmp/run-attributed-batch.sh`, log +`/tmp/attributed-batch.log`). + +- 3 teams (Genesis, Keystone, Spectrum) × 4 games = 12 games/model, order + Haiku → Sonnet → Gemini → Opus, conc 3. +- **Done: Haiku, Sonnet, Gemini (36 games, all attributed).** Opus was running + last and **some Opus games abandon/time out** (known Opus slowness) — it may + finish short. Consider topping Opus up to a clean 12 with + `scripts/bench/run-batch.sh claude opus 2 cc-opus-` (lower + conc; note `run-batch.sh` truncates `results.txt`, so use a per-game append or + a fresh tag if you don't want to lose the games already there). +- Attribution lives in **`/tmp/pk-agentic-v2/-/results.txt`**, one + line per game: `g winner=<0|1|-1> -> ... bid=`. Keys: `cc-haiku`, + `cc-sonnet`, `cc-opus`, `agy-gemini` (match `eval.ModelDisplay`). + `/tmp` is not durable here (see gotchas) — if you need these to survive, + copy them somewhere safe or re-derive. + +Preliminary win rates (context only, tiny n): Haiku ~8% (1/12), Sonnet ~25% +(3/12), Gemini ~67% (8/12), Opus TBD. + +## What REMAINS (the actual task) + +1. **Finish / top up the batch** (esp. Opus). Every game must have a + `status=completed` battle in Postgres with turns; unfinished (`winner=-1`, + open/abandoned) games can't be scored — either rerun them or exclude. +2. **Build the aggregation** (the main missing piece). For each model: + - Read `bid=` lines from `/tmp/pk-agentic-v2/-*/results.txt` → set of + `battle_id`s (skip `winner=-1`). + - For each battle, export from Postgres and run `ScoreDecisions(side 0)`. + - Aggregate: **blunder rate** (headline), **median regret** (NOT mean — + winsorize/clip the ~1e6 missed-lethals or they dominate), **match rate**, + decisions scored, and the win rate for context. + - Emit a per-model table. Suggested home: extend `cmd/decision-eval` with an + aggregate mode (accept a dir of exports or a manifest + model label, output + JSON stats), or a thin `scripts/bench/decision-report.sh` that loops the + bids and pipes to a `-json` mode. **Add a test** (repo rule: every new + ability ships a test in the same commit). +3. **(Optional) Wire the table into the HTML report** (`internal/eval/report.go` + — new section), then republish to both sites (gh-pages of `shaumik/PokeArena` + and `main` of `shaumik/shaumik`) via the git-plumbing publish flow. The + sprite/sample work already lives in `main`. +4. **Open the PR** for `feat/decision-quality-eval` → `main`. + +## Data & attribution facts + +- Postgres container `pk-bench-postgres-1`, DSN + `postgres://pokearena:pokearena@postgres:5432/pokearena`. Access: + `docker exec pk-bench-postgres-1 psql -U pokearena -d pokearena ...`. +- `battle_turns(battle_id, turn_no, log jsonb, state_digest jsonb)`. `state_digest` + IS a marshaled `engine.BattleState` — `json.Unmarshal` straight back (see + `eval.ReplayFromStored`). Stored states are post-turn; phases seen are only + `choosing` and `ended` (replacements are folded into the turn), so the + pre-decision state for turn N is the stored state at turn N-1. +- **Postgres has NO model identity** — `p1_name` is always "Agent", `p2_name` + "AI". Model attribution ONLY comes from the `bid=`→model mapping in the run + dirs. The previous mapping (`/tmp/pk-agentic`) was wiped, which is why we + re-ran. +- Export shape for one battle (feeds `decision-eval -in`): + ```sql + select json_build_object('seed', b.seed, 'winner', b.winner, + 'turns', (select json_agg(json_build_object('state', t.state_digest, 'log', t.log) + order by t.turn_no) from battle_turns t where t.battle_id=b.id)) + from battles b where b.id='' + ``` + +## Environment gotchas (important) + +- **Worktrees get WIPED by concurrent-agent churn.** My original + `/private/tmp/pk-bench` got gutted (lost `.git` + source), and `/tmp/pk-agentic` + was emptied. **Push commits promptly; don't trust `/tmp` or a worktree to + persist.** +- **Do NOT edit the origin repo root** `/Users/shaumikmondal/programming/poke-sys-design` + — a concurrent agent works there (branch `worktree-terminal-ux`, the TUI / + PR #96 sprite work). Read-only is fine. +- Current worktrees: **`/private/tmp/pk-deval`** = this branch + (`feat/decision-quality-eval`); **`/private/tmp/pk-deploy`** = detached + `origin/main`, used to build/redeploy. +- **Stack was rebuilt & redeployed on `origin/main`** (image `pokearena:local` + = `b7fd69bef36f`, from commit `36e11e2`). Postgres volume `pk-bench_pgdata` + preserved (361 completed historical battles + the new batch). Recreate with + `docker compose -f /private/tmp/pk-deploy/docker-compose.yml -p pk-bench up -d`. + **Never `down -v`** (nukes the data). +- **Pre-commit hook** (`.githooks/pre-commit`, build + lint mirroring CI) is in + `main`. Enable in a new worktree with `make hooks`. Bypass one commit with + `PRECOMMIT_SKIP=1`. +- Repo conventions (from the owner): **no Claude co-author trailers** in commits + or PRs; **commit often** (small, green units); **always ship a test** with each + new mechanic in the same commit; verify with the existing CLI before claiming + results. + +## Live-battle harness (for re-running / topping up) + +- Gateway `http://localhost:8080` (ws `ws://localhost:8080`). MCP client + `bin/pokearena-mcp` (build: `make mcp`). Both `claude` and `agy` (Gemini / + Antigravity) CLIs are installed and authenticated; `agy` MCP config at + `~/.gemini/antigravity-cli/mcp_config.json`. +- One game: `scripts/bench/play-live.sh