feat(tools): add generate_presentation tool (native rust engine, ppt-rs) (#2778)#3016
Merged
graycyrus merged 30 commits intoJun 2, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a native Rust PPTX generation tool using the ChangesArtifact Lifecycle and State Management
Python Runtime Infrastructure
Native PPTX Generation Tool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…humansai#2778) tinyhumansai#2776 (PR tinyhumansai#2801) shipped the artifact storage layer with read RPCs (ai_list_artifacts / ai_get_artifact / ai_delete_artifact) but no producer surface — tools that wanted to register a generated file had no public path into the store. Adds three composable helpers: - create_artifact(workspace_dir, kind, title, extension) -> (ArtifactMeta, PathBuf): allocates a fresh UUID-named dir under <workspace>/artifacts/, persists a Pending ArtifactMeta record, returns the meta plus the absolute path the caller writes bytes to. Filename stem is sanitized from the title (alphanumerics + dashes, ≤ 80 chars) so user-supplied titles can't escape the artifact dir. - finalize_artifact(workspace_dir, id, size_bytes): flips status to Ready + persists final size. Idempotent on already-Ready records. - fail_artifact(workspace_dir, id, reason): flips to Failed + persists the reason via the new ArtifactMeta.error field. Extends ArtifactMeta with optional error: Option<String> (serde default + skip_if_none) so list_artifacts / get_artifact callers can surface why a build did not produce a usable file without scraping a separate log. Persisted records that predate this field round-trip fine through serde::default. Used by the tinyhumansai#2778 presentation tool in this PR; future Python / binary-output tools share the same surface so per-tool dirs + status bookkeeping don't fragment.
…inyhumansai#2778) runtime_python previously exposed only stream-oriented primitives (spawn_stdio_process, PythonBootstrap::spawn_stdio) sized for the MCP stdio path. Tools that want a simpler 'run a one-shot script, pipe stdin, wait, capture stdout+stderr' contract were each rolling their own wait-and-bound plumbing. Two new modules: run.rs — run_python_script_to_completion(resolved, spec, stdin, deadline) -> PythonRunOutput { exit_code, stdout, stderr }. Wraps spawn_stdio_process + tokio::time::timeout + wait_with_output. Returns Ok(...) on non-zero exit (callers usually want to quote the stderr in their own error variant); only timeout / spawn failures surface as Err. PythonRunTimeout is its own typed error so call sites can downcast. venv.rs — ensure_venv(name, requirements, config) -> ResolvedPython. First call resolves the base interpreter via PythonBootstrap, creates <cache_dir>/venvs/<name>/ via 'python -m venv', runs 'python -m pip install <requirements>' bounded by a 5-minute timeout, and writes a sorted/deduped requirements.txt for change detection. Subsequent calls short-circuit when the recorded requirements match. Venv name is sanitised against path traversal + separators. Both modules are intentionally narrow — they own subprocess plumbing + caching only, no package-manager features beyond bare pip install. The presentation tool in this PR is the first consumer; future Python-backed tools (tinyhumansai#2780 + later) reuse the same helpers.
…nyhumansai#2778) New tool 'generate_presentation' that takes a structured slide spec {title, author?, theme?, slides[{title?, body?, bullets?, speaker_notes?}]} and produces a .pptx file via a bundled python-pptx helper script. The tool fulfills sub-task tinyhumansai#2778 of tinyhumansai#1535. Module layout (src/openhuman/tools/impl/presentation/): - generate_pptx.py: pure-Python helper, reads JSON spec from stdin, writes PPTX to --output PATH. Single file, no relative imports, no eval. Bundled via include_str! so packaging is bundle-free. - script.rs: include_str! + lazy tempfile materialiser (tokio::sync::OnceCell), cached per-process. - types.rs: GeneratePresentationInput / SlideSpec / Output / PresentationError. Validation caps: ≤ 64 slides, ≤ 2000 chars per field, ≤ 32 bullets/slide. Eager Rust-side validation so the LLM gets InvalidInput { field, reason } instead of a python-pptx traceback. - invoker.rs: PythonInvoker trait + RealPythonInvoker impl. RealPythonInvoker bridges to runtime_python::venv::ensure_venv + run_python_script_to_completion. Trait seam lets unit tests inject a MockPythonInvoker so coverage on success / non-zero / timeout / missing-runtime / missing-package branches does not require a live Python. - mod.rs: Tool trait impl. Router-rule description ('USE THIS ... NOT for ...') per existing tool conventions. 60s generation timeout (matches multimodal PDF cap). - tests.rs: 12 unit tests covering schema shape, every validation rejection path, success via MockPythonInvoker (asserts artifact flipped Pending -> Ready), GenerationFailed via non-zero exit with truncated stderr, Timeout, MissingRuntime, MissingPackage, and missing-output-file recovery. python-pptx==1.0.2 is installed into a managed venv at <runtime_python.cache_dir>/venvs/presentation-pptx/ on first invocation (~30s on cold pip cache); subsequent calls short-circuit inside ensure_venv. The pin protects against historical layout- placeholder breakage in minor-version bumps. Tool registration in tools/ops.rs is gated on config.runtime_python.enabled so disabled deployments never even see the tool in the agent's tool catalog. Orchestrator agent-definition wiring (tinyhumansai#2780) lands separately; this PR registers the tool in all_tools_with_runtime so CLI / bus / test paths can invoke it. Stable name: 'generate_presentation' — coordinated with tinyhumansai#2780.
…ing (tinyhumansai#2778) tests/presentation_tool.rs spins up the real PresentationTool against a temp workspace and a temp runtime_python cache, drives it through a host Python interpreter (prefer_system = true so it does not reach for the managed installer), and asserts: - the produced file starts with the PK\x03\x04 zip magic, and - the zip contains the OOXML [Content_Types].xml manifest so the test is a meaningful end-to-end check that python-pptx actually ran and produced a valid PPTX (not just any zip blob). The test skips with an eprintln! note when the host lacks python3 or python-pptx so local contributors without a Python install still see green. CI gets a 'pip install python-pptx==1.0.2' step in the Rust-core coverage lane (.github/workflows/coverage.yml) so the success branch is exercised on every PR. A second integration test asserts the validation path runs before any Python invocation (rejecting an empty title without spawning a subprocess), which is the fast-feedback contract the agent depends on for InvalidInput correction. Live-verified locally: integration tests pass end-to-end against a real python3 + python-pptx install — the bundled generate_pptx.py script materialised, ensure_venv built a real venv against a temp cache dir, pip install python-pptx==1.0.2 succeeded, the script produced a valid PPTX containing the expected OOXML structure.
tinyhumansai#2778) Without this entry the orchestrator's named-tool whitelist (orchestrator/agent.toml:101 'named = [...]') filters generate_presentation out of the LLM-visible tool catalog, so even though tools/ops.rs registers it globally the web chat / bus path can't reach it. Adding the bare name here closes the loop for the 'create slides' routing case the parent issue tinyhumansai#1535 explicitly calls out, without waiting on the broader tinyhumansai#2780 agent-definition sweep (per-tier tool_filter policy + structured error mapping). disallowed_tools refinements, error-variant surfacing through tool_loop). The duplicate entry there will be a harmless no-op.
oxoxDev
force-pushed
the
feat/2778-presentation-tool
branch
from
ce373d8 to
aa1469c
Compare
coderabbitai
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/openhuman/tools/ops.rs (1)
264-266: ⚡ Quick winAdd structured correlation fields to the new registration-gate log.
At Line 264, the new debug log is grep-friendly but unstructured; add fields (for example
tool = "generate_presentation"andruntime_python_enabled = false) so this branch is easier to aggregate/filter in diagnostics.As per coding guidelines: "In Rust, default to verbose diagnostics on new/changed flows using
log/tracingatdebug/tracelevels with stable grep-friendly prefixes and correlation fields".🤖 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/ops.rs` around lines 264 - 266, The tracing::debug! call that logs "[tools::ops] runtime_python disabled — generate_presentation tool not registered" should be converted to a structured tracing event by adding correlation fields (e.g. tool = "generate_presentation" and runtime_python_enabled = false) so it can be filtered/aggregated; modify the tracing::debug! invocation in src/openhuman/tools/ops.rs to include those key/value fields while keeping the same human-readable message prefix for grep-friendliness.src/openhuman/tools/impl/presentation/invoker.rs (1)
87-155: ⚡ Quick winAdd debug/trace diagnostics for the invocation lifecycle.
Line 87–155 currently logs only one warning branch; add start/outcome debug logs (script, output path, deadline, classified outcome/exit code) for consistent tool observability.
As per coding guidelines "In Rust, default to verbose diagnostics on new/changed flows using
log/tracingatdebug/tracelevels with stable grep-friendly prefixes and correlation fields".🤖 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/invoker.rs` around lines 87 - 155, Add trace/debug diagnostics around the RealPythonInvoker::run flow: emit a trace at method start with script_path, output_path, deadline and VENV_NAME; after ensure_venv succeeds log the resolved venv (resolved) at debug; before calling run_python_script_to_completion log the PythonLaunchSpec (spec.args) at trace; on run error log outcome classification (Timeout vs MissingRuntime) with err string at debug; and on completion log the final InvocationOutcome with exit_code and short summaries of stdout/stderr (or their lengths) at debug/trace so all branches of run (Success, NonZeroExit, Timeout, MissingRuntime, PackageInstallFailed) are consistently observable. Reference symbols: RealPythonInvoker::run, ensure_venv, VENV_NAME, PythonLaunchSpec, run_python_script_to_completion, InvocationOutcome.
🤖 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/runtime_python/run.rs`:
- Around line 65-89: The stdin write/close path can block indefinitely because
write_all/shutdown are awaited before the timeout is applied; wrap the write_all
and shutdown calls in the same deadline timeout used for process output (use the
existing deadline variable) so they fail if the deadline passes. Concretely,
when you take child.stdin (the block using child.stdin.take(),
stdin_handle.write_all(&payload), and stdin_handle.shutdown()), execute those
async operations via timeout(deadline, ...) and propagate the timeout error with
context similar to the existing output wait (include spec.script_path in the
context) so write and shutdown are bounded by the same deadline as the
output_future.
In `@src/openhuman/runtime_python/venv.rs`:
- Around line 73-79: The info log is emitting raw requirement strings (variable
requirements) which can contain sensitive URLs/credentials; update the
tracing::info! call in this module (the tracing::info! invocation inside the
venv setup function in venv.rs) to avoid printing full requirement values by
replacing requirements with a redacted representation (e.g., map each
requirement to a scrubbed/masked value or just log the count or package names
without URLs) before passing to tracing::info!, or introduce a small helper
(e.g., redact_requirements or scrub_requirements) to transform the requirements
variable and log that safe summary instead of the raw requirements.
- Around line 174-199: The create_venv function currently awaits cmd.output()
with no bound; wrap the output() future in tokio::time::timeout (e.g., a
reasonable Duration constant) so the venv creation cannot hang indefinitely,
then handle the Err(timeout) by returning an error (bail! or with_context)
indicating the timeout and allow kill_on_drop to clean up the child; on
Ok(inner) proceed as before checking inner.status and stderr. Ensure you import
tokio::time::timeout and use a named timeout Duration constant so the timeout
value is clear and adjustable.
In `@src/openhuman/tools/impl/presentation/generate_pptx.py`:
- Around line 63-71: The main function parses the --output argument but doesn't
enforce that it's an absolute path; after args = parser.parse_args() add a guard
using os.path.isabs(args.output) and call parser.error(" --output must be an
absolute path") or sys.exit(1) if it returns False so the program fails fast
when a non-absolute path is supplied; import os (and sys if using sys.exit) and
reference main, parser, args and the --output flag when making this change.
In `@src/openhuman/tools/impl/presentation/mod.rs`:
- Around line 181-186: The info log in the presentation generation path
currently emits user-supplied input.title directly via tracing::info (target:
"presentation"), which may leak PII; change the log to redact the title by
logging derived metadata instead (e.g., title length or a boolean has_title) and
keep slide_count and context ("[presentation] generation request accepted");
locate the tracing::info call in mod.rs and replace the title = %input.title
field with a safe metadata field such as title_length = input.title.len() or
title_present = !input.title.is_empty().
- Around line 197-209: When errors occur after create_artifact (e.g., in
script::materialise_script, serde_json::to_vec, or self.invoker.run inside
execute), catch those failures instead of propagating with ?, update the created
artifact to a failed state (call the artifact failure/update method used in this
module) with the error details, and then return the error; specifically wrap the
calls to script::materialise_script(), serde_json::to_vec(&input), and
self.invoker.run(...) so any Err triggers artifact.fail/update (or the
equivalent method on the object returned by create_artifact) before returning
the original error.
In `@src/openhuman/tools/impl/presentation/script.rs`:
- Around line 33-40: The code currently creates a deterministic temp directory
by joining std::env::temp_dir() with process id and then writes GENERATE_PPTX_PY
into it; replace that with a secure, unique temp dir and an atomic/new-file
write: use tempfile (e.g.
tempfile::Builder::new().prefix("openhuman-presentation-").tempdir() or
tempfile::TempDir::new_in(...)) to create an unpredictable TempDir (reference
the dir variable creation), then construct path =
dir.path().join("generate_pptx.py"), and open/write the file using a
create_new/open options approach
(tokio::fs::OpenOptions::new().write(true).create_new(true) or equivalent) so
the write fails if the file already exists and avoids symlink/toctou issues when
writing GENERATE_PPTX_PY; ensure you propagate the same with_context messages
for errors (the with_context calls around directory creation and file write).
In `@src/openhuman/tools/impl/presentation/types.rs`:
- Around line 136-155: The validate_input logic is missing a length check for
input.theme, allowing arbitrarily long themes; update the validate_input
function to check input.theme (e.g., if let Some(theme) =
input.theme.as_deref()) and return PresentationError::InvalidInput with field
"theme" and reason format!("must be ≤ {MAX_TEXT_CHARS} chars") when
theme.chars().count() > MAX_TEXT_CHARS, matching the existing title/author
validation pattern and using the same MAX_TEXT_CHARS cap.
In `@tests/presentation_tool.rs`:
- Around line 21-30: The two preflight checks call python_available() and
python_pptx_importable() separately and can end up using different Python
binaries; change the flow to resolve a single Python executable once (e.g. via a
new resolve_python_executable() or by altering python_available() to return the
resolved path) and pass that resolved path into the import check (call
python_pptx_importable(resolved_path) or similar) and into later code that
invokes Python (the code referenced around Line 59 and the similar block at
112-132), so both availability and import checks — and the runtime invocation —
use the same interpreter.
---
Nitpick comments:
In `@src/openhuman/tools/impl/presentation/invoker.rs`:
- Around line 87-155: Add trace/debug diagnostics around the
RealPythonInvoker::run flow: emit a trace at method start with script_path,
output_path, deadline and VENV_NAME; after ensure_venv succeeds log the resolved
venv (resolved) at debug; before calling run_python_script_to_completion log the
PythonLaunchSpec (spec.args) at trace; on run error log outcome classification
(Timeout vs MissingRuntime) with err string at debug; and on completion log the
final InvocationOutcome with exit_code and short summaries of stdout/stderr (or
their lengths) at debug/trace so all branches of run (Success, NonZeroExit,
Timeout, MissingRuntime, PackageInstallFailed) are consistently observable.
Reference symbols: RealPythonInvoker::run, ensure_venv, VENV_NAME,
PythonLaunchSpec, run_python_script_to_completion, InvocationOutcome.
In `@src/openhuman/tools/ops.rs`:
- Around line 264-266: The tracing::debug! call that logs "[tools::ops]
runtime_python disabled — generate_presentation tool not registered" should be
converted to a structured tracing event by adding correlation fields (e.g. tool
= "generate_presentation" and runtime_python_enabled = false) so it can be
filtered/aggregated; modify the tracing::debug! invocation in
src/openhuman/tools/ops.rs to include those key/value fields while keeping the
same human-readable message prefix for grep-friendliness.
🪄 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: 05501243-a797-4880-8cce-829b920ac1a5
📒 Files selected for processing (18)
.github/workflows/coverage.ymlsrc/openhuman/agent_registry/agents/orchestrator/agent.tomlsrc/openhuman/artifacts/mod.rssrc/openhuman/artifacts/store.rssrc/openhuman/artifacts/store_tests.rssrc/openhuman/artifacts/types.rssrc/openhuman/runtime_python/mod.rssrc/openhuman/runtime_python/run.rssrc/openhuman/runtime_python/venv.rssrc/openhuman/tools/impl/mod.rssrc/openhuman/tools/impl/presentation/generate_pptx.pysrc/openhuman/tools/impl/presentation/invoker.rssrc/openhuman/tools/impl/presentation/mod.rssrc/openhuman/tools/impl/presentation/script.rssrc/openhuman/tools/impl/presentation/tests.rssrc/openhuman/tools/impl/presentation/types.rssrc/openhuman/tools/ops.rstests/presentation_tool.rs
…yhumansai#2778) The author/title/slide-field checks bound input size, but the optional theme field was never validated. Apply the same MAX_TEXT_CHARS cap so a caller cannot bypass the input-size guardrail by stuffing the theme slot.
…rors (tinyhumansai#2778) CR findings: - info log emitted user-supplied input.title verbatim; swap for title_chars + has_author so the structured log carries no PII. - script::materialise_script / serde_json::to_vec / invoker.run all used `?`, so any error left the artifact stuck in Pending. Wrap the setup-through-invocation block so an error flips the artifact to Failed with a structured reason before returning.
…inyhumansai#2778) CR finding: the materialised script path was derived deterministically from std::process::id() and written via tokio::fs::write, which would follow a pre-created symlink or overwrite an attacker-planted file in the shared temp directory. Switch to a uuid-suffixed filename opened with O_EXCL (OpenOptions::create_new) so a hostile pre-create fails the open instead of redirecting or clobbering the script.
…ansai#2778) The bundled script's contract requires an absolute output path, but the script never enforced it. Add a defence-in-depth guard so a future regression on the Rust caller cannot silently redirect the generated .pptx to a cwd-relative location.
…humansai#2778) CR finding: write_all + shutdown on the child's stdin were awaited unbounded, so a stuck pipe could keep the function past `deadline` before the wait_with_output timeout ever started. Compute remaining time from a single Instant::now() anchor and wrap every awaited stdin/wait operation in timeout(remaining, _) so the whole call is bounded by `deadline`.
…umansai#2778) CR findings on venv.rs: - info log dumped raw requirement strings, which can legitimately include credentialed/private index URLs. Log the count instead. - create_venv awaited `python -m venv` output() with no bound, so a stuck interpreter could hang the parent task indefinitely. Wrap the spawn in tokio::time::timeout(VENV_INSTALL_TIMEOUT, _) so the same budget as pip install applies to bootstrap.
…humansai#2778) CR finding: python_available() and python_pptx_importable() probed [python3, python] independently, so they could succeed on different interpreters — and then the real tool invocation in execute() would pick yet another, producing nondeterministic skips/failures. Resolve the binary once via resolved_python() and pass that name into the pptx-import probe so both checks pin to the same interpreter.
Contributor
There was a problem hiding this comment.
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/presentation/generate_pptx.py (1)
80-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDecode stdin as UTF-8 explicitly in generate_pptx.py
src/openhuman/tools/impl/presentation/generate_pptx.pyreadsraw = sys.stdin.read()and only catchesOSError; a UTF-8 decode failure raisesUnicodeDecodeError(aValueError) and will escape, producing an uncaught traceback (exit 1) despite the script contract stating exit 2 on input/runtime errors. The Rust caller passes UTF-8 JSON bytes viaserde_json::to_vec(&input)and does not set any UTF-8-forcing env vars for the Python child (spawn_stdio_processonly appliesspec.env, andinvoker.rsdoesn’t populate it), so decoding remains locale-dependent.Suggested fix
try: - raw = sys.stdin.read() + raw = sys.stdin.buffer.read().decode("utf-8") except OSError as err: _exit_error(f"failed to read stdin: {err}") return + except UnicodeDecodeError as err: + _exit_error(f"stdin payload was not valid UTF-8: {err}") + return🤖 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/generate_pptx.py` around lines 80 - 84, The stdin read in generate_pptx.py currently uses raw = sys.stdin.read() and only catches OSError, which misses UTF-8 decode errors; change to read bytes from sys.stdin.buffer (or decode the read explicitly) and decode using 'utf-8' inside the try block, and expand the except to catch UnicodeDecodeError (and ValueError if needed) so any decode failure calls _exit_error(...) and returns the proper exit code; locate the read/except around raw and update the handling to ensure _exit_error is used for decode failures.
🤖 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.
Outside diff comments:
In `@src/openhuman/tools/impl/presentation/generate_pptx.py`:
- Around line 80-84: The stdin read in generate_pptx.py currently uses raw =
sys.stdin.read() and only catches OSError, which misses UTF-8 decode errors;
change to read bytes from sys.stdin.buffer (or decode the read explicitly) and
decode using 'utf-8' inside the try block, and expand the except to catch
UnicodeDecodeError (and ValueError if needed) so any decode failure calls
_exit_error(...) and returns the proper exit code; locate the read/except around
raw and update the handling to ensure _exit_error is used for decode failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 555dcecf-b5f5-4b59-8180-3d52982430d8
📒 Files selected for processing (7)
src/openhuman/runtime_python/run.rssrc/openhuman/runtime_python/venv.rssrc/openhuman/tools/impl/presentation/generate_pptx.pysrc/openhuman/tools/impl/presentation/mod.rssrc/openhuman/tools/impl/presentation/script.rssrc/openhuman/tools/impl/presentation/types.rstests/presentation_tool.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/presentation_tool.rs
- src/openhuman/tools/impl/presentation/mod.rs
- src/openhuman/tools/impl/presentation/types.rs
- src/openhuman/runtime_python/venv.rs
- src/openhuman/runtime_python/run.rs
…lper dismissWalkthroughIfPresent loops on the Skip-tour button for up to 5s and falls back to setting `openhuman:walkthrough_completed` in localStorage when the button never appears. That flag prevents future tours but does not unmount an already-mounted #react-joyride-portal — its full-screen overlay keeps intercepting clicks on the next interaction. After the localStorage fallback, scrub any remaining portal nodes from the DOM so subsequent clicks (skill-install buttons, tabs, provider rows) are not blocked. Tests that already passed are unaffected; tests racing against a slow joyride mount no longer time out waiting on an overlay nobody can dismiss.
12 tasks
Contributor
Author
|
@senamakel @graycyrus need reviews |
oxoxDev
added a commit
to oxoxDev/openhuman
that referenced
this pull request
May 30, 2026
…inyhumansai#2779) Two preemptive fixes so this PR's CI doesn't hit the same lane-2/4 failure currently sticking on tinyhumansai#3016: 1) insights-dashboard.spec.ts was written when /intelligence's default tab was 'memory'. PR tinyhumansai#2998 (Agent Tasks board) changed that default to 'tasks' in Intelligence.tsx:28 without updating this spec — the Memory heading + memory-workspace / memory-actions testids only mount when activeTab === 'memory', so on a fresh Playwright session (no redux-persist, no localStorage) the heading assertion times out at 10s and the spec fails. Fix: after waitForAppReady, dismiss the walkthrough then click the Memory pill (role="tab" name="Memory" — PillTabBar in app/src/components/PillTabBar.tsx) before asserting the panel chrome. Bumped the heading toBeVisible timeout to 15s defensively for slow CI lanes. 2) Back-ported the joyride-portal DOM scrub from f577c9c (currently only on origin/feat/2778-presentation-tool, since this branch was cut before that commit landed). Stripping #react-joyride-portal nodes from the DOM after the localStorage completion fallback prevents the overlay from intercepting subsequent clicks — same hardening as tinyhumansai#3016.
tinyhumansai#2778) CR finding (PR tinyhumansai#3017): the stdin write_all/shutdown ran to completion before wait_with_output drained the child's stdout/stderr. If the script emitted enough output to fill the OS pipe buffer (~64 KiB on Linux) while we were still writing stdin, the child blocked on its output, stopped reading stdin, and write_all blocked too — classic pipe deadlock. The deadline timeout from the earlier round bounded how long we waited but didn't address the deadlock itself. Move the stdin write+shutdown onto a spawned tokio task so the main task can drive wait_with_output concurrently. Both pipes make progress against each other. On deadline timeout we drop the child handle (kills the process, breaks the pipe) and join the writer task so it surfaces any error before we return. The slide-spec payload for the presentation tool can easily exceed 64 KiB once user content is interpolated, so this was not theoretical. Existing run.rs unit tests (round-trip stdin, non-zero exit, timeout on runaway script) continue to pass.
…e portals The previous one-shot scrub stripped #react-joyride-portal nodes at helper exit, but the walkthrough effect re-mounts a fresh portal on later hash-route navigations (e.g. spec calls dismissWalkthroughIfPresent then goto'/#/skills' or clicks a tab that triggers a route change). The localStorage flag prevents future tours from starting, but a portal that mounted before the flag was persisted lingers and the new mount keeps coming back. After the localStorage fallback, install an idempotent MutationObserver on document.body that removes any #react-joyride-portal as soon as it appears, for the rest of the page lifetime. Re-runs of the helper no-op via a window flag. Fixes the lane 3 settings-channels-permissions and lane 4 skills-registry flakes where the channels-tab / skill-install click was intercepted by an overlay that mounted after the helper exited.
Closed
7 tasks
Contributor
Author
|
@coderabbitai full review — all 12 inline threads were resolved against the original python-pptx implementation. The PR has since been rewritten in commit |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/openhuman/artifacts/store_tests.rs (1)
16-16: ⚡ Quick winAssert the new
errorfield in the round-trip test.
make_meta()now seedserror: None, butsave_and_get_roundtrip()never checks it, so a regression inArtifactMeta.errorserialization would still pass. Add at least aNoneassertion here, and ideally oneSome(...)case too.Also applies to: 20-36
🤖 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/artifacts/store_tests.rs` at line 16, The round-trip test omits asserting the new ArtifactMeta.error field, so update the tests that call make_meta() and save_and_get_roundtrip() to assert the error field is preserved: add an assertion that meta.error is None for the existing make_meta() case, and add a separate test case that constructs a meta with error = Some("...") and verifies save_and_get_roundtrip() returns the same Some(...) value; target the test helpers and assertions around make_meta(), save_and_get_roundtrip(), and the ArtifactMeta.error field to ensure both None and Some(...) are covered.src/openhuman/artifacts/store.rs (1)
247-275: 💤 Low valueOptional: replace
out.chars().count()without.len().Every pushed char is ASCII (
a-z/0-9/-/_), soout.len()equals the char count but is O(1) instead of re-scanning the buffer each iteration (currently O(n²)). Bounded by the 80-char cap, so impact is small.♻️ Proposed tweak
out.push(mapped); - if out.chars().count() >= MAX_SANITIZED_FILENAME_LEN { + if out.len() >= MAX_SANITIZED_FILENAME_LEN { break; }🤖 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/artifacts/store.rs` around lines 247 - 275, The loop in sanitize_filename_stem currently checks out.chars().count() to enforce MAX_SANITIZED_FILENAME_LEN, which rescans the buffer each iteration; change that check to use out.len() for O(1) length checking. Update the conditional inside sanitize_filename_stem that reads "if out.chars().count() >= MAX_SANITIZED_FILENAME_LEN" to "if out.len() >= MAX_SANITIZED_FILENAME_LEN" (keeping the same MAX_SANITIZED_FILENAME_LEN symbol and all other logic intact).
🤖 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 @.github/workflows/coverage.yml:
- Around line 96-105: Remove the now-stale GitHub Actions step that installs
python-pptx and its explanatory comment block from the coverage workflow: delete
the entire step that runs "python3 -m pip install --user --quiet
python-pptx==1.0.2" along with the surrounding comment lines about the
generate_presentation tool and runtime_python; the presentation tests
(tests/presentation_tool.rs) now use the Rust ppt-rs engine only so the
networked pip install is unnecessary.
In `@src/openhuman/agent_registry/agents/orchestrator/agent.toml`:
- Around line 163-171: The TOML comment above the "generate_presentation" agent
entry is stale (mentions a python-pptx helper and subprocess) — update the
comment to reflect the current implementation using the native Rust ppt-rs
engine, remove references to Python runtime/subprocess, and clarify that output
still lands in the workspace artifacts directory and the tool returns the
artifact id + absolute path so the orchestrator can quote it.
In `@src/openhuman/runtime_python/venv.rs`:
- Around line 50-107: ensure_venv can race when two callers for the same name
miss the cache; wrap the entire cache-check-and-rebuild sequence in a per-venv
lock so only one task/process builds the venv at a time. Specifically, in
ensure_venv around the call to try_cached_venv(...) and all subsequent
operations that mutate venv_dir/lock_path (remove_dir_all, create_dir_all,
create_venv, pip_install, and writing lock_path), acquire a unique lock keyed by
name or venv_dir; if cross-process races are possible use an on-disk advisory
lock (e.g., open a lock file next to venv_dir and flock it) otherwise use an
in-process async Mutex keyed by name; release the lock after the requirements
lock is written so subsequent callers re-check try_cached_venv and reuse the
completed venv.
- Around line 56-62: The cache lookup in try_cached_venv only compares
REQUIREMENTS_LOCK/want_lock and ignores the resolved interpreter, allowing reuse
across different resolved Python/runtime config; update the cache scheme so the
resolved interpreter identity (e.g., resolved Python path/version/source string)
is persisted alongside the venv metadata and used in try_cached_venv validation
(or include the interpreter id in the venv_dir cache key), ensure callers no
longer return version: String::new() but populate the saved version, and apply
the same check/persistence to the other cached-venv check sites (the alternate
cache lookup around render_requirements_lock/venv_dir/lock_path) so mismatched
interpreter resolutions invalidate the cache.
---
Nitpick comments:
In `@src/openhuman/artifacts/store_tests.rs`:
- Line 16: The round-trip test omits asserting the new ArtifactMeta.error field,
so update the tests that call make_meta() and save_and_get_roundtrip() to assert
the error field is preserved: add an assertion that meta.error is None for the
existing make_meta() case, and add a separate test case that constructs a meta
with error = Some("...") and verifies save_and_get_roundtrip() returns the same
Some(...) value; target the test helpers and assertions around make_meta(),
save_and_get_roundtrip(), and the ArtifactMeta.error field to ensure both None
and Some(...) are covered.
In `@src/openhuman/artifacts/store.rs`:
- Around line 247-275: The loop in sanitize_filename_stem currently checks
out.chars().count() to enforce MAX_SANITIZED_FILENAME_LEN, which rescans the
buffer each iteration; change that check to use out.len() for O(1) length
checking. Update the conditional inside sanitize_filename_stem that reads "if
out.chars().count() >= MAX_SANITIZED_FILENAME_LEN" to "if out.len() >=
MAX_SANITIZED_FILENAME_LEN" (keeping the same MAX_SANITIZED_FILENAME_LEN symbol
and all other logic intact).
🪄 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: f6897a7b-a425-442a-993a-98e09a01934b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/coverage.ymlCargo.tomlapp/test/playwright/helpers/core-rpc.tssrc/openhuman/agent_registry/agents/orchestrator/agent.tomlsrc/openhuman/artifacts/mod.rssrc/openhuman/artifacts/store.rssrc/openhuman/artifacts/store_tests.rssrc/openhuman/artifacts/types.rssrc/openhuman/runtime_python/downloader.rssrc/openhuman/runtime_python/downloader_tests.rssrc/openhuman/runtime_python/mod.rssrc/openhuman/runtime_python/run.rssrc/openhuman/runtime_python/venv.rssrc/openhuman/tools/impl/mod.rssrc/openhuman/tools/impl/presentation/engine.rssrc/openhuman/tools/impl/presentation/mod.rssrc/openhuman/tools/impl/presentation/tests.rssrc/openhuman/tools/impl/presentation/types.rssrc/openhuman/tools/ops.rs
graycyrus
reviewed
Jun 1, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@oxoxDev hey! solid work on this — the native-Rust pivot is the right call and the artifact lifecycle helpers are well-designed. a few things are blocking clean CI before i can approve:
CI failures:
- "Rust Core Coverage" is failing, and i'm pretty sure the culprit is the step you added to
coverage.yml. it installspython-pptx==1.0.2and its comment still says "The presentation tool (#2778) bundles a python-pptx-backed script" — but after the engine refactor the tool uses ppt-rs and has no Python path at all. iftests/presentation_tool.rsstill references the Python venv path, it'll fail here. either remove the pip install step or update it to match what the coverage run actually needs. - "E2E / web lane 1/4" is failing — unrelated to this PR possibly, but worth checking if any of your walkthrough changes in
core-rpc.tsare implicated.
Stale comment in agent.toml:
The comment you added alongside "generate_presentation" says "Synthesises a .pptx from a structured slide spec via a bundled python-pptx helper." Should say ppt-rs / native Rust engine after the refactor. Minor but someone reading the TOML will be confused.
Open CodeRabbit findings (not yet addressed):
-
engine.rs:130—tokio::time::timeoutwrappingspawn_blockingdoes not cancel the blocking thread. After the 30s deadline fires, ppt-rs keeps running in the blocking thread pool until it finishes or panics. For desktop-only use the blast radius is small, but if a pathological input slips pastvalidate_inputand wedges ppt-rs, you'll leak a blocking thread for the full duration of every concurrent request. Worth addressing before merge. -
presentation/mod.rs(finalize) — iffinalize_artifactfails (e.g. disk full when writingmeta.jsonafter the.pptxbytes are already on disk), the artifact staysPendingand the user gets a stuck spinner with no error message. CodeRabbit's suggested fix — catch the error, callfail_artifact, then propagate — is the right shape.
fix the CI + those two items and i'll approve.
…gine swap Addresses two graycyrus comments on PR tinyhumansai#3016 post-engine-swap: 1. `.github/workflows/coverage.yml` — remove the "Install python-pptx for generate_presentation tool tests" step. The engine swapped from python-pptx (subprocess + managed venv) to native Rust `ppt-rs` in `e0e2a2e5`; the integration test no longer shells out to python3, and the `pip install python-pptx==1.0.2` step is dead weight on the cargo-llvm-cov runner. 2. `src/openhuman/agent_registry/agents/orchestrator/agent.toml` — update the `generate_presentation` tool comment from "via a bundled python-pptx helper" to "via a native Rust engine (`ppt-rs`) running in-process — no Python subprocess, no managed venv" so the routing-doc matches the actual implementation.
graycyrus
reviewed
Jun 1, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@oxoxDev thanks for the follow-up — the stale pip install python-pptx==1.0.2 step in coverage.yml and the stale comment in agent.toml are both cleaned up, appreciate it. marking those threads resolved.
CI is still failing (Rust Core Coverage + E2E lane 1/4), so holding off on approval. one new issue to flag before you close out:
venv.rs is still present in this PR. The two CodeRabbit threads on runtime_python/venv.rs that you closed as "obsolete after engine swap" are not obsolete — venv.rs (353 lines) still ships as a new file in this PR. Commit e0e2a2e5 removed generate_pptx.py, invoker.rs, and script.rs, but did not touch venv.rs. Those two findings — the concurrent first-call race at line 107 (ensure_venv can be entered by two callers simultaneously, both miss cache, and race through remove_dir_all + create_venv + pip install) and the cache invalidation gap at line 62 (lock file only records requirement strings, not interpreter version, so a Python upgrade won't trigger a venv rebuild) — both need to be addressed before merge. The race in particular will produce a corrupted venv under concurrent load when two tools request the same named env for the first time.
fix CI and address the two venv.rs CodeRabbit threads and i'll approve.
The merge from upstream/main at 10f1a19 left a dangling `workspace_dir.to_path_buf()` reference at src/openhuman/tools/ops.rs:454 that the surrounding `all_tools_with_runtime` signature does not provide. Switch to `root_config.workspace_dir.clone()` — the function already takes `root_config: &Config`, and `Config` exposes `workspace_dir: PathBuf` (src/openhuman/config/schema/types.rs:53) which is the same path `PresentationTool::new` consumed under the older signature. Restores `cargo check --lib` compile.
oxoxDev
added a commit
to oxoxDev/openhuman
that referenced
this pull request
Jun 2, 2026
…refactor PR tinyhumansai#3074 (`feat(security): separate agent action sandbox from internal workspace state`) removed the `workspace_dir` local from `all_tools_with_runtime`. The PresentationTool registration on this branch still referenced `workspace_dir.to_path_buf()` and failed to compile after the merge of upstream/main into pr/3017. Swap to `root_config.workspace_dir.clone()` — `Config` already exposes `workspace_dir: PathBuf` and `root_config` is in scope as the function parameter. Same fix as on tinyhumansai#3016's branch.
oxoxDev
added a commit
to oxoxDev/openhuman
that referenced
this pull request
Jun 2, 2026
…refactor Same fix as on feat/2779 (tinyhumansai#3017) and feat/2778 (tinyhumansai#3016): PR tinyhumansai#3074 removed the workspace_dir local from all_tools_with_runtime. Swap PresentationTool registration to root_config.workspace_dir.clone() so the merge from upstream/main into pr/3026 compiles again.
oxoxDev
added a commit
to oxoxDev/openhuman
that referenced
this pull request
Jun 2, 2026
…refactor Same fix as on feat/2779 (tinyhumansai#3017), feat/2778 (tinyhumansai#3016), and feat/3024 (tinyhumansai#3026): PR tinyhumansai#3074 removed the workspace_dir local from all_tools_with_runtime. Swap PresentationTool registration to root_config.workspace_dir.clone() so the merge from upstream/main into pr/3029 compiles again.
Contributor
Author
|
@coderabbitai full review — all inline threads now resolved (two stale |
Contributor
|
✅ Actions performedFull review triggered. |
graycyrus
previously approved these changes
Jun 2, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/openhuman/runtime_python/venv.rs (2)
50-107:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftConcurrent first-calls for the same
namecan race.Two concurrent callers that both miss the cache check at line 61 will race through
remove_dir_all→create_venv→pip_install→ lock write, potentially leaving a half-installed venv or deleting each other's work mid-bootstrap. Consider adding a per-name lock (in-processMutexkeyed by name, or an on-disk advisory lock if cross-process races are possible).Since this is scaffolding for future tools rather than code exercised today, this can be addressed before the first consumer is wired up.
🤖 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/runtime_python/venv.rs` around lines 50 - 107, Concurrent callers can race in ensure_venv (between try_cached_venv and writing REQUIREMENTS_LOCK), leading to torn or deleted venvs; fix by serializing per-name bootstrap: introduce a global per-name lock map (e.g., Lazy static HashMap/DashMap from name -> Arc<tokio::sync::Mutex<()>>) and acquire the mutex for the given name at the start of ensure_venv before re-checking try_cached_venv, then perform remove_dir_all/create_venv/pip_install/write lock while holding it; release afterward. Reference symbols: ensure_venv, try_cached_venv, venv_dir, lock_path, create_venv, pip_install.
144-166:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCached venv reuse ignores interpreter version drift.
try_cached_venvcompares only the requirements lock, not the base interpreter identity. IfRuntimePythonConfiglater resolves a different Python version, the stale venv will still be returned. Additionally,version: String::new()is returned for cached hits, so callers cannot detect drift. Consider persisting the interpreter path/version alongside the lock, or encoding the interpreter identity into the cache key.🤖 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/runtime_python/venv.rs` around lines 144 - 166, try_cached_venv currently only checks the requirements lock and returns ResolvedPython with an empty version, which allows reuse of a venv created by a different base interpreter; update the caching logic to include the base interpreter identity (path and/or version) in the cache key or persist it alongside the lock (e.g. write interpreter path/version into the same lock file), then in try_cached_venv read that stored interpreter identity and compare it against the currently resolved interpreter from RuntimePythonConfig (or whatever caller-provided want identity) before returning; also populate ResolvedPython.version from the stored metadata (instead of String::new()) so callers can detect drift. Ensure you use locate_venv_python and ResolvedPython as the return path and update any code that writes the cache to include the interpreter metadata.
🧹 Nitpick comments (2)
src/openhuman/runtime_python/venv.rs (1)
250-265: 💤 Low valueSimplify
run_pip_installparameter handling.The function accepts
spec: &PythonLaunchSpecbut only usesspec.args.clone()(line 264), and line 262 (let _ = spec;) explicitly suppresses the unused-variable warning. Consider either:
- Passing
args: &[String]directly, or- Removing the intermediate
launchand usingspecdirectly if the contract allows.♻️ Proposed simplification
-async fn run_pip_install( - resolved: &ResolvedPython, - spec: &PythonLaunchSpec, -) -> Result<super::run::PythonRunOutput> { +async fn run_pip_install( + resolved: &ResolvedPython, + args: &[String], +) -> Result<super::run::PythonRunOutput> { // pip install is the heavy-weight first-call path; reuse the // generic run helper but with the venv-install timeout instead of // the per-tool default. - // - // We rebuild spec as a positional one to satisfy - // spawn_stdio_process's contract: script_path -> first arg after - // `python`. We pass `-m` as the "script" and `pip install …` as - // args, matching the `python -m pip install …` form. - let _ = spec; // placeholder — actually consumed below let mut launch = PythonLaunchSpec::new(std::path::PathBuf::from("-m")); - launch.args = spec.args.clone(); + launch.args = args.to_vec(); launch.unbuffered = false;And update the call site (line 239):
- let output = run_pip_install(&resolved, &spec).await?; + let output = run_pip_install(&resolved, &spec.args).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/runtime_python/venv.rs` around lines 250 - 265, The run_pip_install function currently takes spec: &PythonLaunchSpec but only uses spec.args.clone() and creates a new PythonLaunchSpec named launch; change the API to accept only the arguments (e.g., args: &[String] or Vec<String>) and eliminate the unused spec and the intermediate launch, or alternatively use the provided spec directly and remove the new PythonLaunchSpec creation; update references to spec.args.clone() (and the call site that invokes run_pip_install) to pass the args parameter (or the original spec) and remove the let _ = spec; placeholder and the launch variable if no longer needed (symbols: run_pip_install, PythonLaunchSpec, launch, spec.args).src/openhuman/tools/impl/presentation/types.rs (1)
10-12: ⚡ Quick winUpdate the stale Python-era Rustdoc/comments.
These comments still describe the removed
python-pptx/Python-helper path, which now contradicts the nativeppt-rsimplementation and makes the validation/output contract harder to follow.As per coding guidelines: "Ship rustdoc / code comments with new/changed Rust behavior; update
AGENTS.mdor architecture docs when rules or user-visible behavior change".Also applies to: 28-29, 50-52, 60-62, 132-134
🤖 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/types.rs` around lines 10 - 12, Update the stale Rustdoc in src/openhuman/tools/impl/presentation/types.rs to remove references to "python-pptx" and the Python-helper path and instead describe the native ppt-rs behavior and exact validation/output contract; locate the comment blocks around the "Maximum length of a single text field (title, body, individual bullet, speaker notes)" and the other ranges noted (lines covering bullets and validation) and rewrite them to state the current max-length rule, which fields it applies to, and how ppt-rs handles truncation/encoding/validation; ensure the docstrings reference the relevant types/constants in the file (e.g., the max-length constant and any validation functions/methods) and, if this is a user-visible behavior change, update AGENTS.md or architecture docs accordingly.
🤖 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/tools/impl/presentation/mod.rs`:
- Around line 197-208: The code logs the absolute artifact path
(output_path.display()) both in the failure reason and in tracing::warn, which
may leak PII; change both usages to avoid full paths by using the artifact id
(meta.id) or a redacted filename-only value (e.g., output_path.file_name())
instead, update the reason passed to fail_artifact(&self.workspace_dir,
&meta.id, &reason).await to not include output_path.display(), and replace the
tracing::warn path field with a non-sensitive identifier (meta.id or
filename-only) while retaining the existing err and target fields.
In `@src/openhuman/tools/impl/presentation/types.rs`:
- Around line 176-217: The validation treats slides with bullets like [" "] as
non-empty because has_bullets is set from slide.bullets.is_empty(); change the
has_bullets computation in the input.slides iteration to consider only bullets
with non-whitespace content (e.g., set has_bullets =
slide.bullets.iter().any(|b| !b.trim().is_empty())), so slides with only
whitespace bullets fail the "must have at least one of title / body / bullets"
check (keep other length/bullets checks as-is).
---
Duplicate comments:
In `@src/openhuman/runtime_python/venv.rs`:
- Around line 50-107: Concurrent callers can race in ensure_venv (between
try_cached_venv and writing REQUIREMENTS_LOCK), leading to torn or deleted
venvs; fix by serializing per-name bootstrap: introduce a global per-name lock
map (e.g., Lazy static HashMap/DashMap from name -> Arc<tokio::sync::Mutex<()>>)
and acquire the mutex for the given name at the start of ensure_venv before
re-checking try_cached_venv, then perform
remove_dir_all/create_venv/pip_install/write lock while holding it; release
afterward. Reference symbols: ensure_venv, try_cached_venv, venv_dir, lock_path,
create_venv, pip_install.
- Around line 144-166: try_cached_venv currently only checks the requirements
lock and returns ResolvedPython with an empty version, which allows reuse of a
venv created by a different base interpreter; update the caching logic to
include the base interpreter identity (path and/or version) in the cache key or
persist it alongside the lock (e.g. write interpreter path/version into the same
lock file), then in try_cached_venv read that stored interpreter identity and
compare it against the currently resolved interpreter from RuntimePythonConfig
(or whatever caller-provided want identity) before returning; also populate
ResolvedPython.version from the stored metadata (instead of String::new()) so
callers can detect drift. Ensure you use locate_venv_python and ResolvedPython
as the return path and update any code that writes the cache to include the
interpreter metadata.
---
Nitpick comments:
In `@src/openhuman/runtime_python/venv.rs`:
- Around line 250-265: The run_pip_install function currently takes spec:
&PythonLaunchSpec but only uses spec.args.clone() and creates a new
PythonLaunchSpec named launch; change the API to accept only the arguments
(e.g., args: &[String] or Vec<String>) and eliminate the unused spec and the
intermediate launch, or alternatively use the provided spec directly and remove
the new PythonLaunchSpec creation; update references to spec.args.clone() (and
the call site that invokes run_pip_install) to pass the args parameter (or the
original spec) and remove the let _ = spec; placeholder and the launch variable
if no longer needed (symbols: run_pip_install, PythonLaunchSpec, launch,
spec.args).
In `@src/openhuman/tools/impl/presentation/types.rs`:
- Around line 10-12: Update the stale Rustdoc in
src/openhuman/tools/impl/presentation/types.rs to remove references to
"python-pptx" and the Python-helper path and instead describe the native ppt-rs
behavior and exact validation/output contract; locate the comment blocks around
the "Maximum length of a single text field (title, body, individual bullet,
speaker notes)" and the other ranges noted (lines covering bullets and
validation) and rewrite them to state the current max-length rule, which fields
it applies to, and how ppt-rs handles truncation/encoding/validation; ensure the
docstrings reference the relevant types/constants in the file (e.g., the
max-length constant and any validation functions/methods) and, if this is a
user-visible behavior change, update AGENTS.md or architecture docs accordingly.
🪄 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: 1c7d955c-83c4-4cfd-955d-84afb582f1b0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlsrc/openhuman/agent_registry/agents/orchestrator/agent.tomlsrc/openhuman/artifacts/mod.rssrc/openhuman/artifacts/store.rssrc/openhuman/artifacts/store_tests.rssrc/openhuman/artifacts/types.rssrc/openhuman/runtime_python/downloader.rssrc/openhuman/runtime_python/downloader_tests.rssrc/openhuman/runtime_python/mod.rssrc/openhuman/runtime_python/run.rssrc/openhuman/runtime_python/venv.rssrc/openhuman/tools/impl/mod.rssrc/openhuman/tools/impl/presentation/engine.rssrc/openhuman/tools/impl/presentation/mod.rssrc/openhuman/tools/impl/presentation/tests.rssrc/openhuman/tools/impl/presentation/types.rssrc/openhuman/tools/ops.rs
graycyrus
reviewed
Jun 2, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
High-level review (base of the #1535 stack). The native-Rust ppt-rs engine is a clean call — dropping the Python runtime removes the cold-start latency and the ~50MB venv footprint, and keeps generation fully in-process with no subprocess/sandbox surface. Tool contract + artifact JSON shape are preserved across the engine swap, so the downstream UI PR (#3017) shouldn't need rework.
Two notes inline, plus a coordination point: this is the base of the stack and should merge first. #3017 is branched off this branch (pre-refactor), so until it rebases onto a merged main it still carries the old python-pptx engine — worth merging this one first and asking #3017 to rebase clean.
Nothing blocking here from a high-level pass.
| whatsapp-rust-tokio-transport = { version = "0.5", optional = true, default-features = false } | ||
| whatsapp-rust-ureq-http-client = { version = "0.5", optional = true } | ||
| wacore = { version = "0.5", optional = true, default-features = false } | ||
| ppt-rs = "0.2.14" |
Contributor
There was a problem hiding this comment.
ppt-rs links pulldown-cmark + syntect unconditionally (for its markdown/code-highlight paths, which this tool never exercises), so we pay ~10-15MB of binary bloat for code we don't call. The PR body already notes this. Could be worth filing the upstream issue to gate clap/pulldown-cmark/syntect behind a non-default feature, and tracking it so we can shrink the binary later. Not blocking — just flagging so it doesn't get lost.
Contributor
Author
There was a problem hiding this comment.
Not blocking, confirmed. Tracking the upstream ppt-rs feature-gating ask as a follow-up — the unused clap/pulldown-cmark/syntect paths cost ~10-15MB and a default-features-off + opt-in markdown/highlight features would let us shrink the binary later. Will open an issue against the ppt-rs repo before this PR lands and link the tracking URL here.
| pub mod process; | ||
| pub mod resolver; | ||
| pub mod run; | ||
| pub mod venv; |
Contributor
There was a problem hiding this comment.
runtime_python::{run, venv} ship here but, after the native-Rust engine refactor, are not consumed by PresentationTool (the PR body confirms this). They're scaffolding for future Python-backed tools. That's a reasonable call, but unused public modules tend to bit-rot and confuse readers — please make sure the "intentionally unused, reserved for future Python tools" rationale is captured in the module docstring here (not just the PR description), or consider deferring these two modules to the PR that actually needs them.
Contributor
Author
There was a problem hiding this comment.
Addressed in f64a439. Added a module rustdoc on runtime_python/mod.rs that calls out the engine-swap commit (e0e2a2e5), enumerates current consumers (stdio MCP servers), captures the "intentionally retained as scaffolding for the next Python-backed tool" rationale, and notes the fallback (delete in a standalone cleanup PR if no consumer materialises). Future readers won't waste cycles wiring it back into the presentation path.
…pace-only bullets (tinyhumansai#2778) Two coderabbit threads on PR tinyhumansai#3016: 1. mod.rs:206 [major] — `tracing::warn!(path = %output_path.display(), …)` on the artifact write-error branch leaks the absolute workspace path, which on macOS includes `/Users/<username>/…` and on Windows includes `C:\Users\<username>\…`. Per `feedback_redact_paths_and_ids_in_public` this is PII in structured logs. Swap to `artifact_id` + `filename` (basename only, redacted of the workspace prefix). The user-facing `reason` string still mentions the filename so the failure stays actionable in the UI. 2. types.rs:183 [minor] — `has_bullets = !slide.bullets.is_empty()` accepts whitespace-only entries (`[" "]`), but `build_slides()` later trims and drops them. A slide with only whitespace bullets would pass the "at least one of title/body/bullets" gate yet render blank in the final deck. Tighten the check to `slide.bullets.iter().any(|b| !b.trim().is_empty())` so the validation matches the downstream filter. Tests: scoped `cargo test --lib openhuman::tools::implementations::presentation` all 15 pass.
tinyhumansai#2778) Per graycyrus on PR tinyhumansai#3016: \`runtime_python::{run, venv}\` are no longer consumed by \`PresentationTool\` after the tinyhumansai#2778 engine refactor (commit \`e0e2a2e5\` swapped python-pptx subprocess → native Rust \`ppt-rs\` in-process), and unused public modules tend to bit-rot and confuse readers. Add a module rustdoc that: - enumerates the current consumers (stdio MCP servers) - explicitly calls out that \`PresentationTool\` no longer uses this module, with a pointer to the refactor commit - captures the "intentionally retained as scaffolding for the next Python-backed tool" rationale so a future reader doesn't waste cycles wiring it back into the presentation path - notes the fallback: delete the unused submodules in a standalone cleanup PR if no Python-backed consumer materialises
5 tasks
… native Rust (tinyhumansai#2778) Per graycyrus on PR tinyhumansai#3016: after the tinyhumansai#2778 engine refactor swapped the presentation tool from a python-pptx subprocess to the native-Rust `ppt-rs` crate (commit `e0e2a2e5`), the python-related scaffolding this PR originally introduced is dead weight. Removing it. Subtractive changes: - **Delete `src/openhuman/runtime_python/run.rs`** (295 lines) — `run_python_script_to_completion` + `PythonRunOutput` + `PythonRunTimeout`. No external consumers (the only caller was the old `invoker.rs` in the python-pptx engine, removed by `e0e2a2e5`). - **Delete `src/openhuman/runtime_python/venv.rs`** (353 lines) — `ensure_venv`. No external consumers (only the old python-pptx invocation path called this). - **Revert `src/openhuman/runtime_python/mod.rs`** to upstream/main — drops the `pub mod run; pub mod venv;` + `pub use` re-exports + the rustdoc that justified keeping them. - **Revert `src/openhuman/runtime_python/downloader.rs`** to upstream/main — drops the pre-release-version guard that was added to keep the python-pptx flow from picking beta Python interpreters. The remaining `runtime_python` module (bootstrap / extractor / process / resolver) is pre-existing on main and continues to back stdio MCP servers; the pre-release guard isn't theirs to ship. - **Revert `src/openhuman/runtime_python/downloader_tests.rs`** to upstream/main — corresponding tests for the dropped guard. - **`src/openhuman/tools/impl/presentation/types.rs`** — - Update three field rustdocs that still referred to "python-pptx" as the renderer; they now name `ppt-rs`. - Drop the line claiming the JSON output is "mirrored in the Python helper's stdout JSON shape" — no Python helper. - **Remove `PresentationError::MissingRuntime` and `MissingPackage` dead variants.** They were marked `#[allow(dead_code)]` and kept "for downstream pattern-match stability" after the engine swap. Zero callers — `PresentationError` isn't serialized cross-process and isn't exposed outside the module, so the stability argument is speculative. A future runtime-dependent fallback (e.g. a LibreOffice headless renderer for `format = "pdf"`) can re-add them with concrete construction sites. Net: -770 lines of dead python scaffolding removed. Compile + the 15 scoped presentation tests pass.
Contributor
Author
|
@graycyrus per your nudge — pushed Net -770 lines of dead code removed from this PR:
15 scoped presentation tests still pass. Compile clean. Module rustdoc historical notes about the engine swap remain (they document the cross-PR contract that #3017 / #3026 / #3029 rely on; not python residue). |
graycyrus
approved these changes
Jun 2, 2026
oxoxDev
added a commit
to oxoxDev/openhuman
that referenced
this pull request
Jun 2, 2026
…sai#2779) Post-tinyhumansai#3016 rebase cleanup. With the presentation tool now on the native-Rust `ppt-rs` engine (commit `e0e2a2e5`, merged via tinyhumansai#3016 → main as `166fc6af6`), two python-era artifacts on this branch are dead weight: - **Delete `tests/presentation_tool.rs`** (132 lines) — drove the real Python interpreter end-to-end and asserted .pptx output via `python3 -c "import pptx, zipfile, ..."`. The native-Rust engine has its own E2E coverage via the inline `tests.rs` (`ppt-rs` round-trip → zip with `[Content_Types].xml`), so this file is redundant. Also no longer runnable: it spins up `runtime_python::ensure_venv` which was deleted from this branch in the previous merge commit. - **Remove the `Install python-pptx for generate_presentation tool tests` step from `.github/workflows/coverage.yml`** (10 lines). It was there to make the deleted integration test's success branch reachable on the cargo-llvm-cov runner. With the test gone, the pip install + 10MB python-pptx wheel download per coverage run is pure waste. Net: -142 lines of scaffolding. Compile + native-Rust presentation tests still pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
generate_presentationagent tool that converts a structured slide spec ({title, author?, theme?, slides: [{title?, body?, bullets?, speaker_notes?}]}) into a.pptxfile via a pure-Rust engine (ppt-rs, Apache-2.0), persisted as a typed artifact in the workspace.pip installon first use, no subprocess. Cold-call latency drops from ~2-5 s (Python venv setup) to <100 ms. User install footprint −50 MB+.artifacts::storeproducer surface —create_artifact/finalize_artifact/fail_artifact— plus optionalArtifactMeta.errorso failed builds surface a reason instead of an indefinite spinner.runtime_python::venv+runtime_python::rungeneral-purpose primitives ship in this PR as scaffolding for future Python-backed tools (OCR, data-science scratch, etc.). The presentation tool itself does not consume them after the engine refactor — they remain available, unused by the tool.generate_presentationintools/ops.rs(always-on; no Python gate) and on the orchestrator agent'snamedtool list so the web-chat path can drive it today; the broader agent-definition wiring + grounding rule lands in Wire PPT generation and file attachments into agent definitions and error handling #2780 (PR feat(orchestrator): wire generate_presentation + require grounding (#2780) #3029).This is sub-task 3 of 5 for parent #1535 ("Add multimodal support for files and PPT generation"). #2776 is merged (PR #2801); #2777 is in review (PR #2954); #2779 (React
ArtifactCard, PR #3017), #3024 (Files-in-this-chat panel, PR #3026), and #2780 (orchestrator wiring + grounding, PR #3029) are stacked on top.Problem
Users asking the agent to build slide decks today have no working path. The agent either guesses at
shellsnippets (python -c '…') that the sandbox blocks, or it falls back to text and apologises. Parent issue #1535 explicitly calls out a reliable PPT workflow as acceptance criterion #2, and #2776 already shipped the artifact storage layer (ArtifactMeta/ArtifactKind::Presentation/ai_*_artifactRPCs) waiting for a producer.Structural gaps that block the tool:
ai_list_artifacts/ai_get_artifact/ai_delete_artifact) and the persistence layer (<workspace>/artifacts/<uuid>/meta.json + payload) but nocreate_artifacthelper. Each generator would otherwise reimplementuuid + mkdir + write meta.json + flip status..pptxwriter in the workspace dep graph.docx-rscovers Word,rust_xlsxwritercovers Excel — both deliberate native-Rust generators that drop external runtimes. The PowerPoint equivalent was missing.Solution
artifacts::storeproducer helpers (new)create_artifact(workspace_dir, kind, title, extension) -> (ArtifactMeta, PathBuf): allocates a UUID-named dir, persists aPendingArtifactMeta, returns the meta + the absolute path the producer writes bytes to. Filename stem is sanitized from the title (alphanumerics + dashes, ≤ 80 chars).finalize_artifact(workspace_dir, id, size_bytes): flipsPending → Ready+ persists final size. Idempotent.fail_artifact(workspace_dir, id, reason): flips toFailed+ persists reason via the newArtifactMeta.error: Option<String>field (serde default + skip-if-none, so persisted records that predate the field round-trip fine).tools/impl/presentation/(new, native-Rust engine)Module layout:
mod.rsPresentationToolstruct +Tooltrait impl. Router-ruledescription("USE THIS … NOT for …"). Stateless ctor (new(workspace_dir)) — noConfigarg needed, no runtime resolution. 30 s generation timeout.types.rsGeneratePresentationInput/SlideSpec/Output/PresentationErrorenum (InvalidInput,GenerationFailed,GenerationTimeout; plusMissingRuntime/MissingPackagereserved for downstream pattern-match stability and a possible future LibreOffice-headlessformat = "pdf"fallback).validate_inputruns eagerly so the LLM gets a structured error it can self-correct on.truncate_stderrcaps at 500 chars (UTF-8-safe).engine.rsppt-rswrapper. Synthesises a title slide frominput.title(+ optionalauthorbyline) prepended to the content slides, soslide_countsemantics match the original contract (excludes the synthetic title slide). Drives generation throughtokio::task::spawn_blocking+tokio::time::timeoutso the synchronous library work neither blocks the async executor nor can wedge the agent loop.tests.rsdescription, validation rejection branches (empty title / empty slides / empty slide / oversize body / too many slides), real end-to-end happy path driving the engine + artifact pipeline.Engine unit tests live alongside in
engine.rs::tests— 4 tests covering theSlideSpec→ppt-rsmapping (title-slide prepend, blank-entry filtering), the OOXML round-trip (re-opens the byte buffer as a zip, asserts all spec-required entries —[Content_Types].xml,_rels/.rels,ppt/presentation.xml,ppt/_rels/presentation.xml.rels,ppt/theme/theme1.xml,ppt/slideMasters/slideMaster1.xml,ppt/slideLayouts/slideLayout1.xml,docProps/core.xml,docProps/app.xml,ppt/slides/slide{1,2}.xml,ppt/notesSlides/notesSlide*.xmlwhen notes set), and the timeout surface under a 1-ns deadline.Input validation caps: ≤ 64 slides, ≤ 32 bullets/slide, ≤ 2000 chars per field. Tool returns the artifact id + absolute path so the agent can quote it.
runtime_python::venv+runtime_python::run(new, unused-by-this-tool)Ship as general-purpose primitives for future Python-backed tools (OCR pipelines, data-science scratch executors, …). Not consumed by
PresentationToolafter the engine refactor. Kept in the PR rather than ripped out because they are reusable scaffolding the next Python-backed tool would otherwise re-add commit-for-commit.ensure_venv(name, requirements, config) -> Result<ResolvedPython>— lazy managed venv under<runtime_python.cache_dir>/venvs/<name>/.requirements.txtchange detection short-circuits unchanged invocations.run_python_script_to_completion(resolved, spec, stdin, deadline) -> PythonRunOutput— one-shot stdin → wait → capture-output contract bounded bydeadline;PythonRunTimeoutis its own type so call sites can downcast.Registration
tools/ops.rs::all_tools_with_runtimealways registersPresentationTool::new(workspace_dir.to_path_buf()). Noruntime_python.enabledgate.agents/orchestrator/agent.tomladds"generate_presentation"to itsnamedlist so the orchestrator can drive the tool today. The companion grounding rule (orchestrator must callmemory_tree/delegate_research/query_memorybeforegenerate_presentationfor topical/factual decks) lands in Wire PPT generation and file attachments into agent definitions and error handling #2780 (PR feat(orchestrator): wire generate_presentation + require grounding (#2780) #3029).Engine refactor history (in-branch)
This PR originally shipped a python-pptx-based engine (subprocess + bundled
generate_pptx.py+runtime_python::venvconsumer). After a maintainer request to drop the Python dep, the final commit (refactor(presentation): swap python-pptx subprocess for native rust engine) replaces the engine withppt-rswhile preserving:generate_presentation)GeneratePresentationInput/OutputJSON shape.pptxextensionSo downstream PRs in the #1535 chain need no rework — the engine is a sealed implementation detail behind the tool contract. The intermediate python-pptx commits remain in the branch history (one logical refactor at the end documents the swap); the rendered PR diff vs
mainshows the final shape only.Sample 5-slide deck generated by the native engine: 14 673 bytes, opens cleanly in PowerPoint / Keynote / Google Slides.
Submission Checklist
tests.rscovering schema contract, router-ruledescription, all 5 validation rejection branches, real engine + artifact-pipeline end-to-end happy path. 4 engine unit tests inengine.rs::testscovering theSlideSpec→ppt-rsmapping, OOXML zip round-trip (re-opens output, asserts every spec-required OOXML entry exists), timeout surface. Plus the artifacts + runtime_python helper tests carried in from the earlier commits.engine.rs+mod.rs::execute+types.rs::validate_inputis exercised by the 14 inline tests. Run locally:cargo test --lib openhuman::tools::implementations::presentation(14 pass) +cargo test --lib openhuman::runtime_python(existing suite, unchanged on this branch) +cargo test --lib openhuman::artifacts(existing suite).docs/TEST-COVERAGE-MATRIX.md. The tool is a new agent capability surface, not a release-cut feature; Surface artifact downloads and generation progress in the React UI #2779 (UIArtifactCard) is the right place to add a user-facing row when uploads ship.## Relatedpip install.docs/RELEASE-MANUAL-SMOKE.md).Closes #NNNin the## RelatedsectionImpact
runtime_python.enabledgate). No mobile/web/CLI behaviour change.validate_input.ppt-rstransitive deps (pulldown-cmark+syntectare linked unconditionally byppt-rseven though the markdown / code-highlighting paths are unused). Counterfactual: the python-pptx path added ~50 MB+ of managed Python interpreter + venv on first use at the user'sruntime_python.cache_dir.pip install.ppt-rsis pure Rust (nounsafe, Apache-2.0).<workspace>/artifacts/<uuid>/<title>.pptx. No data leaves the host.ArtifactMeta.erroris additive withserde(default), so existing on-disk records parse unchanged.runtime_python::{venv, run}are new modules — no behaviour change for existingspawn_stdiousers.ArtifactCardrenders the generated deck in chat.memory_tree/delegate_research/query_memorybeforegenerate_presentationon topical decks) +UnsupportedFileTypeerror variant.ppt-rsupstream issue: gateclap/pulldown-cmark/syntectbehind a non-default feature so library consumers don't pay for thepptclibinary's deps. Would shave ~5-10 MB.ppt-rssupports it viaadd_image(Image).Related
ArtifactCard(depends on this)AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/2778-presentation-toole0e2a2e5 refactor(presentation): swap python-pptx subprocess for native rust engineValidation Run
cargo test --lib openhuman::tools::implementations::presentation(14/14 ✓ — 11 tool + 4 engine including OOXML zip round-trip)cargo check --lib✓cargo fmt --check✓Behavior Changes
generate_presentationwith a valid slide spec, a.pptxfile is written to the workspace artifacts directory via the native-Rustppt-rsengine, anArtifactMetais persisted asReady, and the tool returns the artifact id + absolute path. On any failure path the artifact is persisted asFailedwith a reason.Parity Contract
artifacts::store's existing public API (list/get/deleteRPCs) is byte-identical. Existing on-diskmeta.jsonrecords parse unchanged thanks to#[serde(default)]on the newerrorfield. Tool name + input / output JSON schema unchanged across the python-pptx → native-rust engine swap, so downstream PRs in the Add multimodal support for files and PPT generation #1535 chain (feat(chat): ArtifactCard + Tauri download + backend artifact events (#2779) #3017, feat(chat): Files in this chat — persistent per-chat artifact panel (#3024) #3026, feat(orchestrator): wire generate_presentation + require grounding (#2780) #3029) need no rework.Duplicate / Superseded PR Handling
Summary by CodeRabbit