build(memory): gate the git-backed diff ledger behind memory-git - #5477
Conversation
📝 WalkthroughWalkthroughThe change separates contributor and product feature sets, adds exact forwarding validation, applies product features across Rust CI and coverage, and gates Git-backed memory functionality behind ChangesFeature gates and memory Git integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant ProductFeaturesScript
participant Cargo
participant FeatureForwardingCheck
CI->>ProductFeaturesScript: read product-features.txt
ProductFeaturesScript->>Cargo: provide product feature list
Cargo->>FeatureForwardingCheck: expose core and shell feature sets
FeatureForwardingCheck->>CI: report exact forwarding result
sequenceDiagram
participant DesktopBuild
participant EmbeddedDriver
participant MemoryDiffControllers
participant MemoryDiffTool
DesktopBuild->>EmbeddedDriver: enable memory-git
EmbeddedDriver->>MemoryDiffControllers: register memory-diff namespace
EmbeddedDriver->>MemoryDiffTool: expose diff capability
MemoryDiffControllers->>MemoryDiffTool: register MemoryDiffTool
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@AGENTS.md`:
- Line 431: Update the kernel-profile figures in the memory-git capability table
and the baseline references near the stated package/native-build counts so they
consistently reflect the PR objective: 307 to 304 packages and five to three
native builds. If the older six-build baseline is intentionally retained,
explicitly label it as historical rather than presenting it as current.
In `@docs/library-minimal-recipe.md`:
- Around line 42-48: Update the table in the minimal recipe documentation to
remove the obsolete Default column and its historical values, including
desktop-automation. Replace it with current Contrib and Product columns sourced
from the documented feature definitions, while preserving the Decision column
for library-host choices.
In `@scripts/ci/product-features.sh`:
- Around line 23-30: Update the LIST construction in the product-features
parsing flow so an input containing only comments or blank lines does not
terminate the script when grep finds no matches under pipefail and set -e. Use
awk or explicitly handle grep’s status, while preserving the existing empty LIST
check and its documented diagnostic and exit behavior.
In `@src/openhuman/memory/guard/provider_tests.rs`:
- Around line 307-320: Replace the fixed 200 ms sleep in the affected guard
success test with deterministic synchronization that waits for all relevant bus
delivery work to complete, using an available receiver, completion handle, or
test-bus drain mechanism after store/recall publishing. Then retain the filtered
assertion on recorder.seen for the "recording" driver, preserving the test’s
verification that successful guarded operations publish no event.
🪄 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: 1218eb8f-1809-423b-b145-253daa21f787
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (101)
.github/workflows/ci-lite.yml.github/workflows/test-reusable.ymlAGENTS.mdCargo.tomlapp/src-tauri/Cargo.tomlapp/src-tauri/src/whatsapp_data/mod.rsapp/src/components/settings/panels/VoicePanel.tsxdocs/library-minimal-recipe.mdscripts/__tests__/feature-forwarding.test.mjsscripts/ci/check-feature-forwarding.mjsscripts/ci/product-features.shscripts/ci/product-features.txtscripts/ci/rust-coverage-changed.shscripts/kernel-floor.limitsscripts/lib/feature-forwarding.mjsscripts/test-rust-with-mock.shsrc/core/all_tests.rssrc/core/bus.rssrc/core/jsonrpc.rssrc/core/mod.rssrc/openhuman/agent/artifacts/store_tests.rssrc/openhuman/agent/bus.rssrc/openhuman/agent/harness/session/runtime_tests.rssrc/openhuman/agent/harness/session/turn/tools.rssrc/openhuman/agent/learning/extract/signature.rssrc/openhuman/agent/learning/profile_md_renderer.rssrc/openhuman/agent/learning/startup.rssrc/openhuman/agent/orchestration/run_ledger_finalize.rssrc/openhuman/agent/orchestration/run_ledger_finalize_tests.rssrc/openhuman/agent/tinyagents/tools.rssrc/openhuman/agent/triage/escalation.rssrc/openhuman/agent/triage/evaluator.rssrc/openhuman/agent/triage/events.rssrc/openhuman/channels/bus.rssrc/openhuman/channels/host/adapters.rssrc/openhuman/channels/proactive.rssrc/openhuman/channels/providers/telegram/approval_surface.rssrc/openhuman/channels/providers/telegram/approval_surface_tests.rssrc/openhuman/channels/providers/telegram/bus.rssrc/openhuman/channels/providers/telegram/bus_tests.rssrc/openhuman/channels/routes_tests.rssrc/openhuman/channels/runtime/dispatch/processor.rssrc/openhuman/channels/runtime/test_support.rssrc/openhuman/channels/tests/health.rssrc/openhuman/channels/tests/runtime_dispatch.rssrc/openhuman/config/ops/agent.rssrc/openhuman/cron/bus.rssrc/openhuman/cron/scheduler_tests.rssrc/openhuman/desktop/notifications/bus.rssrc/openhuman/flows/bus.rssrc/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rssrc/openhuman/inference/provider/factory_tests.rssrc/openhuman/inference/provider/openhuman_backend_model.rssrc/openhuman/inference/provider/ops/http_error.rssrc/openhuman/inference/provider/ops_tests.rssrc/openhuman/integrations/composio/ops/direct_mode.rssrc/openhuman/integrations/task_sources/bus.rssrc/openhuman/meet/backend_bot/calendar.rssrc/openhuman/memory/binding.rssrc/openhuman/memory/conversations/bus.rssrc/openhuman/memory/diff/mod.rssrc/openhuman/memory/diff/ops.rssrc/openhuman/memory/diff/stub.rssrc/openhuman/memory/driver/embedded/mod.rssrc/openhuman/memory/global.rssrc/openhuman/memory/guard/audit.rssrc/openhuman/memory/guard/provider_tests.rssrc/openhuman/memory/ops/sync.rssrc/openhuman/memory/store/client.rssrc/openhuman/memory/store/content/mod.rssrc/openhuman/memory/sync/composio/bus.rssrc/openhuman/memory/sync_events.rssrc/openhuman/memory/sync_pipeline_e2e_tests.rssrc/openhuman/memory/tinycortex/seal.rssrc/openhuman/memory/tinycortex/sync.rssrc/openhuman/memory/tree/ingest.rssrc/openhuman/memory/tree/tree_runtime/bus.rssrc/openhuman/security/approval/gate.rssrc/openhuman/security/credentials/bus.rssrc/openhuman/security/credentials/session_support.rssrc/openhuman/security/devices/bus.rssrc/openhuman/security/egress/emit_tests.rssrc/openhuman/security/keyring_consent/policy.rssrc/openhuman/skills/bus.rssrc/openhuman/skills/ops_create.rssrc/openhuman/skills/webhooks/bus.rssrc/openhuman/tools/mod.rssrc/openhuman/tools/ops.rssrc/openhuman/voice/bus.rssrc/openhuman/web_chat/event_bus.rstests/agent_harness_e2e.rstests/calendar_grounding_e2e.rstests/composio_list_tools_stack_overflow_regression.rstests/config_auth_app_state_connectivity_e2e.rstests/json_rpc_e2e.rstests/monitor_agent_e2e.rstests/subconscious_conversation_e2e.rstests/subconscious_fullstack_e2e.rstests/subconscious_triggers_e2e.rsvendor/tinycortex
`[features] default` was the product set, so every contributor paid for the whole product on every edit: web3's ethers/secp256k1 cohort, `documents`' zstd/bzip2 native builds, the cpal/hound/arboard/enigo/rdev stack behind `voice`+`inference`, `contacts`' macOS objc2 cohort, `crash-reporting`'s sentry tree, `tui`'s ratatui. A bare `cargo check` — and therefore rust-analyzer on every keystroke — resolved 567 packages and ran 7 native C/C++ builds. `default` is now the CONTRIBUTOR set: 9 gates that cost almost nothing to compile, so `cargo check` still typechecks nearly the whole tree. Measured: **567 -> 356 packages, 7 -> 5 native builds.** The kernel floor is untouched at 307/284/5 — this axis does not intersect the `flows` profile. THIS DOES NOT CHANGE WHAT SHIPS. `app/src-tauri` has declared `openhuman_core` with `default-features = false` since tinyhumansai#1061 and never inherited `default`; that is precisely why a forwarding guard had to exist at all. `check-feature-forwarding.mjs` worked by diffing the shell's forwarding list against `[features] default`. That is a SUBSET check, and it was sound only while `default` was the product set. Shrinking `default` makes it weaker with every gate removed and vacuous if `default` ever empties — silently re-arming and ~93k Sentry events. Landing the flip without the rewrite would have been the same bug with a longer fuse. The product set is now explicit, in `scripts/ci/product-features.txt`, and the guard asserts three things: 1. the shell forwards EXACTLY that file — set equality, both directions. A dropped gate fails on `missing`; a gate the shell grew that the product never claimed fails on `unexpected`. Equality cannot pass vacuously. 2. every name in the file is a gate the core actually declares (catches a typo or a gate renamed out from under it). 3. every `default` gate is forwarded or allow-listed — the original check, retained because it still catches a contributor-set gate nobody shipped. Two parsers read that file — this one and `product-features.sh`, which the CI lanes use to build `--features`. A test asserts they agree, because if they drifted CI would compile a different set than the guard checks. A lane that relied on default features silently stops covering the product. Every lane that builds or tests the product now passes `--features "$(bash scripts/ci/product-features.sh)"`: core clippy, the unit lane, the changed-files coverage lane, and `scripts/test-rust-with-mock.sh`. Clippy runs TWICE, once per set — a lint that only fires with the gates off (an import left unused once a domain is compiled out) is invisible to the product lane and would be red on every contributor's machine while CI is green. Four `tests/*.rs` targets name symbols that only exist behind `voice`, `web3`, `inference` or `crash-reporting`, so they now declare `required-features`. Without it a bare `cargo test` fails to COMPILE, in files the contributor did not touch. With it cargo SKIPS them — the same silent-skip trap `--bins` without `bin-tools` already had, which is why the lane comments spell it out: `json_rpc_e2e` alone is >12k lines of RPC contract coverage. `tui` ends up in NEITHER set (default-OFF and deliberately not forwarded), so nothing in CI would compile it at all. The feature-gate-smoke lane now checks it explicitly; a future gate in that position needs the same. Note `required-features` is the blunt instrument — it costs the whole target when a gate is off, where tinyhumansai#5021's per-symbol `#[cfg]` cleanup would keep the ungated tests running. Taken deliberately for these four; noted in Cargo.toml. Verified: contributor `cargo check --lib --tests`, product `cargo check --all-targets --features <product>`, `--no-default-features --features tui`, the 27 guard self-tests, the kernel-floor ratchet, and `cargo fmt --all --check`. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Takes the kernel profile to **2 native builds — the spec target** (G6 in
MIGRATION-PLAN, and the goal named in `kernel-floor.limits`' own baseline
entry). `git2` with vendored libgit2 leaves and takes `libgit2-sys` **and**
`libz-sys` with it; together with the `runtime-node` gate already on main, only
`libsqlite3-sys` and `ring` remain, and both are load-bearing. Kernel floor
305/282/4 -> 302/279/2, from 418 names / 6 native when the program started.
`memory-git` is default-OFF, product-ON, carrying `dep:git2` plus tinycortex's
`git-diff` and `wiki-git`.
## The type carve-out is what makes this gateable
`memory::diff::types` compiles in BOTH builds. The always-on subconscious
memory profile renders `CrossSourceDiff` and `ChangeKind` into agent prompts,
so stubbing them would put two definitions of one serde shape in the tree, free
to drift. tinycortex needed the matching split first — its `git-diff` gated the
whole `memory::diff` module, so a libgit2-less host could not name the types at
all. Its `memory::diff::{types,source}` are ungated now (they reach no `git2`
symbol; only `ledger.rs` and `ledger_helpers.rs` do) and the
`Ledger`/`DiffEngine` half stays behind the feature. Gitlink bumped to it.
## Stub vs `#[cfg]`, decided per call site
Stubbed: the three `ops` entry points always-on code reaches —
`auto_snapshot_after_sync` (`memory::sources::sync`), `diff_since_checkpoint`
and `create_checkpoint` (the subconscious profile). Those domains stay
feature-unaware.
They return an `Err` naming the gate, not an empty diff. An empty
`CrossSourceDiff` asserts "your world did not change", which the subconscious
would act on; the error routes into a path the caller already handles by
logging and skipping.
`#[cfg]`'d: the registration sites, which want absence. The schema aggregators
return empty vecs (`memory_diff` becomes unknown-method and drops off
`/schema`) and `MemoryDiffTool` disappears from its single registration site —
a registered tool that always errors is worse than an absent one, because the
model keeps choosing it and reporting the failure to the user.
## Capability honesty
The embedded driver drops `Capability::Diff` from `advertised_capabilities()`
and `as_diff()` returns `None`. Both halves move together deliberately:
`audit_provider` fails on either alone, which is the check that keeps them from
drifting, and callers may trust the advertised set rather than probing every
accessor.
## The wiki mirror degrades, it does not fail
`content::wiki_git::commit_summaries` is gated at its two call sites rather
than stubbed. The git wiki is a derived view — the summary's own content file
is written either way — so skipping it loses the mirror, not the summary.
`seal.rs`'s `summary_committed` gains a `#[cfg(not(...))]` sibling returning
`Ok(())`: accurate rather than lenient, since nothing the caller depended on
failed to happen. `tests/memory_artifacts_e2e.rs` opens the ledger with
`git2::Repository::open`, so it declares `required-features = ["memory-git"]`.
## Review follow-ups from tinyhumansai#5477 also folded in
- `scripts/ci/product-features.sh` died with a bare exit 1 and no message on a
comments-only file: `grep -v` exits 1 when it selects nothing, and `set -e`
aborted inside the command substitution, making the explicit diagnostic
unreachable. Fixed, and the helper now takes an optional path so the
self-test can drive that path with a fixture rather than mutating the
checked-in file. Two regression tests.
- Stale figures in AGENTS.md corrected against measurement (contributor
353/3-native, product 540/7-native, kernel baseline 302/279/2).
- `docs/library-minimal-recipe.md`'s obsolete single `Default` column replaced
with the real Contrib/Product pair rather than annotated as wrong.
Co-authored-by: Medulla <medulla@tinyhumans.ai>
3f70170 to
a499125
Compare
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)
.github/workflows/ci-lite.yml (1)
227-227: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-494): Download of Code Without Integrity Check
Reachability: External
Use one reviewed, immutable CI image reference.
These containers resolve
ghcr.io/tinyhumansai/openhuman_ci:latestat runtime. A moved tag can execute unreviewed shell code in CI. Pin these image references to reviewed commits or SHA digests instead oflatest; action pins do not pin the container image.
.github/workflows/ci-lite.yml#L227: Pin thefrontend-checksimage..github/workflows/ci-lite.yml#L346: Pin therust-qualityimage..github/workflows/ci-lite.yml#L429: Pin therust-feature-gate-smokeimage..github/workflows/ci-lite.yml#L576: Pin therust-rss-benchimage..github/workflows/ci-lite.yml#L624: Pin therust-core-coverageimage..github/workflows/ci-lite.yml# L722: Pin therust-tauri-coverageimage..github/workflows/ci-lite.yml#L814: Pin thescripts-testsimage..github/workflows/ci-lite.yml#L853: Pin thetest-inventoryimage..github/workflows/ci-lite.yml#L951: Pin thetinycortex-testsimage..github/workflows/test-reusable.yml#L55: Pin thei18n-coverageimage..github/workflows/test-reusable.yml#L80: Pin theunit-testsimage..github/workflows/test-reusable.yml#L123: Pin therust-core-testsimage..github/workflows/test-reusable.yml#L220: Pin thetinycortex-testsimage..github/workflows/test-reusable.yml#L291: Pin therust-tauri-testsimage.🤖 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 @.github/workflows/ci-lite.yml at line 227, Replace the mutable latest image reference used by the frontend-checks job in .github/workflows/ci-lite.yml:227 with the reviewed immutable commit reference or SHA digest. Apply the same pinning change to the rust-quality, rust-feature-gate-smoke, rust-rss-bench, rust-core-coverage, rust-tauri-coverage, scripts-tests, test-inventory, and tinycortex-tests images in .github/workflows/ci-lite.yml at lines 346, 429, 576, 624, 722, 814, 853, and 951, and to the i18n-coverage, unit-tests, rust-core-tests, tinycortex-tests, and rust-tauri-tests images in .github/workflows/test-reusable.yml at lines 55, 80, 123, 220, and 291. Use one reviewed immutable reference consistently instead of latest at every listed site.Source: Linters/SAST tools
🤖 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 `@AGENTS.md`:
- Around line 358-359: Update the Product row in AGENTS.md to document 17 gates
instead of 16, matching the 17 entries in scripts/ci/product-features.txt,
including memory-git. Leave the package and native-build counts unchanged.
- Line 432: The web3 dependency lists incorrectly include curve25519-dalek and
omit exclusive EVM crates. Update AGENTS.md lines 432-432 and
docs/library-minimal-recipe.md lines 57-57 to replace bitcoin, curve25519-dalek
with bitcoin, ethers-core, ethers-signers, coins-bip39, keeping both documents
consistent.
In `@scripts/kernel-floor.limits`:
- Line 45: Update the comment describing the 356→353 package-count change to
identify it as the contributor/default profile, or explicitly state the feature
selection, instead of calling it the default full-feature profile.
---
Outside diff comments:
In @.github/workflows/ci-lite.yml:
- Line 227: Replace the mutable latest image reference used by the
frontend-checks job in .github/workflows/ci-lite.yml:227 with the reviewed
immutable commit reference or SHA digest. Apply the same pinning change to the
rust-quality, rust-feature-gate-smoke, rust-rss-bench, rust-core-coverage,
rust-tauri-coverage, scripts-tests, test-inventory, and tinycortex-tests images
in .github/workflows/ci-lite.yml at lines 346, 429, 576, 624, 722, 814, 853, and
951, and to the i18n-coverage, unit-tests, rust-core-tests, tinycortex-tests,
and rust-tauri-tests images in .github/workflows/test-reusable.yml at lines 55,
80, 123, 220, and 291. Use one reviewed immutable reference consistently instead
of latest at every listed site.
🪄 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: ac379ba0-650d-42ae-b361-076a8ee11143
📒 Files selected for processing (14)
.github/workflows/ci-lite.yml.github/workflows/test-reusable.ymlAGENTS.mdCargo.tomlapp/src-tauri/Cargo.tomldocs/library-minimal-recipe.mdscripts/__tests__/feature-forwarding.test.mjsscripts/ci/product-features.shscripts/ci/product-features.txtscripts/kernel-floor.limitsscripts/test-rust-with-mock.shsrc/core/all_tests.rssrc/openhuman/tools/ops.rsvendor/tinycortex
🚧 Files skipped from review as they are similar to previous changes (8)
- src/core/all_tests.rs
- src/openhuman/tools/ops.rs
- app/src-tauri/Cargo.toml
- scripts/test-rust-with-mock.sh
- Cargo.toml
- vendor/tinycortex
- scripts/tests/feature-forwarding.test.mjs
- scripts/ci/product-features.sh
| | **Contributor** | `[features] default` in `Cargo.toml` | What a bare `cargo check`, `cargo test` and rust-analyzer compile. 9 cheap gates. **353 packages / 3 native builds** (`libsqlite3-sys`, `lzma-sys`, `ring`). | | ||
| | **Product** | `scripts/ci/product-features.txt` | What the shipped desktop app has. 16 gates. **540 packages / 7 native builds** (adds `bzip2-sys`, `libgit2-sys`, `libz-sys`, `zstd-sys`). | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Change the product gate count to 17.
scripts/ci/product-features.txt contains 17 product gates, including memory-git. Line [359] says 16. Make the documented count match the manifest.
🤖 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 `@AGENTS.md` around lines 358 - 359, Update the Product row in AGENTS.md to
document 17 gates instead of 16, matching the 17 entries in
scripts/ci/product-features.txt, including memory-git. Leave the package and
native-build counts unchanged.
| | ------- | ------- | ------- | ----- | ---------- | | ||
| | `voice` | OFF | ON | the `openhuman::voice` family (incl. `voice::audio_toolkit`) — STT/TTS providers, dictation server, always-on listening, podcast audio + email | `hound`, `lettre` | | ||
| | `inference` | OFF | ON | the `cpal` audio-device stack: microphone capture for voice, plus `desktop::accessibility::permissions`' mic-permission probe. Implied by `voice`. Off ⇒ the probe reports `Unknown`. **The name is historical** — it used to gate the bundled whisper.cpp STT engine, which no longer exists (see the scope note below); do not rename it, it is forwarded by name from the shell manifest and asserted by `INFERENCE_COMPILED_IN` | `cpal` | | ||
| | `web3` | OFF | ON | the `openhuman::web3` family (`web3`, `web3::wallet`, `web3::x402`) — crypto wallet (multi-chain sign/broadcast), swaps/bridges/dapp calls, x402 machine payments | `bitcoin`, `curve25519-dalek` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one correct web3 dependency list. Both documents list curve25519-dalek as shed, but it remains enabled through ed25519-dalek, and both omit the exclusive EVM crates.
AGENTS.md#L432-L432: replacebitcoin, curve25519-dalekwithbitcoin, ethers-core, ethers-signers, coins-bip39.docs/library-minimal-recipe.md#L57-L57: apply the same dependency list.
📍 Affects 2 files
AGENTS.md#L432-L432(this comment)docs/library-minimal-recipe.md#L57-L57
🤖 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 `@AGENTS.md` at line 432, The web3 dependency lists incorrectly include
curve25519-dalek and omit exclusive EVM crates. Update AGENTS.md lines 432-432
and docs/library-minimal-recipe.md lines 57-57 to replace bitcoin,
curve25519-dalek with bitcoin, ethers-core, ethers-signers, coins-bip39, keeping
both documents consistent.
| # `None`, in lockstep — `audit_provider` fails on either | ||
| # half alone), and the git wiki mirror of summary nodes is | ||
| # skipped while the summaries themselves are still | ||
| # written. Default (full-feature) profile moved 356 -> 353. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Name the 353-package profile correctly.
After this PR, Cargo default is the contributor set, not the full product set. “Default (full-feature) profile” can misclassify the 356→353 measurement. Use “contributor/default profile” or state the exact feature selection.
🤖 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 `@scripts/kernel-floor.limits` at line 45, Update the comment describing the
356→353 package-count change to identify it as the contributor/default profile,
or explicitly state the feature selection, instead of calling it the default
full-feature profile.
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0146 · 155,274 in / 47,957 out · 120,315 cached (77%) · z-ai/glm-5.2
critique: $0.0074 · 50,400 in / 27,601 out · 39,247 cached (78%) · z-ai/glm-5.2
security: $0.0033 · 29,049 in / 11,743 out · 23,336 cached (80%) · z-ai/glm-5.2
tests: $0.0013 · 28,670 in / 2,197 out · 21,228 cached (74%) · z-ai/glm-5.2
description: $0.0021 · 30,219 in / 5,691 out · 23,111 cached (76%) · z-ai/glm-5.2
| assert.deepEqual(result.unknown, [], 'product gates that are not real core gates'); | ||
| }); | ||
|
|
||
| test('the shell helper reports an empty gate list instead of dying silently', () => { |
There was a problem hiding this comment.
Prior memory-git off-stub finding not visible in this file
The prior finding asked for a test asserting the memory-git off-stub returns Err rather than Ok-empty. No code related to memory-git appears anywhere in this file's diff, so the finding is neither addressed nor refuted here. It likely concerns a different file and remains open from this review's perspective.
*[RULE] no new root-level .rs files ·
| # gated-off domains, so every CI lane that builds or tests "the product" now | ||
| # passes `--features "$(scripts/ci/product-features.sh)"`. If you add a lane, | ||
| # decide which of the two sets it is testing and say so. | ||
| default = ["media", "skills", "flows", "mcp", "channels", "medulla", "http-server", "scheduler-gate", "file-logging"] |
There was a problem hiding this comment.
Account for meet being dropped from the default feature set
The diff drops meet from the default feature set alongside web3, documents, voice/inference, contacts, crash-reporting, tui, and runtime-node, but the explanatory comment enumerating the turned-off gates does not mention meet at all. The stated rationale is that only the gates that "carry the graph" (expensive native-build cohorts) are turned off, and everything else stays on because it is cheap to compile. If meet is cheap it should remain in default; if it is expensive it should be listed alongside the other cohorts. As written, the comment and the feature list disagree, and a contributor reading the comment has no way to know that meet was intentionally excluded.
default = ["media", "skills", "flows", "mcp", "channels", "medulla", "http-server", "scheduler-gate", "file-logging"]Old default contained meet; the new one does not, and no comment accounts for it.
[RULE] New Cargo domain features must be forwarded to app/src-tauri/Cargo.toml's features list, or explicitly added to INTENTIONALLY_NOT_FORWARDED with a reason. ·
|
|
||
| ```bash | ||
| scripts/kernel-floor.sh flows # CI Linux: 312 packages / 285 names / 6 native | ||
| scripts/kernel-floor.sh flows # CI Linux: 304 packages / 281 names / 3 native |
There was a problem hiding this comment.
kernel-floor example output no longer matches the stated CI baseline
The inline example and the CI baseline were identical in the old text (both 312 / 285 / 6) but now disagree. The example comment reads 304 packages / 281 names / 3 native while the baseline paragraph below it reads 302 packages / 279 unique names / 2 native builds. One of these is stale — likely the example, which appears to reflect an intermediate state (before aws-lc-sys left) rather than the final 2026-08-09 baseline. A reader copying the example to verify the ratchet will get a mismatch and not know which number to trust.
**[RULE] ** ·
| // Guards the guard: empty input would make every assertion below vacuous. | ||
| assert.ok(productFeatures.length > 0, 'expected to parse at least one product gate'); | ||
| assert.ok(coreFeatureNames.length > 0, 'expected to parse at least one core gate name'); | ||
| const result = checkProductForwarding({ productFeatures, coreFeatureNames, shell }); |
There was a problem hiding this comment.
Assert result.ok in the real-manifest equality test
This integration test checks that the real product file and shell agree, but it only asserts on missing, unexpected, and unknown. If the real shell has defaultFeatures: true, checkProductForwarding returns ok: false with reason: 'shell-inherits-defaults' (as demonstrated by the unit test at line 210). In that scenario missing, unexpected, and unknown can all be empty arrays, which would make all three deepEqual assertions pass vacuously — the test would report success even though the guard is actually failing. Add assert.equal(result.ok, true) (or at minimum assert result.reason is undefined) so the test cannot pass when the shell inherits defaults.
[RULE] Prefer behavior over implementation ·
| pub mod ops { | ||
| use super::*; | ||
|
|
||
| /// See [`super::super::ops::auto_snapshot_after_sync`]. |
There was a problem hiding this comment.
Add a test asserting the memory-git off-stub returns Err, not Ok-empty
The memory-git-off stub in src/openhuman/memory/diff/stub.rs returns Err from all three ops entry points, and the stub's own doc-comment explains why this matters: an empty CrossSourceDiff would tell the subconscious profile "your world did not change" and it would act on that, whereas Err makes it log and skip. That is a load-bearing behavioural choice with no test behind it. The two tests added in src/core/all_tests.rs assert controller registration/absence — they do not call auto_snapshot_after_sync, create_checkpoint, or diff_since_checkpoint and assert they return Err. If someone later changed Err(DISABLED.to_string()) to Ok(Snapshot::default()) or Ok(CrossSourceDiff::default()), no test would fail, and the failure mode is exactly the silent "nothing changed" report the stub exists to prevent. The #[cfg(not(feature = "memory-git"))] test lane is where this belongs — a test that calls each stub entry point and asserts it returns Err containing the feature name.
[RULE] Tests that hard-assert a gated family must be #[cfg]-gated in lockstep with the feature. ·
What this change touches23 files, +1003 -77 across 10 components. The code graph knows nothing about these files yet — normal for newly added files, and a cold index otherwise. flowchart LR
n0["scripts/ci<br/>4 files +208 -19"]:::changed
n1["src/openhuman<br/>8 files +179 -5<br/>1 finding"]:::flagged
n2["root<br/>2 files +156 -27<br/>2 findings"]:::flagged
n3["scripts/__tests__<br/>1 file +158 -3<br/>2 findings"]:::flagged
n4["scripts/lib<br/>1 file +138 -3"]:::changed
n5[".github/workflows<br/>2 files +49 -5"]:::changed
n6["scripts<br/>2 files +44 -2"]:::changed
n7["src/core<br/>1 file +44 -0"]:::changed
n8["docs<br/>1 file +23 -13"]:::changed
n9["app/src-tauri<br/>1 file +4 -0"]:::changed
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. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
…nTakes the kernel profile to **2 native builds — the spec target** (G6 in\nMIGRATION-PLAN, and the goal named in `kernel-floor.limits`' own baseline\nentry). `git2` with vendored libgit2 leaves and takes `libgit2-sys` **and**\n`libz-sys` with it; together with the `runtime-node` gate already on main, only\n`libsqlite3-sys` and `ring` remain, and both are load-bearing. Kernel floor\n305/282/4 -> 302/279/2, from 418 names / 6 native when the program started.\n\n`memory-git` is default-OFF, product-ON, carrying `dep:git2` plus tinycortex's\n`git-diff` and `wiki-git`.\n\n## The type carve-out is what makes this gateable\n\n`memory::diff::types` compiles in BOTH builds. The always-on subconscious\nmemory profile renders `CrossSourceDiff` and `ChangeKind` into agent prompts,\nso stubbing them would put two definitions of one serde shape in the tree, free\nto drift. tinycortex needed the matching split first — its `git-diff` gated the\nwhole `memory::diff` module, so a libgit2-less host could not name the types at\nall. Its `memory::diff::{types,source}` are ungated now (they reach no `git2`\nsymbol; only `ledger.rs` and `ledger_helpers.rs` do) and the\n`Ledger`/`DiffEngine` half stays behind the feature. Gitlink bumped to it.\n\n## Stub vs `#[cfg]`, decided per call site\n\nStubbed: the three `ops` entry points always-on code reaches —\n`auto_snapshot_after_sync` (`memory::sources::sync`), `diff_since_checkpoint`\nand `create_checkpoint` (the subconscious profile). Those domains stay\nfeature-unaware.\n\nThey return an `Err` naming the gate, not an empty diff. An empty\n`CrossSourceDiff` asserts "your world did not change", which the subconscious\nwould act on; the error routes into a path the caller already handles by\nlogging and skipping.\n\n`#[cfg]`'d: the registration sites, which want absence. The schema aggregators\nreturn empty vecs (`memory_diff` becomes unknown-method and drops off\n`/schema`) and `MemoryDiffTool` disappears from its single registration site —\na registered tool that always errors is worse than an absent one, because the\nmodel keeps choosing it and reporting the failure to the user.\n\n## Capability honesty\n\nThe embedded driver drops `Capability::Diff` from `advertised_capabilities()`\nand `as_diff()` returns `None`. Both halves move together deliberately:\n`audit_provider` fails on either alone, which is the check that keeps them from\ndrifting, and callers may trust the advertised set rather than probing every\naccessor.\n\n## The wiki mirror degrades, it does not fail\n\n`content::wiki_git::commit_summaries` is gated at its two call sites rather\nthan stubbed. The git wiki is a derived view — the summary's own content file\nis written either way — so skipping it loses the mirror, not the summary.\n`seal.rs`'s `summary_committed` gains a `#[cfg(not(...))]` sibling returning\n`Ok(())`: accurate rather than lenient, since nothing the caller depended on\nfailed to happen. `tests/memory_artifacts_e2e.rs` opens the ledger with\n`git2::Repository::open`, so it declares `required-features = ["memory-git"]`.\n\n## Review follow-ups from tinyhumansai#5477 also folded in\n\n- `scripts/ci/product-features.sh` died with a bare exit 1 and no message on a\n comments-only file: `grep -v` exits 1 when it selects nothing, and `set -e`\n aborted inside the command substitution, making the explicit diagnostic\n unreachable. Fixed, and the helper now takes an optional path so the\n self-test can drive that path with a fixture rather than mutating the\n checked-in file. Two regression tests.\n- Stale figures in AGENTS.md corrected against measurement (contributor\n 353/3-native, product 540/7-native, kernel baseline 302/279/2).\n- `docs/library-minimal-recipe.md`'s obsolete single `Default` column replaced\n with the real Contrib/Product pair rather than annotated as wrong.\n\n
…\n\nbuild(memory): gate the git-backed diff ledger behind `memory-git`\n
Why
The largest native-build shed in the kernelization program.
git2with vendored libgit2 leaves the kernel profile and takeslibgit2-sysandlibz-syswith it — two of the five remaining native C builds, in one gate.flows)default)That is the target. MIGRATION-PLAN G6 set two native builds as the goal, and
scripts/kernel-floor.limitshas carried it as the stated aim since the 2026-08-01 baseline of 418 names / 6 native. What remains islibsqlite3-sysandring— the memory store and TLS, both load-bearing. Reaching it does not retire the ratchet; an unmeasured floor grows back.(The before-column moved because
maingained theruntime-nodegate while this was open, which shedlzma-sys.memory-gitsheds the other two.)memory-gitis default-OFF, product-ON, and carriesdep:git2+ tinycortex'sgit-diffandwiki-git.The type carve-out is what makes this gateable at all
memory::diff::typescompiles in both builds. The always-on subconscious memory profile rendersCrossSourceDiffandChangeKindinto agent prompts, so stubbing them would put two definitions of one serde shape in the tree, free to drift apart.tinycortex needed the matching split first — its
git-diffgated the wholememory::diffmodule, so a libgit2-less host could not so much as name the types. tinyhumansai/tinycortex#141 leavesmemory::diff::{types,source}ungated there (they areserde/std-only; onlyledger.rsandledger_helpers.rstouch git2) and keeps theLedger/DiffEnginehalf behind the feature.Stub vs
#[cfg], decided per call siteStubbed — the three
opsentry points always-on code reaches:auto_snapshot_after_sync(memory::sources::sync),diff_since_checkpointandcreate_checkpoint(the subconscious profile). Those domains stay feature-unaware.They return an
Errnaming the gate, not an empty diff. An emptyCrossSourceDiffasserts "your world did not change" — which the subconscious would faithfully act on. The error routes into a path the caller already handles by logging and skipping.#[cfg]'d — the registration sites, because they want absence: the schema aggregators return empty vecs (memory_diffbecomes unknown-method and drops off/schema) andMemoryDiffTooldisappears from its single registration site. A registered tool that always errors is worse than an absent one; the model keeps choosing it and reporting the failure back to the user.Capability honesty
The embedded driver drops
Capability::Difffromadvertised_capabilities()andas_diff()returnsNone. Both halves move together deliberately —audit_providerfails on either alone, which is the check that keeps them from drifting, and callers are entitled to trust the advertised set rather than probing every accessor.The wiki mirror degrades, it does not fail
content::wiki_git::commit_summariesis gated at its two call sites rather than stubbed. The git wiki is a derived view: the summary's own content file is written either way, so skipping the mirror loses the mirror, not the summary.seal.rs'ssummary_committedgains a#[cfg(not(...))]sibling returningOk(())— accurate rather than lenient, since nothing the caller depended on failed to happen.tests/memory_artifacts_e2e.rsopens the ledger withgit2::Repository::open, so it declaresrequired-features = ["memory-git"].Tests
Both-ways tests in
core::all_tests. The OFF half assertsmemoryis still registered alongsidememory_diffbeing gone — the gate removes the git ledger, not the memory domain — because splitting that into two tests would let one regress silently.Verification
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Round-1 review feedback
product-features.shempty-list diagnostic unreachablegrep -vexits 1 when it selects nothing, andset -eaborted inside the command substitution — a comments-only file exited 1 with no output at all. Now{ grep -v ... || true; }, with the reason commented. The helper also takes an optional path so the self-test can drive that branch with a fixture instead of mutating the checked-in file; two regression tests added.library-minimal-recipe.mdobsoleteDefaultcolumndesktop-automationgets dashes — that gate no longer exists.provider_tests.rsbus commentmain.tinyhumansai/tinycortex#141 also took its two review points: the carve-out tests moved to a
#[path]sibling file matching that directory's convention, and the serde test now round-trips instead of only serialising — so a gated-awayDeserializederive fails it too.