feat(medulla_local): supervise a local medulla-serve child (Flavor A draft) - #5105
Conversation
Draft §5.2 config seam: a [subconscious] block with engine = local|medulla (default local) plus MedullaLocalConfig.serve_entry. Inert serde, compiled in all builds; omitting the block preserves historical behaviour exactly.
…draft) New src/openhuman/medulla_local/ domain behind the default-ON medulla-local Cargo feature (forwarded to app/src-tauri). Mirrors runtime_python_server: versioned ready/hello handshake, id-correlated NDJSON over a unix socket, per-request timeout, restart-and-retry-once, start-failure backoff, stderr drain. Serve->host reverse-RPC ports answered this draft: inference (routed onto the per-role Provider factory; orchestrator/reasoning/compress -> agentic/ reasoning/summarization roles) and tools (a small curated read-only allowlist from build_runtime_tools, results passed through unchanged); every other port is refused port_unavailable. Registers the medulla_local RPC namespace (status/instruct) under DomainGroup::Agent as a registration-site gate. Mock-JSONL server tests cover handshake, instruct round trip, inference callback routing, and restart-on-death.
…ected Draft §5.2: when subconscious.engine = medulla, run_tick observes as usual then enqueues one medulla-serve instruct summarising the tick context (instead of the local tinyagents reflect graph) and advances the baseline via commit. The branch is #[cfg(feature = medulla-local)] and only taken when the flag is set, so the default (local) path is byte-identical. A both-ways test pins that an unset flag drives the local reflect graph.
…ig + env The compiled-in serve-entry fallback was an absolute path on the implementer's own machine, so any other checkout that set subconscious.engine="medulla" without also setting serve_entry hit a path that only exists on one laptop -- and it violated the repo rule against committing machine-local configuration. Resolve the entry from the explicit config value, then the OPENHUMAN_MEDULLA_SERVE_ENTRY env override, and otherwise None (there is no portable compiled-in default: medulla-v1's built dist/serve lives outside this repo). NodeServeConnector now holds Option<PathBuf> and bails with an actionable message when unconfigured.
…hake NodeServeConnector always sent hello.tools = [], so a real medulla-serve child never learned the curated read-only tools existed and the model could never emit a tools.invoke call -- leaving OpenhumanHostPorts::invoke_tool and its ReadOnly enforcement dead in practice. Add HostPorts::tool_specs(), implemented by OpenhumanHostPorts from the same curated (allowlisted + read-only) surface invoke_tool resolves against, so the advertised set and the invocable set are identical by construction. Wire it into the hello params. Add a mock-serve test that drives a tools.invoke port call and asserts both the hello advertisement and the dispatch reached the host ports.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds the feature-gated ChangesLocal Medulla integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SubconsciousInstance
participant MedullaSupervisor
participant medulla_serve
participant OpenhumanHostPorts
SubconsciousInstance->>MedullaSupervisor: enqueue instruction
MedullaSupervisor->>medulla_serve: send instruct request
medulla_serve->>OpenhumanHostPorts: request inference or tool
OpenhumanHostPorts-->>medulla_serve: return host-port result
medulla_serve-->>MedullaSupervisor: return instruction receipt
MedullaSupervisor-->>SubconsciousInstance: report enqueue result
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
# Conflicts: # Cargo.toml
Boots the real Axum JSON-RPC router with no serve entry configured and asserts status/instruct fail cleanly with the actionable "serve entry not configured" error — no Node child, no network. Closes the Test Inventory orphan-file gap (tests/medulla_local_e2e.rs was untracked).
…ments Reword doc comments that named private medulla-v1 symbols/paths (createAgentHarness, MedullaModule, the docs/specs/medulla-serve-protocol.md file) to generic phrasing: "agent-harness facade", "serve-side module registry", and "medulla-serve NDJSON protocol, v1". Comment-only change, no behavior affected.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/tinyhumansai/openhuman/blob/9c56759cc14f2636fce268506b34defffa4b8635//tmp/openhuman-review-4d60834/src/openhuman/medulla_local/server.rs#L24-L26
When building the shipped Windows desktop app, this default-on medulla-local feature is forwarded to app/src-tauri, but the module unconditionally imports and uses tokio::net::unix, which is only available on Unix targets. That means Windows packaging cannot compile as soon as this feature is enabled; add a Windows-compatible transport or cfg(unix)/stub the feature for non-Unix targets.
AGENTS.md reference: AGENTS.md:L22-L25
https://github.com/tinyhumansai/openhuman/blob/9c56759cc14f2636fce268506b34defffa4b8635//tmp/openhuman-review-4d60834/src/openhuman/medulla_local/server.rs#L582-L586
After the first successful start, ensure_started returns the cached supervisor without comparing it to the newly loaded Config. The cached OpenhumanHostPorts keeps the original Arc<Config>, so if the user changes Agent access, action_dir, trusted roots, or model/provider settings while medulla is running, subsequent inference/tool callbacks continue using the old security policy and paths until the process dies. Please key the cache on the relevant config signature or refresh/rebuild the ports before reusing it.
AGENTS.md reference: AGENTS.md:L99-L108
https://github.com/tinyhumansai/openhuman/blob/9c56759cc14f2636fce268506b34defffa4b8635//tmp/openhuman-review-4d60834/src/openhuman/medulla_local/server.rs#L503-L512
This retry path treats every Connection::request error as a transport failure, but Connection::request_raw also returns errors for structured res.ok=false responses from the child. If medulla-serve rejects an instruct/status request with a normal protocol error such as bad_request or not_ready, the host will kill a healthy child and replay the same request, potentially losing session state or duplicating side effects. Only reset/retry on connection/read/write failures, and surface non-retryable serve errors directly.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… rejections Classify supervised request failures with a typed RequestError so the restart-and-retry-once path fires only when the established connection broke mid-request (process death, closed socket, IO failure, timeout). An ok=false response is an application-level rejection over a healthy connection: killing and respawning the child and replaying the request cannot help, can lose session state, and can duplicate side effects, so it now surfaces immediately (downcastable, carrying the wire error code), as do undecodable results and connect/handshake failures. Regression tests: a mock ok=false rejection surfaces the typed error with no respawn (connection count stays 1, connection stays live), and the restart-on-death test now pins exactly one respawn.
ensure_started previously reused the cached supervisor forever: the cached host ports capture the Arc<Config> they were built from, so a change to Agent access, action_dir, trusted roots, or model/provider settings kept servicing inference/tool callbacks under the old policy until the child died. Mirror the runtime_python_server precedent (its backends() comparison): both live cache variants now carry a fingerprint of the config snapshot; a changed config drops the cached supervisor (killing the child once the last in-flight handle releases its connection) and rebuilds. The serde-skipped runtime path roots (workspace_dir, action_dir) are folded into the fingerprint explicitly. A config change also bypasses the start-failure backoff, since the new snapshot may be exactly what fixes the startup failure. Regression tests: the backoff is keyed on the fingerprint (same config fails fast, changed config attempts a fresh build against the new snapshot), and the fingerprint tracks serve-entry and action_dir changes.
The serve transport is a unix domain socket, and tokio only provides tokio::net::unix on unix targets — with the default-ON medulla-local feature forwarded to app/src-tauri, the unconditional import broke Windows packaging. Gate the real supervisor behind #[cfg(unix)] and add a server_unsupported.rs stub for other targets that keeps the same public surface: ensure_started fails with a typed, downcastable UnsupportedPlatformError, status folds it into a well-formed not-running snapshot, and instruct fails cleanly. A portable transport (e.g. stdio) can lift this later without touching callers. The e2e suite now asserts the platform-appropriate startup message, and the stub carries its own tests that run on non-unix targets. Verified with the full local gate set on unix; a local Windows cross-check is not possible here (native C deps need an MSVC toolchain), so non-unix correctness rests on the cfg boundary: nothing outside #[cfg(unix)] names a unix-only symbol.
|
Addressed all three Codex findings (the review has no inline threads, so replying here per finding): P1 — unix-socket supervisor breaks Windows packaging → fixed in e9b975a. The real supervisor ( P2 — stale config reused by the cached supervisor → fixed in a4933c9. Mirrors the P2 — restart-and-retry on serve application errors → fixed in f4fee4a. Request failures are now classified with a typed Gates run locally: |
coderabbitai
Bot
added
feature
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0d44ec598
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn config_fingerprint(config: &Config) -> Result<u64> { | ||
| use std::hash::{Hash, Hasher}; | ||
| let encoded = | ||
| serde_json::to_string(config).context("encoding config for medulla cache fingerprint")?; | ||
| let mut hasher = std::collections::hash_map::DefaultHasher::new(); | ||
| encoded.hash(&mut hasher); |
There was a problem hiding this comment.
When the config contains any multi-entry HashMap fields (for example teams or agents), serializing the whole Config to JSON makes this fingerprint depend on HashMap iteration order. Freshly loaded but semantically identical configs can therefore hash differently, causing ensure_started to treat every status/tick as a config change, drop the cached supervisor, kill/restart the child, and bypass the startup backoff even though nothing changed. Use a deterministic representation, or hash only the relevant fields with sorted map entries.
Useful? React with 👍 / 👎.
| let prefix = self.log_prefix(); | ||
| let started = std::time::Instant::now(); | ||
| let tick_at = now_secs(); | ||
| let _my_generation = self.tick_generation.fetch_add(1, Ordering::SeqCst) + 1; |
There was a problem hiding this comment.
In the medulla engine path, _my_generation is incremented but never compared before profile.commit() advances the baseline. If another tick/config change supersedes this one while observe or instruct_tick is in flight, the stale medulla tick still commits its observation and persists last_tick_at; the existing local graph explicitly skips commit when the generation changed. Add the same generation check before both medulla commit paths.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/openhuman/medulla_local/ops.rs (1)
63-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the
medulla_localRPC error paths, not just the success paths. Both handlers convert failures straight into a string and return, with no log line — the shared root cause is that only the success path of this new RPC surface follows the repo's grep-friendly-diagnostics-for-errors guideline.
src/openhuman/medulla_local/ops.rs#L63-L79: add awarn!("{prefix} medulla_local.instruct failed: {error:#}")-style line before.map_err(...)returns ininstruct_handler(and consider one forstatus_handler's implicit-failure path, though that one already folds into a well-formed status).src/openhuman/medulla_local/schemas.rs#L98-L104: log theserde_json::from_valuedeserialization error inhandle_instructbefore converting it to a string, so malformed RPC payloads are greppable in production logs.🤖 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/medulla_local/ops.rs` around lines 63 - 79, Add warning logs for both medulla_local RPC failure paths: in src/openhuman/medulla_local/ops.rs lines 63-79, log the error before instruct_handler converts instruct_tick failures via map_err, and consider logging status_handler’s implicit-failure path while preserving its existing status response; in src/openhuman/medulla_local/schemas.rs lines 98-104, log serde_json::from_value deserialization errors before converting them to strings. Use the existing grep-friendly medulla_local error-log format.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/medulla_local/server.rs`:
- Around line 576-592: Update request to bypass the retry/reset flow for the
non-idempotent instruct operation, returning its transport error directly to the
caller. Preserve the existing retry behavior for other retryable operations and
the non-retryable error handling.
In `@src/openhuman/subconscious/instance.rs`:
- Around line 361-430: The run_tick_medulla quiet and successful enqueue paths
must guard baseline updates against superseded generations. Before calling
profile.commit, updating tick state, or persisting last_tick_at, compare the
captured generation from tick_generation with the current generation; skip these
updates when another tick has advanced it, while preserving the existing result
and failure behavior.
---
Nitpick comments:
In `@src/openhuman/medulla_local/ops.rs`:
- Around line 63-79: Add warning logs for both medulla_local RPC failure paths:
in src/openhuman/medulla_local/ops.rs lines 63-79, log the error before
instruct_handler converts instruct_tick failures via map_err, and consider
logging status_handler’s implicit-failure path while preserving its existing
status response; in src/openhuman/medulla_local/schemas.rs lines 98-104, log
serde_json::from_value deserialization errors before converting them to strings.
Use the existing grep-friendly medulla_local error-log format.
🪄 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: 555bdcdd-7f7c-4914-8021-f067b8476d22
📒 Files selected for processing (22)
Cargo.tomlapp/src-tauri/Cargo.tomldocs/TEST-COVERAGE-MATRIX.mdsrc/core/all.rssrc/openhuman/config/schema/mod.rssrc/openhuman/config/schema/subconscious.rssrc/openhuman/config/schema/types.rssrc/openhuman/medulla_local/host_ports.rssrc/openhuman/medulla_local/mod.rssrc/openhuman/medulla_local/ops.rssrc/openhuman/medulla_local/ports.rssrc/openhuman/medulla_local/protocol.rssrc/openhuman/medulla_local/schemas.rssrc/openhuman/medulla_local/server.rssrc/openhuman/medulla_local/server_tests.rssrc/openhuman/medulla_local/server_unsupported.rssrc/openhuman/medulla_local/types.rssrc/openhuman/mod.rssrc/openhuman/runtime_node/ops.rssrc/openhuman/subconscious/instance.rssrc/openhuman/subconscious/instance_tests.rstests/medulla_local_e2e.rs
… retrying The wire instruct op carries no client-supplied idempotency key (instructionId is serve-assigned, returned only in the receipt), so a restart-and-retry after a mid-request transport break could enqueue the same instruction twice. Exclude non-idempotent ops from the retry path: reset the broken connection but surface the typed RequestError::MaybeApplied so the caller reconciles via status before re-issuing. Idempotent ops (status) keep restart-and-retry-once.
…rations run_tick_medulla bumped tick_generation but never re-checked it, so a superseded tick could still commit a stale baseline and advance last_tick_at. Mirror the local graph path's supersession guard on both medulla commit edges (quiet window and post-enqueue), discarding the stale observation while still counting the tick.
Config carries HashMap-backed fields whose serde emission order is unstable across processes, so hashing the raw serde_json string could spuriously invalidate the supervisor cache and restart the child for a byte-identical config. Hash the JSON tree with object keys visited in sorted order (type-tagged, arrays order-significant) instead.
Grep-friendly warn lines for the instruct handler failure, malformed instruct params, and the unavailable-serve status fold, per the repo's diagnostics-for-errors guideline.
… and canonical fingerprint
|
Addressed the remaining CodeRabbit findings from the latest review round:
Full gates rerun locally with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5de85f0022
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| content: response.text(), | ||
| reasoning_content: None, | ||
| model: model_id, | ||
| tool_calls: Vec::new(), |
There was a problem hiding this comment.
When medulla-serve sends an inference.invoke that includes tools, this path still returns an empty tool_calls list, and the ModelRequest built just above does not attach the incoming tool specs. The OpenHuman model adapter only advertises/parses tools when ModelRequest.tools is non-empty (src/openhuman/tinyagents/model.rs), so the curated tools advertised during hello can never actually be invoked; any medulla cycle that needs file_read, grep, or web_fetch silently degrades to a text-only model turn.
Useful? React with 👍 / 👎.
# Conflicts: # Cargo.toml
coderabbitai
Bot
added
rust-core
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9afc522f06
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| loop { | ||
| let line = self | ||
| .next_line(REQUEST_TIMEOUT) | ||
| .await | ||
| .map_err(RequestError::Transport)?; |
There was a problem hiding this comment.
When serve keeps emitting event/call frames but never sends the correlated res (for example, a wedged cycle still streaming progress), this loop restarts the 60s timeout for every frame because REQUEST_TIMEOUT is applied only to next_line(). The RPC/subconscious tick can then hang indefinitely instead of tripping the supervisor's retry/backoff path; wrap the whole response wait in one deadline or carry a fixed deadline across skipped frames.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 758d3a8 (config knob in 78263de). The await-res loop now carries one wall-clock deadline per request — subconscious.medulla_local.request_deadline_secs, default 300s (0 falls back to the default) — computed once at write time, with each read window shortened to min(REQUEST_TIMEOUT, remaining). Interleaved call/event frames still feed the per-read idle timeout but can no longer extend how long a request stays pending. Expiry surfaces as the typed RequestError::Transport deadline error, so it rides the existing policy axis unchanged: idempotent status keeps restart-and-retry-once, while the non-idempotent instruct fails fast as MaybeApplied (the request reached serve but its outcome was never observed) with no replay. Regression tests: overall_deadline_bounds_instruct_despite_continuous_events (mock streams event frames forever, never answers the res; asserts MaybeApplied, exactly one submission, no respawn, bounded wait) and overall_deadline_bounds_idempotent_status_with_one_retry (transport deadline error after exactly one respawn).
| fn status(&self) -> MedullaLocalStatus { | ||
| MedullaLocalStatus { | ||
| enabled: true, | ||
| running: true, |
There was a problem hiding this comment.
When an already-handshaken medulla-serve child exits between requests, openhuman.medulla_local_status calls snapshot() and this cached handshake path still returns running: true without any I/O or Child::try_wait() check. The status UI/API will show a false healthy child until a later request touches the socket; poll or reset the connection before advertising running=true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 758d3a8. The cached handshake state is no longer trusted as proof of life: before snapshot reports status and before ensure/request reuse the cached connection, the supervised child is probed via Child::try_wait (non-blocking reap; connections without a child — mock-listener tests — report alive). A dead child now (a) reports running: false with an explicit "child exited; it will be respawned on the next request" message, (b) transitions the cache to the restartable empty state, and (c) causes the next request to respawn a fresh child instead of writing into the dead transport — which also removes a false-MaybeApplied path for an instruct that provably never reached serve. Regression test: dead_child_reports_not_running_and_respawns_on_next_request — a stand-in supervised child is killed externally while the mock transport stays open; asserts the snapshot flips to not-running with the exited message, then a subsequent instruct respawns (exactly one extra connection) and status reports running again.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/medulla_local/server.rs (1)
108-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider inverting
op_is_idempotentto an allowlist.The current blocklist (
!matches!(op, "instruct")) fails open: any future non-idempotent wire op will be silently retried unless someone remembers to add it here. Given the whole point of this function is to prevent duplicate side effects, an allowlist of known-safe-to-retry ops (currently just"status") would fail closed instead — a forgotten new mutating op stays non-retried by default rather than risking a duplicate side effect.♻️ Proposed refactor
-fn op_is_idempotent(op: &str) -> bool { - !matches!(op, "instruct") -} +fn op_is_idempotent(op: &str) -> bool { + matches!(op, "status") +}🤖 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/medulla_local/server.rs` around lines 108 - 121, Change op_is_idempotent to use an explicit allowlist of known-safe retry operations, currently returning true only for "status". Ensure unknown operations, including future mutating wire operations and "instruct", return false so they do not retry by default.
🤖 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/openhuman/medulla_local/server.rs`:
- Around line 108-121: Change op_is_idempotent to use an explicit allowlist of
known-safe retry operations, currently returning true only for "status". Ensure
unknown operations, including future mutating wire operations and "instruct",
return false so they do not retry by default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc0e8d1a-7cb2-4423-923e-bedcf2558628
📒 Files selected for processing (11)
Cargo.tomlapp/src-tauri/Cargo.tomldocs/TEST-COVERAGE-MATRIX.mdsrc/core/all.rssrc/openhuman/medulla_local/ops.rssrc/openhuman/medulla_local/schemas.rssrc/openhuman/medulla_local/server.rssrc/openhuman/medulla_local/server_tests.rssrc/openhuman/mod.rssrc/openhuman/subconscious/instance.rssrc/openhuman/subconscious/instance_tests.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/openhuman/mod.rs
- app/src-tauri/Cargo.toml
- src/core/all.rs
- Cargo.toml
- docs/TEST-COVERAGE-MATRIX.md
- src/openhuman/medulla_local/schemas.rs
- src/openhuman/subconscious/instance.rs
- src/openhuman/medulla_local/ops.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
Add subconscious.medulla_local.request_deadline_secs (default 300s; an explicit 0 falls back to the default) with a request_deadline() accessor, so the serve supervisor can bound one request end to end regardless of interleaved frame traffic.
…d liveness Two supervisor gaps: - The await-res loop applied only a per-read idle timeout that reset on every inbound frame, so a child streaming frames without ever answering could keep a request pending indefinitely. One wall-clock deadline (subconscious.medulla_local.request_deadline_secs) now bounds the whole loop; expiry surfaces as the typed transport error, so an idempotent op keeps restart-and-retry-once while a non-idempotent op fails fast as MaybeApplied. - snapshot/ensure/request trusted the cached handshake state, reporting running=true (and reusing the connection) after the child had died between requests. The supervised child is now probed via try_wait before the cache is trusted: a dead child reports running=false with an honest message, transitions the cache to the restartable empty state, and the next request respawns instead of writing into a dead transport (which would have misreported MaybeApplied for an instruct that never reached serve). Regression tests: deadline trip under continuous events for instruct (MaybeApplied, no replay, no respawn) and status (transport error after exactly one retry), and external child kill detected by snapshot with respawn-on-next-request.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66ed757352
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return Ok(res.result.unwrap_or(Value::Null)); | ||
| } | ||
| FrameKind::Call => self.handle_call(frame).await, |
There was a problem hiding this comment.
When medulla-serve sends an inference.invoke or tools.invoke callback while this request is waiting for its res, this await runs outside the remaining request_deadline_secs. A slow or hung model provider/tool can therefore hold the supervisor connection lock well past the configured total deadline instead of timing out and entering the retry/backoff path; wrap callback handling in the remaining deadline and return a timeout ret when it expires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16fd1c7. Port-callback servicing (inference.invoke / tools.invoke) now runs under the same wall-clock deadline as the response wait: the remaining time is computed before dispatch and the callback future is bounded by a tokio timeout with it, so a hung provider or tool can no longer suspend the deadline and pin the request + connection lock. Expiry surfaces through the exact path a read-timeout expiry takes — the typed transport deadline error — so the existing policy split is preserved unchanged: non-idempotent instruct fails fast as MaybeApplied with no replay, idempotent status keeps restart-and-retry-once. Regression tests: overall_deadline_bounds_instruct_during_hung_port_callback (mock issues a callback, the host's inference port never resolves; asserts bounded MaybeApplied, exactly one submission and one callback dispatch, no respawn) and overall_deadline_bounds_status_during_hung_port_callback (typed transport deadline error after exactly one respawn).
| self.connector.describe() | ||
| ); | ||
| self.reset().await; | ||
| self.request_once(op, params).await.map_err(Into::into) |
There was a problem hiding this comment.
When an idempotent request is retried and the replacement attempt also fails with a transport-class error, this returns without clearing self.connection. Since request_once leaves the failed Connection cached, later status/instruct calls first reuse a known-bad transport, costing another full deadline and potentially surfacing MaybeApplied for an instruct sent into a stale connection; reset on retry failure before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 713503f. The retry-failure path now clears the connection cache too: when the idempotent retry itself fails with a transport-class error, the replacement connection is reset before returning, so the next request starts from a clean establish instead of first burning a full deadline on a known-bad transport (and potentially misreading a stale-connection instruct as MaybeApplied). Non-transport retry failures (serve rejection, protocol) still leave the healthy connection cached. Regression test: retry_failure_resets_cache_so_next_request_reestablishes — the mock drops the connection on status for the first two connections; after the typed transport error (exactly one respawn), a subsequent non-idempotent instruct re-establishes on a fresh third connection and succeeds with exactly one submission, rather than being written into the stale transport.
… callbacks A serve->host port callback (inference.invoke / tools.invoke) was awaited outside the overall request deadline: a hung provider or tool could pin the request and the supervisor's connection lock indefinitely. Callback servicing is now bounded by the remaining wall-clock deadline (tokio timeout); expiry surfaces through the same typed transport-deadline path as a read-timeout expiry, preserving the instruct->MaybeApplied / status->retry-once policy split.
…try also fails When an idempotent request's retry attempt broke mid-request too, the replacement connection stayed cached: the next request would first reuse a known-bad transport, costing another full deadline and potentially surfacing MaybeApplied for an instruct written into a stale connection. A retry that fails with a transport-class error now resets the cache, so the next request starts from a clean establish.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a2d312758
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| config.workspace_dir.hash(&mut hasher); | ||
| config.action_dir.hash(&mut hasher); |
There was a problem hiding this comment.
When the config leaves subconscious.medulla_local.serve_entry empty, build_supervisor resolves the child path from OPENHUMAN_MEDULLA_SERVE_ENTRY, but this fingerprint only hashes the serialized Config plus the skipped path roots. If a user fixes an unconfigured setup by setting that env var, or points it at a different serve build, the same fingerprint keeps returning the cached Failed backoff or the existing connector for the old path, so the documented env override is ignored until the process restarts or the TOML changes. Fold the resolved serve entry/env value into this cache key.
Useful? React with 👍 / 👎.
| Err(error) => { | ||
| warn!("{prefix} medulla instruct failed: {error:#}"); | ||
| let mut state = self.state.lock().await; | ||
| state.consecutive_failures += 1; | ||
| state.total_ticks += 1; |
There was a problem hiding this comment.
When instruct_tick returns a transport-class MaybeApplied error after the non-idempotent instruct may already have reached serve, this arm treats it like an ordinary failure and leaves the observation uncommitted. The next heartbeat will observe the same changes and enqueue another wake instruction, even though the supervisor’s error contract says this exact case must be reconciled via status before reissuing to avoid duplicate instructions. Downcast this error and reconcile/dedupe before allowing the next tick to send the same wake again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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/openhuman/config/schema/subconscious.rs`:
- Around line 91-100: Update request_deadline to emit a grep-friendly warning
containing the configured numeric value when request_deadline_secs is zero, then
emit a debug diagnostic containing the effective numeric deadline before
returning the Duration. Use the existing logging conventions and log only
numeric values, while preserving the current fallback and return behavior.
- Around line 94-100: Update SubconsciousConfig::request_deadline to cap or
reject request_deadline_secs values that could overflow the medulla request
path’s Instant deadline calculation, while preserving the default behavior for
zero. Add a boundary test using a value near u64::MAX and verify the resulting
deadline remains safe.
🪄 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: 51ff0f30-a4c5-4eae-9561-e951cd4bcabb
📒 Files selected for processing (4)
docs/TEST-COVERAGE-MATRIX.mdsrc/openhuman/config/schema/subconscious.rssrc/openhuman/medulla_local/server.rssrc/openhuman/medulla_local/server_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/TEST-COVERAGE-MATRIX.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fa92db8ae
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let existing = supervisor.clone(); | ||
| drop(guard); | ||
| existing.ensure().await?; | ||
| return Ok(existing); |
There was a problem hiding this comment.
When a previously healthy medulla-serve child exits and the next ensure() cannot reconnect (for example the entry point was removed, or the new child never creates the socket), this early return propagates the error while leaving SupervisorCache::Ready in place. Subsequent status/heartbeat/instruct calls with the same config bypass the Failed cache and immediately try to spawn again, so a broken serve file can cost every poll/tick another handshake timeout instead of respecting START_FAILURE_BACKOFF; update the cache to Failed on restart failures from the cached supervisor path too.
Useful? React with 👍 / 👎.
|
Addressed both findings from the latest review in f344ac6: the zero-fallback now emits a warn with configured vs effective values, and deadlines are clamped to a 24h ceiling (MAX_REQUEST_DEADLINE_SECS) with a near-u64::MAX boundary test — an oversized value can no longer reach Instant+Duration arithmetic. @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f344ac698c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[cfg(feature = "medulla-local")] | ||
| if config.subconscious.engine.is_medulla() { | ||
| return self.run_tick_medulla(config).await; |
There was a problem hiding this comment.
When a slim/headless build is compiled without medulla-local but the persisted config still has subconscious.engine = "medulla", this #[cfg] removes the branch entirely and the tick silently falls through to the local tinyagents graph. That means a build that intentionally dropped the medulla domain can still run local subconscious inference and commit baselines under a mode the user did not select; return/record a disabled-engine error or skip the tick when the feature is absent instead of executing the local path.
AGENTS.md reference: AGENTS.md:L221-L223
Useful? React with 👍 / 👎.
1 participant
Summary
src/openhuman/medulla_local/(behind the default-ONmedulla-localCargo feature, correctly forwarded toapp/src-tauri/Cargo.toml) supervising a localmedulla-serveNode child process viaruntime_node, usingruntime_python_server-style JSONL supervision (versioned handshake, id-correlated frames, restart-and-retry, stderr drain) over a Unix domain socket.Providerfactory (tiersorchestrator/reasoning/compress), and a curated read-only tool allowlist is advertised in thehellohandshake and enforced on everyinvoke.medulla_localRPC namespace (status/instruct).subconscious.engine = "local" | "medulla"config selector: when set to"medulla", heartbeat ticks route through amedulla_localinstruct call; the default ("local") remains byte-identical to prior behavior (covered by test).OPENHUMAN_MEDULLA_SERVE_ENTRYenv var — no compiled-in paths.This is the Flavor-A milestone per
docs/medulla-flavors-plan.mdintinyhumansai/workflow-medulla. Originally opened as a draft to unblock the umbrella cross-repo chain; undrafted after the review rounds below.Problem
docs/medulla-flavors-plan.md(Flavor A) calls for openhuman to be able to run against a locally-supervised Medulla brain instead of only backend-hosted inference, without disturbing the existing defaultlocalsubconscious path or shipping any new compiled-in paths/secrets.Solution
medulla_localfollows the existing facade module shape (mod.rs,types.rs,protocol.rs,schemas.rs,ops.rs,server.rs,ports.rs,host_ports.rs) and is gated end-to-end by themedulla-localCargo feature (default ON, forwarded to the desktop shell inapp/src-tauri/Cargo.toml).runtime_python_server's proven JSONL-over-socket approach: versioned handshake, id-correlated request/response frames, automatic restart-and-retry on child death, and continuous stderr draining into structured logs.Providerfactory (orchestrator/reasoning/compresstiers), so provider selection, auth, and rate limiting stay centralized in openhuman.hellohandshake and re-enforced on everyinvoke(defense in depth against a compromised or buggy serve process).subconscious.engineis additive: the default"local"value takes the exact same code path as before (parity-tested), and"medulla"is opt-in, routing heartbeat ticks throughmedulla_local::instruct.OPENHUMAN_MEDULLA_SERVE_ENTRY, matching the "no hardcoded paths" rule already applied to other runtime children.Validation
cargo fmt,cargo check,cargo clippygreen withGGML_NATIVE=OFF.medulla_locallib tests: 23/23 passing (incl. non-idempotent-instruct fail-fast, idempotent retry-once, and canonical-fingerprint stability).subconsciousconfig tests 6/6 and instance tests 10/10 passing (includes the"local"byte-identical parity test and the medulla-path supersession guards).feat/e2e-serve-chaindocker harness intinyhumansai/workflow-medulla(Leg C: realopenhuman-coreagainst a real stagedmedulla-serve) — all legs green.Submission Checklist
medulla_local::server_testsandsubconscious::instance_tests.5de85f002(CI Lite run 29894766074).docs/TEST-COVERAGE-MATRIX.md(commit c0d44ec; retry/fingerprint semantics refreshed in 5de85f0).## Related— 11.3.3.Providerfactory.Closes #NNNin the## Relatedsection — N/A: tranche-2 feature branch with no tracking issue; the plan doc isdocs/medulla-flavors-plan.mdin the umbrella repo (see PR body).Impact
src/) and the desktop shell forwarding (app/src-tauri/Cargo.toml). No UI surface yet.subconscious.engine = "local"is byte-identical to pre-change behavior;medulla-localis a default-ON, opt-in-at-runtime feature.Related
diff-coverlane result on this diff.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/medulla-localValidation Run
pnpm --filter openhuman-app format:check— N/A, noapp/srcchanges in this PRpnpm typecheck— N/A, noapp/srcchanges in this PRmedulla_locallib tests (17/17),subconsciousconfig tests (6/6)cargo fmt,cargo check --manifest-path Cargo.toml(GGML_NATIVE=OFF) — greenapp/src-tauri/Cargo.toml;cargo check --manifest-path app/src-tauri/Cargo.toml— not independently re-run in this pass, flagging for reviewer follow-upValidation Blocked
command:fullpnpm test:coverage/ diff-cover gateerror:not executed in this passimpact:diff coverage verified ≥ 80% by both cargo-llvm-cov lanes on the final headBehavior Changes
subconscious.engine = "medulla"); default behavior unchanged.Parity Contract
subconscious.engine = "local"(default) is byte-identical, pinned by test.medulla_localcompiles out cleanly when themedulla-localfeature is disabled (facade module pattern consistent withvoice/mcp/tui).Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
subconsciousconfiguration (engine selection, serve-entry resolution, and request deadlines).medulla_localsupervisedstatus/instructRPC controls backed by a localmedulla-serveprocess, including read-only tool access and clean idle/unavailable reporting.Documentation
medulla_localsupervision and retry/fail-fast behavior.Tests