fix ListCrewMembers

pkg/appview/storage/crew.go

@@ -3,6 +3,7 @@ package storage
 import (
 	"context"
 	"fmt"
+	"io"
 	"log/slog"
 	"net/http"
 
@@ -75,7 +76,12 @@ func requestCrewMembership(ctx context.Context, holdEndpoint, serviceToken strin
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated {
-		return fmt.Errorf("requestCrew failed with status %d", resp.StatusCode)
+		// Read response body to capture actual error message from hold
+		body, readErr := io.ReadAll(resp.Body)
+		if readErr != nil {
+			return fmt.Errorf("requestCrew failed with status %d (failed to read error body: %w)", resp.StatusCode, readErr)
+		}
+		return fmt.Errorf("requestCrew failed with status %d: %s", resp.StatusCode, string(body))
 	}
 
 	return nil

pkg/hold/pds/crew.go

@@ -3,6 +3,7 @@ package pds
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -122,9 +123,10 @@ func (p *HoldPDS) ListCrewMembers(ctx context.Context) ([]*CrewMemberWithKey, er
 
 	if err != nil {
 		// ErrDoneIterating is expected when we stop walking early
-		if err == repo.ErrDoneIterating {
+		// Use errors.Is to handle wrapped errors (indigo wraps with %w in MST walk)
+		if errors.Is(err, repo.ErrDoneIterating) {
 			// Successfully stopped at collection boundary
-		} else if err.Error() == "mst: not found" || strings.Contains(err.Error(), "not found") {
+		} else if strings.Contains(err.Error(), "not found") {
 			// If the collection doesn't exist yet (empty repo or no records created),
 			// return empty list instead of error
 			return []*CrewMemberWithKey{}, nil

pkg/hold/pds/xrpc_test.go

@@ -1289,6 +1289,201 @@ func TestHandleRequestCrew_MethodNotAllowed(t *testing.T) {
 	t.Skip("Method validation is now handled by chi router, not individual handlers")
 }
 
+// TestHandleRequestCrew_WithAuth tests successful crew membership request with authenticated user
+// This test exercises the complete flow including the ListCrewMembers path with empty crew list,
+// which previously caused a 500 error due to improper handling of wrapped ErrDoneIterating
+func TestHandleRequestCrew_WithAuth(t *testing.T) {
+	handler, ctx := setupTestXRPCHandler(t)
+
+	// Update captain record to allow all crew
+	_, err := handler.pds.UpdateCaptainRecord(ctx, true, true, false) // public=true, allowAllCrew=true
+	if err != nil {
+		t.Fatalf("Failed to update captain record: %v", err)
+	}
+
+	// Remove the bootstrap-created owner crew member to get an empty crew list
+	// (Bootstrap automatically adds the owner as a crew admin)
+	crewBefore, err := handler.pds.ListCrewMembers(ctx)
+	if err != nil {
+		t.Fatalf("Failed to list crew members before test: %v", err)
+	}
+	for _, member := range crewBefore {
+		if err := handler.pds.RemoveCrewMember(ctx, member.Rkey); err != nil {
+			t.Fatalf("Failed to remove bootstrap crew member: %v", err)
+		}
+	}
+
+	// Verify crew list is now empty (this is the critical condition that triggers the bug)
+	crewAfterCleanup, err := handler.pds.ListCrewMembers(ctx)
+	if err != nil {
+		t.Fatalf("Failed to list crew members after cleanup: %v", err)
+	}
+	if len(crewAfterCleanup) != 0 {
+		t.Fatalf("Expected empty crew list after cleanup, got %d members", len(crewAfterCleanup))
+	}
+
+	// Create authenticated user (injected into context to bypass auth middleware)
+	testUserDID := "did:plc:newuser123"
+	user := &ValidatedUser{
+		DID:        testUserDID,
+		Handle:     "newuser.test",
+		PDS:        "https://pds.test",
+		Authorized: true,
+	}
+
+	// Create request with user in context
+	body := map[string]any{
+		"role":        "member",
+		"permissions": []string{"blob:read", "blob:write"},
+	}
+	bodyBytes, _ := json.Marshal(body)
+	req := httptest.NewRequest(http.MethodPost, atproto.HoldRequestCrew, bytes.NewReader(bodyBytes))
+	req.Header.Set("Content-Type", "application/json")
+
+	// Inject user into request context
+	reqCtx := context.WithValue(req.Context(), contextKeyUser, user)
+	req = req.WithContext(reqCtx)
+
+	w := httptest.NewRecorder()
+
+	// Call handler - this should NOT return 500 even with empty crew list
+	handler.HandleRequestCrew(w, req)
+
+	// Verify successful response
+	if w.Code != http.StatusCreated {
+		t.Errorf("Expected status 201 Created, got %d", w.Code)
+		t.Logf("Response body: %s", w.Body.String())
+	}
+
+	// Verify response structure
+	var response map[string]any
+	if err := json.Unmarshal(w.Body.Bytes(), &response); err != nil {
+		t.Fatalf("Failed to parse response JSON: %v", err)
+	}
+
+	// Check response fields
+	if cid, ok := response["cid"].(string); !ok || cid == "" {
+		t.Error("Expected cid string in response")
+	}
+
+	if status, ok := response["status"].(string); !ok || status != "created" {
+		t.Errorf("Expected status='created', got %v", response["status"])
+	}
+
+	// Verify the crew member was actually added
+	crewAfter, err := handler.pds.ListCrewMembers(ctx)
+	if err != nil {
+		t.Fatalf("Failed to list crew members after request: %v", err)
+	}
+
+	if len(crewAfter) != 1 {
+		t.Fatalf("Expected 1 crew member after request, got %d", len(crewAfter))
+	}
+
+	// Verify it's the correct user
+	if crewAfter[0].Record.Member != testUserDID {
+		t.Errorf("Expected crew member DID %s, got %s", testUserDID, crewAfter[0].Record.Member)
+	}
+}
+
+// TestHandleRequestCrew_AlreadyMember tests requesting crew membership when already a member
+// Should return success without creating duplicate records
+func TestHandleRequestCrew_AlreadyMember(t *testing.T) {
+	handler, ctx := setupTestXRPCHandler(t)
+
+	// Update captain record to allow all crew
+	_, err := handler.pds.UpdateCaptainRecord(ctx, true, true, false)
+	if err != nil {
+		t.Fatalf("Failed to update captain record: %v", err)
+	}
+
+	// Pre-add the user as a crew member
+	testUserDID := "did:plc:existinguser123"
+	_, err = handler.pds.AddCrewMember(ctx, testUserDID, "member", []string{"blob:read", "blob:write"})
+	if err != nil {
+		t.Fatalf("Failed to pre-add crew member: %v", err)
+	}
+
+	// Verify crew list has our test user (+ the bootstrap owner)
+	crewBefore, err := handler.pds.ListCrewMembers(ctx)
+	if err != nil {
+		t.Fatalf("Failed to list crew members: %v", err)
+	}
+
+	// Find our test user in the crew list
+	var foundTestUser bool
+	var testUserCountBefore int
+	for _, member := range crewBefore {
+		if member.Record.Member == testUserDID {
+			foundTestUser = true
+			testUserCountBefore++
+		}
+	}
+	if !foundTestUser {
+		t.Fatalf("Expected to find test user %s in crew list", testUserDID)
+	}
+	if testUserCountBefore != 1 {
+		t.Fatalf("Expected test user to appear once in crew list, got %d times", testUserCountBefore)
+	}
+
+	// Create authenticated user (same DID as pre-added member)
+	user := &ValidatedUser{
+		DID:        testUserDID,
+		Handle:     "existinguser.test",
+		PDS:        "https://pds.test",
+		Authorized: true,
+	}
+
+	// Create request
+	req := httptest.NewRequest(http.MethodPost, atproto.HoldRequestCrew, nil)
+	reqCtx := context.WithValue(req.Context(), contextKeyUser, user)
+	req = req.WithContext(reqCtx)
+
+	w := httptest.NewRecorder()
+
+	// Call handler - should return success with "already_member" status
+	handler.HandleRequestCrew(w, req)
+
+	// Verify successful response (200 OK for already member)
+	if w.Code != http.StatusOK {
+		t.Errorf("Expected status 200 OK, got %d", w.Code)
+		t.Logf("Response body: %s", w.Body.String())
+	}
+
+	// Verify response indicates already a member
+	var response map[string]any
+	if err := json.Unmarshal(w.Body.Bytes(), &response); err != nil {
+		t.Fatalf("Failed to parse response JSON: %v", err)
+	}
+
+	if status, ok := response["status"].(string); !ok || status != "already_member" {
+		t.Errorf("Expected status='already_member', got %v", response["status"])
+	}
+
+	// Verify no duplicate was created
+	crewAfter, err := handler.pds.ListCrewMembers(ctx)
+	if err != nil {
+		t.Fatalf("Failed to list crew members after request: %v", err)
+	}
+
+	// Count how many times our test user appears
+	var testUserCountAfter int
+	for _, member := range crewAfter {
+		if member.Record.Member == testUserDID {
+			testUserCountAfter++
+		}
+	}
+
+	if testUserCountAfter != 1 {
+		t.Errorf("Expected test user to appear exactly once (no duplicate), got %d times", testUserCountAfter)
+	}
+
+	// Verify crew list size didn't change (no duplicates added)
+	if len(crewAfter) != len(crewBefore) {
+		t.Errorf("Expected crew list size to stay the same (%d), got %d", len(crewBefore), len(crewAfter))
+	}
+}
+
 // Tests for DID document endpoints
 
 // TestHandleDIDDocument tests /.well-known/did.json endpoint