feat(module): serve the complete TinyMemory API - #10
Conversation
Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR expands TinyMemory from mandatory capabilities to the complete capability surface. It adds module-owned provider implementations, TinyBus forwarding methods, chat and runtime hosts, shared client construction, configuration fields, and end-to-end capability checks. ChangesTinyMemory capability expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TinyBus
participant MemoryService
participant ModuleMemoryProvider
participant MemoryClient
participant UnifiedMemory
TinyBus->>MemoryService: Send capability request
MemoryService->>ModuleMemoryProvider: Validate capability and delegate
ModuleMemoryProvider->>MemoryClient: Execute operation
MemoryClient->>UnifiedMemory: Read or update memory
UnifiedMemory-->>MemoryClient: Return operation result
MemoryClient-->>ModuleMemoryProvider: Return converted result
ModuleMemoryProvider-->>MemoryService: Return response or error
MemoryService-->>TinyBus: Return bus response
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/tinymemory-module/src/lib.rs (1)
106-108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate before claiming the process setup slot.
Line 107 sets the permanent claim before line 108 can return a validation error. A malformed first configuration prevents every later valid setup request in the same process. Validate the configuration before calling
claim_process_setup.Proposed fix
async fn setup(connection: Connection, mut config: ModuleConfig) -> BusResult<()> { - claim_process_setup()?; config.validate().map_err(setup_error)?; + claim_process_setup()?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tinymemory-module/src/lib.rs` around lines 106 - 108, In setup, validate config with config.validate().map_err(setup_error)? before calling claim_process_setup() so invalid configuration does not consume the process setup slot; retain both existing error mappings and claim behavior for valid configurations.
🧹 Nitpick comments (1)
crates/tinymemory-module/src/provider.rs (1)
448-506: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftUse bounded store queries for KV and relation reads.
kv_getscans all typed records instead of using a typed direct lookup.kv_listfilters and truncates after loading every record.relationsfetches up to 300 rows per SQL query before truncating, andnamespace: Nonemerges two such result sets. Add store-side key, prefix, and limit parameters. The existingMemoryClient::kv_getreturns onlyserde_json::Value, so preserveMemoryKvRecord.updated_atwith a typed lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tinymemory-module/src/provider.rs` around lines 448 - 506, Update the provider methods kv_get, kv_list, and relations to use bounded store-side queries: pass the key to the typed KV lookup, pass prefix and limit to KV listing, and pass limit to graph_relations so each namespace query is bounded before any merge. For kv_get, use the typed lookup needed to retain MemoryKvRecord.updated_at rather than falling back to MemoryClient::kv_get, which returns only serde_json::Value; preserve the existing conversion and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tinymemory-module/src/provider.rs`:
- Around line 921-971: Update accept_source_items so a put_doc failure records
the partial ingest outcome instead of immediately returning and discarding
previously written IDs. Preserve successful writes in outcome.written and
outcome.ids, capture the failed item’s error according to the existing
IngestOutcome/MemoryError contract, and either continue processing remaining
items or return the accumulated outcome alongside the error as supported by the
API.
- Around line 718-747: The entities method currently ignores its _namespace
argument and accesses global entity state, violating namespace scoping. Update
entities to use namespace-aware entity retrieval for both query and top-entity
paths; if the storage layer cannot scope these operations, return a named
unsupported error instead of reading global indexes. Rename the parameter from
_namespace and preserve the existing result mapping and error propagation.
In `@crates/tinymemory-module/src/service/mod.rs`:
- Around line 316-326: Apply the response-size guard consistently in
query_documents, query_source, entities, kv_list, and relations by generalizing
ensure_response_fits to accept and estimate any serializable response. Invoke it
before each method returns its retrieval payload, and return
wire::BUDGET_EXCEEDED when the serialized response would exceed the 16 MiB
TinyBus frame limit.
---
Outside diff comments:
In `@crates/tinymemory-module/src/lib.rs`:
- Around line 106-108: In setup, validate config with
config.validate().map_err(setup_error)? before calling claim_process_setup() so
invalid configuration does not consume the process setup slot; retain both
existing error mappings and claim behavior for valid configurations.
---
Nitpick comments:
In `@crates/tinymemory-module/src/provider.rs`:
- Around line 448-506: Update the provider methods kv_get, kv_list, and
relations to use bounded store-side queries: pass the key to the typed KV
lookup, pass prefix and limit to KV listing, and pass limit to graph_relations
so each namespace query is bounded before any merge. For kv_get, use the typed
lookup needed to retain MemoryKvRecord.updated_at rather than falling back to
MemoryClient::kv_get, which returns only serde_json::Value; preserve the
existing conversion and error handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d56892be-46e9-40d7-8a9c-f3a1252cfbd1
⛔ Files ignored due to path filters (1)
crates/tinymemory-module/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
api/src/host/events.rscore/src/store/client.rscore/src/store/entities.rscore/src/store/factories.rscrates/tinymemory-module/Cargo.tomlcrates/tinymemory-module/src/chat.rscrates/tinymemory-module/src/config.rscrates/tinymemory-module/src/host.rscrates/tinymemory-module/src/lib.rscrates/tinymemory-module/src/provider.rscrates/tinymemory-module/src/service/mod.rscrates/tinymemory-module/tests/module_e2e.rs
Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Also addressed the review-body setup finding: configuration is validated before the permanent process setup slot is claimed. |
Co-authored-by: Medulla <medulla@tinyhumans.ai>
1 participant
Summary
Validation
cargo clippy --manifest-path crates/tinymemory-module/Cargo.toml --all-targets -- -D warningscargo test --manifest-path crates/tinymemory-module/Cargo.tomlSummary by CodeRabbit