# 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