From 1f1fbf0251eef0c55e8e7249c76ff7139727e5d9 Mon Sep 17 00:00:00 2001 From: prosolis <5590409+prosolis@users.noreply.github.com> Date: Fri, 10 Jul 2026 08:55:07 -0700 Subject: [PATCH] Review follow-ups L + I: drop dead openParty, unify the menu-index parser L: openParty had no production callers (invite path uses joinParty -> seatLeader, which seats the leader the same way but reads the owner off the expedition row). Removed it; the tests now seat the leader through a seatLeaderFixture that wraps seatLeader in a transaction. I: promoted parseTemperIndex to parseMenuIndex and routed resolveMagicEquipReply and handleMasterworkEquipReply through it, replacing two hand-inlined copies of the same digit parser. Behaviour-preserving (the parsed flag was redundant with the idx<0 guard); the retry-on-bad-parse disagreement is left as-is since this is a pure dedup. Full plugin suite green. Claude-Session: https://claude.ai/code/session_017mEwUmmS7aQTP2NQXj6rUa --- internal/plugin/adventure_masterwork.go | 14 +----- internal/plugin/adventure_temper.go | 10 ++-- internal/plugin/adventure_temper_test.go | 4 +- internal/plugin/expedition_party.go | 20 ++------ internal/plugin/expedition_party_test.go | 63 +++++++++++------------- internal/plugin/magic_items_gameplay.go | 14 +----- 6 files changed, 48 insertions(+), 77 deletions(-) diff --git a/internal/plugin/adventure_masterwork.go b/internal/plugin/adventure_masterwork.go index fc9bc43..f40d9b9 100644 --- a/internal/plugin/adventure_masterwork.go +++ b/internal/plugin/adventure_masterwork.go @@ -435,18 +435,8 @@ func (p *AdventurePlugin) handleMasterworkEquipReply(ctx MessageContext, interac return p.SendDM(ctx.Sender, "Equip cancelled.") } - // Parse number - idx := 0 - for _, c := range reply { - if c >= '0' && c <= '9' { - idx = idx*10 + int(c-'0') - } else { - break - } - } - idx-- // 1-indexed to 0-indexed - - if idx < 0 || idx >= len(data.Items) { + idx, ok := parseMenuIndex(reply, len(data.Items)) + if !ok { return p.SendDM(ctx.Sender, "Invalid selection. Reply with a number from the list, or \"cancel\".") } diff --git a/internal/plugin/adventure_temper.go b/internal/plugin/adventure_temper.go index be6a446..8ae2756 100644 --- a/internal/plugin/adventure_temper.go +++ b/internal/plugin/adventure_temper.go @@ -140,8 +140,12 @@ func findTemperMaterial(items []AdvItem) (AdvItem, bool) { return AdvItem{}, false } -// parseTemperIndex reads a leading 1-indexed number and bounds-checks it. -func parseTemperIndex(reply string, n int) (int, bool) { +// parseMenuIndex reads a leading 1-indexed number off a menu reply and returns +// the bounds-checked 0-indexed position. The single parser behind every "reply +// with a number from the list" prompt — temper, magic-item equip, masterwork +// equip. Leading whitespace and any trailing text after the digits are ignored; +// a reply with no leading digit, or one outside [1, n], returns ok=false. +func parseMenuIndex(reply string, n int) (int, bool) { idx, parsed := 0, false for _, c := range strings.TrimSpace(reply) { if c < '0' || c > '9' { @@ -214,7 +218,7 @@ func (p *AdventurePlugin) resolveTemperPick(ctx MessageContext, interaction *adv if strings.EqualFold(reply, "cancel") { return p.SendDM(ctx.Sender, "_The forge keeps burning._ Come back when you've decided.") } - idx, ok := parseTemperIndex(reply, len(data.Targets)) + idx, ok := parseMenuIndex(reply, len(data.Targets)) if !ok { p.pending.Store(string(ctx.Sender), interaction) return p.SendDM(ctx.Sender, "Reply with a number from the list, or \"cancel\".") diff --git a/internal/plugin/adventure_temper_test.go b/internal/plugin/adventure_temper_test.go index 2c5862d..1db92d7 100644 --- a/internal/plugin/adventure_temper_test.go +++ b/internal/plugin/adventure_temper_test.go @@ -346,9 +346,9 @@ func TestParseTemperIndex(t *testing.T) { {"cancel", 3, 0, false}, } for _, tc := range tests { - got, ok := parseTemperIndex(tc.in, tc.n) + got, ok := parseMenuIndex(tc.in, tc.n) if ok != tc.wantOK || (ok && got != tc.want) { - t.Errorf("parseTemperIndex(%q, %d) = (%d, %v), want (%d, %v)", + t.Errorf("parseMenuIndex(%q, %d) = (%d, %v), want (%d, %v)", tc.in, tc.n, got, ok, tc.want, tc.wantOK) } } diff --git a/internal/plugin/expedition_party.go b/internal/plugin/expedition_party.go index d4cccf8..be5ef2f 100644 --- a/internal/plugin/expedition_party.go +++ b/internal/plugin/expedition_party.go @@ -115,18 +115,6 @@ func partySize(expeditionID string) (int, error) { return n, nil } -// openParty seats the leader, converting a solo expedition row into a party of -// one. It is idempotent: a second call on the same expedition is a no-op, so -// the invite path can call it unconditionally before adding a member. -func openParty(expeditionID string, leaderID id.UserID) error { - _, err := db.Get().Exec(` - INSERT INTO expedition_party (expedition_id, user_id, role) - VALUES (?, ?, 'leader') - ON CONFLICT (expedition_id, user_id) DO NOTHING`, - expeditionID, string(leaderID)) - return err -} - // joinParty seats a member. It refuses a full roster, a duplicate, and a player // who already leads or rides an expedition of their own — the "one active // expedition per user" rule that startExpedition enforces in code, extended to @@ -171,9 +159,11 @@ func joinParty(expeditionID string, userID id.UserID) error { return tx.Commit() } -// seatLeader is openParty inside a caller's transaction: it reads the owner off -// the expedition row rather than trusting a passed-in id, so the roster's leader -// can never disagree with dnd_expedition.user_id. +// seatLeader converts a solo expedition row into a party of one, inside a +// caller's transaction. It reads the owner off the expedition row rather than +// trusting a passed-in id, so the roster's leader can never disagree with +// dnd_expedition.user_id. Idempotent (ON CONFLICT DO NOTHING), so the invite +// path can call it unconditionally before adding a member. func seatLeader(tx *sql.Tx, expeditionID string) error { var owner string err := tx.QueryRow( diff --git a/internal/plugin/expedition_party_test.go b/internal/plugin/expedition_party_test.go index 6576e9c..cfe2e2a 100644 --- a/internal/plugin/expedition_party_test.go +++ b/internal/plugin/expedition_party_test.go @@ -22,6 +22,24 @@ func seedExpedition(t *testing.T, expeditionID string, owner id.UserID, status s } } +// seatLeaderFixture seats the expedition's leader the way the invite path does — +// through seatLeader in its own transaction — reading the owner off the row that +// seedExpedition already wrote. Replaces the retired openParty helper. +func seatLeaderFixture(t *testing.T, expeditionID string) { + t.Helper() + tx, err := db.Get().Begin() + if err != nil { + t.Fatal(err) + } + if err := seatLeader(tx, expeditionID); err != nil { + _ = tx.Rollback() + t.Fatalf("seatLeader %s: %v", expeditionID, err) + } + if err := tx.Commit(); err != nil { + t.Fatal(err) + } +} + func TestParty_SoloExpeditionHasNoRoster(t *testing.T) { setupEmptyTestDB(t) owner := id.UserID("@solo:example.org") @@ -53,10 +71,9 @@ func TestParty_OpenIsIdempotent(t *testing.T) { owner := id.UserID("@lead:example.org") seedExpedition(t, "exp-1", owner, "active") + // seatLeader is idempotent — a second call on the same expedition is a no-op. for i := 0; i < 2; i++ { - if err := openParty("exp-1", owner); err != nil { - t.Fatalf("openParty #%d: %v", i+1, err) - } + seatLeaderFixture(t, "exp-1") } members, err := partyMembers("exp-1") if err != nil { @@ -71,9 +88,7 @@ func TestParty_JoinSeatsMembersLeaderFirst(t *testing.T) { setupEmptyTestDB(t) owner := id.UserID("@lead2:example.org") seedExpedition(t, "exp-2", owner, "active") - if err := openParty("exp-2", owner); err != nil { - t.Fatal(err) - } + seatLeaderFixture(t, "exp-2") for _, u := range []id.UserID{"@b:x", "@c:x"} { if err := joinParty("exp-2", u); err != nil { t.Fatalf("joinParty %s: %v", u, err) @@ -127,9 +142,7 @@ func TestParty_JoinRefusesFullRoster(t *testing.T) { setupEmptyTestDB(t) owner := id.UserID("@lead3:example.org") seedExpedition(t, "exp-3", owner, "active") - if err := openParty("exp-3", owner); err != nil { - t.Fatal(err) - } + seatLeaderFixture(t, "exp-3") for _, u := range []id.UserID{"@b:x", "@c:x"} { if err := joinParty("exp-3", u); err != nil { t.Fatal(err) @@ -145,9 +158,7 @@ func TestParty_JoinRefusesDuplicate(t *testing.T) { setupEmptyTestDB(t) owner := id.UserID("@lead4:example.org") seedExpedition(t, "exp-4", owner, "active") - if err := openParty("exp-4", owner); err != nil { - t.Fatal(err) - } + seatLeaderFixture(t, "exp-4") if err := joinParty("exp-4", "@b:x"); err != nil { t.Fatal(err) } @@ -167,12 +178,8 @@ func TestParty_JoinRefusesPlayerBusyElsewhere(t *testing.T) { setupEmptyTestDB(t) seedExpedition(t, "exp-5", "@lead5:example.org", "active") seedExpedition(t, "exp-6", "@lead6:example.org", "active") - if err := openParty("exp-5", "@lead5:example.org"); err != nil { - t.Fatal(err) - } - if err := openParty("exp-6", "@lead6:example.org"); err != nil { - t.Fatal(err) - } + seatLeaderFixture(t, "exp-5") + seatLeaderFixture(t, "exp-6") // Owns their own active expedition. seedExpedition(t, "exp-own", "@busy:example.org", "active") @@ -198,9 +205,7 @@ func TestParty_LeaveRemovesMemberButNotLeader(t *testing.T) { setupEmptyTestDB(t) owner := id.UserID("@lead7:example.org") seedExpedition(t, "exp-7", owner, "active") - if err := openParty("exp-7", owner); err != nil { - t.Fatal(err) - } + seatLeaderFixture(t, "exp-7") if err := joinParty("exp-7", "@b:x"); err != nil { t.Fatal(err) } @@ -217,9 +222,7 @@ func TestParty_LeaveRemovesMemberButNotLeader(t *testing.T) { } // Leaving frees the member for their own next run. seedExpedition(t, "exp-7b", "@lead7b:example.org", "active") - if err := openParty("exp-7b", "@lead7b:example.org"); err != nil { - t.Fatal(err) - } + seatLeaderFixture(t, "exp-7b") if err := joinParty("exp-7b", "@b:x"); err != nil { t.Errorf("departed member could not rejoin elsewhere: %v", err) } @@ -229,9 +232,7 @@ func TestParty_DisbandFreesEveryone(t *testing.T) { setupEmptyTestDB(t) owner := id.UserID("@lead8:example.org") seedExpedition(t, "exp-8", owner, "complete") - if err := openParty("exp-8", owner); err != nil { - t.Fatal(err) - } + seatLeaderFixture(t, "exp-8") if err := joinParty("exp-8", "@b:x"); err != nil { t.Fatal(err) } @@ -242,9 +243,7 @@ func TestParty_DisbandFreesEveryone(t *testing.T) { t.Errorf("after disband, partySize = %d, want 1 (no rows)", n) } seedExpedition(t, "exp-8b", "@lead8b:example.org", "active") - if err := openParty("exp-8b", "@lead8b:example.org"); err != nil { - t.Fatal(err) - } + seatLeaderFixture(t, "exp-8b") if err := joinParty("exp-8b", "@b:x"); err != nil { t.Errorf("disbanded member still looks busy: %v", err) } @@ -257,9 +256,7 @@ func TestParty_ActiveExpeditionForResolvesBothRoles(t *testing.T) { owner := id.UserID("@lead9:example.org") member := id.UserID("@member9:example.org") seedExpedition(t, "exp-9", owner, "active") - if err := openParty("exp-9", owner); err != nil { - t.Fatal(err) - } + seatLeaderFixture(t, "exp-9") if err := joinParty("exp-9", member); err != nil { t.Fatal(err) } diff --git a/internal/plugin/magic_items_gameplay.go b/internal/plugin/magic_items_gameplay.go index 21db843..73069a0 100644 --- a/internal/plugin/magic_items_gameplay.go +++ b/internal/plugin/magic_items_gameplay.go @@ -579,18 +579,8 @@ func (p *AdventurePlugin) resolveMagicEquipReply(ctx MessageContext, interaction return p.SendDM(ctx.Sender, "Equip cancelled.") } - idx := 0 - parsed := false - for _, c := range reply { - if c >= '0' && c <= '9' { - idx = idx*10 + int(c-'0') - parsed = true - } else { - break - } - } - idx-- // 1-indexed → 0-indexed - if !parsed || idx < 0 || idx >= len(data.Items) { + idx, ok := parseMenuIndex(reply, len(data.Items)) + if !ok { p.pending.Store(string(ctx.Sender), interaction) return p.SendDM(ctx.Sender, "Invalid selection. Reply with a number from the list, or \"cancel\".") }