Skip to content

feat(bench): embedded-RSS benchmark harness + report-only CI (#5046)#5060

Merged
senamakel merged 11 commits into
tinyhumansai:mainfrom
YellowSnnowmann:feat/embedded-budget-benchmark-5046
Jul 21, 2026
Merged

feat(bench): embedded-RSS benchmark harness + report-only CI (#5046)#5060
senamakel merged 11 commits into
tinyhumansai:mainfrom
YellowSnnowmann:feat/embedded-budget-benchmark-5046

Conversation

@YellowSnnowmann

@YellowSnnowmann YellowSnnowmann commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds the reproducible steady-state RSS benchmark harness the 20–30 MiB embedded budget (Keep embedded OpenHuman within a 20–30 MiB RAM budget for OpenCompany #5046) is measured against, plus a report-only CI lane.
  • New ungated openhuman::proc_metrics lib module: Linux /proc/self/{status,smaps_rollup} sampling + roster aggregation + serde report + human summary.
  • New rss-bench binary (gated behind a default-OFF rss-bench feature) that mirrors the OpenCompany embedding contract: a bare Agent built via Agent::builder() — no CoreBuilder, RPC, or background services.
  • Report-only rust-rss-bench CI job uploads raw samples + a summary; it is deliberately not in pr-ci-gate.needs, so it can never fail a PR.
  • Default/shipped build is byte-identical (feature default-OFF, no new dependencies).

Problem

To hold embedded openhuman_core within the ≤20 MiB target / ≤30 MiB hard steady-state RSS cap for OpenCompany tenants, the budget must first be measurable and reproducible. There is no existing smaps/RSS harness in the repo, and the sibling gate/shed PRs (#5048/#5049/#5051) need a common baseline to measure their savings against. Per the issue, the measurement must land report-only first to establish runner variance before the gate becomes blocking.

Solution

  • proc_metrics: OS-agnostic parse_status / parse_smaps_rollup (&str in — unit-tested without a live /proc); sample_self() is Linux-only and returns a structured error off-Linux rather than fabricating a reading. Reports RSS/PSS/private-clean+dirty/VmHWM/threads/binary-size, aggregated per roster (median RSS, min/max/mean, peak VmHWM).
  • rss-bench: builds 1-agent and 8-agent rosters with an injected mock provider (no network), "none" memory backend, and a per-agent temp workspace; runs one deterministic warm-up turn per agent, settles, then measures 5 fresh child processes per roster (parent re-execs itself). Emits a raw JSON artifact + a Markdown summary.
  • Placement rationale: the pure logic lives in an ungated lib module so the existing coverage lane enforces it; only the Agent-dependent fixture + driver live in the gated bin, keeping benchmark code out of the shipped build.
  • The exact one-line seams to later flip the gate to blocking at 30 MiB (add the job to pr-ci-gate.needs + a threshold step) are documented in the workflow comment; left report-only in this PR per the issue.

Submission Checklist

  • Tests added or updated — proc_metrics has 9 unit tests (parsers, aggregation, serde round-trip, non-Linux error path); the gated bin has 2 fixture tests (roster construction + a warm-up turn that completes with no network).
  • Diff coverage ≥ 80% — the load-bearing logic is in the ungated proc_metrics module, covered by its unit tests via the standard --lib coverage lane; the thin gated bin driver runs its fixture tests in the rust-rss-bench job.
  • Coverage matrix updated — N/A: internal benchmark/CI infrastructure, no user-facing feature row.
  • All affected feature IDs listed — N/A: no matrix feature IDs.
  • No new external network dependencies — no new crates; the benchmark's mock provider makes zero network calls.
  • Manual smoke checklist updated — N/A: does not touch a release-cut surface (default-OFF feature).
  • Linked issue closed via Closes #NNNN/A: foundation slice (harness + report-only CI). The budget itself is met as Add the missing inference gate so whisper-rs and cpal stop linking into every build #5048/Gate the desktop-automation cluster, then extract it out of the core #5049/Remove zero-reference dependencies and unwired domains #5051 land and the gate flips blocking; see ## Related.

Impact

  • CLI/build: adds an opt-in rss-bench binary and a report-only Linux CI job. No runtime impact on the shipped desktop/library build (default-OFF, byte-identical, no new deps).
  • Performance/security: none — measurement-only; the harness performs no network, model loading, or background work beyond the agent turn it drives.

Related

Changes since initial submission (review-driven)

  • Per-agent RSS increment reported (bdf02f7b7) — BenchReport::per_agent_increment_kib() derives the marginal cost of agents 2–8 from the roster spread; addresses the "bounded per-agent growth" criterion.
  • Per-child timeout + reap + flow diagnostics (1ab0459cd, CodeRabbit) — each child spawns with kill_on_drop and a 120s timeout around wait_with_output, so a stalled child is killed/reaped instead of blocking the run; stderr-only diagnostics for spawn/timeout/exit + child build/warm-up/settle/sample.
  • Real no-op Memory fixture (56d30941f, Codex — real bug) — create_memory(MemoryConfig{ backend:"none" }) does not select a no-op backend (create_memory_full always builds a UnifiedMemory + cloud embedder), which was inflating the RSS baseline. Replaced with a zero-allocation NoopMemory, which also better mirrors the OpenCompany host-supplied-memory contract.

⚠️ This PR also contains an unrelated #[cfg(test)]-only event-bus fix (src/core/event_bus/bus.rs + src/openhuman/learning/candidate.rs) cherry-picked from #5063 — see the pinned "Note for reviewers" comment. It self-drops on rebase once #5063 merges.


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: feat/embedded-budget-benchmark-5046

Summary by CodeRabbit

  • New Features

    • Added an optional rss-bench command-line benchmark to measure steady-state RSS/PSS and related metrics across varying agent roster sizes.
    • Produces a JSON benchmark report including schema, commit, and kernel/build context, plus an at-a-glance human-readable summary with RSS budget/cap warnings.
  • Chores

    • Introduced a lightweight CI “report-only” job that runs the benchmark, captures bench-rss.json, and uploads it as an artifact without impacting pull-request gating.

…inyhumansai#5046)

Add the reproducible steady-state RSS benchmark that the 20-30 MiB embedded
budget is measured against, plus a report-only CI lane.

- `openhuman::proc_metrics` (ungated): Linux /proc/self/{status,smaps_rollup}
  sampling + roster aggregation + serde report + human summary. OS-agnostic
  parsers are unit-tested; `sample_self()` is Linux-only and errors elsewhere
  rather than fabricating a reading.
- `rss-bench` binary (gated behind the default-OFF `rss-bench` feature): mirrors
  the OpenCompany embedding contract — a bare `Agent` via `Agent::builder()` (no
  CoreBuilder / RPC / background services), injected mock provider + `"none"`
  memory + per-agent temp workspace, 1- and 8-agent rosters, one warm-up turn +
  settle, sampled across 5 fresh child processes per roster.
- Report-only `rust-rss-bench` CI job: uploads raw samples + a human summary;
  deliberately not in `pr-ci-gate.needs`, so it can never fail a PR until the
  30 MiB gate is flipped to blocking in a follow-up.

The default/shipped build is byte-identical (feature is default-OFF, no new deps).
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds Linux process-memory metrics, a feature-gated Rust RSS benchmark for isolated agent rosters, JSON and Markdown reporting, unit tests, and a report-only CI job triggered by rust-core changes.

Changes

RSS benchmark

Layer / File(s) Summary
Process metrics and report contracts
src/openhuman/proc_metrics/mod.rs, src/openhuman/mod.rs
Adds /proc parsers, Linux sampling, roster aggregation, benchmark report types, summaries, thresholds, and tests.
Isolated benchmark child process
Cargo.toml, src/bin/rss_bench.rs
Adds the optional rss-bench binary, deterministic mock-backed agents, isolated workspaces, warm-up and RSS settling, and child-process tests.
Parent aggregation and CLI dispatch
src/bin/rss_bench.rs
Adds argument parsing, child re-execution, timeout and output handling, report aggregation, metadata collection, and optional JSON output.
Report-only CI integration
.github/workflows/ci-lite.yml
Builds, tests, runs, summarizes, and uploads the RSS benchmark report without connecting the job to PR gating.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

  • tinyhumansai/openhuman#5046 — Adds the reproducible embedded-memory benchmark and report-only CI described by the issue.
  • tinyhumansai/openhuman#5048 — Also addresses embedded RSS through Cargo feature and build-target gating.

Suggested labels: feature, rust-core

Poem

I’m a rabbit with a report in my paws,
Measuring memory without network flaws.
Rosters warm, then settle just right,
JSON hops through the CI light.
RSS numbers nibble the cap—
Then upload neatly in one artifact wrap.

Sequence Diagram(s)

sequenceDiagram
  participant CI as CI Lite
  participant Parent as rss-bench parent
  participant Child as rss-bench child
  participant Metrics as proc_metrics
  CI->>Parent: Run benchmark
  Parent->>Child: Execute each roster size and repeat
  Child->>Metrics: Sample settled process RSS
  Metrics-->>Child: Return ProcSample JSON
  Child-->>Parent: Return child output
  Parent->>Parent: Aggregate BenchReport
  Parent-->>CI: Write summary and bench-rss.json
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: an embedded RSS benchmark harness plus a report-only CI lane.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

…inyhumansai#5046)

Add BenchReport::per_agent_increment_kib(), which derives the marginal
steady-state RSS cost of each additional agent from the smallest and largest
rosters — (median_rss(max) - median_rss(min)) / (max_size - min_size). For the
default {1, 8} rosters this is the per-agent cost of agents 2-8, which the human
summary now prints. Returns None when fewer than two distinct roster sizes were
measured. Two unit tests cover the computed value and the single-roster case.

Addresses the issue's "bounded per-agent growth" criterion (the report now
reports the incremental cost, not just the two roster totals).
@YellowSnnowmann
YellowSnnowmann marked this pull request as ready for review July 20, 2026 11:05
@YellowSnnowmann
YellowSnnowmann requested a review from a team July 20, 2026 11:05
@coderabbitai coderabbitai Bot added feature Net-new user-facing capability or product behavior. working A PR that is being worked on by the team. labels Jul 20, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bdf02f7b70

ℹ️ 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".

Comment thread src/bin/rss_bench.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@src/bin/rss_bench.rs`:
- Around line 302-310: Add stderr-only debug/trace logging across
src/bin/rss_bench.rs, including main flow, roster construction, warm-up,
settling, child spawn and completion, report writes, and failures; keep child
stdout limited to its single JSON sample. In src/openhuman/proc_metrics/mod.rs,
trace sampling entry and exit plus read failures without including sensitive
data.
- Around line 203-209: Update the child-process execution in the benchmark flow
around tokio::process::Command to add a per-child timeout. Log child spawn,
timeout, kill, and exit events at debug level; on timeout, kill the child
process and await it afterward to reap it, while preserving contextual errors
for spawn and normal output handling.
🪄 Autofix (Beta)

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

Run ID: e60d26e1-cd86-4a44-be72-5348ce8b94e5

📥 Commits

Reviewing files that changed from the base of the PR and between d13a559 and bdf02f7.

📒 Files selected for processing (5)
  • .github/workflows/ci-lite.yml
  • Cargo.toml
  • src/bin/rss_bench.rs
  • src/openhuman/mod.rs
  • src/openhuman/proc_metrics/mod.rs

Comment thread src/bin/rss_bench.rs Outdated
Comment thread src/bin/rss_bench.rs
tinyhumansai#5046)

From CodeRabbit review:
- Give each benchmark child a 120s wall-clock budget: spawn with kill_on_drop and
  wrap wait_with_output in a timeout, so a stalled child is killed and reaped
  rather than blocking the whole run until the outer CI job timeout.
- Add stderr-only diagnostics for the benchmark flow (child spawn/timeout/exit,
  and roster build/warm-up/settle/sample in the child) so a run is observable;
  kept on stderr so they never corrupt the single JSON line the parent parses
  from stdout.
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 20, 2026
…lake

Under cfg(test) the crate's ~12.6k unit tests share two process-global bounded
resources sized for production: the event bus's 256-slot broadcast channel
(src/core/event_bus/bus.rs) and the learning candidate ring's 1024 entries
(src/openhuman/learning/candidate.rs, whose drainer does not run in the unit
suite). Under the concurrent full-suite publish load a slow subscriber lags and
drops events (RecvError::Lagged), and the candidate ring evicts a test's own
candidates — intermittently failing bus-delivery tests such as
learning::startup::tests::learning_subscriber_fires_with_no_channel_configured.

Raise both floors to 65536 for the TEST BINARY ONLY (cfg(test)); production keeps
256 / 1024 and ships byte-identical. No new dependency, no serialization, no
weakened assertion — the shared globals simply get enough headroom that neither
drops nor evicts under the artificial suite-wide load.
…ated (tinyhumansai#5046)

Codex review: `create_memory(MemoryConfig{ backend: "none", .. })` does not select
a no-op backend — `create_memory_full` always builds a `UnifiedMemory` (SQLite +
the default cloud embedder) regardless of `backend`. So every measured agent was
allocating a memory store + embedder, inflating the 1/8-agent RSS baseline with
memory-store setup rather than measuring the bare agent under the OpenCompany
embedding contract (host supplies its own Memory).

Replace the fixture's `create_memory` call with a zero-allocation `NoopMemory`
that implements the `Memory` trait as inert (store discards, recall/list/get
return empty, count 0). This mirrors the host-supplied-memory contract and keeps
the reading on the agent harness itself.
@coderabbitai coderabbitai Bot added memory Memory store, memory tree, recall, summarization, and embeddings in src/openhuman/memory/. rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure. and removed working A PR that is being worked on by the team. labels Jul 20, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/bin/rss_bench.rs (1)

266-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the parent/child report protocol.

The included tests only exercise roster construction and warm-up; they do not cover valid/invalid child JSON, empty stdout, non-zero exits, or report serialization. Extract the child-output parsing into a unit-testable helper and cover those cases.

As per coding guidelines, “New or changed flows must include … unit-test it in Rust.”

Also applies to: 404-436

🤖 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 `@src/bin/rss_bench.rs` around lines 266 - 338, The run_parent child-output
flow lacks unit-testable coverage for protocol failures and report
serialization. Extract parsing of child status, stdout, and JSON into a
dedicated helper near run_parent, preserving contextual errors for invalid JSON,
empty stdout, and non-zero exits; then add Rust unit tests covering valid JSON,
invalid/empty output, failed status, and BenchReport serialization.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@src/bin/rss_bench.rs`:
- Around line 266-338: The run_parent child-output flow lacks unit-testable
coverage for protocol failures and report serialization. Extract parsing of
child status, stdout, and JSON into a dedicated helper near run_parent,
preserving contextual errors for invalid JSON, empty stdout, and non-zero exits;
then add Rust unit tests covering valid JSON, invalid/empty output, failed
status, and BenchReport serialization.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2085c096-acfd-4f76-b406-d7332e4edc9a

📥 Commits

Reviewing files that changed from the base of the PR and between bdf02f7 and 56d3094.

📒 Files selected for processing (3)
  • src/bin/rss_bench.rs
  • src/core/event_bus/bus.rs
  • src/openhuman/learning/candidate.rs

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 20, 2026
@YellowSnnowmann

YellowSnnowmann commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

📌 Note for reviewers — the event-bus change here is an unrelated, self-healing flake fix

This PR carries a small test-only change that is not part of its objective:

  • src/openhuman/learning/startup.rs (test module)
  • src/openhuman/web_chat/event_bus.rs (test module)

They fix two pre-existing full-suite flakes — learning::startup::tests::learning_subscriber_fires_* and web_chat::event_bus::tests::automation_halt_subscriber_handle_publishes_correct_payload.

Root cause (corrected): not a capacity problem. Both tests asserted against the process-global broadcast bus, so under the ~12,600-test parallel suite they received events published by other concurrent tests — cross-test interference, which no channel-capacity bump can fix. (An earlier capacity-floor attempt was pushed and then reverted here once CI proved it ineffective.)

Fix — real isolation:

  • learning::startup now publishes a DocumentCanonicalized event on a private EventBus::create(64) with a locally-subscribed EmailSignatureSubscriber, instead of the shared global bus. A synchronous register_email_signature_subscriber guard test covers the channel-independent registration path (fix(learning): email-signature subscriber + rebuild trigger silently depend on channels being configured #5003).
  • web_chat::event_bus drains foreign events from the shared web-channel bus and matches on event == "automation_halt" rather than taking whatever is first in the buffer.

Production ships byte-identical — the diff is confined to #[cfg(test)] code; no new deps, no weakened assertion.

Why it's in this PR: this PR changes Cargo.toml, which triggers the full coverage suite where the flake reproduces. #5063 (the canonical standalone fix) only gets scoped coverage and cannot self-verify — so the same commit is cherry-picked here so this PR's coverage lane can go green. It self-drops on rebase once #5063 merges to main.

Pre-existing CI red unrelated to this PR (not merge-blocking — not in pr-ci-gate)

…city floor

The earlier test-only capacity floor (5c49a96) did not fix the flake:
the two failing tests share the process-global broadcast bus, so under the
parallel suite they receive events published by *other* tests — a capacity
bump cannot fix cross-test interference. Replace it with real isolation.

- learning::startup: rewrite `learning_subscriber_fires_*` to publish a
  DocumentCanonicalized event on a private `EventBus::create(64)` with a
  locally-subscribed `EmailSignatureSubscriber`, instead of asserting on the
  shared global bus. Add a synchronous `register_email_signature_subscriber`
  guard test for the channel-independent registration path (tinyhumansai#5003).
- web_chat::event_bus: the automation-halt test reads from the shared
  web-channel bus; drain foreign events and match on `event == "automation_halt"`
  rather than taking whatever is first in the buffer.
- Revert the `#[cfg(test)]` capacity floors in event_bus/bus.rs and
  learning/candidate.rs back to origin/main — no longer needed.
@CodeGhost21

Copy link
Copy Markdown
Contributor

Review — looks solid overall. The proc_metrics / rss-bench split is well-executed: pure sampling/aggregation logic lives in an ungated, unit-tested lib module, while only the Agent-dependent fixture + process driver sit behind the default-OFF rss-bench feature, so the shipped build stays byte-identical with no new deps. Good test coverage (parsers, aggregation, serde round-trip, non-Linux error path, over-cap flagging, per-agent increment). The TempDir lifetime handling in Roster + the explicit drop(roster) after sampling are correct and nicely documented.

CI — both red lanes are unrelated to this PR (branch is behind main):

  • Rust Feature-Gate Smoke fails on > openhuman/composio/action_tool.rs missing from the EXPECTED allowlist. This PR touches no composio code — and main's allowlist already includes openhuman/composio/action_tool.rs. The branch just predates that addition.
  • Rust Core Coverage aborts (exit 101) on a failing composio_credentials_state_raw_coverage_e2e::round15_… assertion (tests/raw_coverage/composio_credentials_state_raw_coverage_e2e.rs:314) — again composio, untouched here. (Note: because the suite aborted, the diff-coverage gate itself never actually ran, so that check is still unevaluated.)

Both are resolved by merging/rebasing latest main, which already carries the corrected allowlist and composio test. No code change owed by this PR to make them green.

Minor nits (non-blocking):

  • PR body says the unrelated test-flake fix touches src/core/event_bus/bus.rs + src/openhuman/learning/candidate.rs, but the diff actually touches src/openhuman/learning/startup.rs + src/openhuman/web_chat/event_bus.rs — worth reconciling the description.
  • next_automation_halt filters only on event == "automation_halt", which can't distinguish a halt from a resume event; a concurrent test's halt could still be consumed by the resumed assertion. Lower flake than the old try_recv(), but consider also matching on reason/source to fully close it.

@coderabbitai coderabbitai Bot removed the memory Memory store, memory tree, recall, summarization, and embeddings in src/openhuman/memory/. label Jul 21, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/openhuman/learning/startup.rs (1)

273-325: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add grep-friendly diagnostics to the new event-isolation paths.

These tests specifically address shared-bus flakes, but failures currently omit the test marker, subscription/publish state, expected versus observed count, and lag/foreign-event details.

  • src/openhuman/learning/startup.rs#L273-L325: trace the unique source ID, isolated subscription/publish, expected candidate count, and final observed count.
  • src/openhuman/web_chat/event_bus.rs#L512-L525: trace skipped foreign event names, lag occurrences, and the matched test-owned event marker; do not log payload PII.

As per coding guidelines, “New or changed flows must include verbose, grep-friendly diagnostics covering entry/exit, branches, external calls, retries/timeouts, state transitions, and errors.”

🤖 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 `@src/openhuman/learning/startup.rs` around lines 273 - 325, The isolated
learning startup test around learning_subscriber_fires_on_document_event must
add grep-friendly tracing for the unique source ID, isolated subscription and
publish steps, expected candidate count, and final observed count. In
src/openhuman/web_chat/event_bus.rs lines 512-525, add diagnostics for skipped
foreign event names, lag occurrences, and the matched test-owned event marker
without logging payload PII; ensure both sites cover the requested state
transitions and error paths.

Source: Coding guidelines

🤖 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 `@src/openhuman/web_chat/event_bus.rs`:
- Around line 512-525: Update next_automation_halt to match the test-owned
automation_halt event using a unique source marker, not just ev.event. Also
require the expected engaged value and, for the halted case, the expected
reason; continue draining unrelated events and preserve the existing empty and
closed error handling.

---

Outside diff comments:
In `@src/openhuman/learning/startup.rs`:
- Around line 273-325: The isolated learning startup test around
learning_subscriber_fires_on_document_event must add grep-friendly tracing for
the unique source ID, isolated subscription and publish steps, expected
candidate count, and final observed count. In
src/openhuman/web_chat/event_bus.rs lines 512-525, add diagnostics for skipped
foreign event names, lag occurrences, and the matched test-owned event marker
without logging payload PII; ensure both sites cover the requested state
transitions and error paths.
🪄 Autofix (Beta)

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

Run ID: fd22d1dc-43bb-4672-9b6c-64b107b81474

📥 Commits

Reviewing files that changed from the base of the PR and between 56d3094 and 9906e10.

📒 Files selected for processing (4)
  • .github/workflows/ci-lite.yml
  • Cargo.toml
  • src/openhuman/learning/startup.rs
  • src/openhuman/web_chat/event_bus.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • Cargo.toml
  • .github/workflows/ci-lite.yml

Comment thread src/openhuman/web_chat/event_bus.rs Outdated
…t-benchmark-5046

# Conflicts:
#	src/openhuman/learning/startup.rs
#	src/openhuman/web_chat/event_bus.rs
@coderabbitai coderabbitai Bot added the memory Memory store, memory tree, recall, summarization, and embeddings in src/openhuman/memory/. label Jul 21, 2026
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 21, 2026
Brings tinyhumansai#5082 (learning subscriber test isolation — fixes the flaky
coverage failure) and resolves the prettier format:check failure via
main's formatted files. Cargo.lock resynced (--locked passes root +
app/src-tauri); cargo fmt, prettier, and clippy -D warnings all clean
locally. Adopts main's tauri-cef pin bump.
@coderabbitai coderabbitai Bot removed the memory Memory store, memory tree, recall, summarization, and embeddings in src/openhuman/memory/. label Jul 21, 2026
Comment thread src/bin/rss_bench.rs
.context("--repeat value")?;
}
"--out" => out = Some(PathBuf::from(it.next().context("--out needs a path")?)),
other => anyhow::bail!("unknown argument: {other}"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--roster / --repeat are parsed here, but run_parent (called from main below) always uses the hardcoded DEFAULT_ROSTER_SIZES — only --repeat reaches it. A --roster 4 passed without --child is silently dropped. Since this is a benchmark people poke at by hand, consider either honouring --roster in parent mode or rejecting it there so it fails loudly instead of no-op'ing.

- name: Measure embedded RSS (5 fresh procs x {1,8} agents)
run: ./target/release/rss-bench --out bench-rss.json | tee -a "$GITHUB_STEP_SUMMARY"

- name: Upload raw RSS samples

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the cargo test / cargo build steps above route through scripts/ci-cancel-aware.sh, but this measure step does not. It's short and report-only so the blast radius is tiny, but a cancelled run won't have its watchdog stop the in-flight measurement. Minor consistency nit.

@CodeGhost21 CodeGhost21 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #5060 — feat(bench): embedded-RSS benchmark harness + report-only CI (#5046)

Walkthrough

Adds a reproducible steady-state RSS benchmark for an embedded openhuman_core agent roster: a new ungated proc_metrics lib module (Linux /proc/self/{status,smaps_rollup} sampling + aggregation + serde report + human summary), a default-OFF rss-bench binary that builds 1- and 8-agent rosters over a mock provider / no-op memory and measures 5 fresh child processes per roster, and a report-only rust-rss-bench CI job deliberately kept out of pr-ci-gate.needs. The shipped build is byte-identical (feature default-OFF, no new deps). The load-bearing logic lives in the ungated module so the standard coverage lane enforces it — a clean placement decision that keeps benchmark code out of the shipped binary.

Overall this is well-scoped, well-documented, and well-tested (11 proc_metrics unit tests + 2 gated fixture tests). The Rust RSS Benchmark (report-only) job passes, which exercises cargo test --features rss-bench --bin rss-bench and the stripped-release build — so the Agent::builder / Provider / Memory API usage in the new bin is verified to compile and run. No blocking code issues found.

Changes

File Summary
src/openhuman/proc_metrics/mod.rs New ungated module: /proc parsers (&str-in, unit-tested), sample_self() (Linux-only, structured error off-Linux), roster aggregation, BenchReport serde + human_summary.
src/bin/rss_bench.rs New default-OFF binary: mock provider / echo tool / NoopMemory fixture, roster builder, warm-up + settle, parent/child re-exec driver with per-child timeout + reap.
Cargo.toml Registers the rss-bench bin (required-features = ["rss-bench"]) and the default-OFF rss-bench feature.
src/openhuman/mod.rs pub mod proc_metrics;.
.github/workflows/ci-lite.yml Adds the report-only rust-rss-bench job (not in the gate).

Actionable comments (2)

💡 Refactor / suggestion

1. src/bin/rss_bench.rs:360-402--roster / --repeat are silently ignored in parent mode

parse_args() parses --roster and --repeat, but run_parent always uses the hardcoded DEFAULT_ROSTER_SIZES and only --repeat reaches it — a --roster 4 passed without --child is accepted and silently dropped. The module doc says the parent "re-execs itself --repeat times per roster size," so --repeat works, but --roster looks operative from the CLI and isn't. Since this is a benchmark people will poke at by hand, either honour --roster in parent mode (override DEFAULT_ROSTER_SIZES when present) or reject it there so it fails loudly instead of no-op'ing.

🧹 Nitpick

2. .github/workflows/ci-lite.yml (measure step) — inconsistent ci-cancel-aware.sh wrapping

The cargo test / cargo build steps route through scripts/ci-cancel-aware.sh, but the ./target/release/rss-bench --out bench-rss.json | tee ... measure step does not. It's short and report-only so the blast radius is tiny, but a cancelled run won't have its watchdog stop the in-flight measurement. Minor consistency nit.

Nitpicks

  • src/bin/rss_bench.rs:228settle() is documented as a "No-op off Linux," but it still performs one 200 ms sleep before breaking (first sample_self() errors → last = 0, next read unwrap_or(last) = 0, abs_diff < 256 → break). Harmless; the doc slightly overstates.

Verified / looks good

  • proc_metrics parsers correctly avoid false matches (Pss: vs Pss_Anon:/Pss_Dirty: via the trailing colon) and default missing keys to zero — both covered by tests.
  • median_u64 even-count path is overflow-safe (a + (b - a)/2) and per_agent_increment_kib guards the single-roster / zero-span cases with checked_sub + saturating_sub. Tested.
  • NoopMemory fixture is the right call — the review history shows create_memory({backend:"none"}) still builds a UnifiedMemory + cloud embedder, which would have inflated the baseline; injecting a zero-alloc no-op both fixes that and mirrors the host-supplied-memory embedding contract.
  • Child driver uses kill_on_drop(true) + timeout(120s) around wait_with_output (which drains both pipes, no buffer deadlock) for a clean kill-and-reap on a stalled child. Sound.
  • Feature is genuinely default-OFF: the Feature Forwarding Gate check passes, and the shipped build carries no new deps.

Notes for the author / CI status

Three checks are red, and after tracing each I believe none are caused by this PR's code — but they do block the merge, so please confirm/re-run:

  • Scripts Self-Tests — fails on core default gate not forwarded to the shell: tui. That's the tui feature (from #5084, already on main), not rss-bench. Pre-existing, unrelated.
  • Rust Core Coverage — fails inside agent_harness_e2e (approval_gate_deny/approve/timeout, subagent_clarification_flow) — timing-sensitive e2e tests unrelated to proc_metrics. Note these ran at all because editing Cargo.toml trips the config-level full-suite fallback (per the two-lane CI model), so this PR is exposed to those flakes even though it doesn't touch them.
  • PR CI Gate — aggregate; red only because of the two above.

Because the coverage lane aborted on the unrelated flakes, diff-cover never got to evaluate the ≥80% diff-coverage gate on the actual changed lines. A re-run (or landing the flake fixes) is needed before the gate can go green. Leaving this as a comment rather than an approval only because of that red CI — the code itself LGTM.

…ell self-test

The 'real shell manifest forwards every real core default' self-test asserted
every default-ON gate is forwarded to the desktop shell, ignoring the
INTENTIONALLY_NOT_FORWARDED allowlist that the CI checker respects. Once the
tui gate (tinyhumansai#5084) became default-ON — a terminal subcommand deliberately not
shipped in the Tauri shell — the two disagreed and Scripts Self-Tests failed
with 'core default gate not forwarded to the shell: tui'.

Move the allowlist into scripts/lib/feature-forwarding.mjs (one source of truth)
and have the self-test skip allowlisted gates, matching the checker. The lane is
path-gated so this was dormant on main; it surfaced here.
@senamakel
senamakel merged commit bea380d into tinyhumansai:main Jul 21, 2026
18 of 20 checks passed
YellowSnnowmann added a commit to YellowSnnowmann/openhuman that referenced this pull request Jul 21, 2026
Reconcile dep-gating with main (now incl. tinyhumansai#5052/tinyhumansai#5060/tinyhumansai#5061/tinyhumansai#5062):
- Cargo.toml: union gating (optional axum/sentry/ppt-rs + http-server/
  inference/documents/crash-reporting features) with main's tower + tui deps
  and desktop-automation/tui features; default = union of both feature sets.
- app/src-tauri: shell forwards crash-reporting/http-server/desktop-automation
  (feature-forwarding gate passes; tui allowlisted).
- all_tests.rs: union of http-server + desktop-automation gate tests.
- types.rs: keep main's redirect_links security denylist entry (tinyhumansai#5052).
Cargo.lock + app/src-tauri/Cargo.lock resynced (--locked passes); cargo fmt,
feature-forwarding gate, and clippy -D warnings all clean locally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Net-new user-facing capability or product behavior. rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants