Remaining items from #475 (Core Rust Engineering Review)
The high-impact, low-risk items were resolved in PRs #484 and #489. These remaining items are architectural refactors that require careful ownership restructuring.
Medium — Performance
Hot-path parameter clone reduction
evaluate_article_with_service (service.rs) double-clones the parameter BTreeMap
- Each recursion level in
evaluate_internal_traced (engine.rs) clones the full parameter map again
- Fix: Accept parameters by reference; only clone when transferring ownership to
RuleContext::new
Arc for resolution cache hits
- Cache hits (
service.rs) deep-clone two full BTreeMap<String, Value> (outputs + output_provenance)
- Fix: Wrap cached values in
Arc<BTreeMap> so cache hits are O(1) clone
Medium — Idioms
Rc<RefCell<TraceBuilder>> → &mut TraceBuilder
service.rs, context.rs, engine.rs — engine is single-threaded per execution
Rc<RefCell> adds heap allocation + runtime borrow checking for no benefit
- Fix: Pass
Option<&mut TraceBuilder> through the call chain. Requires restructuring the TraceGuard drop-based pop pattern.
Newtype IDs (LawId, ArticleNumber, BwbId)
law_id, article_number, bwb_id are all plain String across engine, pipeline, harvester, corpus
- Compiler can't catch mix-ups (passing a
bwb_id where a law_id is expected)
- Fix: Introduce newtypes with
Deref<Target=str>, Display, Serialize/Deserialize. Ripples across all packages.
Low
output_index uses null-byte sentinel in HashMap key (resolver.rs:74) — use (String, String) tuple key instead
Operation::name() and ActionOperation::operation_name() duplicate all name strings — unify via strum or delegation
Remaining items from #475 (Core Rust Engineering Review)
The high-impact, low-risk items were resolved in PRs #484 and #489. These remaining items are architectural refactors that require careful ownership restructuring.
Medium — Performance
Hot-path parameter clone reduction
evaluate_article_with_service(service.rs) double-clones the parameterBTreeMapevaluate_internal_traced(engine.rs) clones the full parameter map againRuleContext::newArcfor resolution cache hitsservice.rs) deep-clone two fullBTreeMap<String, Value>(outputs + output_provenance)Arc<BTreeMap>so cache hits are O(1) cloneMedium — Idioms
Rc<RefCell<TraceBuilder>>→&mut TraceBuilderservice.rs,context.rs,engine.rs— engine is single-threaded per executionRc<RefCell>adds heap allocation + runtime borrow checking for no benefitOption<&mut TraceBuilder>through the call chain. Requires restructuring theTraceGuarddrop-based pop pattern.Newtype IDs (
LawId,ArticleNumber,BwbId)law_id,article_number,bwb_idare all plainStringacross engine, pipeline, harvester, corpusbwb_idwhere alaw_idis expected)Deref<Target=str>,Display,Serialize/Deserialize. Ripples across all packages.Low
output_indexuses null-byte sentinel in HashMap key (resolver.rs:74) — use(String, String)tuple key insteadOperation::name()andActionOperation::operation_name()duplicate all name strings — unify viastrumor delegation