fix: resolve tripleshot judge race that misreports attempt as failed (#663)

The bridge's gate.Complete() triggers TeamCompletedEvent (which
dispatches startJudge as a goroutine) before publishing
BridgeTaskCompletedEvent (which sets Attempts[i].Status). The
startJudge goroutine wins the race and snapshots the last attempt
as "working", causing the judge to always report one attempt failed.

Fix: eagerly set attempt status in onTeamCompleted under the lock,
before dispatching the goroutine. Also addressed pre-existing issues:

- Log discarded tmManager.Stop() error instead of _ =
- Extract abortStart() to stop already-started bridges on partial failure
- Upgrade SetMaxRetries failure log level from Warn to Error
- Skip redundant status/timestamp overwrite in onBridgeTaskCompleted
- Guard unknown team IDs in bridge event handlers

Author: Iron-Ham

CHANGELOG.md

@@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **Triple-Shot Judge Misreports Failed Attempt** - Fixed race condition where the judge always reported one attempt as failed despite all three succeeding. The bridge's `gate.Complete()` triggers `TeamCompletedEvent` (which dispatches `startJudge` as a goroutine) before publishing `BridgeTaskCompletedEvent` (which sets `Attempts[i].Status`). The `startJudge` goroutine would snapshot the last attempt's status as "working" before `onBridgeTaskCompleted` could set it to "completed". Fixed by eagerly setting attempt status in `onTeamCompleted` under the lock, before dispatching the goroutine. Also fixed: discarded `Stop()` error now logged, `SetMaxRetries` failures upgraded from Warn to Error, `onBridgeTaskCompleted` skips redundant status overwrites, and unknown team IDs are guarded in event handlers.
+
 - **Triple-Shot Spurious Second Pass** - Fixed duplicate instance creation in triple-shot workflows. Two root causes: (1) TaskQueue's `defaultMaxRetries=2` caused failed attempt/judge tasks to retry, spawning new instances. Fixed by calling `SetMaxRetries(taskID, 0)` after team creation. (2) Judge "merge" recommendations caused `json.Unmarshal` to fail when the LLM wrote `suggested_changes` as a string instead of `[]string`. The evaluation file parse failure triggered a retry, creating a second judge. Fixed by adding `FlexibleStringSlice` type (mirrors existing `FlexibleString`) to tolerate string/array mismatches in all LLM-parsed sentinel file structs (`Evaluation`, `AttemptEvaluationItem`, `AdversarialReviewFile`).
 
 - **Teamwire TUI Freeze** - Fixed TUI freeze when starting a triple-shot in teamwire mode. `coordinator.Start()` was called synchronously in the Bubble Tea `Update()` handler, blocking the event loop while bridges created git worktrees. Moved startup to an async `tea.Cmd` so the UI remains responsive during initialization.

internal/orchestrator/workflows/tripleshot/teamwire/AGENTS.md

@@ -32,6 +32,7 @@ TeamCoordinator
 - **Two-phase Start** — `Start()` must not hold `tc.mu` when calling `Bridge.Start()`. The bridge's claim loop publishes `BridgeTaskStartedEvent` synchronously, and the handler `onBridgeTaskStarted` acquires `tc.mu`. Holding the lock through `Start()` → bridge claim → event publish → handler → lock = deadlock. The fix: `registerStart()` holds/releases the lock, then `Start()` creates bridges outside it.
 - **Event subscription timing** — Subscriptions must happen before `Bridge.Start()` launches the claim loop. Currently done in `registerStart()` (Phase 1, under lock, before Phase 2 bridge creation) — this is the safe window. Don't move subscriptions after Phase 2 begins. For test assertions where you need events, subscribe before calling `Start()`. For production callbacks, use `SetCallbacks` before `Start`.
 - **`onTeamCompleted` dispatches to goroutine** — The handler for `team.completed` dispatches `startJudge()` via `go` to avoid deadlock. The synchronous event bus would block if `startJudge` tried to publish events while the bus's `Publish` goroutine holds a lock.
+- **`onTeamCompleted` must set attempt status eagerly** — The bridge calls `gate.Complete(taskID)` before publishing `BridgeTaskCompletedEvent`. `gate.Complete` triggers `TeamCompletedEvent` synchronously (via the team detecting all tasks done), which dispatches `go startJudge()`. `BridgeTaskCompletedEvent` fires next and runs `onBridgeTaskCompleted` (which sets `Attempts[i].Status`). Since `startJudge` runs in a separate goroutine, it races with `onBridgeTaskCompleted` for the last-completing attempt. To prevent `startJudge` from snapshotting a stale "working" status, `onTeamCompleted` sets `Attempts[i].Status` itself under the lock, before dispatching the goroutine.
 - **Retries disabled for tripleshot tasks** — `registerStart()` and `startJudge()` call `SetMaxRetries(taskID, 0)` to disable TaskQueue's default retry logic (`defaultMaxRetries=2`). Without this, failed attempt/judge tasks would return to Pending and spawn duplicate instances, appearing as a spurious "second pass." The triple-shot workflow has its own redundancy (3 independent attempts), so retrying individual tasks is counterproductive.
 - **Every `onJudgeCompleted` failure path must publish `TripleShotJudgeCompletedEvent`** — Use the `failJudge()` helper, which sets session error, transitions to `PhaseFailed`, fires callbacks, and publishes the event. Forgetting the event on one path breaks downstream listeners.
 - **Session mutation lock discipline** — `tsManager.Session()` returns a raw `*Session` pointer; the `tsManager.mu` RLock only protects the pointer swap, not field access. All session field reads *and* mutations (`JudgeID`, `CompletedAt`, `Error`, `Attempts[i].*`) must hold `tc.mu`. `GetWinningBranch()` also holds `tc.mu` for reads. The lock order `tc.mu → tsManager.mu` is safe (no reverse path exists). Functions like `failJudge` and `startJudge` error paths acquire `tc.mu` for mutations, then release before `notifyCallbacks`/`bus.Publish` to avoid deadlock.

internal/orchestrator/workflows/tripleshot/teamwire/teamcoordinator.go

