Run document synthesis in the tinydocs TinyBus module - #5491
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change adds feature-gated native-module loading with registry, platform selection, caching, RPC, and configuration support. Document generation and PDF extraction now use the TinyDocs module. Presentation image handling uses shared TinyDocs types. ChangesNative module platform and configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentTool
participant DocumentClient
participant ModuleHost
participant TinyDocs
AgentTool->>DocumentClient: request document operation
DocumentClient->>ModuleHost: ensure TinyDocs is ready
ModuleHost->>TinyDocs: load or connect to module
DocumentClient->>TinyDocs: send wire request and streamed bytes
TinyDocs-->>DocumentClient: return output chunks
DocumentClient-->>AgentTool: document bytes, text, or structured error
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
48575dd to
e888a9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/tools/impl/document/engine.rs (1)
136-153: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the unused
docx_entry_namesanddocx_entry_bodyhelpers. The remaining tests do not call them, so they triggerdead_codewarnings under-D warnings. Keep thezipdependency because other production code and tests still use it.🤖 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/tools/impl/document/engine.rs` around lines 136 - 153, Remove the unused docx_entry_names and docx_entry_body helper functions from the document tests. Leave the zip dependency intact because it is still required elsewhere.
🧹 Nitpick comments (10)
src/core/runtime/builder.rs (1)
257-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression assertions for
DomainSet::modules.
domain_set_presets_have_expected_flagsdoes not includeDomainGroup::Modules. Add assertions thatfull()allows it and thatharness(),embedded(),kernel(), andnone()reject it. This will test the new preset values and theallowsmapping.Also applies to: 288-288, 336-336, 373-373, 402-402, 431-431
🤖 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/core/runtime/builder.rs` at line 257, Extend domain_set_presets_have_expected_flags with DomainGroup::Modules assertions: verify full() allows it, while harness(), embedded(), kernel(), and none() reject it, covering the preset values and allows mapping.src/openhuman/modules/boot.rs (1)
56-66: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEager module loading has no deadline on the startup path.
The loop awaits
ops::ensure_loadedfor each eager module. Withmodules.allow_download = true, that call can download and verify an artifact. Nothing bounds how long it takes. A slow or stalled release endpoint therefore delays boot for as long as the underlying HTTP client allows, and the doc comment at lines 23-25 promises only that boot does not fail, not that it completes promptly.Wrap each eager load in a deadline so a slow module degrades to an unavailable feature instead of a slow start.
♻️ Proposed change to bound each eager load
+/// How long boot waits for one eager module before giving up on it. +const EAGER_LOAD_DEADLINE: std::time::Duration = std::time::Duration::from_secs(60); + pub async fn load_declared_modules(config: &Config) { @@ for record in registry::ALL { if record.load != LoadPolicy::Eager { continue; } - if let Err(reason) = ops::ensure_loaded(config, record.id).await { - log::warn!( - "[modules] eager module '{}' did not load: {reason}", - record.id - ); - } + match tokio::time::timeout(EAGER_LOAD_DEADLINE, ops::ensure_loaded(config, record.id)).await + { + Ok(Ok(())) => {} + Ok(Err(reason)) => log::warn!( + "[modules] eager module '{}' did not load: {reason}", + record.id + ), + Err(_) => log::warn!( + "[modules] eager module '{}' did not load within the boot deadline", + record.id + ), + } } }Note that a timeout here abandons the wait but does not cancel the work, because
ensure_loadedholds the resolve gate. Combine this with moving the download off the worker thread, as noted insrc/openhuman/modules/ops.rsat line 147.🤖 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/modules/boot.rs` around lines 56 - 66, Bound each eager module load in the startup loop by wrapping ops::ensure_loaded(config, record.id) with the project’s async timeout/deadline mechanism, using the appropriate startup duration constant. Preserve the existing warning behavior for both load errors and deadline expiry, so a timed-out module is treated as unavailable while boot continues promptly.src/openhuman/modules/documents.rs (1)
235-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe release does not run when the caller's deadline cancels the call.
The doc comment at line 237 states that the release runs whether or not the read succeeded. That holds for an error return. It does not hold for cancellation. Lines 20-25 delegate deadlines to the callers, and the document tools wrap these calls in
tokio::time::timeout. When that deadline fires,collectis dropped at theread_allawait, andReleaseOutputat line 243 never runs. A timed-out document therefore stays in the module's budget until its TTL expires, which is the exact outcome the comment says the release prevents.The module expires the output, so this self-heals. Correct the doc comment so it does not claim more than the code does, and consider a drop guard that issues the release on cancellation.
🤖 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/modules/documents.rs` around lines 235 - 252, Update the collect doc comment to state that ReleaseOutput runs after read_all returns, including read errors, but is not guaranteed when cancellation drops the future; do not claim unconditional release. Avoid adding a drop guard unless the implementation explicitly supports issuing the asynchronous release during cancellation.src/openhuman/modules/host.rs (1)
96-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA lost initialisation race leaves an orphaned broker running.
runtime()spawns the broker task at line 106 before it reachesget_or_initat line 132. If two callers race, both spawn a broker on their ownMemoryBus. The loser'sModuleRuntimedrops, but its broker task was already spawned and is never aborted, so a second broker runs for the process lifetime. The comment at lines 130-131 accounts for the dropped runtime but not for the spawned task.
tokio::sync::OnceCellserialises the async initialiser, so only one broker is ever constructed.♻️ Proposed change to initialise under an async OnceCell
-use std::sync::OnceLock; +use tokio::sync::OnceCell; @@ -/// The module bus, built once on first use. -static RUNTIME: OnceLock<ModuleRuntime> = OnceLock::new(); +/// The module bus, built once on first use. +static RUNTIME: OnceCell<ModuleRuntime> = OnceCell::const_new(); @@ pub async fn runtime() -> tinybus::Result<&'static ModuleRuntime> { - if let Some(existing) = RUNTIME.get() { - return Ok(existing); - } - - let transport = MemoryBus::new(); - let broker = Broker::new(); - ... - let runtime = ModuleRuntime { host, connection }; - Ok(RUNTIME.get_or_init(|| runtime)) + RUNTIME + .get_or_try_init(|| async { + let transport = MemoryBus::new(); + let broker = Broker::new(); + broker.spawn(transport.clone()); + let host = ModuleHost::new(broker); + let connection = Connection::connect(transport.connect().await?).await?; + Ok(ModuleRuntime { host, connection }) + }) + .await }🤖 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/modules/host.rs` around lines 96 - 133, Update runtime() to perform the entire broker, transport, host, connection, and ModuleRuntime construction inside the async OnceCell initializer, rather than spawning the broker before get_or_init. Use the OnceCell’s async initialization API so concurrent callers share the single constructed runtime and no losing broker task is created; preserve the existing error propagation and returned runtime behavior.src/openhuman/tools/impl/presentation/engine.rs (1)
129-141: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPre-size the payload buffer.
payloadstarts empty and grows by repeated doubling while every image is appended. A deck may legally carry tens of MiB, as the module documentation at Lines 18-20 states, so the buffer is reallocated and copied many times. The total length is known before the loop.♻️ Proposed change
- let mut payload = Vec::new(); + let total_image_bytes: usize = images + .iter() + .flat_map(|slide| slide.iter()) + .map(|image| image.bytes.len()) + .sum(); + let mut payload = Vec::with_capacity(total_image_bytes); let mut slides = Vec::with_capacity(input.slides.len());🤖 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/tools/impl/presentation/engine.rs` around lines 129 - 141, Pre-size the payload buffer before iterating in the presentation-building function by summing the byte lengths of all resolved images and passing that total to Vec::with_capacity. Keep the existing image appending and wire-image construction behavior unchanged.src/openhuman/tools/impl/document/engine.rs (1)
37-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stale blocking-boundary comment and the now-unneeded clone.
There is no blocking closure any more.
documents::generate_docx(&config, &owned)at Line 72 borrows the spec, andinputis already a reference with a long enough lifetime. The clone copies the whole spec for no reason, and the comment describes code that was deleted.♻️ Proposed change
- // Clone across the blocking boundary — cheap relative to the synthesis, - // and it keeps the blocking closure `'static`. - let owned = input.clone(); let started = std::time::Instant::now(); - let section_count = owned.sections.len(); + let section_count = input.sections.len(); let deadline_secs = deadline.as_secs(); - let title_chars = owned.title.chars().count(); + let title_chars = input.title.chars().count();And at Line 72:
- let call = timeout(deadline, documents::generate_docx(&config, &owned)).await; + let call = timeout(deadline, documents::generate_docx(&config, input)).await;🤖 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/tools/impl/document/engine.rs` around lines 37 - 43, Remove the stale blocking-boundary comment and eliminate the owned clone in the surrounding document-generation setup. Update the dependent references, including the documents::generate_docx call and metadata calculations, to use the existing input reference directly while preserving the current behavior.src/openhuman/agent/multimodal.rs (2)
1460-1460: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the full-buffer copy for PDF bytes.
bytesis already an ownedVec<u8>, andextract_pdf_textonly borrows it internally (&bytes).to_vec()therefore allocates a second copy of the whole attachment, up to the multimodal size cap, for every PDF. Change the parameter to&[u8]and pass&bytes.♻️ Proposed change
- match extract_pdf_text(bytes.to_vec()).await { + match extract_pdf_text(&bytes).await {And in the function definition:
-async fn extract_pdf_text(bytes: Vec<u8>) -> Result<String, String> { +async fn extract_pdf_text(bytes: &[u8]) -> Result<String, String> {🤖 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/agent/multimodal.rs` at line 1460, Update extract_pdf_text to accept a borrowed byte slice (&[u8]) instead of owning a Vec<u8>, then change its call in the surrounding PDF handling flow to pass &bytes directly rather than bytes.to_vec(). Preserve the existing extraction behavior while eliminating the full-buffer allocation.
1638-1644: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReuse the loaded configuration for PDF extraction.
When a caller already has an
Arc<Config>, pass it through the attachment pipeline.extract_pdf_textcurrently callsConfig::load_or_init()once per PDF, repeating config I/O for messages with multiple PDFs.🤖 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/agent/multimodal.rs` around lines 1638 - 1644, Update the attachment pipeline and extract_pdf_text to accept and reuse the caller’s existing Arc<Config> instead of calling Config::load_or_init() per PDF. Pass that Arc through every relevant invocation, preserve the current extraction behavior, and remove the redundant configuration load inside extract_pdf_text.src/openhuman/tools/impl/presentation/tests.rs (1)
283-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the converted tests and confirm the warning-to-payload wiring is still covered.
These three tests, and the ones at Lines 315-323 and 341-348, no longer call
execute. They callresolve_imagesdirectly, so theexecute_skips_*names no longer describe what they assert. Rename them toresolve_images_skips_*.The conversion also removes the only assertions that image warnings reach the tool payload. That wiring is now untested, because the two tests that exercise
executeare ignored. Add one test that feeds a warning list through the payload-building step without invoking the module, or confirm the coverage exists elsewhere.#!/bin/bash # Look for any remaining assertion that image warnings reach the tool payload. rg -nP -C5 '"warnings"|image_warnings|warnings\]' src/openhuman/tools/impl/presentation/🤖 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/tools/impl/presentation/tests.rs` around lines 283 - 292, Rename the three tests that directly call resolve_images, including the tests around the referenced cases, from execute_skips_* to resolve_images_skips_*; then add or verify a focused test covering propagation of image warnings into the tool payload without invoking the module, using the existing payload-building path and warning fields.src/openhuman/tools/impl/document/tests.rs (1)
163-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale engine reference in the ignored test.
The comment states the test "drives the real docx-rs engine". The writer now runs inside the
tinydocsmodule, so this description no longer matches the code path. Update the comment to name the module path.Also confirm that a CI job runs this ignored test with a built module. Otherwise the DOCX happy path has no automated coverage in this repository.
♻️ Proposed comment update
- // End-to-end: drives the real docx-rs engine + artifact pipeline. + // End-to-end: drives the tinydocs module writer + artifact pipeline.🤖 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/tools/impl/document/tests.rs` around lines 163 - 168, Update the end-to-end comment in execute_happy_path_returns_artifact_metadata to reference the tinydocs module path instead of the docx-rs engine. Also verify the CI configuration invokes this ignored test using a built tinydocs module; add or update that CI job if no such coverage exists.
🤖 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 `@docs/specs/2026-08-11-loadable-native-modules.md`:
- Around line 106-115: Mark the documented build-and-copy procedure as
Linux-specific, including the commands using libtinydocs_module.so and
/tmp/oh-modules. Do not imply the unchanged example applies to macOS or Windows;
platform-specific alternatives are unnecessary unless added explicitly.
In `@src/openhuman/modules/documents.rs`:
- Around line 255-281: Update read_all so handle.total_bytes cannot cause an
unbounded allocation or read loop: reject declared lengths above an appropriate
absolute maximum, and initialize Vec capacity using the smaller of total_bytes
and READ_CHUNK rather than the full declared length. Preserve the existing
empty-chunk mismatch error and normal chunk accumulation behavior for valid
sizes.
In `@src/openhuman/modules/host.rs`:
- Around line 148-179: The module-backed test in src/openhuman/modules/host.rs
lines 148-179 must bound the proxy.call("GenerateDocx", ()) operation with
tokio::time::timeout so a dead broker fails promptly instead of hanging; retain
the assertion that the timed result is an error. In
src/openhuman/modules/boot.rs lines 82-90, update
boot_tolerates_an_empty_search_path to avoid claiming the global runtime by
setting config.modules.enabled = false and asserting the disabled path, matching
the neighboring test, or mark it #[ignore] alongside other module-backed tests.
In `@src/openhuman/modules/ops_tests.rs`:
- Around line 100-108: Update
the_install_directory_is_namespaced_under_openhuman to assert that the install
directory’s parent component is “openhuman” rather than accepting any path
ending in “modules”; remove the unconditional dir.ends_with("modules")
alternative while preserving the existing diagnostic message.
In `@src/openhuman/modules/ops.rs`:
- Around line 147-205: Move the synchronous download call in resolve to a
blocking thread using the async runtime’s blocking-task mechanism, await its
result, and propagate join or download errors without changing existing
behavior. Apply the same treatment to load_local when it invokes load_file
synchronously, using its 'static runtime and record references.
In `@src/openhuman/modules/platform.rs`:
- Around line 90-118: Restrict the libc-symbol implementation of glibc_version
to cfg(all(target_os = "linux", target_env = "gnu")) so musl builds never
reference gnu_get_libc_version. Update the fallback cfg to cover all other
targets and retain its None result.
In `@src/openhuman/tools/impl/document/engine.rs`:
- Around line 155-181: Update
generate_yields_clean_structured_result_under_zero_deadline to isolate
configuration access by assigning a temporary workspace through
OPENHUMAN_WORKSPACE or injecting a temporary Config into generate. Ensure
Config::load_or_init cannot read, create, migrate, or modify the active user’s
config.toml while preserving the test’s existing outcome assertions.
In `@src/openhuman/tools/impl/presentation/tests.rs`:
- Around line 163-167: Update the happy-path assertion in the test to format and
print the entire result rather than calling result.text(), since this result
contains JSON content. Preserve the existing assertion condition and failure
message context while using the result’s full debug/display representation.
In `@vendor/tinydocs`:
- Line 1: Align the TinyDocs registry entries and vendored artifact references
with commit c97d2aca4d76729a1ad05c40cf3169d878231623: publish artifacts built
from that commit, update the registry version and asset names, and replace all
SHA-256 values with hashes for the new artifacts. Ensure the published interface
matches the pinned Documents five-method API and OutputRef rather than the
released Docx.GenerateDocx-only interface.
---
Outside diff comments:
In `@src/openhuman/tools/impl/document/engine.rs`:
- Around line 136-153: Remove the unused docx_entry_names and docx_entry_body
helper functions from the document tests. Leave the zip dependency intact
because it is still required elsewhere.
---
Nitpick comments:
In `@src/core/runtime/builder.rs`:
- Line 257: Extend domain_set_presets_have_expected_flags with
DomainGroup::Modules assertions: verify full() allows it, while harness(),
embedded(), kernel(), and none() reject it, covering the preset values and
allows mapping.
In `@src/openhuman/agent/multimodal.rs`:
- Line 1460: Update extract_pdf_text to accept a borrowed byte slice (&[u8])
instead of owning a Vec<u8>, then change its call in the surrounding PDF
handling flow to pass &bytes directly rather than bytes.to_vec(). Preserve the
existing extraction behavior while eliminating the full-buffer allocation.
- Around line 1638-1644: Update the attachment pipeline and extract_pdf_text to
accept and reuse the caller’s existing Arc<Config> instead of calling
Config::load_or_init() per PDF. Pass that Arc through every relevant invocation,
preserve the current extraction behavior, and remove the redundant configuration
load inside extract_pdf_text.
In `@src/openhuman/modules/boot.rs`:
- Around line 56-66: Bound each eager module load in the startup loop by
wrapping ops::ensure_loaded(config, record.id) with the project’s async
timeout/deadline mechanism, using the appropriate startup duration constant.
Preserve the existing warning behavior for both load errors and deadline expiry,
so a timed-out module is treated as unavailable while boot continues promptly.
In `@src/openhuman/modules/documents.rs`:
- Around line 235-252: Update the collect doc comment to state that
ReleaseOutput runs after read_all returns, including read errors, but is not
guaranteed when cancellation drops the future; do not claim unconditional
release. Avoid adding a drop guard unless the implementation explicitly supports
issuing the asynchronous release during cancellation.
In `@src/openhuman/modules/host.rs`:
- Around line 96-133: Update runtime() to perform the entire broker, transport,
host, connection, and ModuleRuntime construction inside the async OnceCell
initializer, rather than spawning the broker before get_or_init. Use the
OnceCell’s async initialization API so concurrent callers share the single
constructed runtime and no losing broker task is created; preserve the existing
error propagation and returned runtime behavior.
In `@src/openhuman/tools/impl/document/engine.rs`:
- Around line 37-43: Remove the stale blocking-boundary comment and eliminate
the owned clone in the surrounding document-generation setup. Update the
dependent references, including the documents::generate_docx call and metadata
calculations, to use the existing input reference directly while preserving the
current behavior.
In `@src/openhuman/tools/impl/document/tests.rs`:
- Around line 163-168: Update the end-to-end comment in
execute_happy_path_returns_artifact_metadata to reference the tinydocs module
path instead of the docx-rs engine. Also verify the CI configuration invokes
this ignored test using a built tinydocs module; add or update that CI job if no
such coverage exists.
In `@src/openhuman/tools/impl/presentation/engine.rs`:
- Around line 129-141: Pre-size the payload buffer before iterating in the
presentation-building function by summing the byte lengths of all resolved
images and passing that total to Vec::with_capacity. Keep the existing image
appending and wire-image construction behavior unchanged.
In `@src/openhuman/tools/impl/presentation/tests.rs`:
- Around line 283-292: Rename the three tests that directly call resolve_images,
including the tests around the referenced cases, from execute_skips_* to
resolve_images_skips_*; then add or verify a focused test covering propagation
of image warnings into the tool payload without invoking the module, using the
existing payload-building path and warning fields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c55980b-34d0-407f-b971-acf50e91f620
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
AGENTS.mdCargo.tomlapp/src-tauri/Cargo.tomldocs/specs/2026-08-11-loadable-native-modules.mdscripts/kernel-floor.limitssrc/core/all.rssrc/core/all_tests.rssrc/core/runtime/builder.rssrc/openhuman/agent/multimodal.rssrc/openhuman/config/schema/mod.rssrc/openhuman/config/schema/modules.rssrc/openhuman/config/schema/types.rssrc/openhuman/memory/diff/mod.rssrc/openhuman/memory/diff/stub.rssrc/openhuman/mod.rssrc/openhuman/modules/boot.rssrc/openhuman/modules/documents.rssrc/openhuman/modules/documents_tests.rssrc/openhuman/modules/host.rssrc/openhuman/modules/mod.rssrc/openhuman/modules/ops.rssrc/openhuman/modules/ops_tests.rssrc/openhuman/modules/platform.rssrc/openhuman/modules/registry.rssrc/openhuman/modules/schemas.rssrc/openhuman/modules/types.rssrc/openhuman/tools/impl/document/engine.rssrc/openhuman/tools/impl/document/tests.rssrc/openhuman/tools/impl/document/types.rssrc/openhuman/tools/impl/presentation/engine.rssrc/openhuman/tools/impl/presentation/image_util.rssrc/openhuman/tools/impl/presentation/mod.rssrc/openhuman/tools/impl/presentation/tests.rssrc/openhuman/tools/impl/presentation/types.rssrc/openhuman/tools/ops_tests.rssrc/openhuman/web3/wallet/chains/btc.rsvendor/tinybusvendor/tinydocs
💤 Files with no reviewable changes (1)
- src/openhuman/tools/impl/presentation/image_util.rs
`cargo check -p openhuman --lib` does not compile on main. `memory-git` is
default-OFF, and the off-state has two breaks:
- `memory::diff::mod` re-exports `tools::MemoryDiffTool` ungated, but `pub mod
tools` is `#[cfg(feature = "memory-git")]`. The module's own doc comment says
the tool is cfg'd out at its registration site, so the re-export should be
gated with it.
- `memory::diff::stub` imports `super::types::{Checkpoint, CrossSourceDiff,
Snapshot}`, and there is no `types` submodule. Those types are the module's
ungated type carve-out, re-exported flat from tinycortex by `mod.rs`, so the
import is `super::`.
Both are the failure mode AGENTS.md warns about for every gate: the disabled
build is the only thing that catches this, and the smoke lane does not run it.
No behaviour change in either configuration — with `memory-git` on, neither line
is reached differently.
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Moves the `vendor/tinybus` gitlink from dfcdd2c to 6ca0b0b and turns on the `modules` feature, which compiles the dynamic module host: the loader that admits a `cdylib` through the ABI descriptor, manifest, dependency and SHA-256 gates. That is the machinery `openhuman::modules` uses to run a codec outside this binary. 6ca0b0b also carries tinyhumansai/tinybus#9, chunked flow-controlled streams, which is how a `.pdf` or a deck's images reach a module: a payload larger than the 16 MiB frame cap could not otherwise cross the bus at all, and a `Vec<u8>` inside a frame costs about 3.5 bytes per byte because the frame is JSON. Two things make this cheaper than it looks. The bump is additive across every surface this crate touches. `connection.rs`, `events/`, `global.rs`, `native.rs` and `tinybus-macros` gained code and lost almost none between the two commits, and `lib.rs` only adds `pub mod module`, `pub mod stream` and `pub mod build_info`. Nothing in `src/` needed an edit for the bump itself: the lib compiles and all 197 bus-related tests pass unchanged. The feature adds **zero crates**. It wants `ureq`, `flate2`, `tar`, `zip`, `tempfile` and `toml`, and every one is already in the lock — `ureq` via the runtime installers, the archive stack via the Node and Python toolchain extractors, `toml` via config. No `name =` line is added to or removed from `Cargo.lock`; the changed lines are tinybus's new feature edges. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Stands up `openhuman::modules`: the domain that can run a capability as a compiled `cdylib` outside this binary instead of linking it in. Nothing consumes it yet — the document tools move over next — so this lands as infrastructure plus its RPC surface. The point is a dependency boundary that survives compilation. A codec is not kernel work, and each one drags a tree of parsers into a binary that mostly does something else; moving one out removes its dependencies from the build rather than merely gating them. What that costs is process isolation, and the module docs say so plainly rather than in a footnote. A loaded module shares this address space, these privileges and this crash domain: tinybus's deadlines and caught panics contain ordinary misbehaviour, not a segfault. `dlopen` runs code before any symbol can be inspected, so the ABI, manifest and digest gates decide what is *admitted*, never what is *safe*. Modules are first-party code that ships separately; anything untrusted belongs in a process. Four decisions worth the reader's time. **The registry is a compiled-in `const` table.** Which modules exist, which interfaces they claim, and which bytes are legitimate are build-time decisions. A registry a server could add entries to would be remote code execution with a download step, so neither config nor RPC can name an artifact — config only chooses whether modules load, whether this host may fetch them, and where a developer's own build lives. **Digests are pinned in source as the host's half of a two-sided check.** tinybus fetches the release's own `checksum.toml`, compares it with ours, hashes the download, and extracts only after. Pinning here makes that auditable offline and makes a release re-cut under the same tag stop matching rather than silently replacing what runs in-process. **Artifact selection returns an ordered list, not one answer.** A target triple is not enough: a `.so` built against glibc 2.39 fails to `dlopen` on a 2.35 host with a symbol-version error the ABI gate cannot phrase helpfully. `platform` probes glibc via `gnu_get_libc_version`, prefers the newest build that could work, and falls through on admission failure. A musl or BSD host gets an empty list, because "unsupported" now beats a download that cannot load. **Failures are cached, not retried.** tinybus never unloads a library, so a refused or faulted module cannot reach a different outcome without a restart. Retrying would pay a download and a `dlopen` per tool call to arrive at the same error, so the error is returned directly and says a restart is needed. Modules run on their own in-process broker, because `OnceBus::init_in_process` builds its `Broker` privately and `ModuleHost::new` needs one. The consequence is named in `host.rs`: a module cannot publish a `DomainEvent`. Right for a codec, and the thing to revisit if a module ever needs to. Wiring: `DomainGroup::Modules` with its `DomainSet` field on in `full()` and off in the embedding presets — a host embedding the harness should not get a native loader by default. All three non-compiler-enforced drift guards are answered explicitly: no store, no subscribers, no agent tools of its own. 35 tests, none of which touch the network: the whole platform table is pure and unit-tested per host, the registry is checked against it so the two cannot drift, and the refusal paths (disabled, unknown, unsupported, downloads-off) are all reachable offline. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Moves all three document operations off in-process codecs and onto the loaded `tinydocs` module: `generate_document`, `generate_presentation`, and the PDF text extraction in the multimodal ingest path. The dependency drop follows in the next commit; this one changes where the work happens. Each tool keeps exactly the policy only a host can supply — its deadline, its artifact bookkeeping, and its own agent-facing error shape — and gives up the synthesis. `modules::documents` is the seam: a tool asks for bytes and gets bytes or a reason, without knowing about streams, held outputs, or wire error names. Payloads move the way the bus wants them to. A `.pdf` and a deck's images ride TinyBus streams, so flow control and the size cap are the bus's rather than re-implemented; a produced document is pulled back in chunks, because a served object cannot open a stream to its caller. A deck's images share one stream, concatenated in slide order with each length declared, and `build_request` builds the deck and the payload in one pass so the two cannot disagree. Three things this turned up that are worth reading. **`strict(true)` was wrong, and testing against the real release proved it.** Strict admission additionally refuses a module whose rustc version differs from the host's, and the published v0.1.11 artifact is refused outright: `rustc version does not match in strict mode`. Released artifacts are built by CI on whatever toolchain the runner had and this crate pins its own, so a mismatch is the normal case. Permissive is now the setting, with the reasoning recorded where the choice is made — everything protecting the address space is still enforced, only the toolchain string is relaxed. **Loading is hoisted out of the generation deadline.** A first use may download and verify an artifact; charging that against a document timeout means the first document a user ever asks for is the one that fails. `ensure_ready` runs before the clock starts. **The module bus belongs to the runtime that creates it**, which is the trap tinybus documents and this domain now documents too. It never arises in the core, which has one runtime, but two `#[tokio::test]` functions each build their own, and the second to call a loaded module finds a broker whose tasks died with the first — the call hangs rather than fails. The three module-backed tool tests are therefore `#[ignore]`d and must run one per process; all three pass that way against a locally built artifact. Test coverage moved rather than shrank. The OOXML round trips left with the writer, into `tinydocs`, which asserts them against the bytes it produces; asserting them again through a bus call would test one behaviour twice. What replaces them is what only exists here: the deck/payload agreement, the error mapping, and the image-resolution warnings — which now drive `resolve_images` directly instead of needing a rendered deck, so they no longer depend on a module at all. `image_util.rs` is deleted. Identification and measurement live in `tinydocs::spec::image`, which is ungated precisely so a host can build a spec without the writer, and one implementation means the host and the module cannot disagree about what is embeddable. Co-authored-by: Medulla <medulla@tinyhumans.ai>
The three document codecs now run in the loaded `tinydocs` module, so they leave
this build. `documents` becomes `["dep:tinydocs", "modules"]`, and `tinydocs` is
consumed with `default-features = false` — the wire contract without any writer.
Measured, not asserted:
* 39 crates leave `Cargo.lock` entirely, 0 added: docx-rs, ppt-rs,
pdf-extract and their tails — lopdf, syntect, pulldown-cmark, xml-rs,
quick-xml, zip 0.6, zstd, bzip2, encoding_rs, euclid, ttf-parser,
adobe-cmap-parser, cff-parser, postscript, type1-encoding-parser, and the
rest.
* The product profile — the one that ships — goes from 505 to 448 unique
crate names.
* `scripts/assert-shed.sh` confirms all seven named codecs absent from both
the `documents` profile and the full product profile. Not `cargo tree -i`,
which the script's own header explains is unreliable as a shed proof.
Sharing the contract rather than re-declaring it is why `tinydocs` stays at all.
The specs are what an LLM is shown as a JSON tool schema and what the module
validates against; two definitions would drift, and the drift would be a tool
description promising limits the module does not enforce. `spec` is dep-free and
ungated precisely so a host can take it without a codec, which is what
`default-features = false` now does.
The interesting half of this commit is a regression the ratchet caught.
Enabling `tinybus/modules` on the dependency itself — which is how the earlier
bus commit did it — put a `dlopen` loader plus `ureq` and an archive stack into
the KERNEL profile, because `tinybus` is always-on surface. 305 -> 308 packages,
282 -> 285 names, for a workflow-only host that can never load a module. The
claim in that commit that the feature costs zero crates was measured against the
full lockfile and was true there; it was wrong about the profile that matters.
So the loader moves behind this crate's own default-ON `modules` feature, which
forwards `tinybus/modules` and is implied by `documents`. The kernel profile is
back to 305/282/2, byte-identical to upstream/main. Both halves of the gate are
pinned by tests, and the off-half matters more than usual: what this feature
compiles in is a `dlopen` loader, so a build that opted out must have no way to
reach one rather than a loader that merely refuses. Forwarded to the desktop
shell, and `check-feature-forwarding.mjs` passes.
Note for review: `scripts/check-kernel-floor.sh` fails on this branch — and
fails identically on pristine upstream/main, which resolves the same 305/282
against a 302/279 limit. Inherited, not caused here, and deliberately not
papered over by raising the limit.
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Records what a reader needs before touching `openhuman::modules`, and corrects three places the codec move made stale. `CLAUDE.md` gains rows for `documents` and `modules` in the gate table, and a section on the module host that leads with the trade rather than the mechanism: a dependency boundary that survives compilation, paid for with process isolation. The five decisions a future change is most likely to get wrong are stated with their reasons — the compiled-in registry (a config-addable one would be RCE with a download step), the pinned digests as the host's half of a two-sided check, ordered artifact selection (a glibc-2.39 build does not `dlopen` on 2.35), permissive admission (strict refuses the real published artifact), and the second broker. Two operational facts are written down because both cost time to discover: The bus belongs to whichever runtime creates it. The core has one and never notices; two `#[tokio::test]` functions each build their own, and the second to call a loaded module *hangs* against a dead broker rather than failing. That is why the module-backed tests are `#[ignore]`d — not because they need an artifact, but because they need their own process. `tinybus/modules` must never be enabled on the dependency. `tinybus` is always-on kernel surface, so doing so puts a `dlopen` loader plus `ureq` into the kernel profile for a host that cannot use one. Corrected: the note claiming `tinydocs` is exclusive to `documents` now says it is taken types-only; the default-vs-product explainer no longer implies `documents` still carries zstd/bzip2 native builds; and the `DomainGroup` families sentence names `Modules`, which it has to, since that list is how a reader knows the runtime axis covers every family. `docs/specs/` gets the spec, in the shape the other kernel specs use — problem, goals, non-goals, behaviour, invariants, acceptance criteria — plus the two things that belong upstream in tinybus: a reply-stream seam would delete the module's output store, and per-interface method lists would let one module serve transfer and format interfaces separately. `pnpm docs:check` passes; nothing in `platform/about_app` names these tools, so there was nothing to update there. Co-authored-by: Medulla <medulla@tinyhumans.ai>
`cargo fmt -- --check` does not pass on main: one test in `web3::wallet::chains::btc` is wrapped differently from what rustfmt 1.9.0 — the version `rust-toolchain.toml` pins at 1.96.1 — produces. Whitespace only, no behaviour. Unrelated to the rest of this branch, and carried here because `format:check` is a repo-wide gate: a branch that cannot pass it cannot merge, and leaving a one-line reflow to be rediscovered by whoever next runs `cargo fmt` is worse than a commit that says what it is. Co-authored-by: Medulla <medulla@tinyhumans.ai>
tinydocs#6 merged and v0.1.12 shipped, so the registry now points at a real release instead of the pre-port v0.1.11 whose digests predated every commit in that PR. All eleven per-platform SHA-256 digests are taken verbatim from the release's own `checksum.toml`, which is the point of pinning them: tinybus re-fetches that manifest and refuses on disagreement, so the copy here is a second, offline-auditable gate rather than the only one. Recomputing them from a local build would have produced numbers that agree with themselves and nothing else. Verified end to end against the published artifact, not a local build: with `~/.cache/openhuman/modules` deleted and no `OPENHUMAN_MODULE_PATH` set, the host resolves the release, downloads it, checks it against both digests, extracts, `dlopen`s and admits it, and produces an openable `.docx` — and a `.pptx` whose image crossed as a bus stream. That run also confirms the admission decision made earlier in this branch: strict mode refuses the real artifact on a rustc mismatch, permissive accepts it, and everything protecting the address space is still enforced. Gitlink moves to tinydocs main at 7c907265fbc99c45397676202047ab2ac84e8643, which carries the merge plus the release commit. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Two CI failures, both mine, both from adding the `modules` gate. **Feature Forwarding Gate.** The guard asserts the shell's forwarded list and `scripts/ci/product-features.txt` are equal *in both directions*, and I only satisfied one: the shell forwards `modules`, the product list did not claim it. That direction exists for a reason — it is what stops the shipped app quietly growing a domain the CI product lanes never compile or test. Claiming it here is also the honest answer rather than a formality: `documents` is in the product set and implies `modules`, so the product build already resolves it. I reported this gate as passing locally earlier in the branch. It was not — I read the exit status of a `tail` in a pipeline rather than the script's, and the script prints its complaint and still gets its status swallowed. The number was wrong, not the tool. **Rust Quality.** `scripts/check-linux-tls-dependencies.sh` runs cargo with `--locked`, and `app/src-tauri/Cargo.lock` had not been regenerated after the shell manifest gained the `modules` feature. One line of lockfile; the step now passes for both Cargo worlds. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Update the tinydocs dependency to version 0.1.12 in the Cargo.lock file to incorporate the latest bug fixes and improvements from the upstream crate. Co-authored-by: Medulla <medulla@tinyhumans.ai>
e888a9f to
2b557f2
Compare
CI triage: what is mine and what is
|
…e loop - `read_all` refuses a module-declared length over 64 MiB and no longer sizes `Vec::with_capacity` from an untrusted number: an allocation failure aborts the process rather than returning an error. - `glibc_version` is `target_env = "gnu"` only; musl does not export the symbol, so the binary would fail to link before the `None` fallback could run. - `ops` runs the download, digest check, extraction and `dlopen` under `spawn_blocking` rather than on the tokio worker polling the future. - The host runtime tests are one function: `runtime()` is a process-global, and a second `#[tokio::test]` would call into a broker whose tasks died with the first runtime — hanging rather than failing. - `document::engine::generate` splits into a `generate_with(config, ...)` half so its tests stop reading, and on a fresh machine writing, the real user config. - The install-directory assertion checked two arms where the first implied the second, so it could not fail; the presentation assertion reported `result.text()`, which is empty for a Json-only result. - The spec's local-module recipe names the filename each platform builds.
Every PR run today fails on 20 identical 403s: the ProductHunt badge URLs in the translated READMEs, which the site serves only to browsers. The last green run on `main` predates the block, and no PR touching those files caused it. Excluded the host the same way reddit, x.com and star-history already are — a badge image that refuses bots is not a broken link, and leaving it there trains everyone to ignore a red check that is `continue-on-error` anyway.
Summary
.docx/.pptxgeneration and PDF text extraction now run in thetinydocsTinyBus module, loaded at runtime and verified against a digest compiled into the build.Cargo.lock, 0 added. The product profile — what ships — goes from 505 to 448 unique crate names.docx-rs,ppt-rs,pdf-extractand their font/PostScript/XML tails are gone from the graph, not merely gated.openhuman::modulesdomain: the module host, a compiled-in registry of modules this build trusts, glibc-aware artifact selection, and amodulesRPC namespace.mainare fixed or flagged; details under Impact.Problem
The
documentsgate carried three codecs and their transitive trees — 39 crates, four native C builds — to support three agent tools. None of it is kernel work.Gating them helped the builds that turned them off and did nothing for the build that turns them on, which is the one that ships. What was needed was a boundary that survives compilation: the capability present, the dependencies absent.
Solution
A module is a compiled
cdylibspeaking the tinybus module ABI, attached to a private in-process broker as an ordinary bus peer. Five decisions are worth a reviewer's attention:consttable. Neither config nor RPC can name an artifact to load — a registry a server could extend would be remote code execution with a download step.[modules]config controls only whether modules load, whether this host may fetch them, and where a developer's own build lives.checksum.toml, compares it with ours, hashes the download, and extracts only after. Pinning makes it auditable offline and makes a release re-cut under the same tag stop matching..sobuilt against glibc 2.39 fails todlopenon 2.35 with a symbol-version error the ABI gate cannot phrase helpfully.modules::platformprobes glibc, prefers the newest build that could work, and falls through on admission failure. A musl host gets an empty list — "unsupported" beats a download that cannot load.rustc version does not match in strict mode. Releases are built on whatever toolchain CI had; this crate pins its own. Strict would have meant the feature never worked in the field while every local build looked fine. Everything protecting the address space is still enforced; only the toolchain string is relaxed.The trade, stated plainly: a loaded module shares this address space, these privileges and this crash domain, and tinybus never unloads it. The ABI, manifest and digest gates decide what is admitted, never what is safe. Modules are first-party code that ships separately.
Submission Checklist
modules::, plus rewritten engine tests. Weighted towards failure paths: unsupported host, downloads disabled, module disabled, unknown module, registry/platform-table drift, and the deck/payload agreement that a corrupt image would violate.tinydocs(where the code now lives) and gained tests for what remains host-side.cargo llvm-covrun locally over the changed files.N/A: no feature row exists for the document tools(the matrix's only document entry, 4.2.8, is composer attachments and is unaffected).## Related—N/A, none affected.documents_tests.rsand themodules::opstests run withallow_download = false, and the three tests that need a real module are#[ignore]d. The runtime download of a verified module artifact is a product behaviour, not a test dependency, and it is called out under Impact for the reviewer.N/A: no release-cut surface changes; the tools' behaviour and outputs are unchanged.Closes #NNN—N/A, no tracking issue.Impact
Runtime: the first use of a document tool downloads and verifies a ~6 MB artifact from a pinned GitHub release, then caches it. It is deliberate and configurable —
modules.allow_download = falsepins a host to locally installed artifacts — but it is a genuine product decision and worth confirming rather than waving through. (It is not a test dependency; see the checklist item above.)Platform: on a host outside the 11 published targets — musl, or a BSD — the three tools report unavailable instead of working. Artifacts are published for Linux (glibc ≥ 2.35), macOS and Windows on x86_64 and arm64.
Security: a loaded module is trusted in-process native code. The gates above are what stands between a release and this process's address space; the registry being compiled in is what stops that surface being reachable from config or RPC.
Compatibility: no schema, config-migration or wire changes.
[modules]is additive with a serde default, soCURRENT_SCHEMA_VERSIONis unchanged.Three pre-existing problems on
mainEach verified against pristine
mainrather than assumed:cargo check -p openhuman --libdoes not compile.memory-gitis default-OFF andmemory/diffhas an ungated re-export of a gated module plus an import of atypessubmodule that does not exist. Fixed in its own commit — it blocked everything else.cargo fmt -- --checkfails onweb3/wallet/chains/btc.rsunder the pinned rustfmt. Fixed in its own commit, whitespace only.scripts/check-kernel-floor.shfails, at 305 packages / 282 names against a 302/279 limit. Not fixed and deliberately not papered over — the branch resolves the same numbers asmain, so this is inherited growth. Raising the limit to hide it is exactly what that file exists to prevent.The ratchet did catch a real regression of mine, which is why the numbers match: enabling
tinybus/moduleson the dependency put adlopenloader plusureqinto the kernel profile (305 → 308), becausetinybusis always-on surface. It is forwarded from this crate's ownmodulesfeature instead.Related
vendor/tinydocsgitlink points at tinydocsmainincluding that release, andmodules::registrypins the eleven per-platform digests taken verbatim from the release'schecksum.toml. No longer blocked.module_export!would let one module serve transfer and format interfaces separately.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
tinydocs-modulemainValidation Run
pnpm --filter openhuman-app format:check—cargo fmt -- --checkclean (noapp/srcchanges in this PR)pnpm typecheck— N/A, no TypeScript changedcargo test -p openhuman --lib --features "$(scripts/ci/product-features.sh)"overimplementations::document,implementations::presentation,modules::,core::all— 177 passed, 0 failed, 3 ignoredcargo fmt -- --checkclean;cargo clippy -p openhuman --lib --features <product>— 0 findingscargo check --manifest-path app/src-tauri/Cargo.tomlclean (themodulesgate is forwarded to the shell;check-feature-forwarding.mjspasses)Additionally, and these are the ones that prove the change:
scripts/assert-shed.shoverdocx-rs ppt-rs pdf-extract syntect pulldown-cmark lopdf xml-rs— all absent from both thedocumentsprofile and the full product profile. Notcargo tree -i, which that script's own header explains is unreliable as a shed proof.scripts/kernel-floor.sh flows— 305/282/2, identical tomain.--features documents,--no-default-features --features flows, and the full product set all compile.#[ignore]d module-backed tests, run one per process — all pass end to end: tool → image resolution → bus stream → module →.pptx→ held output → chunked pull → artifact on disk.~/.cache/openhuman/modulesdeleted and noOPENHUMAN_MODULE_PATHset, the host resolves the release, downloads it, verifies it against both the release's ownchecksum.tomland the digest pinned inmodules::registry, extracts,dlopens and admits it, and produces an openable.docx— and a.pptxwhose image crossed as a bus stream. This is the run that proves the whole chain, and it is also what confirms the permissive-admission decision: strict mode refuses the real artifact on a rustc mismatch.Validation Blocked
command:fullcargo test -p openhuman --lib(whole suite, parallel)error:stack overflow in a differentagent::harness::sessiontest on each run, plus order-sensitive failures inarchivist/git_attributionimpact:none from this branch — reproduced on pristinemainwith the original tinybus, and the affected tests pass individually on this branch exactly as they do onmain. Verified rather than assumed.Behavior Changes
Parity Contract
slide_countstill excludes the synthetic title slide; image failures still degrade to a skip-with-warning rather than failing a deck; a PDF that cannot be extracted still degrades toFilePayload::Reference;ai_regeneratefor presentations is untouched.modulesgate are pinned by tests (modules_controllers_{registered_when_feature_on,absent_when_feature_off}) — the off-half matters more than usual, because what the feature compiles in is adlopenloader, so an opted-out build must have no way to reach one rather than a loader that refuses. All three non-compiler-enforcedDomainGroupdrift guards are answered explicitly.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Documentation