Repository navigation
Conversation
init_master_key returns () on main. The call site from the session-store ownership change used the Result-returning variant, so the core did not compile.
Ingested::job is now Option (None when the engine consolidates on its own), so only a present job is enqueued. EngineSettings gained consolidation; None keeps the engine default.
CreateConversationThread gained working_dir in the pinned tinyagents; live voice threads have none.
vendor/tinymemory is already at 1.23.4; the lockfile still named 1.23.1.
…he legacy backend key Other sections legitimately write backend keys (observability, computer, voice) and the migration's own legacy_backend_unsupported marker, so a substring check over the whole file always failed.
Tiny Sweeper reviewTiny Sweeper completed its review; deterministic results follow. State: Changes requested Review snapshot
Completeness: Complete What changedNo supported behavioral explanation was produced. Features
Tests
Findings
Previously reported and still active
Resolved this pass
Pending checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS) Before merge
How this fits togetherflowchart LR
n0["CoreContext<br/>changed"]:::changed
n1["Runtime<br/>changed"]:::changed
n2["DomainSet"]:::impacted
n3["join"]:::impacted
n4["new"]:::impacted
n5["WorkspaceBinding"]:::impacted
n6["init_with_config"]:::impacted
n0 -->|uses| n2
n0 -->|uses| n5
n1 -->|uses| n2
n4 -->|uses| n2
n6 -->|uses| n0
n6 -->|uses| n2
n6 -->|calls| n3
n6 -->|calls| n4
n6 -->|uses| n5
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
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe changes update core configuration, runtime, memory, and live voice code. They also adjust Rust coverage lane dependencies and the coverage script’s Cargo toolchain selection. ChangesCore updates
Rust coverage CI
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change restores the core build and adjusts the Rust coverage CI lane prerequisites. No concrete merge-blocking risk was identified in the supplied evidence. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The CI execution environment changes, but existing isolation controls remain in place. No introduced security issue was established. Runner permissions and background-job recovery behavior remain incompletely verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
A rabbit checks the lanes at dawn Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0035 · 205,354 in / 10,174 out · 25,600 cached (12%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0015 · 95,581 in / 4,005 out · 12,184 cached (13%) · gpt-5.6-luna
security: $0.0012 · 80,842 in / 2,844 out · 8,936 cached (11%) · gpt-5.6-luna
tests: $0.0001 · 11,400 in / 937 out · 3,072 cached (27%) · glm-5.3-flash
description: $0.0000 · 6,207 in / 97 out · 1,408 cached (23%) · glm-5.3-flash
e2e: $0.0001 · 7,526 in / 681 out · 0 cached (0%) · glm-5.3-flash
| .ingest_with(document, WriteOptions::accepted()) | ||
| .await?; | ||
| jobs::enqueue(config, layout(config).root(), vec![ingested.job]).await; | ||
| if let Some(job) = ingested.job { |
There was a problem hiding this comment.
Add a test that ingest with no job does not enqueue
The guard changes behaviour: when ingest_with yields no job, jobs::enqueue is no longer called. Previously the job list was enqueued unconditionally, so a regression back to the old call (or a future refactor of ingested.job into an Option-unwrapping helper) would silently enqueue nothing or panic. Nothing in the diff or existing tests pins either half of the contract: that a job-producing ingest still enqueues, and that a job-less ingest does not. A sibling test asserting the jobs queue is empty (and, for the job-producing path, contains the ingested job) would fail if this regresses.
[RULE] missing-test-coverage ·
tinysweeper
Bot
added
the
priority: p2
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0163 · 321,480 in / 22,266 out · 29,410 cached (9%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0083 · 154,150 in / 10,531 out · 16,775 cached (11%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0076 · 129,294 in / 7,963 out · 12,507 cached (10%) · gpt-5.6-luna
tests: $0.0001 · 5,671 in / 411 out · 0 cached (0%) · glm-5.3-flash
description: $0.0001 · 6,519 in / 378 out · 0 cached (0%) · glm-5.3-flash
e2e: $0.0002 · 16,249 in / 1,255 out · 64 cached (0%) · glm-5.3-flash
| .unwrap(); | ||
| assert!(saved.contains("engine = \"\"")); | ||
| assert!(!saved.contains("backend")); | ||
| let saved: toml::Table = toml::from_str(&saved).unwrap(); |
There was a problem hiding this comment.
Add a test that ingest with no job does not enqueue
This revision only parses the saved TOML and checks the migrated memory fields; it still does not exercise the ingest path with no job or assert that no enqueue occurs. The previously reported behavior remains unprotected by a regression test, so add a focused test covering that case.
[RULE] missing-test ·
| // needs to decrypt secrets. No-op if already called (e.g. from | ||
| // run_core_from_args for the CLI). | ||
| crate::security::keyring::init_master_key().map_err(anyhow::Error::msg)?; | ||
| crate::security::keyring::init_master_key(); |
There was a problem hiding this comment.
Add a test that ingest with no job does not enqueue
This revision only changes keyring initialization and does not address the previously identified case where ingest without a job must not enqueue work. Keep the existing concern and add a regression test covering that behavior.
[RULE] missing-test ·
| parent_thread_id: None, | ||
| labels: None, | ||
| personality_id: None, | ||
| working_dir: None, |
There was a problem hiding this comment.
Add a test that ingest with no job does not enqueue
This revision does not add the previously requested regression test proving that ingest without a job does not enqueue work. The behavior remains unprotected against regressions; add the test in the appropriate sibling test module. This is reported as a late finding because the pull request changed only the thread initializer, not the ingest path, and the gap is unchanged from the earlier review.
[RULE] missing-regression-test ·
| let mut installed = PROVIDER | ||
| .write() | ||
| .unwrap_or_else(PoisonError::into_inner); | ||
| let mut installed = PROVIDER.write().unwrap_or_else(PoisonError::into_inner); |
There was a problem hiding this comment.
Add a test that ingest with no job does not enqueue
The earlier regression-test concern is still unresolved: this revision adds no test covering the no-job ingest path and its enqueue behavior. Keep the test in the sibling test module so a future change cannot regress this contract unnoticed.
[RULE] missing-regression-test ·
| .ingest_with(document, WriteOptions::accepted()) | ||
| .await?; | ||
| jobs::enqueue(config, layout(config).root(), vec![ingested.job]).await; | ||
| if let Some(job) = ingested.job { |
There was a problem hiding this comment.
Add a test that ingest with no job does not enqueue
This new branch changes queueing behavior when ingestion returns no background job, but the diff adds no regression test covering that case. Add a test that verifies ingestion with no job leaves the queue unchanged, so a future refactor cannot reintroduce an empty or invalid enqueue.
Additional e2e observation
Ingest with no job has no end-to-end test
[RULE] e2e-uncovered
This change alters observable behaviour: when ingested.job is None, nothing is enqueued, where previously an enqueue always happened. No end-to-end test drives this. A test would have to run the system end to end, ingest a document that produces no job (the replayed/duplicate path), and assert that no job was added to the jobs queue — for example via an RPC surface that lists pending jobs, or by checking the jobs store after an ingest through the running core. The lexical candidates mentioning 'memory' (chat-agent-plan.spec.ts) do not touch ingest or the jobs queue.
[RULE] missing-test ·
| parent_thread_id: None, | ||
| labels: None, | ||
| personality_id: None, | ||
| working_dir: None, |
There was a problem hiding this comment.
Add a test that ingest with no job does not enqueue
The missing-thread test still only awaits save and does not inspect the resulting job queue. A regression that enqueues work despite having no thread would pass unnoticed; assert that the queue remains empty after this save.
Additional critique observation
Add a test that ingest with no job does not enqueue
[RULE] missing-behavior-test
This fixture update keeps the missing-thread test limited to verifying that save returns without failing. It still does not assert that ingesting a transcript without an associated job leaves the job queue unchanged, so a regression that enqueues work in this path would pass the suite. Add an assertion against the relevant queue/store after the no-thread save.
[RULE] missing-regression-test ·
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…skills 0.2.8 and tinymemory 1.23.4 The root lockfile moved with those pins; crates/openhuman-app/Cargo.lock did not, so the desktop --locked dependency check failed.
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0087 · 182,836 in / 12,755 out · 15,289 cached (8%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0042 · 71,992 in / 5,459 out · 9,932 cached (14%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0040 · 69,648 in / 3,545 out · 5,357 cached (8%) · gpt-5.6-luna
tests: $0.0001 · 6,936 in / 145 out · 0 cached (0%) · glm-5.3-flash
description: $0.0001 · 8,211 in / 376 out · 0 cached (0%) · glm-5.3-flash
e2e: $0.0001 · 9,307 in / 826 out · 0 cached (0%) · glm-5.3-flash
| # to create ~/.rustup inside the write-confined jail. Put the already selected | ||
| # toolchain binaries first; cargo --version then needs no rustup home writes. | ||
| # Keep cargo-installed subcommands on PATH after the toolchain directory. | ||
| if command -v rustup >/dev/null 2>&1; then |
There was a problem hiding this comment.
Resolve the toolchain without depending on RUSTUP_HOME
When RUSTUP_HOME is absent, rustup which cargo normally looks for its metadata under the default $HOME/.rustup. In the sandbox described by this script, that directory is not where the container's toolchain is installed, so the command fails, the conditional silently skips the PATH update, and subsequent cargo invocations still resolve to the rustup proxy and may try to write the confined home directory. Use the container's configured toolchain path directly or explicitly provide the correct rustup home before resolving cargo; do not silently continue when resolution fails.
[RULE] rustup-home-resolution ·
| } | ||
| }); | ||
|
|
||
| test("a core-only change installs the node deps the mock backend needs before rust coverage", () => { |
There was a problem hiding this comment.
Add an end-to-end test for ingest with no job
This new test only verifies CI dependency ordering; it does not exercise the previously identified ingest-without-a-job path. That path still lacks an end-to-end regression test, so the behavior can regress without detection. Add coverage that invokes ingest with no job and asserts that no enqueue occurs.
[RULE] missing-end-to-end-test ·
tinysweeper
Bot
added
priority: p1
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0030 · 87,531 in / 8,881 out · 11,184 cached (13%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0014 · 29,061 in / 2,428 out · 5,862 cached (20%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0012 · 16,541 in / 2,607 out · 5,130 cached (31%) · gpt-5.6-luna
tests: $0.0001 · 7,095 in / 502 out · 0 cached (0%) · glm-5.3-flash
description: $0.0001 · 8,385 in / 471 out · 64 cached (1%) · glm-5.3-flash
e2e: $0.0001 · 9,466 in / 552 out · 64 cached (1%) · glm-5.3-flash
| # toolchain binaries first; cargo --version then needs no rustup home writes. | ||
| # Keep cargo-installed subcommands on PATH after the toolchain directory. | ||
| if command -v rustup >/dev/null 2>&1; then | ||
| if COV_TOOLCHAIN_CARGO="$(rustup which cargo 2>/dev/null)"; then |
There was a problem hiding this comment.
Resolve the toolchain without depending on RUSTUP_HOME
When RUSTUP_HOME is unset and the rustup proxy is on PATH, rustup which cargo can itself fail because it cannot locate the rustup toolchain state. This takes the else branch, which explicitly retains the proxy on PATH; the subsequent cargo invocations then retry the same proxy and may attempt to create ~/.rustup inside the write-confined sandbox. The fallback needs to locate or receive the already-installed toolchain binaries without invoking rustup state, rather than continuing with the proxy.
Additional security observation
Resolve the toolchain without depending on RUSTUP_HOME
[RULE] toolchain-resolution
When RUSTUP_HOME is unset, rustup which cargo normally cannot locate the installation outside HOME. The fallback explicitly leaves the rustup proxy on PATH, which then tries to access or create ~/.rustup inside the write-confined sandbox and can make the coverage job fail. Resolve the installed toolchain through the container's known toolchain location, or fail before invoking the proxy rather than treating this fallback as successful.
[RULE] toolchain-resolution ·
| .ingest_with(document, WriteOptions::accepted()) | ||
| .await?; | ||
| jobs::enqueue(config, layout(config).root(), vec![ingested.job]).await; | ||
| if let Some(job) = ingested.job { |
There was a problem hiding this comment.
Ingest with no job still has no end-to-end test
The behavioural change — ingest now skips jobs::enqueue when ingested.job is None — has an external surface: an ingested document whose job is not queued must not produce background processing, while the receipt/view is still returned. No end-to-end harness in this diff or the candidate list drives ingest through the running core and asserts that no job appears (or that enqueue is skipped) for the no-job case; the candidates only mention memory in routing strings. A test would have to ingest a document through the running system and observe the job queue. Carried over from the earlier review at the same severity; the code is unchanged since.
[RULE] e2e-uncovered ·
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0026 · 84,399 in / 7,514 out · 9,428 cached (11%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0011 · 22,191 in / 1,848 out · 5,862 cached (26%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0009 · 12,241 in / 2,154 out · 3,566 cached (29%) · gpt-5.6-luna
tests: $0.0001 · 7,075 in / 397 out · 0 cached (0%) · glm-5.3-flash
description: $0.0001 · 8,362 in / 450 out · 0 cached (0%) · glm-5.3-flash
e2e: $0.0001 · 9,446 in / 326 out · 0 cached (0%) · glm-5.3-flash
| # to create ~/.rustup inside the write-confined jail. Put the already selected | ||
| # toolchain binaries first; cargo --version then needs no rustup home writes. | ||
| # Keep cargo-installed subcommands on PATH after the toolchain directory. | ||
| if command -v rustup >/dev/null 2>&1; then |
There was a problem hiding this comment.
Resolve the toolchain without depending on RUSTUP_HOME
This still invokes rustup which cargo before adding the toolchain directory to PATH. When RUSTUP_HOME is omitted and the rustup metadata is installed outside $HOME, rustup cannot find the selected toolchain and this branch exits with an error; the script therefore still cannot run in the sandbox described by the comment. Resolve the toolchain from the container's known installation or otherwise arrange a toolchain path without requiring rustup metadata in the confined home.
Additional security observation
Resolve Cargo without depending on RUSTUP_HOME
[RULE] rustup-home-dependency
rustup which cargo is itself a rustup operation and uses rustup's toolchain configuration, which is located through RUSTUP_HOME (or the default under HOME). In the sandbox described by this script, that configuration is not forwarded and may not exist at the default location, so this branch exits before coverage can run—the same toolchain-resolution failure this change is intended to avoid. Locate the already-installed toolchain directly or otherwise provide a rustup-independent Cargo path instead of invoking rustup which.
[RULE] toolchain-resolution ·
| .ingest_with(document, WriteOptions::accepted()) | ||
| .await?; | ||
| jobs::enqueue(config, layout(config).root(), vec![ingested.job]).await; | ||
| if let Some(job) = ingested.job { |
There was a problem hiding this comment.
Ingest with no job has no end-to-end test
The diff changes enqueue to run only when a job exists (if let Some(job) = ingested.job { jobs::enqueue(...) }), but no end-to-end test exercises this path. An end-to-end test would have to drive an ingest that produces no job and assert no file appears under layout(config).root() — the branch's behavioural surface (whether the jobs queue stays empty when the engine consolidation is disabled) is only reachable through the running system. Coverage so far is unit-level only.
[RULE] missing-e2e-coverage ·
|
Superseded by #7058, which restored main (compile fixes, keyring hardening, CI node install and toolchain PATH, app lockfile). Closing. |
1 participant
Summary
mainat8c9c480a4edoes not compileopenhuman-core: six errors, all semantic collisions between changes that merged on 2026-10-07. Each was correct on its own branch. This PR fixes all six without changing behaviour.It also fixes one test that came in with those merges but never ran, because the crate did not build.
It applies rustfmt to the two files
cargo fmt --checkrejects onmain.Merged with
mainafter chore(vendor): pin tinymcp v0.4.0 and tinyskills v0.2.8 #7054. That PR already broughtCargo.lockto tinymemory 1.23.4, so this PR's lockfile commit is now a no-op and the lockfile matchesmain.Restores the CI fix that merge 89e341c dropped (from 2680111):
rust-core-coverageinstalls node deps on a core-only change again. Without themscripts/mock-api-server.mjscannot importws, and everymemory_v2_e2etest times out waiting for the mock backend.rust-coverage.shputs the selected toolchain first onPATHagain, for the sandboxed acting-tool tests, and stops with an error when rustup cannot resolve it.Refreshes
crates/openhuman-app/Cargo.lockfor tinymcp 0.4.0, tinyskills 0.2.8 and tinymemory 1.23.4. The desktop--lockeddependency check failed because only the root lockfile had moved.Problem
main(for example chore(vendor): pin tinymcp v0.4.0 and tinyskills v0.2.8 #7054), because the core build stops at:core/runtime/context.rs:231:init_master_key().map_err(..)?, butinit_master_keyreturns()onmain.memory/brain.rs:251: tinymemory 1.23.4 madeIngested::jobanOption<BackgroundJob>.memory/engine.rs:254: tinymemory 1.23.4 addedEngineSettings::consolidation.voice/live/session.rs:105andvoice/live/persist_tests.rs:30: the pinned tinyagents addedCreateConversationThread::working_dir.config/schema/types/config_clone.rs:14: the hand-writtenConfigclone has novoice_livefield.mainstayed green, so the break was not visible frommainitself.Solution
context.rs: callinit_master_key()the way the keyring defines it. The Result-returning keyring hardening from 092787c is inmain's history, but its content is not in the current tree (encrypted_file_backend.rs,lib.rsandopenhuman-tuiall use the()form). Restoring it is out of scope for a compile fix.brain.rs: enqueue the belief build only when the engine returns one.Nonemeans the engine consolidates on its own.engine.rs:consolidation: None, which keeps the engine's default (tinyhumansis alwaysscheduled).working_dir: None, since a voice conversation has no working directory.config_clone.rs: clonevoice_live.voice/live/ws_tests.rs: drop afuturesimport that the parent glob already brings in. It produced an unused-import warning.config/schema/load_migration_tests.rs:load_or_init_disables_and_persists_legacy_memory_backendasserted the saved file contains nobackendsubstring anywhere. Other sections legitimately writebackendkeys (observabilitybackend = "otel", computerbackend = "tinycomputer", voicemode = "backend"), and so does the migration's ownlegacy_backend_unsupportedmarker. It now parses the TOML and checks the[memory]table:engine = ""and nobackendkey.Submission Checklist
[memory]table it is about.Impact
main.Related
main, including the TinyHiveMind and OpenCompany pins that follow chore(vendor): pin tinymcp v0.4.0 and tinyskills v0.2.8 #7054AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check)pnpm typecheck)cargo test -p openhuman --lib --features "$(bash scripts/ci/product-features.sh)"withRUST_MIN_STACK=67108864: 8304 passed.secure_opensymlink tests: macOS tempdirs sit behind the/var→/private/varsymlink;sandbox::grants/proc: no/procon macOS;cargo test -p openhuman-embed: all pass.cargo test -p openhuman-cli --features "<product>,bin-tools" --no-run: builds.cargo fmt --all -- --checkclean;cargo clippy -p openhuman -p openhuman-cli -p openhuman-tinyhumans -- -D warningswith the product features is clean.Validation Blocked
command:cargo test -p openhuman-cli --features "<product>,bin-tools" --test in_process_all -- domain_modules_e2e::legacy_memory_backend_is_off_and_persisted_through_json_rpcerror:memory_engine_getreturns{"engine":"tinyhumans","status":"off","reason":"no TinyHumans backend is available"}after the test writes a legacy[memory] backend = "sqlite"config. The RPC is not reading the file the test wrote (it is the onlyconfig.tomlunder the harness$HOME). The unit-level migration test passes.impact:this one in-process test, added with fix(memory): keep legacy local profiles off hosted memory #7041, still fails and is not addressed here. Which config the in-process RPC resolves after the session-store and context changes is for the owner to confirm.Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit