docs: ARCHITECTURE_REFACTORING.md — SOLID + DRY optimizations

Refactors wiki-graph-rag plan to eliminate antipatterns: DRY violations fixed: - TF-IDF logic scattered → DocumentScorer trait (GlobalTfIdfScorer, ProjectTfIdfScorer, SemanticScorer) - Policy loading duplicated → PolicyProvider trait (VaultPolicyProvider, DatabasePolicyProvider, CachedPolicyProvider) - RBAC fat method → AccessChecker trait (AccessLevelChecker, RoleChecker, PermissionChecker) - Test setup repeated → OidcClaimsBuilder, AccessPolicyBuilder fixtures SOLID principles applied: - Single Responsibility: each scorer/checker does one thing - Open/Closed: add new scorers/providers without modifying existing code - Liskov Substitution: all DocumentScorer impls consistent - Interface Segregation: AuditLogger doesn't force unused methods - Dependency Inversion: depend on traits, not concrete types ScoringPipeline orchestrates multiple scorers with RRF fusion AccessDecisionEngine orchestrates multiple checkers with short-circuit eval PolicyProvider supports Vault/Postgres/Redis transparently Implementation priority: 1. ScoringPipeline (enables all scoring variants) 2. PolicyProvider trait (pluggable policy sources) 3. AccessChecker composition (splits RBAC method) 4. Test fixtures (reduce duplication immediately)
2026-08-30 20:33:50 -07:00
parent d974b2e180
commit 06497196ad
1 changed files with 586 additions and 0 deletions
@@ -0,0 +1,586 @@
+# Architecture Refactoring: DRY + SOLID Optimization
+
+## Problem Analysis
+
+The wiki-graph-rag plan has design antipatterns:
+
+```
+Current (Tightly Coupled):
+TfIdfIndex ─┬─ score_chunk()     [duplicated logic]
+            ├─ cache management  [responsibility mixing]
+            └─ build_from_vault() [depends on Vault directly]
+
+RbacEngine ─┬─ load_policy()     [coupled to YAML format]
+            ├─ check_access()    [fat method, multiple concerns]
+            └─ audit_log()       [mixed responsibilities]
+
+WikiLinkParser ─ resolve_path() called in multiple places
+                 (not DRY if logic varies by context)
+```
+
+## Refactored Design (SOLID Principles)
+
+### 1. Scoring Layer (DRY + S + D)
+
+**Trait-based scoring abstraction:**
+
+```rust
+/// Single Responsibility: score a document given a query
+pub trait DocumentScorer: Send + Sync {
+    async fn score(&self, query: &str, doc_id: &str) -> Result<f32>;
+    fn name(&self) -> &str;  // for debugging/metrics
+}
+
+/// Global scoring (TF-IDF over entire corpus)
+pub struct GlobalTfIdfScorer {
+    vocabulary: Arc<BTreeMap<String, TermStats>>,
+}
+
+impl DocumentScorer for GlobalTfIdfScorer {
+    async fn score(&self, query: &str, doc_id: &str) -> Result<f32> {
+        self.compute_tfidf(query, doc_id)
+    }
+    fn name(&self) -> &str { \"global-tfidf\" }
+}
+
+/// Project-scoped scoring (TF-IDF within project boundaries)
+pub struct ProjectTfIdfScorer {
+    project: String,
+    vocabulary: Arc<BTreeMap<String, TermStats>>,
+}
+
+impl DocumentScorer for ProjectTfIdfScorer {
+    async fn score(&self, query: &str, doc_id: &str) -> Result<f32> {
+        self.compute_project_tfidf(query, doc_id)
+    }
+    fn name(&self) -> &str { \"project-tfidf\" }
+}
+
+/// Semantic scoring (vector similarity)
+pub struct SemanticScorer {
+    embeddings: Arc<EmbeddingsClient>,
+    pgvector: Arc<PostgresPool>,
+}
+
+impl DocumentScorer for SemanticScorer {
+    async fn score(&self, query: &str, doc_id: &str) -> Result<f32> {
+        let query_vec = self.embeddings.embed(query).await?;
+        let doc_vec = self.pgvector.get_embedding(doc_id).await?;
+        Ok(cosine_similarity(&query_vec, &doc_vec))
+    }
+    fn name(&self) -> &str { \"semantic\" }
+}
+
+/// Metadata-boosted scorer (composition, not inheritance)
+pub struct MetadataBoostingScorer {
+    base_scorer: Arc<dyn DocumentScorer>,
+    metadata: Arc<ChunkMetadataCache>,
+    boost_factor: f32,
+}
+
+impl DocumentScorer for MetadataBoostingScorer {
+    async fn score(&self, query: &str, doc_id: &str) -> Result<f32> {
+        let base_score = self.base_scorer.score(query, doc_id).await?;
+        
+        // Compose: base score + metadata boost
+        if let Some(metadata) = self.metadata.get(doc_id).await {
+            let boost = if self.matches_category(query, &metadata.category) {
+                self.boost_factor
+            } else {
+                1.0
+            };
+            Ok(base_score * boost)
+        } else {
+            Ok(base_score)
+        }
+    }
+    fn name(&self) -> &str { \"metadata-boosting\" }
+}
+```
+
+**Multi-Scorer Orchestrator (Strategy + Adapter patterns):**
+
+```rust
+pub struct ScoringPipeline {
+    scorers: Vec<(String, f32, Arc<dyn DocumentScorer>)>,  // name, weight, scorer
+}
+
+impl ScoringPipeline {
+    pub fn new() -> Self { ... }
+    
+    /// Add scorer with weight
+    pub fn with_scorer(
+        mut self,
+        name: &str,
+        weight: f32,
+        scorer: Arc<dyn DocumentScorer>,
+    ) -> Self {
+        self.scorers.push((name.to_string(), weight, scorer));
+        self
+    }
+    
+    /// Execute all scorers in parallel, fuse results (RRF)
+    pub async fn score(&self, query: &str, doc_id: &str) -> Result<f32> {
+        let scores = futures::stream::iter(&self.scorers)
+            .then(|(_, _, scorer)| async move { scorer.score(query, doc_id).await })
+            .collect::<Result<Vec<_>>>()
+            .await?;
+        
+        // RRF: weighted sum of normalized scores
+        let weighted_sum: f32 = self.scorers
+            .iter()
+            .zip(scores)
+            .map(|((_, weight, _), score)| weight * score)
+            .sum();
+        
+        Ok(weighted_sum / self.scorers.iter().map(|(_, w, _)| w).sum::<f32>())
+    }
+}
+
+// Usage:
+let pipeline = ScoringPipeline::new()
+    .with_scorer(\"global-tfidf\", 0.1, Arc::new(global_tfidf))
+    .with_scorer(\"project-tfidf\", 0.3, Arc::new(project_tfidf))
+    .with_scorer(\"semantic\", 0.6, Arc::new(semantic));
+    // Metadata boost is applied as decorator
+    
+let final_score = pipeline.score(query, doc_id).await?;
+```
+
+**Benefits:**
+- ✅ DRY: scoring logic lives in one place per scorer type
+- ✅ S: each scorer has one responsibility
+- ✅ O: add new scorers without modifying existing ones
+- ✅ L: all implement DocumentScorer consistently
+- ✅ D: depend on trait, not concrete types
+
+---
+
+### 2. Policy Layer (DRY + O + D)
+
+**Trait-based policy provider:**
+
+```rust
+/// Single interface for policy retrieval (not YAML-specific)
+#[async_trait]
+pub trait PolicyProvider: Send + Sync {
+    async fn get_policy(
+        &self,
+        resource_type: &str,  // \"project\" | \"skill\"
+        resource_name: &str,
+    ) -> Result<AccessPolicy>;
+    
+    async fn get_all_policies(&self, resource_type: &str) -> Result<Vec<AccessPolicy>>;
+    
+    /// Cache invalidation/reload
+    async fn invalidate_cache(&self, resource_type: &str, name: &str) -> Result<()>;
+}
+
+/// Vault implementation (YAML files)
+pub struct VaultPolicyProvider {
+    vault_root: PathBuf,
+    cache: Arc<RwLock<LruCache<String, AccessPolicy>>>,
+}
+
+#[async_trait]
+impl PolicyProvider for VaultPolicyProvider {
+    async fn get_policy(&self, resource_type: &str, name: &str) -> Result<AccessPolicy> {
+        let cache_key = format!(\"{}:{}\", resource_type, name);
+        
+        // Check cache first
+        if let Some(policy) = self.cache.read().await.get(&cache_key) {
+            return Ok(policy.clone());
+        }
+        
+        // Load from Vault
+        let path = match resource_type {
+            \"project\" => self.vault_root.join(\"projects\").join(name).join(\"_access.yaml\"),
+            \"skill\" => self.vault_root.join(\"shared\").join(\"skills\").join(name).join(\"_access.yaml\"),
+            _ => return Err(anyhow!(\"Unknown resource type\")),
+        };
+        
+        let content = tokio::fs::read_to_string(&path).await?;
+        let policy = serde_yaml::from_str::<AccessPolicy>(&content)?;
+        
+        // Cache it
+        self.cache.write().await.put(cache_key, policy.clone());
+        
+        Ok(policy)
+    }
+    
+    async fn invalidate_cache(&self, resource_type: &str, name: &str) -> Result<()> {
+        let cache_key = format!(\"{}:{}\", resource_type, name);
+        self.cache.write().await.remove(&cache_key);
+        Ok(())
+    }
+}
+
+/// Database implementation (alternative backend)
+pub struct DatabasePolicyProvider {
+    db: Arc<PostgresPool>,
+    cache: Arc<RwLock<LruCache<String, AccessPolicy>>>,
+}
+
+#[async_trait]
+impl PolicyProvider for DatabasePolicyProvider {
+    async fn get_policy(&self, resource_type: &str, name: &str) -> Result<AccessPolicy> {
+        // Query Postgres instead of Vault
+        sqlx::query_as::<_, AccessPolicy>(
+            \"SELECT * FROM access_policies WHERE resource_type = $1 AND resource_name = $2\"
+        )
+        .bind(resource_type)
+        .bind(name)
+        .fetch_one(&*self.db)
+        .await
+        .map_err(|e| anyhow!(e))
+    }
+}
+
+/// Redis cache layer (decorator)
+pub struct CachedPolicyProvider {
+    inner: Arc<dyn PolicyProvider>,
+    redis: Arc<redis::aio::Connection>,
+    ttl_secs: u64,
+}
+
+#[async_trait]
+impl PolicyProvider for CachedPolicyProvider {
+    async fn get_policy(&self, resource_type: &str, name: &str) -> Result<AccessPolicy> {
+        let cache_key = format!(\"policy:{}:{}\", resource_type, name);
+        
+        // Check Redis
+        if let Ok(cached) = redis::cmd(\"GET\")
+            .arg(&cache_key)
+            .query_async(&mut *self.redis.clone())
+            .await
+        {
+            if let Some(json) = cached {
+                return Ok(serde_json::from_str(&json)?);
+            }
+        }
+        
+        // Fall back to inner provider
+        let policy = self.inner.get_policy(resource_type, name).await?;
+        
+        // Cache in Redis
+        let json = serde_json::to_string(&policy)?;
+        let _ = redis::cmd(\"SET\")
+            .arg(&cache_key)
+            .arg(&json)
+            .expire(self.ttl_secs)
+            .query_async(&mut *self.redis.clone())
+            .await;
+        
+        Ok(policy)
+    }
+}
+```
+
+**Usage:**
+
+```rust
+// Vault-only (simple)
+let provider = Arc::new(VaultPolicyProvider::new(vault_root));
+
+// Vault with Redis cache (faster)
+let provider = Arc::new(CachedPolicyProvider {
+    inner: Arc::new(VaultPolicyProvider::new(vault_root)),
+    redis: redis_conn,
+    ttl_secs: 3600,
+});
+
+// Or Postgres directly (if migrated)
+let provider = Arc::new(DatabasePolicyProvider::new(db));
+
+// All work the same way
+let policy = provider.get_policy(\"project\", \"poimen\").await?;
+```
+
+**Benefits:**
+- ✅ DRY: policy loading logic isolated
+- ✅ O: swap Vault ↔ Postgres ↔ Redis without changing RBAC engine
+- ✅ D: RbacEngine depends on trait, not concrete provider
+
+---
+
+### 3. RBAC Decision Engine (S + I + D)
+
+**Split fat method into single-purpose checkers:**
+
+```rust
+/// Single responsibility: one access check
+#[async_trait]
+pub trait AccessChecker: Send + Sync {
+    async fn check(&self, claims: &OidcClaims, policy: &AccessPolicy) -> Result<bool>;
+    fn description(&self) -> &str;  // for logging
+}
+
+pub struct AccessLevelChecker;
+#[async_trait]
+impl AccessChecker for AccessLevelChecker {
+    async fn check(&self, claims: &OidcClaims, policy: &AccessPolicy) -> Result<bool> {
+        match policy.access_level.as_str() {
+            \"public\" => Ok(true),
+            \"private\" => Ok(claims.groups.contains(&policy.owner_group)),
+            \"group\" => Ok(claims.groups.iter().any(|g| policy.allowed_groups.contains(g))),
+            _ => Err(anyhow!(\"Unknown access level\")),
+        }
+    }
+    fn description(&self) -> &str { \"access_level\" }
+}
+
+pub struct RoleChecker {
+    policy: Arc<AccessPolicy>,
+}
+#[async_trait]
+impl AccessChecker for RoleChecker {
+    async fn check(&self, claims: &OidcClaims, _: &AccessPolicy) -> Result<bool> {
+        if let Some(required_role) = &self.policy.required_role {
+            Ok(claims.roles.contains(required_role))
+        } else {
+            Ok(true)  // No requirement
+        }
+    }
+    fn description(&self) -> &str { \"role\" }
+}
+
+pub struct PermissionChecker {
+    policy: Arc<AccessPolicy>,
+}
+#[async_trait]
+impl AccessChecker for PermissionChecker {
+    async fn check(&self, claims: &OidcClaims, _: &AccessPolicy) -> Result<bool> {
+        if let Some(required_perm) = &self.policy.required_permission {
+            Ok(claims.permissions.contains(required_perm))
+        } else {
+            Ok(true)  // No requirement
+        }
+    }
+    fn description(&self) -> &str { \"permission\" }
+}
+
+/// Orchestrator: compose all checkers
+pub struct AccessDecisionEngine {
+    checkers: Vec<Arc<dyn AccessChecker>>,
+    audit: Arc<dyn AuditLogger>,
+}
+
+impl AccessDecisionEngine {
+    pub fn new(audit: Arc<dyn AuditLogger>) -> Self {
+        Self {
+            checkers: vec![
+                Arc::new(AccessLevelChecker),
+                Arc::new(RoleChecker { ... }),
+                Arc::new(PermissionChecker { ... }),
+            ],
+            audit,
+        }
+    }
+    
+    pub async fn check_access(
+        &self,
+        claims: &OidcClaims,
+        resource_type: &str,
+        resource_name: &str,
+        policy: &AccessPolicy,
+    ) -> Result<bool> {
+        // Evaluate all checkers (short-circuit on first failure)
+        let mut allowed = true;
+        let mut reason = String::new();
+        
+        for checker in &self.checkers {
+            match checker.check(claims, policy).await {
+                Ok(true) => {}
+                Ok(false) => {
+                    allowed = false;
+                    reason = checker.description().to_string();
+                    break;
+                }
+                Err(e) => return Err(e),
+            }
+        }
+        
+        // Audit log (always)
+        self.audit.log_decision(AccessDecision {
+            user_id: claims.sub.clone(),
+            resource_type: resource_type.to_string(),
+            resource_name: resource_name.to_string(),
+            decision: if allowed { \"allow\" } else { \"deny\" }.to_string(),
+            reason,
+        }).await?;
+        
+        Ok(allowed)
+    }
+}
+```
+
+**Audit logger trait (pluggable):**
+
+```rust
+#[async_trait]
+pub trait AuditLogger: Send + Sync {
+    async fn log_decision(&self, decision: AccessDecision) -> Result<()>;
+    async fn get_decisions(&self, user_id: &str, limit: u32) -> Result<Vec<AccessDecision>>;
+}
+
+pub struct PostgresAuditLogger {
+    db: Arc<PostgresPool>,
+}
+
+#[async_trait]
+impl AuditLogger for PostgresAuditLogger {
+    async fn log_decision(&self, decision: AccessDecision) -> Result<()> {
+        sqlx::query(
+            \"INSERT INTO rbac_audit_log (user_id, resource_type, resource_name, decision, reason) VALUES ($1, $2, $3, $4, $5)\"
+        )
+        .bind(&decision.user_id)
+        .bind(&decision.resource_type)
+        .bind(&decision.resource_name)
+        .bind(&decision.decision)
+        .bind(&decision.reason)
+        .execute(&*self.db)
+        .await?;
+        Ok(())
+    }
+}
+```
+
+**Benefits:**
+- ✅ S: each checker has one responsibility
+- ✅ I: AuditLogger doesn't force you to implement unused methods
+- ✅ D: AccessDecisionEngine depends on traits
+- ✅ D: Easy to add/remove checkers without modifying the engine
+
+---
+
+### 4. Test Fixtures & Builders (DRY)
+
+**Reusable test helpers:**
+
+```rust
+// tests/fixtures/mod.rs
+pub mod builders {
+    use crate::*;
+    
+    pub struct OidcClaimsBuilder {
+        sub: String,
+        groups: Vec<String>,
+        roles: Vec<String>,
+        permissions: Vec<String>,
+    }
+    
+    impl OidcClaimsBuilder {
+        pub fn new(sub: &str) -> Self {
+            Self {
+                sub: sub.to_string(),
+                groups: vec![],
+                roles: vec![\"viewer\".to_string()],
+                permissions: vec![],
+            }
+        }
+        
+        pub fn group(mut self, group: &str) -> Self {
+            self.groups.push(group.to_string());
+            self
+        }
+        
+        pub fn build(self) -> OidcClaims {
+            OidcClaims {
+                sub: self.sub,
+                groups: self.groups,
+                roles: self.roles,
+                permissions: self.permissions,
+            }
+        }
+    }
+    
+    pub struct AccessPolicyBuilder {
+        access_level: String,
+        owner_group: String,
+        allowed_groups: Vec<String>,
+    }
+    
+    impl AccessPolicyBuilder {
+        pub fn public() -> Self {
+            Self {
+                access_level: \"public\".to_string(),
+                owner_group: Default::default(),
+                allowed_groups: Default::default(),
+            }
+        }
+        
+        pub fn group_level(mut self, groups: Vec<&str>) -> Self {
+            self.access_level = \"group\".to_string();
+            self.allowed_groups = groups.iter().map(|s| s.to_string()).collect();
+            self
+        }
+        
+        pub fn private(mut self, owner: &str) -> Self {
+            self.access_level = \"private\".to_string();
+            self.owner_group = owner.to_string();
+            self
+        }
+        
+        pub fn build(self) -> AccessPolicy {
+            AccessPolicy {
+                access_level: self.access_level,
+                owner_group: self.owner_group,
+                allowed_groups: self.allowed_groups,
+                required_role: None,
+                required_permission: None,
+            }
+        }
+    }
+}
+
+// Usage in tests:
+#[tokio::test]
+async fn test_rbac_project_access_group() {
+    let claims = OidcClaimsBuilder::new(\"charlie\")
+        .group(\"platform-team\")
+        .group(\"devops-team\")
+        .build();
+    
+    let policy = AccessPolicyBuilder::public()
+        .group_level(vec![\"platform-team\", \"devops-team\"])
+        .build();
+    
+    let engine = AccessDecisionEngine::new(Arc::new(MockAuditLogger));
+    let allowed = engine.check_access(&claims, \"project\", \"poimen\", &policy).await?;
+    assert!(allowed);
+}
+```
+
+**Benefits:**
+- ✅ DRY: test data construction centralized
+- ✅ Readable: fluent API
+- ✅ Maintainable: single place to update test helpers
+
+---
+
+## Summary: What Improves?
+
+| Aspect | Before | After |
+|---|---|---|
+| **Code duplication** | TF-IDF logic in 3 places | 1 trait, N implementations |
+| **New scorers** | Modify TfIdfIndex | Add new struct implementing DocumentScorer |
+| **Policy source** | Vault-only (hardcoded) | Trait: swap Vault/Postgres/Redis |
+| **RBAC checks** | 1 fat method (300 lines) | 3 single-purpose checkers |
+| **Audit logging** | Hardcoded to Postgres | Pluggable AuditLogger trait |
+| **Test setup** | Repeated builder code | Reusable builders in fixtures |
+| **New scorer weight** | Modify RRF fusion algorithm | Add to ScoringPipeline |
+
+---
+
+## Implementation Priority
+
+1. **Scoring Pipeline** (enables all scoring variants to coexist)
+2. **PolicyProvider trait** (enables swappable policy sources)
+3. **AccessChecker composition** (splits fat RBAC method)
+4. **Test fixtures** (reduce test duplication immediately)
+
+After this refactoring, the system becomes:
+- **Extensible** — add scorers/checkers/providers without modifying core
+- **Testable** — mock implementations for each trait
+- **Maintainable** — single responsibility for each component
+- **DRY** — no duplicated logic