From 285090716766c287547d1972c35ba9e246b807c7 Mon Sep 17 00:00:00 2001 From: rock Date: Sun, 30 Aug 2026 20:36:49 -0700 Subject: [PATCH] docs: merge ARCHITECTURE_REFACTORING into memory-wiki-graph-rag-optimization.md Integrated SOLID + DRY optimizations as new section: - Scoring pipeline (DocumentScorer trait, ScoringPipeline orchestrator) - Policy provider (PolicyProvider trait, pluggable Vault/Postgres/Redis) - RBAC decision engine (AccessChecker composition, short-circuit eval) - Test fixtures (OidcClaimsBuilder, AccessPolicyBuilder) Implementation priority: 1. ScoringPipeline (Phase 3) 2. PolicyProvider trait (Phase 7) 3. AccessChecker composition (Phase 7) 4. Test fixtures (All phases) Unified doc now has: architecture + concrete implementation + SOLID refactoring. --- docs/ARCHITECTURE_REFACTORING.md | 586 --------------------- docs/memory-wiki-graph-rag-optimization.md | 265 ++++++++++ 2 files changed, 265 insertions(+), 586 deletions(-) delete mode 100644 docs/ARCHITECTURE_REFACTORING.md diff --git a/docs/ARCHITECTURE_REFACTORING.md b/docs/ARCHITECTURE_REFACTORING.md deleted file mode 100644 index 5848598..0000000 --- a/docs/ARCHITECTURE_REFACTORING.md +++ /dev/null @@ -1,586 +0,0 @@ -# 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 diff --git a/docs/memory-wiki-graph-rag-optimization.md b/docs/memory-wiki-graph-rag-optimization.md index 3adcb12..516ef40 100644 --- a/docs/memory-wiki-graph-rag-optimization.md +++ b/docs/memory-wiki-graph-rag-optimization.md @@ -1290,6 +1290,271 @@ cargo bench --bench wiki_graph_retrieval +--- + +## Architecture Refactoring: SOLID + DRY Optimization + +### Problem Analysis + +Without refactoring, the design has antipatterns: + +``` +Current (Tightly Coupled): +TfIdfIndex ─┬─ score_chunk() [duplicated logic across GlobalTfIdfIndex, + │ ProjectTfIdfIndex, ChunkMetadataIndex] + ├─ cache management [responsibility mixing] + └─ build_from_vault() [coupled to Vault directly] + +RbacEngine ─┬─ load_policy() [duplicated in multiple methods] + ├─ check_access() [fat method, 300+ lines, multiple concerns] + └─ audit_log() [mixed responsibility] + +WikiLinkParser ─ resolve_path() called in multiple places + (logic varies, not DRY) +``` + +### Solution: Trait-Based Architecture + +#### 1. Scoring Pipeline (Single DocumentScorer trait) + +**Problem:** TF-IDF scoring logic duplicated in GlobalTfIdfIndex, ProjectTfIdfIndex, SemanticScorer, ChunkMetadataIndex. + +**Solution:** + +```rust +/// Single interface: one scorer, one job +pub trait DocumentScorer: Send + Sync { + async fn score(&self, query: &str, doc_id: &str) -> Result; + fn name(&self) -> &str; // for debugging/metrics +} + +// All scoring variants implement the same trait +pub struct GlobalTfIdfScorer { vocabulary: Arc<...> } +impl DocumentScorer for GlobalTfIdfScorer { ... } + +pub struct ProjectTfIdfScorer { project: String, vocabulary: Arc<...> } +impl DocumentScorer for ProjectTfIdfScorer { ... } + +pub struct SemanticScorer { embeddings: Arc<...>, pgvector: Arc<...> } +impl DocumentScorer for SemanticScorer { ... } + +pub struct MetadataBoostingScorer { + base_scorer: Arc, // Composition, not inheritance + metadata: Arc, + boost_factor: f32, +} +impl DocumentScorer for MetadataBoostingScorer { ... } +``` + +**Multi-Scorer Orchestrator:** + +```rust +pub struct ScoringPipeline { + scorers: Vec<(String, f32, Arc)>, // name, weight, scorer +} + +impl ScoringPipeline { + pub fn new() -> Self { ... } + + 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 with 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)); + +let final_score = pipeline.score(query, doc_id).await?; +``` + +**Benefits:** ✅ DRY | ✅ S | ✅ O | ✅ L | ✅ D | ✅ Testable + +#### 2. Policy Provider (Pluggable backend) + +**Problem:** RbacEngine tightly coupled to Vault. To support Postgres or Redis requires modifying multiple methods. + +**Solution:** + +```rust +#[async_trait] +pub trait PolicyProvider: Send + Sync { + async fn get_policy(&self, resource_type: &str, resource_name: &str) -> Result; + async fn invalidate_cache(&self, resource_type: &str, name: &str) -> Result<()>; +} + +// Vault implementation +pub struct VaultPolicyProvider { + vault_root: PathBuf, + cache: Arc>>, +} + +// Alternative: Postgres backend +pub struct DatabasePolicyProvider { + db: Arc, +} + +// Decorator: Redis cache layer +pub struct CachedPolicyProvider { + inner: Arc, + redis: Arc, + ttl_secs: u64, +} + +// Usage (all identical interface): +let provider: Arc = match env { + "vault" => Arc::new(VaultPolicyProvider::new(vault_root)), + "postgres" => Arc::new(DatabasePolicyProvider::new(db)), +}; + +let policy = provider.get_policy("project", "poimen").await?; +``` + +**Benefits:** ✅ O | ✅ D | ✅ Composition | ✅ Testable + +#### 3. RBAC Decision Engine (Composition of checkers) + +**Problem:** RbacEngine.check_access() is 300+ lines mixing access level, role, permission, and audit concerns. + +**Solution:** + +```rust +#[async_trait] +pub trait AccessChecker: Send + Sync { + async fn check(&self, claims: &OidcClaims, policy: &AccessPolicy) -> Result; + fn description(&self) -> &str; +} + +// Single-purpose checkers +pub struct AccessLevelChecker; // public | group | private +pub struct RoleChecker; // verify required_role +pub struct PermissionChecker; // verify required_permission + +impl AccessChecker for AccessLevelChecker { ... } +impl AccessChecker for RoleChecker { ... } +impl AccessChecker for PermissionChecker { ... } + +/// Orchestrator: compose all checkers (short-circuit evaluation) +pub struct AccessDecisionEngine { + checkers: Vec>, + policy_provider: Arc, + audit: Arc, +} + +impl AccessDecisionEngine { + pub async fn check_access( + &self, + claims: &OidcClaims, + resource_type: &str, + resource_name: &str, + ) -> Result { + let policy = self.policy_provider.get_policy(resource_type, resource_name).await?; + + // Evaluate all checkers (short-circuit on failure) + for checker in &self.checkers { + if !checker.check(claims, &policy).await? { + self.audit.log_decision(deny(checker.description())).await?; + return Ok(false); + } + } + + self.audit.log_decision(allow()).await?; + Ok(true) + } +} +``` + +**Benefits:** ✅ S | ✅ I | ✅ D | ✅ Easy to extend | ✅ Easy to test + +#### 4. Test Fixtures (Reusable builders) + +**Problem:** Test setup code repeated (creating OidcClaims, AccessPolicy, etc.). + +**Solution:** + +```rust +// tests/fixtures/builders.rs +pub struct OidcClaimsBuilder { + sub: String, + groups: Vec, + roles: Vec, + permissions: Vec, +} + +impl OidcClaimsBuilder { + pub fn new(sub: &str) -> Self { ... } + pub fn group(mut self, group: &str) -> Self { ... } + pub fn role(mut self, role: &str) -> Self { ... } + pub fn permission(mut self, perm: &str) -> Self { ... } + pub fn build(self) -> OidcClaims { ... } +} + +pub struct AccessPolicyBuilder { ... } + +// Usage in tests: +#[tokio::test] +async fn test_rbac_project_access_group() { + let claims = OidcClaimsBuilder::new("charlie") + .group("platform-team") + .permission("memory:read") + .build(); + + let policy = AccessPolicyBuilder::public() + .group(vec!["platform-team"]) + .build(); + + let engine = AccessDecisionEngine::new(provider, audit); + let allowed = engine.check_access(&claims, "project", "poimen").await?; + assert!(allowed); +} +``` + +**Benefits:** ✅ DRY | ✅ Readable | ✅ Maintainable | ✅ Flexible + +### Implementation Priority + +1. **ScoringPipeline** (Phase 3) — unblocks all TF-IDF work +2. **PolicyProvider trait** (Phase 7) — enables Vault/Postgres/Redis swap +3. **AccessChecker composition** (Phase 7) — splits fat RBAC method +4. **Test fixtures** (All phases) — immediate DRY wins + +### Summary: SOLID + DRY Improvements + +| Aspect | Before | After | +|---|---|---| +| **Code duplication** | TF-IDF logic in 3+ places | 1 trait, N implementations | +| **New scorer** | Modify TfIdfIndex | New struct + impl DocumentScorer | +| **Policy source** | Vault-only | Swap PolicyProvider trait | +| **RBAC checks** | 1 fat method (300 lines) | 3 single-purpose checkers | +| **Audit logging** | Hardcoded to Postgres | Pluggable AuditLogger trait | +| **Test setup** | Repeated code | Reusable builders | +| **Testability** | Hard to mock | Mock any trait | + --- ## Expected Outcomes