@@ -154,22 +154,14 @@ func (tc *TeamCoordinator) Start(ctx context.Context) error {
 		teamID := tc.attemptTeamIDs[i]
 		t := mgr.Team(teamID)
 		if t == nil { // Coverage: defensive — Team() returns nil if concurrent Stop() clears state.
-			tc.mu.Lock()
-			tc.unsubscribeEvents()
-			tc.cancel()
-			tc.started = false
-			tc.mu.Unlock()
+			tc.abortStart()
 			return fmt.Errorf("teamcoordinator: team %q not found after AddTeam", teamID)
 		}
 
 		recorder := tc.buildAttemptRecorder(i)
 		b := bridge.New(t, factory, checker, recorder, tc.bus, tc.bridgeOpts...)
 		if err := b.Start(tc.ctx); err != nil { // Coverage: Bridge.Start only fails if already started.
-			tc.mu.Lock()
-			tc.unsubscribeEvents()
-			tc.cancel()
-			tc.started = false
-			tc.mu.Unlock()
+			tc.abortStart()
 			return fmt.Errorf("teamcoordinator: start bridge for %q: %w", teamID, err)
 		}
 
@@ -255,7 +247,7 @@ func (tc *TeamCoordinator) registerStart(ctx context.Context) (*team.Manager, er
 		if t != nil {
 			taskID := fmt.Sprintf("attempt-%d-task", i)
 			if err := t.Hub().TaskQueue().SetMaxRetries(taskID, 0); err != nil {
-				tc.logger.Warn("failed to disable retries for attempt task",
+				tc.logger.Error("failed to disable retries for attempt task",
 					"task_id", taskID, "error", err)
 			}
 		}
@@ -315,7 +307,26 @@ func (tc *TeamCoordinator) Stop() {
 
 	// Stop the team manager.
 	if tc.tmManager != nil {
-		_ = tc.tmManager.Stop()
+		if err := tc.tmManager.Stop(); err != nil {
+			tc.logger.Warn("failed to stop team manager", "error", err)
+		}
+	}
+}
+
+// abortStart cleans up partial state when the bridge-creation loop in Start()
+// encounters an error. It stops already-started bridges, unsubscribes events,
+// cancels the context, and marks the coordinator as not started.
+func (tc *TeamCoordinator) abortStart() {
+	tc.mu.Lock()
+	bridges := make([]*bridge.Bridge, len(tc.bridges))
+	copy(bridges, tc.bridges)
+	tc.bridges = nil
+	tc.unsubscribeEvents()
+	tc.cancel()
+	tc.started = false
+	tc.mu.Unlock()
+	for _, b := range bridges {
+		b.Stop()
 	}
 }
 
@@ -393,6 +404,21 @@ func (tc *TeamCoordinator) onTeamCompleted(tce event.TeamCompletedEvent) {
 	}
 	completed := tc.completedAttempts
 
+	// Set attempt status now, while we still hold the lock. The bridge
+	// publishes BridgeTaskCompletedEvent (which also sets this status) AFTER
+	// gate.Complete returns — but gate.Complete is what triggers this
+	// TeamCompletedEvent. If we wait for onBridgeTaskCompleted, startJudge
+	// (dispatched below as a goroutine) races with it and may snapshot the
+	// status as "working" instead of "completed".
+	session := tc.tsManager.Session()
+	now := time.Now()
+	session.Attempts[attemptIndex].CompletedAt = &now
+	if tce.Success {
+		session.Attempts[attemptIndex].Status = ts.AttemptStatusCompleted
+	} else {
+		session.Attempts[attemptIndex].Status = ts.AttemptStatusFailed
+	}
+
 	tc.logger.Info("attempt team completed",
 		"team_id", tce.TeamID,
 		"attempt_index", attemptIndex,
@@ -448,7 +474,11 @@ func (tc *TeamCoordinator) onBridgeTaskStarted(bse event.BridgeTaskStartedEvent)
 		}
 	}
 
-	// Must be the judge team.
+	if bse.TeamID != "judge" {
+		tc.logger.Warn("unexpected team ID in bridge.task_started", "team_id", bse.TeamID)
+		tc.mu.Unlock()
+		return
+	}
 	tc.mu.Unlock()
 	tc.notifyCallbacks(func(cb *ts.CoordinatorCallbacks) {
 		if cb.OnJudgeStart != nil {
@@ -467,20 +497,30 @@ func (tc *TeamCoordinator) onBridgeTaskCompleted(bce event.BridgeTaskCompletedEv
 
 	for i, id := range tc.attemptTeamIDs {
 		if bce.TeamID == id {
+			// Status and CompletedAt may already be set by onTeamCompleted
+			// (which fires before this handler for the last-completing attempt).
+			// Only update if not already terminal to avoid overwriting the
+			// earlier, more accurate CompletedAt timestamp.
 			session := tc.tsManager.Session()
-			now := time.Now()
-			session.Attempts[i].CompletedAt = &now
+			status := session.Attempts[i].Status
+			if status != ts.AttemptStatusCompleted && status != ts.AttemptStatusFailed {
+				now := time.Now()
+				session.Attempts[i].CompletedAt = &now
+				if bce.Success {
+					session.Attempts[i].Status = ts.AttemptStatusCompleted
+				} else {
+					session.Attempts[i].Status = ts.AttemptStatusFailed
+				}
+			}
 
 			if bce.Success {
-				session.Attempts[i].Status = ts.AttemptStatusCompleted
 				tc.mu.Unlock()
 				tc.notifyCallbacks(func(cb *ts.CoordinatorCallbacks) {
 					if cb.OnAttemptComplete != nil {
 						cb.OnAttemptComplete(i)
 					}
 				})
 			} else {
-				session.Attempts[i].Status = ts.AttemptStatusFailed
 				tc.mu.Unlock()
 				tc.notifyCallbacks(func(cb *ts.CoordinatorCallbacks) {
 					if cb.OnAttemptFailed != nil {
@@ -493,6 +533,11 @@ func (tc *TeamCoordinator) onBridgeTaskCompleted(bce event.BridgeTaskCompletedEv
 	}
 
 	// Judge team completion.
+	if bce.TeamID != "judge" {
+		tc.logger.Warn("unexpected team ID in bridge.task_completed", "team_id", bce.TeamID)
+		tc.mu.Unlock()
+		return
+	}
 	tc.mu.Unlock()
 	tc.onJudgeCompleted(bce)
 }
@@ -665,7 +710,7 @@ func (tc *TeamCoordinator) startJudge() {
 
 	// Disable retries for the judge task — same rationale as attempt tasks.
 	if err := judgeTeam.Hub().TaskQueue().SetMaxRetries("judge-task", 0); err != nil {
-		tc.logger.Warn("failed to disable retries for judge task", "error", err)
+		tc.logger.Error("failed to disable retries for judge task", "error", err)
 	}
 
 	factory := newAttemptFactory(tc.orch, tc.session)

internal/orchestrator/workflows/tripleshot/teamwire/teamcoordinator_test.go

@@ -971,6 +971,121 @@ func TestTeamCoordinator_OnTeamCompleted_UnknownTeam(t *testing.T) {
 	}
 }
 
+// TestTeamCoordinator_OnTeamCompleted_SetsAttemptStatus verifies that
+// onTeamCompleted sets the attempt status eagerly. Without this, startJudge
+// (dispatched as a goroutine from onTeamCompleted) races with
+// onBridgeTaskCompleted and may snapshot the last attempt as "working".
+func TestTeamCoordinator_OnTeamCompleted_SetsAttemptStatus(t *testing.T) {
+	setup := func(t *testing.T) *TeamCoordinator {
+		t.Helper()
+		bus := event.NewBus()
+		orch := newTestOrch(t)
+		session := newTestSession()
+
+		tc, _ := NewTeamCoordinator(TeamCoordinatorConfig{
+			Orchestrator: orch,
+			BaseSession:  session,
+			Bus:          bus,
+			BaseDir:      t.TempDir(),
+			Task:         "test task",
+		})
+
+		tc.mu.Lock()
+		tc.started = true
+		tc.attemptTeamIDs = [3]string{"attempt-0", "attempt-1", "attempt-2"}
+		tc.mu.Unlock()
+		return tc
+	}
+
+	t.Run("success", func(t *testing.T) {
+		tc := setup(t)
+
+		tce := event.NewTeamCompletedEvent("attempt-1", "Attempt 2", true, 1, 0)
+		tc.onTeamCompleted(tce)
+
+		// The status should be set immediately by onTeamCompleted, without
+		// needing onBridgeTaskCompleted to fire.
+		s := tc.Session()
+		if s.Attempts[1].Status != ts.AttemptStatusCompleted {
+			t.Errorf("Attempts[1].Status = %q, want %q", s.Attempts[1].Status, ts.AttemptStatusCompleted)
+		}
+		if s.Attempts[1].CompletedAt == nil {
+			t.Error("Attempts[1].CompletedAt is nil, want non-nil")
+		}
+		// Unrelated attempts should be unaffected.
+		if s.Attempts[0].Status != "" {
+			t.Errorf("Attempts[0].Status = %q, want empty", s.Attempts[0].Status)
+		}
+	})
+
+	t.Run("failure", func(t *testing.T) {
+		tc := setup(t)
+
+		tce := event.NewTeamCompletedEvent("attempt-2", "Attempt 3", false, 0, 1)
+		tc.onTeamCompleted(tce)
+
+		s := tc.Session()
+		if s.Attempts[2].Status != ts.AttemptStatusFailed {
+			t.Errorf("Attempts[2].Status = %q, want %q", s.Attempts[2].Status, ts.AttemptStatusFailed)
+		}
+		if s.Attempts[2].CompletedAt == nil {
+			t.Error("Attempts[2].CompletedAt is nil, want non-nil")
+		}
+	})
+}
+
+// TestTeamCoordinator_OnBridgeTaskCompleted_SkipsTerminalStatus verifies that
+// onBridgeTaskCompleted does not overwrite status/CompletedAt when the attempt
+// is already in a terminal state (set earlier by onTeamCompleted).
+func TestTeamCoordinator_OnBridgeTaskCompleted_SkipsTerminalStatus(t *testing.T) {
+	bus := event.NewBus()
+	orch := newTestOrch(t)
+	session := newTestSession()
+
+	tc, _ := NewTeamCoordinator(TeamCoordinatorConfig{
+		Orchestrator: orch,
+		BaseSession:  session,
+		Bus:          bus,
+		BaseDir:      t.TempDir(),
+		Task:         "test task",
+	})
+
+	completedCh := make(chan int, 1)
+	tc.SetCallbacks(&ts.CoordinatorCallbacks{
+		OnAttemptComplete: func(idx int) { completedCh <- idx },
+	})
+
+	tc.mu.Lock()
+	tc.started = true
+	tc.attemptTeamIDs = [3]string{"attempt-0", "attempt-1", "attempt-2"}
+	tc.mu.Unlock()
+
+	// Pre-set attempt-1 as completed (simulates onTeamCompleted having run first).
+	s := tc.Session()
+	earlyTime := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)
+	s.Attempts[1].Status = ts.AttemptStatusCompleted
+	s.Attempts[1].CompletedAt = &earlyTime
+
+	// Fire onBridgeTaskCompleted — should NOT overwrite the earlier timestamp.
+	bce := event.NewBridgeTaskCompletedEvent("attempt-1", "task-1", "inst-1", true, 1, "")
+	tc.onBridgeTaskCompleted(bce)
+
+	// Callback should still fire.
+	select {
+	case idx := <-completedCh:
+		if idx != 1 {
+			t.Errorf("callback index = %d, want 1", idx)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("timed out waiting for callback")
+	}
+
+	// CompletedAt should retain the earlier timestamp.
+	if !s.Attempts[1].CompletedAt.Equal(earlyTime) {
+		t.Errorf("CompletedAt was overwritten: got %v, want %v", s.Attempts[1].CompletedAt, earlyTime)
+	}
+}
+
 // startInfo captures an attempt start event for test assertions.
 type startInfo struct {
 	idx        int