fix(memory): expose vectorBackend via direct property access in ControllerRegistry#1611
fix(memory): expose vectorBackend via direct property access in ControllerRegistry#1611pauloeduardo wants to merge 3 commits intoruvnet:mainfrom
Conversation
…ollerRegistry
- Add vectorBackend field to RuntimeConfig interface
- Pass vectorBackend and vectorDimension to AgentDB constructor in initAgentDB()
- Access agentdb.vectorBackend directly in createController() since
agentdb.getController() does not support 'vectorBackend' name
AgentDB exposes its vector backend instance as a direct property
(agentdb.vectorBackend) but getController() only supports reflexion,
skills and causalGraph. The previous code called getController('vectorBackend')
which always threw 'Unknown controller: vectorBackend', causing the
vectorBackend controller and its 7 dependents to always fail.
Fixes: ruvnet#1610
Related: ruvnet#1207, ruvnet#1228 (ADR-053)
xkonjin
left a comment
There was a problem hiding this comment.
Clean, focused change. A couple of observations:
-
Nullish coalescing for vectorBackend:
config.vectorBackend ?? 'auto'is good, butvectorDimensionusesconfig.dimension || 384. Ifdimensionis explicitly set to0(edge case), it will fall back to 384. Considerconfig.dimension ?? 384for consistency. -
Property access bypass: The comment explains why
vectorBackendis read directly fromthis.agentdb.vectorBackendinstead of viagetController(). This is fine, but it creates a leaky abstraction: callers now depend on an internal property. If AgentDB ever changes how it stores this, the proxy here will break. Consider adding a lightweight getter upstream in AgentDB so the registry can stay decoupled. -
No test updates: The PR changes the constructor contract of
ControllerRegistry(newvectorBackendoption) and the proxy behavior, but I don't see any test changes. A small unit test verifying thatvectorBackendround-trips correctly through the registry would close the gap.
LGTM otherwise.
- Use ?? instead of || for vectorDimension to handle explicit 0 correctly - Improve comment on vectorBackend direct property access noting the leaky abstraction and adding TODO for upstream getController() support - Add unit test verifying vectorBackend round-trips through ControllerRegistry 354 tests passing
|
All three review points have been addressed in the latest commits:
Ready for re-review. |
Summary
Fixes
vectorBackendand its 7 dependent controllers always failing inControllerRegistry(ADR-053).Closes #1610
Related to #1207, #1228
Root Cause
ControllerRegistry.initAgentDB()was initializing AgentDB with only{ dbPath }, causing AgentDB to fall back to HNSWLib and not expose avectorBackendcontroller.The
createController()case for'vectorBackend'then calledagentdb.getController('vectorBackend'), butAgentDB.getController()only supportsreflexion,skills, andcausalGraph— throwingUnknown controller: vectorBackendfor anything else.AgentDB does expose its vector backend as a direct property (
agentdb.vectorBackend), but this was never accessed.Changes
1.
RuntimeConfig— addvectorBackendfield2.
initAgentDB()— pass vectorBackend and vectorDimension to AgentDBUsing
'auto'as default preserves backward compatibility — AgentDB tries ruvector first and falls back to HNSWLib gracefully.3.
createController('vectorBackend')— access direct property instead of getController()Test Results
Before this fix:
After this fix (with vectorBackend: 'rvf'):
The remaining 7 failures are separate issues — they depend on
vectorBackendbeing active, but have their own initialization problems that are outside the scope of this fix.Environment
ruflo@3.5.51,agentdb@3.0.0-alpha.11agentdb/node_modules/for correct ESM resolution