feat: model settings and system configs

[hanakannzashi]

Resolves #65 #79 #80

[Copilot]

Pull request overview

This PR introduces a model settings feature that allows administrators to control model visibility through a "public" flag. The implementation follows a ports-and-adapters architecture with comprehensive CRUD operations for model settings stored as JSONB in PostgreSQL.

Key Changes:

• Added database migration and repository for storing model settings as flexible JSONB content
• Implemented service layer with full and partial update capabilities
• Created admin-only API endpoints for managing model settings (GET, POST, PATCH)
• Integrated model visibility enforcement in proxy endpoints to restrict non-public models
• Added model list decoration to expose public flags to clients

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
crates/database/src/migrations/sql/V12__add_model_settings.sql	Creates model_settings table with JSONB content column and auto-update trigger
crates/database/src/repositories/model_settings_repository.rs	Implements PostgreSQL repository with upsert operations and default value merging
crates/database/src/repositories/mod.rs	Exports PostgresModelSettingsRepository
crates/database/src/lib.rs	Integrates model_settings_repository into Database struct and adds accessor method
crates/services/src/settings/mod.rs	Module definition for settings domain
crates/services/src/settings/ports.rs	Defines traits and data structures for model settings operations
crates/services/src/settings/service.rs	Implements ModelSettingsService with get, update, and partial update methods
crates/services/src/lib.rs	Exports settings module
crates/api/src/state.rs	Adds model_settings_service to AppState
crates/api/src/models.rs	Defines API request/response models for model settings
crates/api/src/routes/admin.rs	Implements three admin endpoints for getting and updating model settings
crates/api/src/routes/api.rs	Adds model visibility enforcement in proxy_responses and public flag decoration in proxy_model_list
crates/api/src/openapi.rs	Registers model settings endpoints and models in OpenAPI spec
crates/api/src/main.rs	Wires up model_settings_service in application initialization
crates/api/tests/common.rs	Updates test server setup to include model_settings_service
crates/api/tests/model_settings_tests.rs	Adds integration tests for admin endpoints and authorization

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[think-in-universe]

@claude review

[claude]

Code Review - PR #74

Warning CRITICAL ISSUES FOUND

Critical Issues

1. Race Condition: Cache Invalidation Before DB Write
Location: crates/api/src/routes/admin.rs lines 401-411, 474-484, 532-542

Cache is cleared BEFORE database operations. If DB write fails, cache remains invalidated causing stale data.

Fix: Move cache invalidation AFTER successful DB operation.

2. Panic in Request Hot Path
Location: crates/api/src/routes/api.rs line ~1251

.unwrap() on parse() will crash server. Use proper error handling.

3. Missing Transaction Boundaries
Location: crates/database/src/repositories/model_repository.rs:140-171, system_configs_repository.rs:50-80

Update operations use read-modify-write WITHOUT transactions. Concurrent updates cause lost writes.

Fix: Use PostgreSQL jsonb merge in atomic UPDATE or wrap in transaction.

High Priority

4. Cache Stampede - 100 concurrent requests on cache miss all hit DB
5. Blocking SHA-256 - CPU work blocks async runtime, use spawn_blocking
6. Silent Error Swallowing - unwrap_or_default() hides JSON parse failures

Summary

Must fix before merge: Issues 1, 2, 3
Should fix: Issues 4, 5, 6

Status: CHANGES REQUESTED

[think-in-universe]

@claude review

[claude]

PR Review: Model Settings and System Configs

I've analyzed PR #74 for critical issues.

CRITICAL ISSUES FOUND

1. Race Condition in Cache Invalidation (Production Safety)
Location: crates/api/src/routes/admin.rs lines 410, 490, 540

In multi-instance deployments, Instance A updates DB and clears its cache, but Instance B still has stale data for up to 5 minutes. Non-public models could become accessible, wrong system prompts could be injected.

Fix: Use distributed cache with pub/sub, reduce TTL to 30s, or document limitation.

2. Unsafe Header Append (Logic Error)
Location: crates/api/src/routes/api.rs around line 1240

Using headers.append() for content-length creates duplicate headers if one exists. HTTP proxies may reject this.

Fix: Use headers.insert() instead.

