rock/poimen-memory
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

docs: merge ARCHITECTURE_REFACTORING into memory-wiki-graph-rag-optimization.md
Build and Push / Build and push image (push) Canceled after 0s
Details
Build and Push / Test (push) Canceled after 1m9s
Details

Browse Source 
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.
This commit is contained in:
rock
2026-08-30 20:36:49 -07:00
parent 06497196ad
commit 513e79a569
2 changed files with 265 additions and 586 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/ARCHITECTURE_REFACTORING.md
-586
View File
@@ -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<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
docs/memory-wiki-graph-rag-optimization.md
+265
View File
@@ -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<f32>;
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<dyn DocumentScorer>, // Composition, not inheritance
metadata: Arc<ChunkMetadataCache>,
boost_factor: f32,
}
impl DocumentScorer for MetadataBoostingScorer { ... }
```
**Multi-Scorer Orchestrator:**
```rust
pub struct ScoringPipeline {
scorers: Vec<(String, f32, Arc<dyn DocumentScorer>)>, // name, weight, scorer
}
impl ScoringPipeline {
pub fn new() -> Self { ... }
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 with 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));
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<AccessPolicy>;
async fn invalidate_cache(&self, resource_type: &str, name: &str) -> Result<()>;
}
// Vault implementation
pub struct VaultPolicyProvider {
vault_root: PathBuf,
cache: Arc<RwLock<LruCache<String, AccessPolicy>>>,
}
// Alternative: Postgres backend
pub struct DatabasePolicyProvider {
db: Arc<PostgresPool>,
}
// Decorator: Redis cache layer
pub struct CachedPolicyProvider {
inner: Arc<dyn PolicyProvider>,
redis: Arc<redis::aio::Connection>,
ttl_secs: u64,
}
// Usage (all identical interface):
let provider: Arc<dyn PolicyProvider> = 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<bool>;
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<Arc<dyn AccessChecker>>,
policy_provider: Arc<dyn PolicyProvider>,
audit: Arc<dyn AuditLogger>,
}
impl AccessDecisionEngine {
pub async fn check_access(
&self,
claims: &OidcClaims,
resource_type: &str,
resource_name: &str,
) -> Result<bool> {
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<String>,
roles: Vec<String>,
permissions: Vec<String>,
}
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