feat: session ownership, role fix, SSRF fix, E2E suite

[Koopa0]

• Add owner_id to sessions (migration 000002)
• Fix denormalizeRole for multi-turn conversations
• Remove SafeTransport from admin-configured search client
• Configurable rate limiting (KOOPA_RATE_BURST)
• Add 208-assertion E2E test suite

[codereviewbot-ai]

Potential schema inconsistency:
Dropping the owner_id column without first removing any dependent constraints (e.g., foreign keys, triggers) may result in errors or leave the database in an inconsistent state. Ensure all dependencies are explicitly dropped before removing the column.

Recommended solution:
Add statements to drop any foreign key constraints or triggers related to owner_id prior to dropping the column:

ALTER TABLE sessions DROP CONSTRAINT IF EXISTS fk_sessions_owner_id;
-- Drop any triggers if applicable

[codereviewbot-ai]

Ambiguous Default Value for NOT NULL Column

Using DEFAULT '' for a NOT NULL column (owner_id) introduces ambiguity, as empty string is not a true NULL and may complicate queries and logic for orphaned sessions. Consider using NULL (with owner_id nullable) or a more explicit orphan marker (e.g., 'orphan').

Recommended solution:

ALTER TABLE sessions ADD COLUMN owner_id TEXT DEFAULT NULL;

Or, if orphan marker is needed:

ALTER TABLE sessions ADD COLUMN owner_id TEXT NOT NULL DEFAULT 'orphan';

[codereviewbot-ai]

Non-portable Index Definition (DESC)

The use of updated_at DESC in the index definition is not supported in all SQL dialects (e.g., SQLite). This may cause migration failures or ineffective indexing. Verify database compatibility or remove DESC if not supported.

Recommended solution:

CREATE INDEX idx_sessions_owner_id ON sessions(owner_id, updated_at);

[codereviewbot-ai]

Inconsistent Parameter Usage in CreateSession

The CreateSession query uses both $1 and sqlc.arg(owner_id) in the VALUES clause:

INSERT INTO sessions (title, owner_id)
VALUES ($1, sqlc.arg(owner_id))

This inconsistency can lead to parameter binding errors or confusion when generating code with sqlc. It is recommended to use a consistent parameter style, such as sqlc.arg(title) and sqlc.arg(owner_id), for clarity and maintainability.

Recommended fix:

INSERT INTO sessions (title, owner_id)
VALUES (sqlc.arg(title), sqlc.arg(owner_id))

[codereviewbot-ai]

Missing logging for denied session access

When session access is denied in the stream handler, the code returns a forbidden error to the client but does not log the denial. This omission reduces the ability to audit and debug unauthorized access attempts.

Recommendation:
Add a log entry before returning the error:

if !h.sessionAccessAllowed(r, parsedID) {
    h.logger.Warn("session access denied", "sessionId", sessionID, "msgId", msgID)
    WriteError(w, http.StatusForbidden, "forbidden", "session access denied", h.logger)
    return
}

[codereviewbot-ai]

Resource Cleanup on Error:
If store.CreateSession fails, the cleanup() function is not called, which may result in resource leaks. To ensure proper cleanup, call cleanup() before b.Fatalf:

if err != nil {
    cleanup()
    b.Fatalf("creating session: %v", err)
}

[codereviewbot-ai]

In the concurrent session creation test, if a goroutine panics before sending to the errs channel, the main goroutine could block indefinitely when reading from the channel. To improve robustness, consider wrapping the goroutine body in a defer func() { if r := recover(); r != nil { errs <- fmt.Errorf("panic: %v", r) } }() to ensure all errors, including panics, are reported and the channel is not left unread.

Recommended solution:

 go func(id int) {
     defer wg.Done()
     defer func() {
         if r := recover(); r != nil {
             errs <- fmt.Errorf("goroutine %d panic: %v", id, r)
         }
     }()
     // ... rest of the code ...
 }(i)

[codereviewbot-ai]

In the concurrent session deletion test, errors returned from store.Sessions and store.Session in the goroutines are ignored. This could mask underlying issues such as database errors or race conditions. For more robust testing, collect and check errors from these operations, possibly by sending them to a channel and asserting in the main test goroutine.

Recommended solution:

• Add error channels for each operation type and check for errors after wg.Wait().
• Example:

listErrs := make(chan error, numSessions)
getErrs := make(chan error, numSessions)
// ...
go func() {
    defer wg.Done()
    _, err := store.Sessions(ctx, "test-owner", 100, 0)
    if err != nil {
        listErrs <- err
    }
}()
// ...
// After wg.Wait():
for err := range listErrs { t.Errorf("list error: %v", err) }
for err := range getErrs { t.Errorf("get error: %v", err) }

[codereviewbot-ai]

Potential issue: Lack of error handling for malformed session rows in Sessions

In the loop over session rows (for i := range rows), the conversion function sqlcSessionsRowToSession is called, but there is no error handling or validation. If the conversion fails due to malformed data, it could propagate invalid sessions. Consider adding error handling similar to the Messages function, where malformed messages are skipped and logged:

for i := range rows {
    session, err := s.sqlcSessionsRowToSession(rows[i])
    if err != nil {
        s.logger.Warn("skipping malformed session", "session_id", rows[i].ID, "error", err)
        continue
    }
    sessions = append(sessions, session)
}

This will improve robustness and prevent propagation of invalid session data.

[codereviewbot-ai]

Maintainability concern: Role normalization/denormalization functions are not extensible

The normalizeRole and denormalizeRole functions use hardcoded string comparisons for role conversion. If new roles are introduced, these functions will require manual updates, which could lead to inconsistencies and maintenance issues. Consider using a map-based approach for role conversion to improve extensibility:

var roleMap = map[string]string{
    "model": "assistant",
    // add new roles here
}
func normalizeRole(role string) string {
    if dbRole, ok := roleMap[role]; ok {
        return dbRole
    }
    return role
}

This approach makes it easier to add new roles and maintain consistency.