Changeset 0.22.0 (#93)

VERSION

@@ -1 +1 @@
-0.21.6
+0.21.7

docs/AUDIT-0.22.0-PREFLIGHT.md

@@ -0,0 +1,302 @@
+# Pre-Flight Audit — v0.22.0 Kickoff
+
+**Date:** 2026-03-01
+**Current version:** 0.21.6
+**Test instance:** `switchboard-be-test` in `gobha-ai-chat` namespace
+**Schema:** `012_v0214_git.sql` — up to date
+
+---
+
+## 1. Bug: Channel List Corruption (SafeJSON 500)
+
+### Symptoms
+
+```
+23:24:44 ⚠ SafeJSON: marshal failed: json: error calling MarshalJSON for type
+  json.RawMessage: invalid character 'i' looking for beginning of value
+23:24:44 | 500 | GET "/test/api/v1/channels?page=1&per_page=100&type=direct"
+```
+
+One channel in the test database has invalid JSON in its `settings` column. The
+content starts with the character `i` (likely a bare word like `invalid`,
+`inactive`, or `in_progress` — not valid JSON). This kills the entire paginated
+channel list with a 500.
+
+### Root Cause Analysis
+
+**Defense layer 1 — `scanJSON`**: The `ListChannels` scan path at
+`channels.go:236` correctly uses `scanJSON(&ch.Settings)`, which calls
+`json.Valid()` and defaults to `{}` on failure. This should catch corrupt data.
+
+**Defense layer 2 — `SafeJSON`**: Pre-marshals the entire response and returns
+a clean 500 instead of truncated output. This layer IS firing, meaning layer 1
+did NOT catch the problem.
+
+**Inconsistent scan paths**: The Postgres `CreateChannel` RETURNING path at
+`channels.go:323` scans `&ch.Settings` directly WITHOUT `scanJSON`:
+
+```go
+// line 323 — Postgres CreateChannel
+pq.Array(&tags), &ch.Settings, &ch.CreatedAt, &ch.UpdatedAt,
+```
+
+Compare to the SQLite path at line 306 which correctly uses `scanJSON`.
+
+**How the data got corrupted**: Unknown without DB access. Postgres JSONB
+columns reject invalid JSON at write time (`::jsonb` cast), so the corruption
+likely predates the current schema — possibly from a migration, manual DB edit,
+or an older code version that used TEXT instead of JSONB. Run this to find it:
+
+```sql
+SELECT id, title, settings
+FROM channels
+WHERE user_id = '664c71e8-cd8b-47fb-b421-9c48abb815ba'
+  AND settings::text NOT LIKE '{%'
+  AND settings::text NOT LIKE '[%'
+  AND settings::text NOT LIKE '"null"';
+```
+
+### Fixes Required
+
+**P0 — Fix the data** (immediate):
+```sql
+UPDATE channels
+SET settings = '{}'::jsonb
+WHERE settings IS NULL
+   OR NOT (settings::text ~ '^[\[{"]');
+```
+
+**P0 — Consistent scanJSON usage** (`channels.go:323`):
+```go
+// Before (Postgres CreateChannel):
+pq.Array(&tags), &ch.Settings, &ch.CreatedAt, &ch.UpdatedAt,
+
+// After:
+pq.Array(&tags), scanJSON(&ch.Settings), &ch.CreatedAt, &ch.UpdatedAt,
+```
+
+**P1 — Graceful list degradation**: When `SafeJSON` encounters a corrupt item
+in a paginated list, the current behavior drops the entire response. Instead,
+add a `SafeMarshalList` helper that:
+1. Marshals items individually
+2. Skips corrupt items with a warning log (include the row ID)
+3. Returns the healthy items with a `warnings` field in the response
+
+**P1 — Input validation on settings write** (`channels.go:487`):
+```go
+if req.Settings != nil {
+    if !json.Valid([]byte(*req.Settings)) {
+        c.JSON(http.StatusBadRequest, gin.H{"error": "settings must be valid JSON"})
+        return
+    }
+    // ... existing merge logic
+}
+```
+
+**P2 — Audit all RawMessage scan sites**: Search for `&ch.Settings`,
+`&*.Settings`, or any `json.RawMessage` scan that doesn't go through
+`scanJSON`. Every JSONB column scanned into `json.RawMessage` must use the
+wrapper.
+
+### Additional Log Noise
+
+The git endpoints are returning 400s for workspaces without git repos:
+
+```
+23:23:40 | 400 | GET ".../git/status"
+23:23:56 | 400 | GET ".../git/branches"
+```
+
+These are expected (workspace has no `.git` directory), but the frontend should
+suppress these calls when the workspace has no `git_remote_url` configured.
+Frontend fix: check `workspace.git_remote_url` before calling git endpoints.
+
+---
+
+## 2. CI Variables → Deployment Audit
+
+Cross-referencing CI secrets/variables (from Gitea) against `backend.yaml`,
+`config.go`, and runtime logs.
+
+### Secrets (Gitea Secrets → K8s Secrets)
+
+| CI Secret | K8s Secret | Key | Status |
+|-----------|------------|-----|--------|
+| ENCRYPTION_KEY | switchboard-encryption | ENCRYPTION_KEY | ✅ Active |
+| POSTGRES_USER | switchboard-db-credentials | POSTGRES_USER | ✅ Active |
+| POSTGRES_PASSWORD | switchboard-db-credentials | POSTGRES_PASSWORD | ✅ Active |
+| POSTGRES_ADMIN_USER | — | — | ⚠ Unused by app — likely for CI DB bootstrap |
+| POSTGRES_ADMIN_PASSWORD | — | — | ⚠ Unused by app — likely for CI DB bootstrap |
+| S3_ACCESS_KEY | switchboard-s3 | S3_ACCESS_KEY | ✅ Active |
+| S3_SECRET_KEY | switchboard-s3 | S3_SECRET_KEY | ✅ Active |
+| SWITCHBOARD_ADMIN_EMAIL | switchboard-admin | email | ✅ Active |
+| SWITCHBOARD_ADMIN_PASSWORD | switchboard-admin | password | ✅ Active |
+| SWITCHBOARD_ADMIN_USERNAME | switchboard-admin | username | ✅ Active |
+
+### Variables (Gitea Variables → envsubst / K8s env)
+
+| CI Variable | Used In | Value | Status |
+|-------------|---------|-------|--------|
+| DOMAIN | ingress.yaml | gobha.ai | ✅ |
+| NAMESPACE | all manifests | gobha-ai-chat | ✅ |
+| POSTGRES_HOST | backend.yaml env | postgres-primary.postgres.svc.cluster.local | ✅ |
+| PROVIDER | ? | venice | ⚠ **Orphan** — not referenced in any manifest or config.go |
+| S3_BUCKET | switchboard-s3 secret | integration-test | ✅ |
+| S3_ENDPOINT | switchboard-s3 secret | http://192.168.10.29:7480 | ✅ |
+| SEED_PROVIDERS | backend.yaml env (direct) | venice:...:micro | ✅ New (Mar 1) |
+| SEED_USERS | switchboard-seed-users secret | darthvader:...,lukeskywalker:... | ✅ |
+| STORAGE_BACKEND | backend.yaml env | s3 | ✅ |
+| STORAGE_CLASS | storage-pvc.yaml | cephfs | ✅ |
+| STORAGE_SIZE | storage-pvc.yaml | (visible, value cut off) | ✅ |
+
+### Variables Expected by backend.yaml but NOT in CI Screenshots
+
+These are likely set in the CI pipeline YAML directly (not as repo variables):
+
+| Variable | Used In | Expected Source |
+|----------|---------|-----------------|
+| ENVIRONMENT | backend.yaml env | Pipeline per-branch |
+| GIN_MODE | backend.yaml env | Pipeline (release/debug) |
+| BASE_PATH | backend.yaml env | Pipeline per-deploy (/test, /prod, etc.) |
+| POSTGRES_PORT | backend.yaml env | Default 5432 (config.go fallback) |
+| DB_NAME | backend.yaml env | Pipeline per-deploy |
+| DEPLOY_SUFFIX | all manifests | Pipeline (-test, -prod, etc.) |
+| BE_IMAGE | backend.yaml image | Pipeline / registry |
+| IMAGE_TAG | backend.yaml image | Pipeline / git SHA |
+| BE_REPLICAS | backend.yaml replicas | Pipeline |
+| BE_MEMORY_REQUEST | backend.yaml resources | Pipeline |
+| BE_MEMORY_LIMIT | backend.yaml resources | Pipeline |
+| BE_CPU_REQUEST | backend.yaml resources | Pipeline |
+| BE_CPU_LIMIT | backend.yaml resources | Pipeline |
+| EXTRACTION_CONCURRENCY | backend.yaml env | Pipeline (default 3) |
+
+### Issues Found
+
+1. **`PROVIDER` variable is orphaned.** Value is "venice" but nothing in
+   backend.yaml or config.go references it. Likely a legacy variable from
+   before `SEED_PROVIDERS` was added. Clean up or document.
+
+2. **`SEED_PROVIDERS` contains an API key in a CI Variable (not Secret).**
+   The screenshot shows the Venice API key visible in plaintext in the Variables
+   section. This should be moved to Gitea Secrets for proper masking. The
+   backend.yaml needs a corresponding mapping (currently SEED_PROVIDERS may be
+   injected via envsubst or direct env, verify the CI pipeline).
+
+3. **`SEED_USERS` passwords are plaintext in CI.** `password1234` for both
+   test users. Acceptable for test environment but should be documented as
+   test-only. Production pipeline should NOT have SEED_USERS set at all (and
+   `SeedProviders` already blocks production).
+
+4. **Missing `SEED_PROVIDERS` in backend.yaml env block.** The K8s manifest
+   doesn't have a `SEED_PROVIDERS` env var entry. It works because the CI
+   pipeline likely injects it via envsubst into a direct env entry, but this
+   should be made explicit in backend.yaml for documentation parity.
+
+5. **No `JWT_SECRET` in CI secrets.** Config.go defaults to
+   `"dev-secret-change-me"`. The test instance may be running with this
+   default. Add a proper secret for test environments.
+
+6. **No `S3_REGION` or `S3_PREFIX` in CI.** Config.go defaults to
+   `us-east-1` and `""` respectively. Fine for Ceph RGW but should be
+   documented.
+
+### Recommendations
+
+- Move `SEED_PROVIDERS` from CI Variable to CI Secret
+- Remove or document the orphaned `PROVIDER` variable
+- Add `JWT_SECRET` to CI Secrets and wire through backend.yaml
+- Add `SEED_PROVIDERS` env entry to backend.yaml (with secretKeyRef)
+- Add a CI pipeline comment documenting which variables are set inline
+  vs. in repo settings
+
+---
+
+## 3. Test Hardening — Unhappy Paths
+
+Current test coverage is strong on the happy path (164+ integration tests).
+The corruption bug exposes gaps in unhappy-path coverage.
+
+### Missing Test Categories
+
+**A. Data corruption resilience** (integration_test.go):
+
+- [ ] **Corrupt JSONB in channels**: INSERT a channel with valid settings, then
+  UPDATE settings directly via SQL with invalid text. Verify GET /channels
+  still returns 200 with the corrupt channel included (settings defaulted to
+  `{}`).
+- [ ] **Corrupt JSONB in projects**: Same pattern for project settings.
+- [ ] **Corrupt JSONB in teams**: Same for team settings/policies.
+- [ ] **NULL settings column**: Verify channels with NULL settings serialize
+  correctly (should be `{}` or `null`, not crash).
+- [ ] **Corrupt tags column**: Insert channel with malformed tags array.
+  Verify list doesn't 500.
+
+**B. Settings write validation**:
+
+- [ ] **Invalid JSON in channel settings update**: PUT /channels/:id with
+  `{"settings": "not-json"}` — should 400, not write to DB.
+- [ ] **Oversized settings**: PUT with a 10MB settings blob — should be
+  rejected.
+- [ ] **SQL injection via settings merge**: PUT with settings containing SQL
+  metacharacters — verify parameterized query prevents injection.
+
+**C. Scan alignment tests** (safe_json_test.go):
+
+- [ ] **Column count mismatch**: Mock a query result with wrong column count,
+  verify clean error instead of panic.
+- [ ] **Type mismatch**: Scan a TEXT column into json.RawMessage without
+  scanJSON — verify SafeJSON catches it.
+- [ ] **Driver buffer reuse**: Scan multiple rows with RawMessage, verify each
+  row's data is independent (copy, not reference).
+
+**D. Startup resilience** (config/seed tests):
+
+- [ ] **SEED_PROVIDERS with invalid format**: `"openai"` (no key), `":"`,
+  `":::"`, empty string — verify graceful skip.
+- [ ] **SEED_PROVIDERS with unknown provider**: `"unknown:key123"` — verify
+  warning log, not crash.
+- [ ] **SEED_USERS with invalid format**: Malformed CSV — verify skip.
+- [ ] **DATABASE_URL with unreachable host**: Verify startup logs error and
+  serves 503 on API endpoints.
+
+**E. WebSocket reconnection**:
+
+- [ ] **Rapid disconnect/reconnect**: The logs show multiple ws connect/
+  disconnect cycles in quick succession. Verify no goroutine leaks,
+  no duplicate event delivery.
+- [ ] **Auth token expiry during active connection**: Verify clean disconnect
+  with appropriate error, not silent data loss.
+
+**F. Frontend error handling** (manual or e2e):
+
+- [ ] **500 on channel list**: Frontend should show error state, not blank
+  screen. Currently unknown behavior.
+- [ ] **400 on git endpoints for non-git workspace**: Frontend should not
+  call git endpoints when workspace has no git_remote_url.
+- [ ] **Network timeout during streaming completion**: Verify partial
+  response is preserved, user can retry.
+
+### Test Infrastructure Needed
+
+1. **SQL fixture injection**: Helper function to INSERT corrupt data directly
+   via SQL, bypassing store layer validation. For corruption resilience tests.
+
+2. **SafeJSON audit script**: `grep -rn '&.*\.Settings\|json.RawMessage'` to
+   find all RawMessage scan sites. Run as CI lint check to catch regressions.
+
+3. **Integration test for each migration**: Both fresh-install path AND
+   upgrade path from previous version. Currently tested manually — automate.
+
+---
+
+## 4. Quick Wins Before v0.22.0 Work Starts
+
+Priority-ordered items that should land as v0.21.7 patches:
+
+1. **Fix corrupt data in test DB** — SQL UPDATE (5 min)
+2. **Add scanJSON to CreateChannel Postgres path** — one-line fix (5 min)
+3. **Add json.Valid check on settings write** — channels.go UpdateChannel (10 min)
+4. **Move SEED_PROVIDERS to CI Secret** — CI UI change (5 min)
+5. **Suppress git endpoint calls for non-git workspaces** — frontend (15 min)
+6. **Add corruption resilience integration tests** — 2-3 tests (1 hr)
+7. **Add JWT_SECRET to CI Secrets** — CI + backend.yaml (15 min)

server/handlers/channels.go

@@ -320,7 +320,7 @@ func (h *ChannelHandler) CreateChannel(c *gin.Context) {
 		).Scan(
 			&ch.ID, &ch.UserID, &ch.Title, &ch.Type, &ch.Description, &ch.Model, &ch.APIConfigID,
 			&ch.SystemPrompt, &ch.IsArchived, &ch.IsPinned, &ch.Folder, &ch.ProjectID, &ch.WorkspaceID,
-			pq.Array(&tags), &ch.Settings, &ch.CreatedAt, &ch.UpdatedAt,
+			pq.Array(&tags), scanJSON(&ch.Settings), &ch.CreatedAt, &ch.UpdatedAt,
 		)
 		if err != nil {
 			c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to create channel"})
@@ -478,6 +478,11 @@ func (h *ChannelHandler) UpdateChannel(c *gin.Context) {
 		addClause("tags", writeTagsArg(req.Tags))
 	}
 	if req.Settings != nil {
+		// Validate settings is well-formed JSON before writing
+		if !json.Valid([]byte(*req.Settings)) {
+			c.JSON(http.StatusBadRequest, gin.H{"error": "settings must be valid JSON"})
+			return
+		}
 		// JSONB merge: new settings keys overwrite existing, unmentioned keys preserved
 		if database.IsSQLite() {
 			setClauses = append(setClauses, "settings = json_patch(settings, ?)")

server/handlers/safe_json.go

@@ -46,7 +46,15 @@ func (s *jsonScanner) Scan(src interface{}) error {
 			*s.dest = json.RawMessage("{}")
 			return nil
 		}
-		*s.dest = json.RawMessage(v)
+		// CRITICAL: copy the driver buffer. json.RawMessage(v) aliases the
+		// underlying []byte owned by database/sql. The driver may reuse that
+		// memory on the next rows.Scan() call, corrupting our data after the
+		// fact. This caused intermittent SafeJSON 500s on paginated list
+		// endpoints where row N's Settings pointed to row N+1's overwritten
+		// buffer. Fixed in v0.21.7.
+		cp := make([]byte, len(v))
+		copy(cp, v)
+		*s.dest = json.RawMessage(cp)
 	case string:
 		b := []byte(v)
 		if len(b) == 0 || !json.Valid(b) {

server/handlers/safe_json_test.go

@@ -183,3 +183,70 @@ func TestSafeJSON_PaginatedWithCorrupt(t *testing.T) {
 		t.Fatalf("expected 500 for paginated response with corrupt item, got %d", w.Code)
 	}
 }
+
+// ── Driver buffer aliasing tests ────────────
+// These verify the fix for the v0.21.7 corruption bug: scanJSON must
+// copy []byte from the database driver, not alias it. Without the copy,
+// rows.Next() can overwrite the buffer and corrupt previously-scanned
+// json.RawMessage values in a paginated list.
+
+func TestScanJSON_ByteSliceNotAliased(t *testing.T) {
+	var dest json.RawMessage
+	s := scanJSON(&dest)
+
+	// Simulate driver buffer with valid JSON
+	buf := []byte(`{"key":"value"}`)
+	if err := s.Scan(buf); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// Simulate driver reusing the buffer on next rows.Scan()
+	copy(buf, []byte(`invalid_json!!`))
+
+	// dest must still hold the original valid value
+	if !json.Valid(dest) {
+		t.Fatalf("dest corrupted after source buffer reuse: %q", dest)
+	}
+	if string(dest) != `{"key":"value"}` {
+		t.Fatalf("expected original value, got %q", dest)
+	}
+}
+
+func TestScanJSON_ByteSliceIsolation_MultiRow(t *testing.T) {
+	// Simulate scanning two rows through the same driver buffer,
+	// as happens in ListChannels when lib/pq reuses memory.
+	var dest1, dest2 json.RawMessage
+
+	// Row 1
+	buf := make([]byte, 64)
+	row1 := []byte(`{"row":1,"selector":"abc"}`)
+	copy(buf, row1)
+	s1 := scanJSON(&dest1)
+	if err := s1.Scan(buf[:len(row1)]); err != nil {
+		t.Fatalf("row 1 scan error: %v", err)
+	}
+
+	// Row 2 — driver reuses same buffer
+	row2 := []byte(`{"row":2}`)
+	copy(buf, row2)
+	s2 := scanJSON(&dest2)
+	if err := s2.Scan(buf[:len(row2)]); err != nil {
+		t.Fatalf("row 2 scan error: %v", err)
+	}
+
+	// Both must retain their original values
+	if string(dest1) != `{"row":1,"selector":"abc"}` {
+		t.Fatalf("dest1 corrupted by row 2 scan: %q", dest1)
+	}
+	if string(dest2) != `{"row":2}` {
+		t.Fatalf("dest2 unexpected value: %q", dest2)
+	}
+
+	// Both must be independently valid JSON
+	if !json.Valid(dest1) {
+		t.Fatalf("dest1 not valid JSON after multi-row scan: %q", dest1)
+	}
+	if !json.Valid(dest2) {
+		t.Fatalf("dest2 not valid JSON after multi-row scan: %q", dest2)
+	}
+}