From 7d283a08d344204f5a65f921dd493c0dc69d15fe Mon Sep 17 00:00:00 2001 From: rock Date: Sun, 30 Aug 2026 20:33:50 -0700 Subject: [PATCH] =?UTF-8?q?docs:=20ARCHITECTURE=5FREFACTORING.md=20?= =?UTF-8?q?=E2=80=94=20SOLID=20+=20DRY=20optimizations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/ARCHITECTURE_REFACTORING.md | 586 +++++++++++++++++++++++++++++++ 1 file changed, 586 insertions(+) create mode 100644 docs/ARCHITECTURE_REFACTORING.md diff --git a/docs/ARCHITECTURE_REFACTORING.md b/docs/ARCHITECTURE_REFACTORING.md new file mode 100644 index 0000000..5848598 --- /dev/null +++ b/docs/ARCHITECTURE_REFACTORING.md @@ -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; + fn name(&self) -> &str; // for debugging/metrics +} + +/// Global scoring (TF-IDF over entire corpus) +pub struct GlobalTfIdfScorer { + vocabulary: Arc>, +} + +impl DocumentScorer for GlobalTfIdfScorer { + async fn score(&self, query: &str, doc_id: &str) -> Result { + 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>, +} + +impl DocumentScorer for ProjectTfIdfScorer { + async fn score(&self, query: &str, doc_id: &str) -> Result { + self.compute_project_tfidf(query, doc_id) + } + fn name(&self) -> &str { \"project-tfidf\" } +} + +/// Semantic scoring (vector similarity) +pub struct SemanticScorer { + embeddings: Arc, + pgvector: Arc, +} + +impl DocumentScorer for SemanticScorer { + async fn score(&self, query: &str, doc_id: &str) -> Result { + 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, + metadata: Arc, + boost_factor: f32, +} + +impl DocumentScorer for MetadataBoostingScorer { + async fn score(&self, query: &str, doc_id: &str) -> Result { + 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)>, // 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, + ) -> 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 { + let scores = futures::stream::iter(&self.scorers) + .then(|(_, _, scorer)| async move { scorer.score(query, doc_id).await }) + .collect::>>() + .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::()) + } +} + +// 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; + + async fn get_all_policies(&self, resource_type: &str) -> Result>; + + /// 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>>, +} + +#[async_trait] +impl PolicyProvider for VaultPolicyProvider { + async fn get_policy(&self, resource_type: &str, name: &str) -> Result { + 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::(&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, + cache: Arc>>, +} + +#[async_trait] +impl PolicyProvider for DatabasePolicyProvider { + async fn get_policy(&self, resource_type: &str, name: &str) -> Result { + // 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, + redis: Arc, + ttl_secs: u64, +} + +#[async_trait] +impl PolicyProvider for CachedPolicyProvider { + async fn get_policy(&self, resource_type: &str, name: &str) -> Result { + 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; + fn description(&self) -> &str; // for logging +} + +pub struct AccessLevelChecker; +#[async_trait] +impl AccessChecker for AccessLevelChecker { + async fn check(&self, claims: &OidcClaims, policy: &AccessPolicy) -> Result { + 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, +} +#[async_trait] +impl AccessChecker for RoleChecker { + async fn check(&self, claims: &OidcClaims, _: &AccessPolicy) -> Result { + 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, +} +#[async_trait] +impl AccessChecker for PermissionChecker { + async fn check(&self, claims: &OidcClaims, _: &AccessPolicy) -> Result { + 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>, + audit: Arc, +} + +impl AccessDecisionEngine { + pub fn new(audit: Arc) -> 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 { + // 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>; +} + +pub struct PostgresAuditLogger { + db: Arc, +} + +#[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, + roles: Vec, + permissions: Vec, + } + + 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, + } + + 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