Repository navigation
Conversation
… contract The bump and the host code are one commit because they cannot be split: the host cannot implement a trait method the pinned contract does not declare, so either half alone leaves main red. vendor/tinymemory d9a8a6ea (v1.3.0) -> 1a6a24ae (v1.6.0), 26 commits across three releases. Carries our two fixes — the Notion fetch_type fix at both call sites (tinyhumansai#92) and the source-registry permissions fix (tinyhumansai#91) — and a large amount of tinyhumansai#5560 contract work by other contributors, listed in the PR body. vendor/tinycortex b7cf1213 -> 84c994b7 is a required companion, not a second bump: tinymemory v1.6.0 calls chunks::count_chunks_matching and friends, which tinycortex tinyhumansai#157 adds, and openhuman's [patch] forces tinycortex to resolve from its own vendor/ rather than tinymemory's nested pin. One commit. Host changes the new contract requires: - GuardedRetrieval::recall_namespace_recent — carries_content=false, no redaction: the flag describes the outbound payload, and this call sends a namespace and a count. - GuardedEpisodic::insert_event — modelled on tree.append, not insert_turn: an EpisodicEvent carries a namespace and prose, so it is admitted against event.namespace with carries_content=true and its content and subject redacted. - ModuleMemoryProvider::recall_namespace_recent — returns Unsupported naming the pinned artifact. Ok(vec![]) would assert a namespace is empty when the binary simply cannot answer, which no caller could tell apart. - create_segment and append_turn gained an Option<u32> sequence parameter, and ChunkQuery gained six filter fields — neither is in the two-method summary this work was scoped from. - The create_segment comment naming it the only namespaced episodic call is no longer true and is updated here.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change adds guarded episodic event insertion and recent namespace recall. It adds sequence parameters to episodic operations, updates recording providers and tests, reports unsupported module capabilities with typed errors, applies default query fields, and updates vendor references. ChangesEpisodic memory APIs
Vendor pointer updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR updates the memory dependency contract and its host implementations, with reported checks and tests passing; no actionable merge-blocking risk remains beyond normal review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 28.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 9 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f73346bb7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let _ = (namespace, limit); | ||
| Err(MemoryError::unsupported_raw(format!( |
There was a problem hiding this comment.
When a developer loads the locally built v1.6 module with the documented OPENHUMAN_MEMORY_MODULE_ASSUME_FULL_CAPABILITIES=1 override, as_retrieval() advertises the full current contract, but this method still returns Unsupported without contacting that module. Consequently, the newly supported RecallNamespaceRecent member remains unusable and the advertised surface is internally inconsistent; return this pinned-artifact error only when the override is off, and forward the call when it is on.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The two Cargo worlds keep separate lockfiles and CI resolves both with --locked. The root lock moved with the bump; app/src-tauri's did not, so Rust Quality failed on 'cannot update the lock file ... because --locked was passed'. One line: tinymemory 1.3.0 -> 1.6.0.
|
Overlap with #5725, flagged rather than resolved. Raising it here so whoever reviews either PR sees the other; I have not touched #5725 and am not proposing which should win. #5725 ("Route the recall, reset and flush paths through the contract", @YellowSnnowmann, currently More than a conflict, the two disagree on the axis that was deliberately left open:
That last row follows from the one above it, and both readings are internally consistent. This PR was scoped not to re-pin the artifact, which is exactly why it cannot forward: with the pinned binary at 1.3.0 there is no Two facts that may help whoever decides:
So the two are closer to halves of one change than to competing versions of it: this PR carries the gitlinks #5725 needs, and #5725 carries the artifact re-pin that makes this PR's |
|
Closing in favour of #5725 — the maintainer has chosen that approach for the memory-contract work. No criticism of either; they overlap on the same trait methods in the same files ( Three findings from this PR that #5725 should not have to rediscover — recorded here and on #5725:
Consequence worth stating plainly: #5725 does not move any Design notes and the per-impl policy reasoning are preserved in |
1 participant
Summary
vendor/tinymemoryd9a8a6ea(v1.3.0) →1a6a24ae(v1.6.0) — 26 commits, three releases, 53 files.mainred.vendor/tinycortexb7cf1213→84c994b7— one commit, and a required companion rather than a second bump; see below.create_segmentandappend_turneach gained a parameter andChunkQuerygained six fields. Details below.Which commit this pins, and why not an earlier tag
Pinned:
1a6a24ae6d4733943bb23b3c71e80f5139025c61. Thev1.6.0tag is an annotated tag over that exact commit, so the tag and the sha are the same thing — but do not substitute an earlier tag:Both fixes merged at 07:34 on 2026-08-25, after v1.5.0 was cut.
git comparereports each merge commitaheadof v1.4.0 and v1.5.0 andbehindv1.6.0. A reviewer would reasonably assume any tag carries them; only this one does.Problem
tinymemory v1.4.0 added two trait methods with no default bodies, so the pin could not move without host code:
Solution
Five impls (not six —
ModuleMemoryProvidercarries 18Memory*traits andMemoryEpisodicis not among them;Capability::Episodicis absent fromARTIFACT_CAPABILITIES, soas_episodic()never hands back the module driver).GuardedRetrieval::recall_namespace_recent—carries_content: false, no redaction. The flag describes the payload the host sends, not the result it gets back; this call sends a namespace name and a count, so there is nothing to redact.GuardedEpisodic::insert_event— modelled ontree.append, not oninsert_turnbeside it.insert_turnpassesNO_NAMESPACEbecause anEpisodicTurnhas none; anEpisodicEventdoes, so it is admitted againstevent.namespacewithcarries_content: trueand itscontentandsubjectredacted. The knock-on is included:create_segment's comment calling itself "the only episodic call that names a namespace" is no longer true and is updated here.ModuleMemoryProvider::recall_namespace_recent— returnsUnsupportednaming the pinned artifact, and is the one method in that file that does not forward.Ok(vec![])would be the tempting answer and is wrong: the contract says an empty vector from this method means the namespace is empty, so returning it because the binary cannot answer asserts something false no caller can distinguish — the over-claim shape of #5641/#5623 moved into a return value. A bare forward would come back as an untypedOtherafter a round trip (#5598). This introduces a new pattern in that file; every other method forwards unconditionally. It collapses to amodule_call!on the next artifact re-pin.The message is built from
ARTIFACT_CAPABILITIES_PINrather than a literal, so it cannot drift from the pin.What rides along — 26 commits, and most of it is not ours
Ours (4): the Notion
fetch_typefix at both call sites (tinymemory#92,0fd40091+7b32ba95), the source-registry permissions fix (#91,2409d867), and a docs follow-up (ab6736ad).Everything else is #5560 contract work by other contributors, and some of it is behaviour, not plumbing:
recall_namespace_recent— the reason this bump needs host codeinsert_eventmember; per-session sequence threaded through the segment members (this is what changedcreate_segment/append_turnarity); chat attribution; mail headers on the busComposioHostandConfigLoaderinside the module; advances the nested tinycortexThe last two deserve a reviewer's attention: the periodic memory sync loops and the Composio host seam move into the module. That is the same subsystem as the Notion sync fix riding along in this PR, and it is a placement change by another contributor, not a fix of ours.
The nested
vendor/tinycortexadvance inside tinymemory also carries tinycortex #144 (fetch_type) and #146 (render_properties, the openhuman#5500 fix) — the two commits that were stranded when the sync pipelines were ported out of tinycortex. Note they arrive in tinymemory's nested copy; openhuman's[patch]resolvestinycortexfrom its ownvendor/tinycortex, so what actually executes is the top-level pin.The tinycortex companion, and why it is not scope creep
cargo checkfails on the tinymemory bump alone:tinymemory v1.6.0 calls those; tinycortex #157 adds them. tinymemory's own nested pin already points at
84c994b7, but openhuman's[patch]atCargo.toml:1234forcestinycortexto resolve from openhuman'svendor/tinycortex, which overrides it. So v1.6.0's requirement is only satisfied by moving openhuman's own pin. One commit, no choice about which.Larger than the two-method summary
Worth stating plainly, because it was not in the scope this was planned from:
create_segmentgainedstart_seq: Option<u32>andappend_turngainedseq: Option<u32>(the per-session sequence from Skills runtime, onboarding, deep links, and core RPC refinements #95). Threaded through the guard and the recording provider as pass-throughs.ChunkQuerygained six filter fields (entity_ids,entity_kinds,content_contains, and three more). Three call sites now use..Default::default(), which defaults them to "no filter" and preserves each site's existing behaviour exactly. This PR deliberately opts into none of the new filtering.Verification
cargo check --lib --tests— exit 0.cargo fmt --all -- --check— exit 0.cargo clippy -p openhuman --no-deps -- -D warnings— exit 0 (the CI-shaped lane).guard::145 passed ·modules::memory::18 passed ·memory::preferences::7 passed ·memory::tree::98 passed ·memory::tools::43 passed. 0 failed.Three revert-checks, each observed:
insert_event→assertion left != right failed: the credential reached the driver verbatim — redact_outbound was not applied / left: "Authorization: Bearer abcdefghijklmnop".Unsupported→Ok(vec![])in the module provider →the pinned artifact cannot serve this member: [].driver.call_count() == 0, so a guard that decided after touching the driver fails.One thing I could not pin, and a correction to the plan this was built from. The plan expected
NO_NAMESPACEvs&event.namespaceto be caught by "a namespace outside the policy's scope is refused". It is not:admit_writepassesnamespaceonly toguard_span(policy.rs:148), so the two allow and deny identically and differ solely in what the audit span records. I wrote a tracing-capture test that did catch it — reverting toNO_NAMESPACEgaveleft: Some("-") right: Some("team-notes")— but it proved flaky under parallel test execution (tracingcaches callsite interest globally;rebuild_interest_cache()did not fix it), and a flaky test is worse than an admitted gap, so I removed it. The choice is therefore correct-by-construction for whenadmit_writegains real namespace scoping, and untested until then.Impact
Memory subsystem. No RPC or UI surface changes. The
Unsupportedreturn is new observable behaviour for any caller ofrecall_namespace_recenton the module driver — by design, and preferable to the two silent alternatives.Deliberately not in this PR, per standing decisions:
ARTIFACT_CAPABILITIES_PINstays"1.3.0"and the registry keeps itstinymemory-module-1.3.0-*assets. This is why §3'sUnsupportedis needed at all; re-pinning is a separate decision.carries_contentinconsistency is not fixed. Two structurally alike pairs are labelled oppositely today —documents.query_documentssends a user query withtrueand redacts, whileretrieval.recall_namespace_scoredsends one withfalseand does not;tree.appendwrites content withtrueand redacts, whileepisodic.insert_turnwrites user conversation withfalseand does not. Latent, becausecheck_egressandredact_outboundare both no-ops off anExternaldriver — and live the first time one binds.insert_eventfollows thetree.appendside. Flagged for separate filing, not resolved here.Related
#5633— the Notionfetch_typefix reaches openhuman with this bump. Can be hand-closed after merge; both call sites are fixed and shipped.#5635— likewise can be hand-closed after merge.#5727— cannot be closed by this PR. This bump is a live instance of it:ARTIFACT_CAPABILITIESis per-family, and this breaks inside one.Capability::Retrievalstays claimed andthe_capability_list_matches_the_pinned_releasestill passes, while one member of that family is unserved and detectable only at call time. Every existing guard reports green while a real gap opens.#5500was already closed as completed on 2026-08-24 (tinycortex fix(skills): persist OAuth credentials and fix skill auto-start lifecycle #146 plus backend OpenHuman is not available as a one-click cloud deployment #1281), so it needs nothing here — the tinycortex note above is about which copy executes, not about that issue's status.