internal/server/server.go

@@ -71,7 +71,17 @@ func New(
 	lifecycle.Append(fx.Hook{
 		OnStart: func(_ context.Context) error {
 			srv.startupTime = time.Now()
-			go srv.Run() //nolint:contextcheck
+			// Configure, enable Sentry, and build the router
+			// synchronously so that srv.router is fully initialized
+			// before OnStart returns. Any HTTP traffic (including
+			// httptest harnesses that wrap srv as a handler) is
+			// therefore guaranteed to see an initialized router,
+			// eliminating the previous race between SetupRoutes
+			// and ServeHTTP.
+			srv.configure()
+			srv.enableSentry()
+			srv.SetupRoutes()
+			go srv.serve() //nolint:contextcheck
 
 			return nil
 		},
@@ -83,10 +93,16 @@ func New(
 	return srv, nil
 }
 
-// Run starts the server configuration, Sentry, and begins serving.
+// Run configures the server and begins serving. It blocks
+// until shutdown is signalled. Kept for external callers
+// that embed the server outside fx. The fx lifecycle now
+// performs setup synchronously in OnStart and invokes
+// serve directly in a goroutine, so this is only used when
+// the server is driven by hand.
 func (srv *Server) Run() {
 	srv.configure()
 	srv.enableSentry()
+	srv.SetupRoutes()
 	srv.serve()
 }
 
@@ -202,8 +218,6 @@ func (srv *Server) serveUntilShutdown() {
 		Handler:        srv,
 	}
 
-	srv.SetupRoutes()
-
 	srv.log.Info(
 		"http begin listen", "listenaddr", listenAddr,
 	)

internal/service/service.go

@@ -814,28 +814,34 @@ func (s *Service) QueryUserMode(
 	return modes
 }
 
-// ApplyUserMode parses a mode string character by
-// character (e.g. "+wo", "-w") and applies each mode
-// change to the session. Returns the resulting mode string
-// after all changes, or an IRCError on failure.
+// userModeOp is a single parsed user-mode change collected
+// by parseUserModeString before any DB writes happen.
+type userModeOp struct {
+	char   rune
+	adding bool
+}
+
+// ApplyUserMode parses an IRC user-mode string and applies
+// the resulting changes atomically. It supports multiple
+// sign transitions (e.g. "+w-o", "-w+o", "+o-w+w") and
+// rejects malformed input (empty string, no leading sign,
+// bare sign with no mode letters, unknown mode letters,
+// +o which must be set via OPER) with an IRCError. On
+// failure, no persistent change is made. On success, the
+// resulting mode string is returned.
 func (s *Service) ApplyUserMode(
 	ctx context.Context,
 	sessionID int64,
 	modeStr string,
 ) (string, error) {
-	if len(modeStr) < 2 { //nolint:mnd // +/- prefix + ≥1 char
-		return "", &IRCError{
-			Code:    irc.ErrUmodeUnknownFlag,
-			Params:  nil,
-			Message: "Unknown MODE flag",
-		}
+	ops, err := parseUserModeString(modeStr)
+	if err != nil {
+		return "", err
 	}
 
-	adding := modeStr[0] == '+'
-
-	for _, ch := range modeStr[1:] {
+	for _, op := range ops {
 		if err := s.applySingleUserMode(
-			ctx, sessionID, ch, adding,
+			ctx, sessionID, op.char, op.adding,
 		); err != nil {
 			return "", err
 		}
@@ -844,7 +850,79 @@ func (s *Service) ApplyUserMode(
 	return s.QueryUserMode(ctx, sessionID), nil
 }
 
-// applySingleUserMode applies one user mode character.
+// parseUserModeString validates and parses a user-mode
+// string into a list of operations. The string must begin
+// with '+' or '-'; subsequent '+' / '-' characters flip the
+// active sign, and letters between them are applied with
+// the current sign. Every letter must be a recognized user
+// mode for this server, and '+o' is never allowed via MODE
+// (use OPER to become operator). If any character is
+// invalid, no operations are returned and an IRCError with
+// ERR_UMODEUNKNOWNFLAG (501) is returned.
+func parseUserModeString(
+	modeStr string,
+) ([]userModeOp, error) {
+	unknownFlag := &IRCError{
+		Code:    irc.ErrUmodeUnknownFlag,
+		Params:  nil,
+		Message: "Unknown MODE flag",
+	}
+
+	if modeStr == "" {
+		return nil, unknownFlag
+	}
+
+	first := modeStr[0]
+	if first != '+' && first != '-' {
+		return nil, unknownFlag
+	}
+
+	ops := make([]userModeOp, 0, len(modeStr)-1)
+	adding := true
+
+	for _, ch := range modeStr {
+		switch ch {
+		case '+':
+			adding = true
+		case '-':
+			adding = false
+		default:
+			if !isKnownUserModeChar(ch) {
+				return nil, unknownFlag
+			}
+
+			if ch == 'o' && adding {
+				return nil, unknownFlag
+			}
+
+			ops = append(ops, userModeOp{
+				char: ch, adding: adding,
+			})
+		}
+	}
+
+	if len(ops) == 0 {
+		return nil, unknownFlag
+	}
+
+	return ops, nil
+}
+
+// isKnownUserModeChar reports whether the character is a
+// recognized user mode letter.
+func isKnownUserModeChar(ch rune) bool {
+	switch ch {
+	case 'w', 'o':
+		return true
+	default:
+		return false
+	}
+}
+
+// applySingleUserMode applies one already-validated user
+// mode character to the session. parseUserModeString must
+// have validated the character and sign before this runs;
+// the default branch here is defence-in-depth only.
 func (s *Service) applySingleUserMode(
 	ctx context.Context,
 	sessionID int64,
@@ -864,7 +942,6 @@ func (s *Service) applySingleUserMode(
 			return fmt.Errorf("set wallops: %w", err)
 		}
 	case 'o':
-		// +o cannot be set via MODE; use OPER command.
 		if adding {
 			return &IRCError{
 				Code:    irc.ErrUmodeUnknownFlag,

internal/service/service_test.go

@@ -393,136 +393,260 @@ func TestQueryUserMode(t *testing.T) {
 	}
 }
 
-func TestApplyUserModeSingleChar(t *testing.T) {
-	env := newTestEnv(t)
-	ctx := t.Context()
-
-	sid := createSession(ctx, t, env.db, "alice")
-
-	// Apply +w.
-	result, err := env.svc.ApplyUserMode(ctx, sid, "+w")
-	if err != nil {
-		t.Fatalf("apply +w: %v", err)
+// TestApplyUserMode is the rigorous table-driven suite for
+// the shared user-mode parser. It covers every case listed
+// in sneak's review of PR #96 plus a few adjacent ones.
+// Each case asserts the resulting mode string AND the
+// persisted session state, to prove that rejected input
+// leaves no side effects.
+func TestApplyUserMode(t *testing.T) {
+	type caseState struct {
+		oper    bool
+		wallops bool
 	}
 
-	if result != "+w" {
-		t.Errorf("expected +w, got %s", result)
+	// Each case starts from initialState, calls
+	// ApplyUserMode(modeStr), and verifies either the
+	// returned mode string (wantModes) or an IRCError with
+	// wantErrCode. After the call, the DB must match
+	// wantState — proving that rejected input made no
+	// change.
+	cases := []struct {
+		name         string
+		initialState caseState
+		modeStr      string
+		wantModes    string
+		wantErr      bool
+		wantErrCode  irc.IRCMessageType
+		wantState    caseState
+	}{
+		// Happy path: single-char operations.
+		{
+			name:      "+w from empty",
+			modeStr:   "+w",
+			wantModes: "+w",
+			wantState: caseState{wallops: true},
+		},
+		{
+			name:         "-w from +w",
+			initialState: caseState{wallops: true},
+			modeStr:      "-w",
+			wantModes:    "+",
+			wantState:    caseState{},
+		},
+		{
+			name:         "-o from +o",
+			initialState: caseState{oper: true},
+			modeStr:      "-o",
+			wantModes:    "+",
+			wantState:    caseState{},
+		},
+		// Multi-char without sign transitions.
+		{
+			name:         "-wo from +ow",
+			initialState: caseState{oper: true, wallops: true},
+			modeStr:      "-wo",
+			wantModes:    "+",
+			wantState:    caseState{},
+		},
+		// Multi-char with sign transitions.
+		{
+			name:         "+w-o from +o",
+			initialState: caseState{oper: true},
+			modeStr:      "+w-o",
+			wantModes:    "+w",
+			wantState:    caseState{wallops: true},
+		},
+		{
+			name:         "-w+o always rejects +o",
+			initialState: caseState{wallops: true, oper: true},
+			modeStr:      "-w+o",
+			wantErr:      true,
+			wantErrCode:  irc.ErrUmodeUnknownFlag,
+			// +o is rejected before any op applies; wallops
+			// stays set.
+			wantState: caseState{wallops: true, oper: true},
+		},
+		{
+			name:         "+o-w+w rejects because of +o",
+			initialState: caseState{wallops: true, oper: true},
+			modeStr:      "+o-w+w",
+			wantErr:      true,
+			wantErrCode:  irc.ErrUmodeUnknownFlag,
+			// Wallops must NOT be cleared; oper must NOT be
+			// cleared. Rejection is fully atomic.
+			wantState: caseState{wallops: true, oper: true},
+		},
+		{
+			name:        "-x+y rejects unknown -x",
+			modeStr:     "-x+y",
+			wantErr:     true,
+			wantErrCode: irc.ErrUmodeUnknownFlag,
+			wantState:   caseState{},
+		},
+		{
+			name:        "+y-x rejects unknown +y",
+			modeStr:     "+y-x",
+			wantErr:     true,
+			wantErrCode: irc.ErrUmodeUnknownFlag,
+			wantState:   caseState{},
+		},
+		// Malformed prefix — must not silently treat as '-'.
+		{
+			name:        "w no prefix rejects",
+			modeStr:     "w",
+			wantErr:     true,
+			wantErrCode: irc.ErrUmodeUnknownFlag,
+			wantState:   caseState{},
+		},
+		{
+			name: "xw no prefix rejects (would have been" +
+				" silently -w before)",
+			initialState: caseState{wallops: true},
+			modeStr:      "xw",
+			wantErr:      true,
+			wantErrCode:  irc.ErrUmodeUnknownFlag,
+			// Prove wallops is NOT cleared — the whole point
+			// of sneak's review.
+			wantState: caseState{wallops: true},
+		},
+		{
+			name:        "empty string rejects",
+			modeStr:     "",
+			wantErr:     true,
+			wantErrCode: irc.ErrUmodeUnknownFlag,
+			wantState:   caseState{},
+		},
+		// Bare signs with no mode letters reject.
+		{
+			name:        "bare + rejects",
+			modeStr:     "+",
+			wantErr:     true,
+			wantErrCode: irc.ErrUmodeUnknownFlag,
+			wantState:   caseState{},
+		},
+		{
+			name:        "bare - rejects",
+			modeStr:     "-",
+			wantErr:     true,
+			wantErrCode: irc.ErrUmodeUnknownFlag,
+			wantState:   caseState{},
+		},
+		{
+			name:        "+-+ rejects (no mode letters)",
+			modeStr:     "+-+",
+			wantErr:     true,
+			wantErrCode: irc.ErrUmodeUnknownFlag,
+			wantState:   caseState{},
+		},
+		// Unknown mode letters reject whole string.
+		{
+			name:        "+z unknown mode rejects",
+			modeStr:     "+z",
+			wantErr:     true,
+			wantErrCode: irc.ErrUmodeUnknownFlag,
+			wantState:   caseState{},
+		},
+		{
+			name: "+wz rejects whole thing; +w side effect" +
+				" must NOT persist",
+			modeStr:     "+wz",
+			wantErr:     true,
+			wantErrCode: irc.ErrUmodeUnknownFlag,
+			// Wallops must NOT be set.
+			wantState: caseState{},
+		},
+		{
+			name: "+wo rejects whole thing; +w side effect" +
+				" must NOT persist",
+			modeStr:     "+wo",
+			wantErr:     true,
+			wantErrCode: irc.ErrUmodeUnknownFlag,
+			wantState:   caseState{},
+		},
 	}
 
-	// Apply -w.
-	result, err = env.svc.ApplyUserMode(ctx, sid, "-w")
-	if err != nil {
-		t.Fatalf("apply -w: %v", err)
-	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			env := newTestEnv(t)
+			ctx := t.Context()
+			sid := createSession(ctx, t, env.db, "alice")
 
-	if result != "+" {
-		t.Errorf("expected +, got %s", result)
-	}
-}
-
-func TestApplyUserModeMultiChar(t *testing.T) {
-	env := newTestEnv(t)
-	ctx := t.Context()
-
-	sid := createSession(ctx, t, env.db, "alice")
-
-	// Set oper first so we can test +wo (w applied, o
-	// rejected because +o is not allowed via MODE).
-	_ = env.db.SetSessionOper(ctx, sid, true)
-
-	// Apply +w alone should work.
-	result, err := env.svc.ApplyUserMode(ctx, sid, "+w")
-	if err != nil {
-		t.Fatalf("apply +w: %v", err)
-	}
-
-	if result != "+ow" {
-		t.Errorf("expected +ow, got %s", result)
-	}
-
-	// Reset wallops.
-	_ = env.db.SetSessionWallops(ctx, sid, false)
-
-	// Multi-char -ow: should de-oper and remove wallops.
-	_ = env.db.SetSessionWallops(ctx, sid, true)
-
-	result, err = env.svc.ApplyUserMode(ctx, sid, "-ow")
-	if err != nil {
-		t.Fatalf("apply -ow: %v", err)
-	}
-
-	if result != "+" {
-		t.Errorf("expected +, got %s", result)
-	}
-
-	// +wo should fail because +o is not allowed.
-	_, err = env.svc.ApplyUserMode(ctx, sid, "+wo")
-
-	var ircErr *service.IRCError
-	if !errors.As(err, &ircErr) {
-		t.Fatalf("expected IRCError, got %v", err)
-	}
-
-	if ircErr.Code != irc.ErrUmodeUnknownFlag {
-		t.Errorf(
-			"expected ErrUmodeUnknownFlag, got %d",
-			ircErr.Code,
-		)
-	}
-}
-
-func TestApplyUserModeInvalidInput(t *testing.T) {
-	env := newTestEnv(t)
-	ctx := t.Context()
-
-	sid := createSession(ctx, t, env.db, "alice")
-
-	// Too short.
-	_, err := env.svc.ApplyUserMode(ctx, sid, "+")
-
-	var ircErr *service.IRCError
-	if !errors.As(err, &ircErr) {
-		t.Fatalf("expected IRCError for short input, got %v", err)
-	}
-
-	// Unknown flag.
-	_, err = env.svc.ApplyUserMode(ctx, sid, "+x")
-	if !errors.As(err, &ircErr) {
-		t.Fatalf("expected IRCError for unknown flag, got %v", err)
-	}
-
-	if ircErr.Code != irc.ErrUmodeUnknownFlag {
-		t.Errorf(
-			"expected ErrUmodeUnknownFlag, got %d",
-			ircErr.Code,
-		)
-	}
-}
-
-func TestApplyUserModeDeoper(t *testing.T) {
-	env := newTestEnv(t)
-	ctx := t.Context()
-
-	sid := createSession(ctx, t, env.db, "alice")
-
-	// Make oper via DB directly.
-	_ = env.db.SetSessionOper(ctx, sid, true)
-
-	// -o should work.
-	result, err := env.svc.ApplyUserMode(ctx, sid, "-o")
-	if err != nil {
-		t.Fatalf("apply -o: %v", err)
-	}
-
-	if result != "+" {
-		t.Errorf("expected +, got %s", result)
-	}
-
-	// +o should fail.
-	_, err = env.svc.ApplyUserMode(ctx, sid, "+o")
-
-	var ircErr *service.IRCError
-	if !errors.As(err, &ircErr) {
-		t.Fatalf("expected IRCError for +o, got %v", err)
+			if tc.initialState.oper {
+				if err := env.db.SetSessionOper(
+					ctx, sid, true,
+				); err != nil {
+					t.Fatalf("init oper: %v", err)
+				}
+			}
+
+			if tc.initialState.wallops {
+				if err := env.db.SetSessionWallops(
+					ctx, sid, true,
+				); err != nil {
+					t.Fatalf("init wallops: %v", err)
+				}
+			}
+
+			result, err := env.svc.ApplyUserMode(
+				ctx, sid, tc.modeStr,
+			)
+
+			if tc.wantErr {
+				var ircErr *service.IRCError
+				if !errors.As(err, &ircErr) {
+					t.Fatalf(
+						"expected IRCError, got %v", err,
+					)
+				}
+
+				if ircErr.Code != tc.wantErrCode {
+					t.Errorf(
+						"code: want %d got %d",
+						tc.wantErrCode, ircErr.Code,
+					)
+				}
+			} else {
+				if err != nil {
+					t.Fatalf("unexpected error: %v", err)
+				}
+
+				if result != tc.wantModes {
+					t.Errorf(
+						"modes: want %q got %q",
+						tc.wantModes, result,
+					)
+				}
+			}
+
+			// Whether or not the call errored, the persisted
+			// state must match wantState — this is the
+			// atomicity guarantee sneak demanded.
+			gotOper, err := env.db.IsSessionOper(ctx, sid)
+			if err != nil {
+				t.Fatalf("read oper: %v", err)
+			}
+
+			gotWallops, err := env.db.IsSessionWallops(
+				ctx, sid,
+			)
+			if err != nil {
+				t.Fatalf("read wallops: %v", err)
+			}
+
+			if gotOper != tc.wantState.oper {
+				t.Errorf(
+					"oper: want %v got %v",
+					tc.wantState.oper, gotOper,
+				)
+			}
+
+			if gotWallops != tc.wantState.wallops {
+				t.Errorf(
+					"wallops: want %v got %v",
+					tc.wantState.wallops, gotWallops,
+				)
+			}
+		})
 	}
 }