Housekeeping: drop committed binaries, align the MSRV, document core/ - #19
Conversation
`rust_out` and `tmp` are 3.6 MB Linux x86-64 ELF executables with debug info, committed into a repository developed on macOS. Nothing references either name in source, config, CI, or docs; both are the output of an ad-hoc build that was staged by accident, and every clone has carried 7.3 MB of them since. Ignored with anchored paths so a legitimate `tmp` directory inside a crate is unaffected.
The workspace declared three different answers: 1.96 at the root and for the module crate, 1.85 for `core` and both adapters, and nothing at all for `api`. The low ones were not true — the root facade requires 1.96 and depends on all of them, so nothing in this workspace builds on 1.85, and no CI job checked the claim. Aligning on 1.96 immediately surfaced three lints that the understated MSRV had been suppressing: clippy only suggests `is_multiple_of` when the declared minimum supports it. Those are fixed here rather than allowed, since they are a consequence of this change and not separable from it. `api/` gaining an explicit MSRV is worth noting for downstream: it is the dependency-light crate an embedding host binds directly, so it now states a floor rather than leaving consumers to infer one.
The layout section omitted `tinymemory-core` entirely — the largest crate in the repository, and the one a real host actually depends on. It also omitted `crates/tinymemory-module`. Adds both, and states plainly that `core` is not dependency-light the way `api` is, since that asymmetry is the thing a reader most needs to know before choosing which to depend on. Also documents `git submodule update --init --recursive`. Nothing builds without it, and because `core` names its engines by path through `vendor/`, an uninitialized checkout fails at manifest resolution with an error that does not mention submodules.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
How this change flows0 changed behaviours across 2 relationships. 3 surrounding behaviours are shown (60 graph nodes walked). 55 further behaviours left out to keep the diagram readable. flowchart LR
n0["event_insert"]:::impacted
n1["EventRecord"]:::impacted
n2["event_search_fts"]:::impacted
n0 -->|uses| n1
n2 -->|uses| n1
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. |
tinysweeper
Bot
added
the
priority: p3
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Reviewed the three commits that are this PR's own contribution (318fc82, 439d992, d742122). Housekeeping matches §F, the README addition is substantive, and worktrees/ was already covered by a pre-existing .gitignore entry. Three notes, none blocking.
1. MSRV was aligned upward, and that is a choice worth stating. api, core and both adapters went 1.85 → 1.96. That raises the floor on tinymemory-api, which is the crate §D4 exists to keep light and broadly consumable — aligning down to 1.85 was the other available reading of "align to one value". The three is_multiple_of calls in this PR stabilised in 1.87, so 1.96 may overshoot what the workspace actually needs. Could you add a line on why 1.96? (UNVERIFIED which crate genuinely requires it.)
2. Three source edits are not mentioned by any of the three commit messages. bytes.len() % 4 != 0 → !bytes.len().is_multiple_of(4) in store/namespace_store/events.rs, store/namespace_store/segments.rs, and tree/score/embed/mod.rs. They are a clippy consequence of the MSRV bump and behaviourally identical (exact equivalent for usize — checked). Commit-message hygiene only, but a reader diffing "Align the MSRV" does not expect .rs changes.
3. The binaries are gone from the tree but remain in history. §F asked only for removal plus .gitignore, so this meets the spec. But the new .gitignore comment says they were "carried in every clone", and that is still true afterwards — fcfb1634e:rust_out and :tmp are still reachable, and a fresh clone still fetches them. Either soften the comment or open a follow-up for a history rewrite, so the next reader does not conclude the clone got smaller.
Merge order — raising this once, here, for the whole stack
#19–#29 are a single linear stack (I fetched every head and verified parentage; each PR adds exactly one commit on top of its predecessor, and #19 owns three). All eleven target main, with no declared order and no base-chaining. Three consequences:
- Merging #29 first silently merges all thirteen commits, including #23's registry work and #27's API addition, under a PR titled "Extend the module E2E…".
- Merging out of order breaks the build: #24's CI lane references the
memory-gitfeature that #22 introduces, and #25's--test null_providerreferences a file that #21 adds. - The displayed diffs are misleading. Because each is computed against
main, #29 shows +5035/−2188, of which roughly 4,900 belongs to #22–#28. Reviewing the GitHub diff means re-reading the same code eleven times and never seeing what each PR actually did — which I suspect is not the review you want.
Two ways to fix, first preferred:
- Re-target each PR at its predecessor (#20 base →
chore/18-f-housekeeping, #21 base →feat/18-e1-conformance-suite, and so on). GitHub then shows the true per-PR diff, and each retargets tomainautomatically as its base lands. - Or declare the order in each PR body (
Merge after #N) and merge strictly 19 → 29.
main has not moved since the stack was cut (fcfb1634e is still the merge base), so option 1 is cheap right now and gets more expensive with every commit that lands on main.
Nice work overall — the stack is unusually well-documented, and #24 in particular catches a failure mode this org has shipped before.
3 participants
Summary
Step 1 of the sequencing in #18 — housekeeping, so the diff of everything after it is readable. No behavior changes.
Three focused commits:
Remove two committed build artifacts.
rust_outandtmpare 3.6 MB Linux x86-64 ELF executables with debug info, committed into a repository developed on macOS. Nothing references either name in source, config, CI, or docs. Ignored with anchored paths so a legitimatetmpdirectory inside a crate is unaffected.Align the MSRV across every crate. The workspace declared three different answers:
1.96at the root and for the module crate,1.85forcoreand both adapters, and nothing at all forapi. The low ones were not true — the root facade requires 1.96 and depends on all of them, so nothing here builds on 1.85, and no CI job checked the claim.Aligning on 1.96 immediately surfaced three lints the understated MSRV had been suppressing: clippy only suggests
is_multiple_ofwhen the declared minimum supports it. Fixed rather than allowed, since they are a consequence of this change and not separable from it.Document
core/,crates/, and submodule setup in the README. The layout section omittedtinymemory-coreentirely — the largest crate in the repository and the one a real host depends on — andcrates/tinymemory-module. Also documentsgit submodule update --init --recursive: nothing builds without it, and becausecorenames its engines by path throughvendor/, an uninitialized checkout fails at manifest resolution with an error that never mentions submodules.Public API / behavior changes
None. The MSRV declarations move up to match what the workspace already required; the three lint fixes are semantically identical (
x % 4 != 0→!x.is_multiple_of(4)).One note for downstream:
api/now states an explicit MSRV where it previously stated none. It is the dependency-light crate an embedding host binds directly, so it now declares a floor rather than leaving consumers to infer one.Validation
All four contract commands from
AGENTS.md, run from the repository root:cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo build --all-targets --all-featurescargo test --all-featuresTwo corrections to the audit in #18
Worth recording, since both are listed under §1.8 as outstanding:
worktrees/is already handled..gitignorecarriesworktrees/*with!worktrees/.gitkeep, and the directory is empty in a fresh checkout. Nothing to do.api/did not declare1.85. It declared norust-versionat all, which is why it does not appear in the inconsistency the issue describes — it was not inconsistent, it was silent.Related
Part of #18. Next in the sequencing is E1 — the driver conformance suite — which is also what unblocks the
remoteengine selection in tinyhumansai/opencompany#914.