feat(api): add the tinycortex-api contract crate — the stable driver surface - #138
Conversation
RecallOpts<'a> is named both in the mandatory MemoryRecall::recall signature and as a JSON body field on POST /v1/memory/recall. It can be neither: it derives no serde impls and its borrow lifetime cannot travel through an object-safe #[async_trait] method. Add OwnedRecallOpts with the same five fields owned and serde-derived, plus From impls both ways. Both impls destructure exhaustively, so a field added to one form and not the other is a compile error; a runtime round-trip test catches a field that is merely dropped in conversion. Both forms move into a new recall module so the pair is edited together, and are re-exported from types so every historical types::RecallOpts path (including the engine's tinycortex::memory::types:: alias) keeps resolving.
Capabilities are negotiated once at bind time; an unadvertised family means the RPC methods are unregistered and the agent tools absent, not present-and-failing. Capability carries exactly the thirteen contract families. Capabilities is a bitset that serialises as a JSON array of stable snake_case strings (the handshake capabilities[] field), never discriminant integers, so inserting a variant mid-enum cannot silently re-map an already-deployed driver's set. Capabilities::validate() encodes the mandatory rule in code: core, recall, and portability must all be present, and a missing one is reported structurally via MissingMandatoryCapabilities so the bind site can surface which. The enum is deliberately NOT #[non_exhaustive]: adding a family is a minor contract bump and should break every host match that filters registration by family - that compile error is what guarantees the family gets wired.
The 501 -> Unsupported mapping the wire contract specifies had no target: the existing variants are NotFound/Invalid/BudgetExceeded/PathEscape/Io/Serde/Other. The payload is an owned String, not a Capability and not a &'static str, because the transport adapter constructs this from a wire response where the family is a runtime value that may not be a known Capability at all - a driver speaking a newer minor contract version, a vendor extension, or a typo. A Capability field would force the adapter to drop that information or fail parsing; a &'static str cannot be produced from a runtime value without leaking. MemoryError::unsupported(Capability) yields the canonical spelling for the known case, unsupported_raw() preserves the wire string otherwise. Adding a variant is safe: nothing in the engine or the host matches MemoryError exhaustively (construction and matches! only).
…code CONTRACT_VERSION = (1, 0), re-exported at the crate root. Minor bump = a capability was added; major bump = an existing signature changed. is_compatible() puts the bind rule in code rather than prose: compatible exactly when the major halves match. A minor difference is accepted in both directions because capability negotiation already covers it - a remote ahead advertises families this build skips as unknown, a remote behind simply does not advertise families this build knows, which is the ordinary degradation path. Refusing on a minor difference would reject a usable driver and make adding a family a fleet-wide breaking change.
The contract crate cannot name the kernel's generic DriverHealth: a
third-party driver must be able to depend on tinycortex-api without pulling in
the OpenHuman host, and the next subsystem cut over must not inherit generic
kernel vocabulary from a memory crate.
So the contract carries its own MemoryHealth and the host's memory adapter
converts. The conversion is trivial and lossless by construction: a small
closed enum with a reason string, shaped one-for-one against the kernel's
Ready | Degraded { reason } | Down { reason }, not a free-form struct that
would need field-by-field mapping and would drift.
Driver class stays out of the api crate deliberately - embedded/external/null
is a host configuration fact about how a driver was bound, not something a
driver reports about itself.
…pability families, and the null reference driver
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds the ChangesAPI contract and engine integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a2e38cad6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
src/memory/goals/store.rs (1)
301-304: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required
store_tests.rssibling.The implementation under test is
src/memory/goals/store.rs, but Line 303 loadsmutations_tests.rs. Move these cases intosrc/memory/goals/store_tests.rs, or merge them into that sibling if it already exists.Proposed module rename
-#[path = "mutations_tests.rs"] -mod mutations_tests; +#[path = "store_tests.rs"] +mod store_tests;As per coding guidelines, keep tests in per-file
<name>_tests.rssiblings, such asstore.rsandstore_tests.rs, rather than mixing tests into implementation files.🤖 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 `@src/memory/goals/store.rs` around lines 301 - 304, Rename the test module referenced by store.rs from mutations_tests.rs to the required store_tests.rs sibling, moving or merging the existing cases there if that file already exists. Update the module declaration path and preserve all test coverage.Source: Coding guidelines
api/src/capabilities.rs (1)
193-196: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWiden the capability bitset before adding a fourth family.
The public version rule permits additive capability families. The current thirteen families leave only three valid
u16bits. A seventeenth family shifts by 16 and cannot produce a distinct mask. Use a wider private bitset and add a capacity test.Proposed capacity-preserving change
- fn bit(self) -> u16 { - 1 << self.index() + fn bit(self) -> u64 { + 1u64 << self.index() } @@ pub struct Capabilities { - bits: u16, + bits: u64, }Also applies to: 220-223
🤖 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 `@api/src/capabilities.rs` around lines 193 - 196, Update the private capability bitset type used by Capabilities and the family bit() method to a wider integer type that supports at least 17 distinct family bits, preserving existing mask behavior. Adjust related conversions and bitwise operations consistently, and add a capacity test verifying a seventeenth family can produce a distinct mask.
🤖 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 `@api/src/capabilities.rs`:
- Around line 225-229: Update the documentation for Capabilities::empty() in
api/src/capabilities.rs:225-229 to describe it as the default empty set, not the
null driver’s advertised capabilities. In api/src/capabilities_tests.rs:131-140,
remove or correct the null-driver comment while retaining the assertion that the
empty set is the default.
In `@api/src/health.rs`:
- Around line 20-30: Update the wire-format documentation near MemoryHealth to
match the actual health response field used by the model and wire tests:
describe the GET /v1/health payload with reason instead of detail. Only document
an explicit reason-to-detail adapter mapping if that mapping is implemented and
covered by its transport tests.
In `@api/src/null.rs`:
- Around line 179-185: Update MemoryPortability::export_page to return
MemoryError::Invalid for any Some cursor, while preserving the default
successful page for None. Add a regression test invoking export_page with
Some("unexpected") and assert that it returns the required invalid-cursor error.
In `@api/src/traits.rs`:
- Around line 58-79: Update the MemoryStore trait’s store_with_taint method so
it cannot silently discard provenance: either make store_with_taint required for
every backend or have the default implementation return an error when taint
cannot be persisted. Remove the delegation to store and ensure
MemoryTaint::ExternalSync is never stored as internal content.
In `@api/src/version.rs`:
- Around line 10-35: Revise the version-compatibility guidance around the
minor-bump rules in the module documentation: methods added to existing
capability families such as MemoryCore or MemoryRecall must not be treated as
minor-compatible. Require method-level negotiation or endpoint gating by minimum
minor, or explicitly classify such additions as requiring a major bump; retain
minor compatibility only for safely negotiable changes.
In `@src/memory/goals/mod.rs`:
- Around line 45-58: The GoalsDocMutations trait that provides the add, edit,
and delete methods is not in scope for downstream code that imports only
types::GoalsDoc from tinycortex_api. Either add GoalsDocMutations to the public
re-exports in the pub use statement from store (alongside the individual
function exports) to preserve the source-compatible surface, or document this as
a breaking API change in the release notes and migration guide so downstream
users understand they need to import both types::GoalsDoc and the trait to call
mutation methods on GoalsDoc instances.
In `@src/memory/goals/store.rs`:
- Around line 125-131: Update GoalsStore::delete to remove only the same
deterministic matching occurrence that edit resolves, rather than retaining all
items whose IDs differ; preserve the existing NotFound result when no match
exists. Add regression coverage for duplicate IDs parsed by GoalsDoc::parse,
verifying deleting the duplicated ID removes only the first matching goal.
- Around line 81-104: Update save to validate every GoalsDoc.items entry,
including each GoalItem.text, with the existing goal secret/PII validation
before rendering or calling atomic_write. Return the appropriate MemoryError
immediately for invalid content while preserving the existing item and size-cap
checks and persistence flow for valid documents.
---
Nitpick comments:
In `@api/src/capabilities.rs`:
- Around line 193-196: Update the private capability bitset type used by
Capabilities and the family bit() method to a wider integer type that supports
at least 17 distinct family bits, preserving existing mask behavior. Adjust
related conversions and bitwise operations consistently, and add a capacity test
verifying a seventeenth family can produce a distinct mask.
In `@src/memory/goals/store.rs`:
- Around line 301-304: Rename the test module referenced by store.rs from
mutations_tests.rs to the required store_tests.rs sibling, moving or merging the
existing cases there if that file already exists. Update the module declaration
path and preserve all test coverage.
🪄 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
Run ID: c0d0445e-4fb3-4d17-a50e-01258d53c0bf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
Cargo.tomlapi/Cargo.tomlapi/src/capabilities.rsapi/src/capabilities_tests.rsapi/src/chunks.rsapi/src/chunks_tests.rsapi/src/error.rsapi/src/error_tests.rsapi/src/goals.rsapi/src/goals_tests.rsapi/src/health.rsapi/src/health_tests.rsapi/src/lib.rsapi/src/null.rsapi/src/null_tests.rsapi/src/provider/audit.rsapi/src/provider/audit_tests.rsapi/src/provider/content.rsapi/src/provider/driver.rsapi/src/provider/knowledge.rsapi/src/provider/mandatory.rsapi/src/provider/mod.rsapi/src/provider/records.rsapi/src/provider/types.rsapi/src/provider/types_tests.rsapi/src/recall.rsapi/src/recall_tests.rsapi/src/tool_memory.rsapi/src/tool_memory_tests.rsapi/src/traits.rsapi/src/tree.rsapi/src/tree_tests.rsapi/src/types.rsapi/src/types_tests.rsapi/src/version.rsapi/src/version_tests.rsgitbooks/goals-and-tool-memory.mdsrc/memory/chunks/mod.rssrc/memory/error.rssrc/memory/goals/mod.rssrc/memory/goals/mutations_tests.rssrc/memory/goals/reflect.rssrc/memory/goals/store.rssrc/memory/mod.rssrc/memory/score/signals/source_weight.rssrc/memory/score/signals/source_weight_tests.rssrc/memory/store/content/tags.rssrc/memory/store/content/tags_tests.rssrc/memory/tool_memory/mod.rssrc/memory/tree/runtime/mod.rs
💤 Files with no reviewable changes (1)
- src/memory/error.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🧹 Nitpick comments (2)
src/memory/goals/store.rs (1)
301-304: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required
store_tests.rssibling.The implementation under test is
src/memory/goals/store.rs, but Line 303 loadsmutations_tests.rs. Move these cases intosrc/memory/goals/store_tests.rs, or merge them into that sibling if it already exists.Proposed module rename
-#[path = "mutations_tests.rs"] -mod mutations_tests; +#[path = "store_tests.rs"] +mod store_tests;As per coding guidelines, keep tests in per-file
<name>_tests.rssiblings, such asstore.rsandstore_tests.rs, rather than mixing tests into implementation files.🤖 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 `@src/memory/goals/store.rs` around lines 301 - 304, Rename the test module referenced by store.rs from mutations_tests.rs to the required store_tests.rs sibling, moving or merging the existing cases there if that file already exists. Update the module declaration path and preserve all test coverage.Source: Coding guidelines
api/src/capabilities.rs (1)
193-196: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWiden the capability bitset before adding a fourth family.
The public version rule permits additive capability families. The current thirteen families leave only three valid
u16bits. A seventeenth family shifts by 16 and cannot produce a distinct mask. Use a wider private bitset and add a capacity test.Proposed capacity-preserving change
- fn bit(self) -> u16 { - 1 << self.index() + fn bit(self) -> u64 { + 1u64 << self.index() } @@ pub struct Capabilities { - bits: u16, + bits: u64, }Also applies to: 220-223
🤖 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 `@api/src/capabilities.rs` around lines 193 - 196, Update the private capability bitset type used by Capabilities and the family bit() method to a wider integer type that supports at least 17 distinct family bits, preserving existing mask behavior. Adjust related conversions and bitwise operations consistently, and add a capacity test verifying a seventeenth family can produce a distinct mask.
🤖 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 `@api/src/capabilities.rs`:
- Around line 225-229: Update the documentation for Capabilities::empty() in
api/src/capabilities.rs:225-229 to describe it as the default empty set, not the
null driver’s advertised capabilities. In api/src/capabilities_tests.rs:131-140,
remove or correct the null-driver comment while retaining the assertion that the
empty set is the default.
In `@api/src/health.rs`:
- Around line 20-30: Update the wire-format documentation near MemoryHealth to
match the actual health response field used by the model and wire tests:
describe the GET /v1/health payload with reason instead of detail. Only document
an explicit reason-to-detail adapter mapping if that mapping is implemented and
covered by its transport tests.
In `@api/src/null.rs`:
- Around line 179-185: Update MemoryPortability::export_page to return
MemoryError::Invalid for any Some cursor, while preserving the default
successful page for None. Add a regression test invoking export_page with
Some("unexpected") and assert that it returns the required invalid-cursor error.
In `@api/src/traits.rs`:
- Around line 58-79: Update the MemoryStore trait’s store_with_taint method so
it cannot silently discard provenance: either make store_with_taint required for
every backend or have the default implementation return an error when taint
cannot be persisted. Remove the delegation to store and ensure
MemoryTaint::ExternalSync is never stored as internal content.
In `@api/src/version.rs`:
- Around line 10-35: Revise the version-compatibility guidance around the
minor-bump rules in the module documentation: methods added to existing
capability families such as MemoryCore or MemoryRecall must not be treated as
minor-compatible. Require method-level negotiation or endpoint gating by minimum
minor, or explicitly classify such additions as requiring a major bump; retain
minor compatibility only for safely negotiable changes.
In `@src/memory/goals/mod.rs`:
- Around line 45-58: The GoalsDocMutations trait that provides the add, edit,
and delete methods is not in scope for downstream code that imports only
types::GoalsDoc from tinycortex_api. Either add GoalsDocMutations to the public
re-exports in the pub use statement from store (alongside the individual
function exports) to preserve the source-compatible surface, or document this as
a breaking API change in the release notes and migration guide so downstream
users understand they need to import both types::GoalsDoc and the trait to call
mutation methods on GoalsDoc instances.
In `@src/memory/goals/store.rs`:
- Around line 125-131: Update GoalsStore::delete to remove only the same
deterministic matching occurrence that edit resolves, rather than retaining all
items whose IDs differ; preserve the existing NotFound result when no match
exists. Add regression coverage for duplicate IDs parsed by GoalsDoc::parse,
verifying deleting the duplicated ID removes only the first matching goal.
- Around line 81-104: Update save to validate every GoalsDoc.items entry,
including each GoalItem.text, with the existing goal secret/PII validation
before rendering or calling atomic_write. Return the appropriate MemoryError
immediately for invalid content while preserving the existing item and size-cap
checks and persistence flow for valid documents.
---
Nitpick comments:
In `@api/src/capabilities.rs`:
- Around line 193-196: Update the private capability bitset type used by
Capabilities and the family bit() method to a wider integer type that supports
at least 17 distinct family bits, preserving existing mask behavior. Adjust
related conversions and bitwise operations consistently, and add a capacity test
verifying a seventeenth family can produce a distinct mask.
In `@src/memory/goals/store.rs`:
- Around line 301-304: Rename the test module referenced by store.rs from
mutations_tests.rs to the required store_tests.rs sibling, moving or merging the
existing cases there if that file already exists. Update the module declaration
path and preserve all test coverage.
🪄 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
Run ID: c0d0445e-4fb3-4d17-a50e-01258d53c0bf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
Cargo.tomlapi/Cargo.tomlapi/src/capabilities.rsapi/src/capabilities_tests.rsapi/src/chunks.rsapi/src/chunks_tests.rsapi/src/error.rsapi/src/error_tests.rsapi/src/goals.rsapi/src/goals_tests.rsapi/src/health.rsapi/src/health_tests.rsapi/src/lib.rsapi/src/null.rsapi/src/null_tests.rsapi/src/provider/audit.rsapi/src/provider/audit_tests.rsapi/src/provider/content.rsapi/src/provider/driver.rsapi/src/provider/knowledge.rsapi/src/provider/mandatory.rsapi/src/provider/mod.rsapi/src/provider/records.rsapi/src/provider/types.rsapi/src/provider/types_tests.rsapi/src/recall.rsapi/src/recall_tests.rsapi/src/tool_memory.rsapi/src/tool_memory_tests.rsapi/src/traits.rsapi/src/tree.rsapi/src/tree_tests.rsapi/src/types.rsapi/src/types_tests.rsapi/src/version.rsapi/src/version_tests.rsgitbooks/goals-and-tool-memory.mdsrc/memory/chunks/mod.rssrc/memory/error.rssrc/memory/goals/mod.rssrc/memory/goals/mutations_tests.rssrc/memory/goals/reflect.rssrc/memory/goals/store.rssrc/memory/mod.rssrc/memory/score/signals/source_weight.rssrc/memory/score/signals/source_weight_tests.rssrc/memory/store/content/tags.rssrc/memory/store/content/tags_tests.rssrc/memory/tool_memory/mod.rssrc/memory/tree/runtime/mod.rs
💤 Files with no reviewable changes (1)
- src/memory/error.rs
🛑 Comments failed to post (1)
api/src/traits.rs (1)
58-79: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the trait before tracing overrides and callers. ast-grep outline api/src/traits.rs --items all # Inspect explicit overrides, external-sync callers, and stored taint handling. rg -nP --type rust -C 6 \ '\basync\s+fn\s+store_with_taint\s*\(|\bstore_with_taint\s*\(|MemoryTaint::(?:ExternalSync|Internal)' \ . || trueRepository: tinyhumansai/tinycortex
Length of output: 28553
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- api/src/traits.rs ---' sed -n '30,90p' api/src/traits.rs printf '%s\n' '--- src/memory/store/memory_trait.rs ---' sed -n '1,130p' src/memory/store/memory_trait.rs printf '%s\n' '--- store trait and implementations ---' rg -n -P --type rust -C 4 \ 'impl(?:<[^>]+>)?\s+(?:Memory|MemoryProvider|InMemoryMemoryStore)|impl\s+.*Memory|store_with_taint\s*\(' \ src api --glob '*.rs' || trueRepository: tinyhumansai/tinycortex
Length of output: 34022
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- all implementations of api::traits::Memory ---' rg -n -P --type rust 'impl(?:<[^>]+>)?\s+(?:[[:alnum:]_:]+::)?Memory\s+for\s+' . --glob '*.rs' || true printf '%s\n' '--- trait definitions and re-exports ---' rg -n -P --type rust 'pub\s+trait\s+Memory\b|pub\s+use\s+.*Memory\b|mod\s+traits\b|pub\s+mod\s+traits\b' api src --glob '*.rs' || true printf '%s\n' '--- every store_with_taint declaration/call ---' rg -n -P --type rust -C 3 '\bstore_with_taint\s*\(' api src --glob '*.rs' || true printf '%s\n' '--- sync ingestion write boundary ---' rg -n -P --type rust -C 5 'ExternalSync|accept_source_items|store_with_taint|\.store\s*\(' src api --glob '*.rs' | head -n 500 || trueRepository: tinyhumansai/tinycortex
Length of output: 34385
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- engine Memory trait ---' sed -n '1,125p' src/memory/traits.rs printf '%s\n' '--- provider mandatory write contract ---' sed -n '1,75p' api/src/provider/mandatory.rs printf '%s\n' '--- mock implementation ---' sed -n '1,115p' src/memory/tool_memory/test_helpers.rs printf '%s\n' '--- bounded static behavior check ---' python3 - <<'PY' from pathlib import Path import re trait = Path("api/src/traits.rs").read_text() m = re.search( r"async fn store_with_taint\s*\(.*?\)\s*->\s*anyhow::Result<\(\)>\s*\{(?P<body>.*?)\n\s*\}", trait, re.S, ) assert m, "store_with_taint default body not found" body = m.group("body") assert "let _ = taint;" in body assert "self.store(namespace, key, content, category, session_id)" in body print("api/src/traits.rs: default store_with_taint discards taint and delegates to store") impls = [] for path in [Path("src/memory/store/memory_trait.rs"), Path("src/memory/tool_memory/test_helpers.rs")]: text = path.read_text() if re.search(r"async fn store_with_taint\s*\(", text): impls.append(str(path)) print("in-repository Memory implementations overriding store_with_taint:") for path in impls: print(f" {path}") PYRepository: tinyhumansai/tinycortex
Length of output: 253
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- memory trait files ---' git ls-files | rg '(^|/)(traits\.rs|traits/mod\.rs)$|src/memory' | head -n 120 printf '%s\n' '--- provider mandatory write contract ---' sed -n '1,75p' api/src/provider/mandatory.rs printf '%s\n' '--- mock implementation ---' sed -n '1,115p' src/memory/tool_memory/test_helpers.rs printf '%s\n' '--- bounded static behavior check ---' python3 - <<'PY' from pathlib import Path import re trait = Path("api/src/traits.rs").read_text() m = re.search( r"async fn store_with_taint\s*\(.*?\)\s*->\s*anyhow::Result<\(\)>\s*\{(?P<body>.*?)\n\s*\}", trait, re.S, ) assert m, "store_with_taint default body not found" body = m.group("body") assert "let _ = taint;" in body assert "self.store(namespace, key, content, category, session_id)" in body print("api/src/traits.rs: default store_with_taint discards taint and delegates to store") for path in [Path("src/memory/store/memory_trait.rs"), Path("src/memory/tool_memory/test_helpers.rs")]: if path.exists() and re.search(r"async fn store_with_taint\s*\(", path.read_text()): print(f"override found: {path}") PYRepository: tinyhumansai/tinycortex
Length of output: 11288
Provenance Policy Bypass (CWE-345)
Exploitability: Theoretical
Reachability path
● Entry api/src/lib.rs:64 traits │ ▼ ● Sink api/src/traits.rsRequire
store_with_taintto preserve provenance. If a backend inherits the default,MemoryTaint::ExternalSyncis discarded and external content is stored asMemoryTaint::Internal. Make the method required, or returnErrwhen the backend cannot persist taint. Do not delegate external content tostore.🤖 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 `@api/src/traits.rs` around lines 58 - 79, Update the MemoryStore trait’s store_with_taint method so it cannot silently discard provenance: either make store_with_taint required for every backend or have the default implementation return an error when taint cannot be persisted. Remove the delegation to store and ensure MemoryTaint::ExternalSync is never stored as internal content.
Vec<Capability>::deserialize failed the whole handshake decode on one unrecognised family string, contradicting the documented minor-version rule that a newer driver's unknown families should be skipped, not fail the bind. Decode as Vec<String> and filter through Capability::parse instead. Also widens Capabilities' internal bitset from u16 to u64: at 13 of 16 addressable bits, a 17th family would have overflowed the shift. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Cargo rejects a path-only dependency when packaging the root crate for publish (`all dependencies must have a version requirement specified when packaging`). Add the matching version alongside path. Co-authored-by: Medulla <medulla@tinyhumans.ai>
MemoryHealth and its wire tests serialize the degraded/down explanation
as "reason", but the module doc described the GET /v1/health response
as { status, detail }.
Co-authored-by: Medulla <medulla@tinyhumans.ai>
MemoryPortability::export_page accepted every Some(cursor) as a valid terminal page. The null driver never issues a cursor, so any Some value is necessarily one it did not hand out; a corrupted or wrong-driver cursor should fail loudly rather than silently terminate an export. Adds a regression test. Co-authored-by: Medulla <medulla@tinyhumans.ai>
… links GoalsDoc.items and GoalItem.text are public, so a caller could bypass GoalsDocMutations::add/edit's secret/PII validation entirely by constructing a document by hand and calling save directly. save is now the choke point that re-validates every item's text. GoalsDocMutations::delete used retain(), which removes every item sharing an id. GoalsDoc::parse tolerates duplicate ids in a hand-edited/corrupt file, so a delete could silently drop more than the one goal edit() would have addressed. Switched to removing only the first matching occurrence, matching edit's semantics. Also fixes two rustdoc intra-doc links in store.rs that pointed at private items (has_goal_secret/has_goal_pii), which broke under -D warnings. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
PR babysitter status — GREEN AND CLEANInspected head: Final state
Fixes pushed (7 commits on top of
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7af7b8d9f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
is_compatible() only gated on the major half, on the premise that a minor bump is safe because capability negotiation covers the delta. That premise breaks for one case the docs themselves called out as minor-safe: a new method added to a family a driver may already advertise. Negotiation has family granularity, not method granularity — there is no way to advertise 'Core, but without the new method' — so an older driver still advertising Core could be called into a method it never implemented. Reserve that case for a major bump instead of building method-level negotiation or per-endpoint minor gating, keeping the contract's negotiation surface at family granularity throughout. is_compatible's logic and CONTRACT_VERSION are unchanged; only the classification of what counts as minor-safe moves. Adds a test pinning the rule in prose so it can't be re-derived from the code alone. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/memory/goals/store.rs`:
- Around line 246-255: Update save to run validate_goal_text on every
GoalItem.text before enforcing caps or writing, while preserving the existing
secret and PII checks and applying any returned normalization to the item. Add
regression tests that construct GoalsDoc items directly with empty and multiline
text and verify save rejects them.
🪄 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
Run ID: 73624c42-082f-4b12-b4ad-951242163de4
📒 Files selected for processing (11)
Cargo.tomlapi/src/capabilities.rsapi/src/capabilities_tests.rsapi/src/health.rsapi/src/null.rsapi/src/null_tests.rsapi/src/version.rsapi/src/version_tests.rssrc/memory/goals/mutations_tests.rssrc/memory/goals/store.rssrc/memory/goals/store_tests.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/memory/goals/mutations_tests.rs
- Cargo.toml
- api/src/health.rs
- api/src/capabilities_tests.rs
- api/src/null_tests.rs
- api/src/version.rs
- api/src/capabilities.rs
- api/src/null.rs
…et/PII The prior fix to save() only re-ran the secret/PII guard, leaving the same public-field bypass open for the other two GoalsDocMutations invariants: non-empty and single-line text. Switched to validate_goal_text, which checks all three, so a directly-constructed GoalsDoc can no longer persist empty or multi-line goal text either. Adds regression tests for both cases. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Summary
Adds
tinycortex-api: the dependency-light half of TinyCortex, holding the stable surface amemory backend implements. This is the foundation for pluggable memory backends in OpenHuman
(tinyhumansai/openhuman#5372).
The crate carries
MemoryProviderand its thirteen capability families,MemoryHealth,SourceScope,MemoryError::Unsupported,CONTRACT_VERSION+ the compatibility rule, and a nullreference driver. It also absorbs the inert DTOs that previously lived in the engine, which the
engine now re-exports under its existing paths (
tinycortex::memory::{error, traits, types}andfriends) so no downstream import changes.
Why this needs to land upstream
.gitmodulesin openhuman points at this canonical repo, but the tinycortex commit openhumancurrently pins is not reachable from any branch here — so a fresh clone's
git submodule updatealready fails today. This PR fixes that pre-existing breakage as a side effect: merging it makes both
the current pin and the new one reachable from
main.These commits were also, until recently, present in exactly one place on one machine — the git
directory of a pruned worktree, with no remote copy. Getting them onto canonical is the durability
fix, independent of the feature work.
Design notes worth reviewing
anyhow,async-trait,chrono,serde,serde_json,sha2,thiserror,uuid— no native, async-runtime, or storage dependencies. A host must beable to compile against the contract without the engine. Measured downstream: adding this crate
moves openhuman's kernel dependency floor by +1 package / +1 name / 0 native builds, because
all eight deps were already in its graph. Please treat that property as load-bearing when
reviewing future additions here — anything heavier belongs in the engine crate.
tinycortex = { ..., tinycortex-api = { path = "api" } }),and both are workspace members with
default-members = [".", "api"]so a barecargo testcannotsilently skip the contract crate.
Capability::MANDATORYis[Core, Recall, Portability]. Portability is mandatory on purpose:it is what stops any backend becoming a one-way door.
audit_provider()verifies advertised capabilities are actually reachable, so a driver cannotclaim a family it will fail on at first call. The null driver implements ten families it does not
advertise, which makes it the sharpest test of that distinction.
Note on history
The branch is 12 ahead / 4 behind
mainand includesfix(memory): guard chunk tag rewrites, whichlanded here separately as #131 (
e5753bb) with a different SHA.git merge-treereports zeroconflicts.
Please merge rather than squash or rebase. Downstream gitlinks pin
5a2e38cby SHA; rewriting itwould break them and re-create the unreachable-pin problem this PR exists to fix.
Test plan
cargo testat the workspace root covers both members (default-members).capabilities_tests,error_tests,health_tests,null_tests,version_tests,provider/{audit_tests,types_tests}.cargo check,cargo clippy --lib(0 warnings),cargo fmt --check, and the disabled-feature build all pass with this crate wired in.Co-authored-by: Medulla medulla@tinyhumans.ai
Summary by CodeRabbit
New Features
Bug Fixes
Tests