303 lines
12 KiB
Markdown
303 lines
12 KiB
Markdown
# 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)
|