3. System Prompt Injection Fails Silently (Data Handling)
Location: crates/api/src/routes/api.rs around line 1180

If request body isn't valid JSON, system prompt won't be injected but request proceeds anyway.

Fix: Return error if JSON parsing fails when system prompt is configured.

4. PRIVACY VIOLATION (CRITICAL per CLAUDE.md)
Location: crates/api/src/routes/admin.rs lines 331, 384

Logs entire request including system_prompt which violates privacy rules. System prompts could contain sensitive business logic.

Fix: Remove request body logging, only log model_id.

GOOD PATTERNS

• Atomic JSONB merge prevents lost writes
• Proper admin auth on endpoints
• Cache-aside pattern is reasonable

RECOMMENDATIONS

Must fix before merge: Issues 2, 3, 4
Document as limitation: Issue 1

⚠️ 4 critical issues found

[think-in-universe]

@claude review

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Code Review - PR #74: Model Settings and System Configs

Found 4 CRITICAL issues that must be addressed before merging.

---

CRITICAL ISSUE 1: Race Condition in batch_upsert_models Cache Invalidation

Location: crates/api/src/routes/admin.rs:batch_upsert_models

Problem: Cache is invalidated AFTER all database writes complete. This creates a dangerous window where the cache contains stale data while new data is already in the database.

Security Impact: Users can access/use models that admins have just disabled. Under high load, many requests can slip through with incorrect permissions.

Fix: Invalidate cache immediately after each DB write instead of batching at the end.

---

CRITICAL ISSUE 2: Partial Failure Leaves System in Inconsistent State

Location: crates/api/src/routes/admin.rs:batch_upsert_models

Problem: When processing multiple models in a batch, if one fails mid-loop, previous models are already committed to DB but cache invalidation never runs.

Impact: Data inconsistency between cache and database. Users may see wrong model settings until cache expires.

Fix: Move cache invalidation inside loop OR wrap entire operation in a transaction. The per-model invalidation from Issue 1 solves this too.

---

CRITICAL ISSUE 3: Race Condition in Cache Read/Write Pattern

Location: crates/api/src/routes/api.rs lines ~1123-1180

Problem: Classic Time-of-Check-Time-of-Use (TOCTOU) race condition between cache read and cache write. Multiple threads can miss cache simultaneously and issue duplicate DB queries.

Impact: Duplicate DB queries under load defeats caching purpose, cache thrashing, wasted DB connection pool resources.

Fix: Use double-checked locking (hold write lock and check again) or DashMap/moka crate for concurrent cache with built-in race protection.

---

CRITICAL ISSUE 4: Silent Error Swallowing in system_configs_repository

Location: crates/database/src/repositories/system_configs_repository.rs

Problem: unwrap_or_default silently swallows JSON deserialization errors.

Impact: Database corruption goes undetected, invalid JSON silently returns default values, makes debugging nearly impossible. Inconsistent with model_repository which correctly uses ?.

Fix: Use ? operator to propagate errors like model_repository does.

---

Other Observations

Medium Priority: N+1 Query in list_models - Two separate queries (count + data) should use window functions for better performance.

Low Priority: Database migrations are safe and backward compatible. UNIQUE constraints automatically create indexes. SQL injection risk is zero (proper parameterization). No resource leaks found. Logging complies with CLAUDE.md guidelines.

---

Required Actions Before Merge

MUST FIX (Blocking):

1. Issue 1 - Move cache invalidation to immediately after each DB write
2. Issue 2 - Automatically fixed by Issue 1 solution
3. Issue 3 - Fix race condition in cache read/write
4. Issue 4 - Replace unwrap_or_default with proper error propagation

SHOULD FIX:
5. Optimize list_models to use window functions (reduces 2 queries to 1)
6. Add transaction boundaries to batch_upsert_models

---

Recommendation: ⚠️ CHANGES REQUIRED

The PR adds valuable functionality but has critical cache consistency and error handling issues that could cause:

• Security issues (users accessing disabled models)
• Data inconsistency (cache/DB out of sync)
• Performance problems (race conditions causing duplicate DB queries)
• Silent failures (errors being swallowed)

These issues are straightforward to fix. Once addressed, this will be a solid addition to the codebase.