Discord chat

[ptone]

Add discord chat support

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gemini-code-assist]

Code Review

This pull request implements a Discord message broker plugin for the Scion hub, enabling bidirectional communication with rich embeds, interactive components, rate-limited send queues, and per-agent webhook identities. Key feedback highlights a critical deadlock risk in the SendQueue when handling overflows under a mutex lock, a potential nil pointer dereference in Publish(), unsafe byte-based string slicing during modal response truncation, and silent migration failures in the SQLite store. Additionally, replacing the custom errorAs helper with the standard library's errors.As is recommended for better robustness.

[gemini-code-assist]

Critical Concurrency Bug: Potential Deadlock

When the queue is full, the default branch of the select statement is executed. Inside this branch, a blocking read dropped := <-cq.ch is performed while holding the mutex sq.mu.

If the worker goroutine concurrently drains the channel (for example, if maxSize is 1 and the worker reads the only item), the channel becomes empty. Consequently, <-cq.ch will block indefinitely while holding sq.mu. This deadlocks the entire SendQueue and halts all message sending across all channels.

Solution

Use a non-blocking select with a default case to read from cq.ch. If the channel was drained in the meantime, we simply proceed without dropping anything.

Suggested change

	select {
	case cq.ch <- req:
	default:
	dropped := <-cq.ch
	if dropped.result != nil {
	dropped.result <- &sendResult{err: errors.New("dropped: send queue overflow")}
	}
	sq.log.Warn("Send queue overflow, dropped oldest message",
	"channel_id", channelID, "queue_size", sq.maxSize)
	cq.ch <- req
	}
	select {
	case dropped := <-cq.ch:
	if dropped.result != nil {
	dropped.result <- &sendResult{err: errors.New("dropped: send queue overflow")}
	}
	sq.log.Warn("Send queue overflow, dropped oldest message",
	"channel_id", channelID, "queue_size", sq.maxSize)
	default:
	// The channel was drained by the worker in the meantime; no need to drop.
	}
	cq.ch <- req

[gemini-code-assist]

Potential Nil Pointer Dereference

Publish does not check if msg is nil at the beginning of the function. If msg is nil, it will panic with a nil pointer dereference at line 454 (msg.Sender) and other places.

Since the broker runs as a plugin subprocess, a panic will crash the plugin, causing the hub to lose connection to the Discord broker.

Solution

Add a nil check for msg at the top of Publish().

Suggested change

	if session == nil {
	return fmt.Errorf("discord broker not configured")
	}
	if session == nil {
	return fmt.Errorf("discord broker not configured")
	}
	if msg == nil {
	return fmt.Errorf("message is nil")
	}

[gemini-code-assist]

Import "errors"

Import the standard library "errors" package to use errors.As instead of the custom errorAs helper.

Suggested change

	import (
	"fmt"
	"log/slog"
	"net/http"
	"net/url"
	"strconv"
	"strings"
	"sync"
	"github.com/bwmarrin/discordgo"
	)
	import (
	"errors"
	"fmt"
	"log/slog"
	"net/http"
	"net/url"
	"strconv"
	"strings"
	"sync"
	"github.com/bwmarrin/discordgo"
	)

[gemini-code-assist]

Use errors.As

Use the standard library errors.As instead of the custom errorAs helper.

Suggested change

	var restErr *discordgo.RESTError
	if ok := errorAs(err, &restErr); ok {
	if restErr.Response != nil && restErr.Response.StatusCode == http.StatusNotFound {
	return true
	}
	// Discord error code 10015 = Unknown Webhook.
	if restErr.Message != nil && restErr.Message.Code == 10015 {
	return true
	}
	}
	var restErr *discordgo.RESTError
	if errors.As(err, &restErr) {
	if restErr.Response != nil && restErr.Response.StatusCode == http.StatusNotFound {
	return true
	}
	// Discord error code 10015 = Unknown Webhook.
	if restErr.Message != nil && restErr.Message.Code == 10015 {
	return true
	}
	}

[gemini-code-assist]

Use Standard Library errors.As

The custom errorAs function is implemented to avoid importing "errors". However, "errors" is already imported in other files, and the custom implementation does not support custom As(any) bool implementations (which is standard in Go's errors.As). This makes it less robust and prone to maintainability issues.

Solution

Import "errors" and use errors.As directly, then delete this custom helper.

[gemini-code-assist]

Use errors.As

Use the standard library errors.As instead of the custom errorAs helper.

Suggested change

	var restErr *discordgo.RESTError
	if ok := errorAs(err, &restErr); ok {
	return restErr.Response != nil && restErr.Response.StatusCode == statusCode
	}
	var restErr *discordgo.RESTError
	if errors.As(err, &restErr) {
	return restErr.Response != nil && restErr.Response.StatusCode == statusCode
	}

[gemini-code-assist]

Import "strings"

Import the standard library "strings" package to check for expected "duplicate column" errors during database migrations.

Suggested change

	import (
	"context"
	"database/sql"
	"encoding/json"
	"fmt"
	"time"
	_ "modernc.org/sqlite"
	)
	import (
	"context"
	"database/sql"
	"encoding/json"
	"fmt"
	"strings"
	"time"
	_ "modernc.org/sqlite"
	)

[gemini-code-assist]

Silent Failure of Database Migrations

s.db.Exec(m) is called without checking the returned error. While ignoring "duplicate column" errors is expected on subsequent startups, ignoring all other errors (e.g., database locked, disk full, permission denied) can lead to silent failures of database migrations and extremely hard-to-diagnose production issues.

Solution

Check the error and log a warning or return it if it's not a "duplicate column" error.

Suggested change

	func (s *sqliteStore) migrateSchema() {
	migrations := []string{
	`ALTER TABLE channel_links ADD COLUMN show_state_changes INTEGER NOT NULL DEFAULT 1`,
	}
	for _, m := range migrations {
	s.db.Exec(m)
	}
	}
	func (s *sqliteStore) migrateSchema() {
	migrations := []string{
	`ALTER TABLE channel_links ADD COLUMN show_state_changes INTEGER NOT NULL DEFAULT 1`,
	}
	for _, m := range migrations {
	if _, err := s.db.Exec(m); err != nil {
	// Ignore "duplicate column" errors as they are expected on subsequent startups.
	if !strings.Contains(err.Error(), "duplicate column name") {
	// Log or handle other unexpected errors.
	fmt.Printf("Warning: failed to run migration %q: %v\n", m, err)
	}
	}
	}
	}

[gemini-code-assist]

Unsafe String Slicing by Bytes

truncated = truncated[:97] + "..." slices the user-provided responseText by bytes. If the text contains multi-byte UTF-8 characters (like emojis or non-English text), this can slice in the middle of a UTF-8 rune, producing an invalid UTF-8 string.

Solution

Convert the string to a slice of runes first to ensure safe truncation.

Suggested change

	truncated := responseText
	if len(truncated) > 100 {
	truncated = truncated[:97] + "..."
	}
	truncated := responseText
	runes := []rune(responseText)
	if len(runes) > 100 {
	truncated = string(runes[:97]) + "..."
	}