Move the memory-source contracts and readers off the engine (#18 §B4) - #46
Conversation
…nsai#18 §B4) `core/src/sources/` described a source in the *engine's* vocabulary: its types, registry and readers were all re-exports of, or thin delegations to, `crate::engine::backend::sources`. A host binding a different driver could not so much as describe a source. They live in a new `tinymemory-sources` crate now, which names no engine. What moved, and the four decisions inside the move -------------------------------------------------- 5,952 lines: the source types, field validation, the `sources.toml` registry, and the folder/conversation/GitHub/RSS/web-page readers with their tests. Four engine couplings needed a decision rather than a rewrite: 1. `MemoryConfig` -- the readers used exactly one field of it, `config.workspace`. They take `&Path` now. A narrower signature instead of an imported config type. 2. `MemoryEngineResult`/`MemoryError` -- the contract's `MemoryError` already models `PathEscape`, the one variant `ensure_within_base` needs, so `SourceResult<T> = Result<T, tinymemory_api::error:: MemoryError>` is an exact retype, not a widening. A reader now fails in the same vocabulary as the driver that stores what it read. 3. `RawKind` -- defined locally, deliberately a second copy of the engine's. Safe because it never crosses a boundary: `raw_archive_coords` is its only consumer, nothing outside the crate calls that, and core never touches the type. Documented as such at the definition. 4. The engine's `sync` feature gate -- renamed `network`, because `tinymemory-sync` is a sibling crate and one name for two things is how this workspace's `SourceKind` confusion started. The gate earns its keep: default links 63 crates and no HTTP stack; `network` (63 -> 150) adds the GitHub/RSS/web-page readers and is what core asks for. Deliberately NOT done: renaming `SourceItem`/`SourceKind`, which collide in name (not in scope) with different contract-crate concepts. The pairs never meet, and the churn would be ~150 call sites here plus 24 in OpenHuman. The crate doc records the distinction instead. Core keeps its own `SourceReader` trait (over the host `Config`) and its two host-only readers (composio, twitter); only the delegation target changed. OpenHuman imports three functions from `core::sources` -- `apply_kind_defaults`, `get_source`, `list_sources` -- all still re-exported, so downstream is untouched. The engine's copy of `sources/` stays until the tinycortex companion PR deletes it, the same two-step §B3 used (tinymemory#41 + tinycortex#153). The moved code lands under this crate's stricter lints (`unwrap_used`, `expect_used`, `missing_docs` warn): two guarded `unwrap`s rewritten to carry their proof (`is_some_and`, index in the `Option`), the registry's lock-poison `expect` replaced with `PoisonError::into_inner` recovery, reader structs documented. Tests keep `unwrap` by a scoped `cfg_attr(test, allow)` -- a panic in a test is the failure report.
|
Warning Review limit reached
Next review available in: 54 minutes Limit details: You’ve used the included review currently available. 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 within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (39)
✨ 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 |
The clippy fixes landed after the local fmt check ran, so their edits were never formatted. cargo fmt --all; no semantic change.
How this change flows0 changed behaviours across 3 relationships. 5 surrounding behaviours are shown (60 graph nodes walked). 19 further behaviours left out to keep the diagram readable. flowchart LR
n0["SourceReader"]:::impacted
n1["path"]:::impacted
n2["MemorySourceEntry"]:::impacted
n3["push"]:::impacted
n4["parse_selector"]:::impacted
n0 -->|uses| n1
n0 -->|uses| n2
n4 -->|calls| n3
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
Issue #18 §B4 — the source contracts and readers move off the engine.
core/src/sources/described a source in the engine's vocabulary: its types, registry and readers were re-exports of, or thin delegations to,crate::engine::backend::sources. A host binding a different driver could not so much as describe a source. They live in a newtinymemory-sourcescrate now (5,952 lines moved), which names no engine.Why the obvious smaller PR does not exist
Types-first was tried. Core's registry and readers delegate to the engine, and those delegations pass these exact types across the call — repointing the types alone produces
E0308: mismatched types(twoMemorySourceEntrys). The alternatives were a conversion layer (which §A1 just deleted) or moving types + registry + readers together. This PR is the latter.Four decisions inside the move
MemoryConfig→&Path. The readers used exactly one field:config.workspace. They now take the workspace path itself — a narrower signature instead of an imported config type.Errors retype exactly, not loosely. The contract's
MemoryErroralready modelsPathEscape— the one variantensure_within_baseneeds.SourceResult<T> = Result<T, tinymemory_api::error::MemoryError>, so a reader fails in the same vocabulary as the driver that stores what it read.RawKindis a deliberate second copy. Verified safe before copying:raw_archive_coordsis its only consumer, nothing outside the crate calls that, core never touches the type — it cannot cross a boundary. The doc at the definition says when to lift it into the contract instead.The engine's
syncfeature gate is renamednetwork.tinymemory-syncis a sibling crate; one name for two things is how this workspace'sSourceKindconfusion started. The gate pays measurably:networkDeliberately not done
No
SourceItem/SourceKindrename. Both names also exist intinymemory-apimeaning different things (ingest entry with content vs. listing entry; content-kind vs. connector-kind). The pairs never appear in one scope, and renaming would churn ~150 call sites here plus 24 in OpenHuman. The crate doc records the distinction so the duplication reads as considered, not accidental.Blast radius
SourceReadertrait (over the hostConfig) and its two host-only readers (composio,twitter); only the delegation target changed. 9 core files touched.core::sources(apply_kind_defaults,get_source,list_sources) — all still re-exported, verified against its tree.sources/copy stays until a tinycortex companion PR deletes it — the same two-step §B3 used (Run Composio normalisation and storage on a driver that is not TinyCortex (#18 §B3) #41 + tinycortex#153). Companion follows this PR.Lints
The moved code lands under this crate's stricter lints (
unwrap_used,expect_used,missing_docs): two guardedunwraps rewritten to carry their proof (is_some_and, index carried in theOption), the registry's lock-poisonexpectreplaced withPoisonError::into_innerrecovery (guard data is()— nothing torn to inherit), reader structs documented. Tests keepunwrapviacfg_attr(test, allow)— a panic in a test is the failure report, and rewriting 159 assertions would obscure what each checks.Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targetscargo test --workspacecargo test -p tinymemory-sources --features networkcargo deny check--all-features162→166 (the new crate'stoml/uuidsurface)Merge order
Independent of everything currently open. The
vendor/gitlinks are untouched. Next after this: §B1 (sync onto the neutral types), then §B5/§E4 — the last unmet acceptance criterion.