GM lines run only on a confirmed selection (#296)

README.md

@@ -2011,7 +2011,7 @@ part before the bind in one go, then the kit check and the enter phase run as be
    (a bot that carries the item already gets none and nothing is booked), selects the bot by GUID (the agent's own group or view for a hired bot, the roster for a named one; the selection is confirmed by the server first and the line goes out as one session operation, like `.instance unbind all`),
    sends **`.additem <entry>`** and reads `.bot inv` again until the item shows (at most 8 s, else the run fails saying the line may have run on another player or the bags are full). `lead` is the lead bot, `carrier` the run's `carrier` option, else the bot the object steps use (the first bot that is not the lead, tank or healer).
    It **refuses without a GM account of level 3** (`.additem` is a developer's command; the account level is read with `.account`, and the run stops before it sends or binds anything). The line is a setup shortcut: `additem 9240 on <bot> (test setup: Mallet of Zul'Farrak)` in `setup_shortcuts`,
-   so an honest-run check flags it. `give_items` is refused in group and observe mode. Before anything is hired, logged in or sent, the run checks that the agent's game can do what its options need (`give_items`: `.additem` and `.bot inv`; the prepare block; a hired party; an optional leg's bag read) and fails naming the missing ability, so a run that cannot give items does not pay for a hire first. The `.additem` line (like `.instance unbind all`) acts on the selected player and is sent as the chat packet alone: the unit is selected, the server's echo of the selection is awaited, and the line goes out only while that confirmed selection still stands (the core handles a selection and a GM chat line on different threads, so a selection sent right before the line would not be in force); if another request changed the selection meanwhile, nothing is sent and the unit is selected and confirmed again (three tries). It is how a test gets the Mallet for the optional gahzrilla leg below.
+   so an honest-run check flags it. `give_items` is refused in group and observe mode. Before anything is hired, logged in or sent, the run checks that the agent's game can do what its options need (`give_items`: `.additem` and `.bot inv`; the prepare block; a hired party; an optional leg's bag read) and fails naming the missing ability, so a run that cannot give items does not pay for a hire first. The `.additem` line (like `.instance unbind all`) acts on the selected player and is sent as the chat packet alone: the unit is selected, the server's echo of the selection is awaited (an echo that could answer an older select of the unit, with another unit's select sent after it, does not count, nor the target field rewritten by combat or a stun, which leaves the selection unknown; when a new echo could not be told from an old one, or would not come because the field already shows the unit, a neutral selection (none, or the agent itself) is made and confirmed first; reselecting the unit already confirmed needs no echo; `.aura` world buffs and `.respawn` go the same way), and the line goes out only while that confirmed selection still stands (the core handles a selection and a GM chat line on different threads, so a selection sent right before the line would not be in force); if another request changed the selection meanwhile, nothing is sent and the unit is selected and confirmed again (three tries). It is how a test gets the Mallet for the optional gahzrilla leg below.
 
 Every GM line the step sends is a **setup shortcut**: it is in `setup_shortcuts` (for example `.tele name Lcwarra uldaman (prepare fresh)`,
 `.instance unbind all on Lcwarra (prepare fresh)`, `gm visible off`) and is a `gm_shortcut` event of kind `setup` in the trace, so `trace-score`'s

cmd/turtle-agent/commander_gps_test.go

@@ -68,22 +68,27 @@ func TestGPSSelectsTheCommanderBeforeTheLine(t *testing.T) {
 		text, err := g.GPS(context.Background())
 		done <- reply{text, err}
 	}()
+	// The server applies each selection and echoes it. A neutral selection of nothing may come first (when GPS starts before the session
+	// read the create block, the field is unknown: session.SelectConfirmed); anything else, or a line on anything but the commander, fails.
 	var ops []uint16
-	for len(ops) < 2 {
+	inForce := stranger
+	for len(ops) == 0 || ops[len(ops)-1] != opcode.CMSGMessagechat {
 		p, err := r.srv.Next(2 * time.Second)
 		if err != nil {
 			t.Fatalf("after %v: %v", ops, err)
 		}
 		switch p.Op {
 		case opcode.CMSGSetSelection:
-			if guid := packet.NewReader(p.Body).GUID(); guid != 0x2A {
-				t.Fatalf("selected %#x, not the commander (0x2A)", uint64(guid))
+			guid := packet.NewReader(p.Body).GUID()
+			if guid != 0x2A && (guid != 0 || len(ops) != 0) {
+				t.Fatalf("selected %#x, not the commander (0x2A) or a first neutral selection of nothing", uint64(guid))
 			}
+			inForce = guid
 			ops = append(ops, p.Op)
-			r.srv.Send(opcode.SMSGUpdateObject, selfValues(0x2A)) // the server's echo of the selection
+			r.srv.Send(opcode.SMSGUpdateObject, selfValues(guid)) // the server's echo of the selection
 		case opcode.CMSGMessagechat:
-			if len(ops) == 0 {
-				t.Fatal("the .gps line was sent before the commander was selected")
+			if inForce != 0x2A {
+				t.Fatalf("the .gps line was sent with %#x selected, not the commander", uint64(inForce))
 			}
 			if !strings.Contains(string(p.Body), ".gps") {
 				t.Fatalf("chat line %q", p.Body)

cmd/turtle-agent/commander_methods.go

@@ -376,33 +376,15 @@ func (g remoteGame) GMVisible(ctx context.Context, on bool) error {
 // gmSelectConfirmWait is how long the server is given to show a selection before a line that acts on it is given up on.
 var gmSelectConfirmWait = 3 * time.Second
 
-// selectConfirmed selects the unit and waits until the SERVER shows it as the agent's selection (the target field of the agent's own object,
-// which the core sends back after it handled CMSG_SET_SELECTION). The core queues that packet (map thread) apart from a GM's chat line (world
-// thread), so a line sent right after can run on the selection from before: none (the sender itself) or a stale one, which can be another
-// player. Run 6b: `.instance groupunbind all` answered "Glacienne is not in a group" for a selected Lcwarra. An error, with nothing sent, when
-// the server does not show the selection in time.
+// selectConfirmed selects the unit (0: none) and waits until the SERVER is known to have it in force as the agent's selection
+// (session.SelectConfirmed: an echo of the target field of the agent's own object that answers this select and not an older one, nor a
+// combat or stun rewrite of the field). The core queues that packet (map thread) apart from a GM's chat line (world thread), so a line sent
+// right after can run on the selection from before: none (the sender itself) or a stale one, which can be another player. Run 6b:
+// `.instance groupunbind all` answered "Glacienne is not in a group" for a selected Lcwarra; issue #296: a stale echo of the lead, with a
+// stranger's select pending behind it, let `.additem` run on the stranger. An error, with nothing sent, when the server does not show the
+// selection in time.
 func (g remoteGame) selectConfirmed(ctx context.Context, guid packet.GUID) error {
-	s := g.c.cur()
-	if _, err := s.Target(ctx, guid, ""); err != nil {
-		return err
-	}
-	timer := time.NewTimer(gmSelectConfirmWait)
-	defer timer.Stop()
-	tick := time.NewTicker(25 * time.Millisecond)
-	defer tick.Stop()
-	for {
-		sel, ok := s.ServerSelection()
-		if ok && sel == guid {
-			return nil
-		}
-		select {
-		case <-ctx.Done():
-			return ctx.Err()
-		case <-timer.C:
-			return fmt.Errorf("the server did not show %v as the agent's selection within %s (it shows %v): not sending a line that acts on the selection", guid, gmSelectConfirmWait, sel)
-		case <-tick.C:
-		}
-	}
+	return g.c.cur().SelectConfirmed(ctx, guid, gmSelectConfirmWait)
 }
 
 // gmSelectAttempts is how many times selectThenGM selects and confirms again when the selection changed after the confirmation.
@@ -567,28 +549,17 @@ func (g remoteGame) GoXYZ(ctx context.Context, mapID uint32, at dungeon.Point) e
 // around the commander. The selection is cleared first and the server is waited for to show it cleared, else the line would act on that
 // one unit alone (or fail on a player).
 func (g remoteGame) Respawn(ctx context.Context) error {
-	s := g.c.cur()
-	if err := s.ClearTarget(ctx); err != nil {
-		return err
-	}
-	timer := time.NewTimer(gmSelectConfirmWait)
-	defer timer.Stop()
-	tick := time.NewTicker(25 * time.Millisecond)
-	defer tick.Stop()
-	for {
-		if sel, ok := s.ServerSelection(); ok && sel == 0 {
-			break
-		}
-		select {
-		case <-ctx.Done():
-			return ctx.Err()
-		case <-timer.C:
-			sel, _ := s.ServerSelection()
-			return fmt.Errorf("the server did not show the selection cleared within %s (it shows %v): not sending .respawn", gmSelectConfirmWait, sel)
-		case <-tick.C:
-		}
+	return respawnUnselected(ctx, g.c.cur())
+}
+
+// respawnUnselected clears the selection, waits until the server is known to have it cleared (a stale echo of an earlier clear, or the
+// field cleared by the end of a fight, does not count), and sends ".respawn" only while that stands (selectThenGM with no unit).
+func respawnUnselected(ctx context.Context, s *session.Session) error {
+	g := remoteGame{c: &commanderService{cur: func() *session.Session { return s }}}
+	if err := g.selectThenGM(ctx, 0, ".respawn"); err != nil {
+		return fmt.Errorf("respawn with nothing selected: %w", err)
 	}
-	return s.GM(ctx, ".respawn")
+	return nil
 }
 
 // WalkSelf is walk_path for the agent's character.

cmd/turtle-agent/dungeon_leader_pulltest.go

@@ -2,8 +2,6 @@ package main
 
 import (
 	"context"
-	"fmt"
-	"time"
 
 	"github.com/pjerra/turtle-agent/internal/brain"
 	"github.com/pjerra/turtle-agent/internal/dungeon"
@@ -16,28 +14,7 @@ import (
 // Respawn sends the character's own ".respawn" with nothing selected (dungeon.Respawner), as the commander's does: the selection is cleared
 // first and the server is waited for to show it cleared, else the line would act on that one unit alone.
 func (g *sessionGame) Respawn(ctx context.Context) error {
-	s := g.cur()
-	if err := s.ClearTarget(ctx); err != nil {
-		return err
-	}
-	timer := time.NewTimer(gmSelectConfirmWait)
-	defer timer.Stop()
-	tick := time.NewTicker(25 * time.Millisecond)
-	defer tick.Stop()
-	for {
-		if sel, ok := s.ServerSelection(); ok && sel == 0 {
-			break
-		}
-		select {
-		case <-ctx.Done():
-			return ctx.Err()
-		case <-timer.C:
-			sel, _ := s.ServerSelection()
-			return fmt.Errorf("the server did not show the selection cleared within %s (it shows %v): not sending .respawn", gmSelectConfirmWait, sel)
-		case <-tick.C:
-		}
-	}
-	return s.GM(ctx, ".respawn")
+	return respawnUnselected(ctx, g.cur())
 }
 
 // CombatMark is the position of the event log now (dungeon.CombatMeter).