Skip to content

fix(harness-init): show runtime-setup overlay on first run only (Closes #5047)#5066

Merged
senamakel merged 3 commits into
tinyhumansai:mainfrom
M3gA-Mind:fix/GH-5047-runtime-setup-first-run-only
Jul 20, 2026
Merged

fix(harness-init): show runtime-setup overlay on first run only (Closes #5047)#5066
senamakel merged 3 commits into
tinyhumansai:mainfrom
M3gA-Mind:fix/GH-5047-runtime-setup-first-run-only

Conversation

@M3gA-Mind

@M3gA-Mind M3gA-Mind commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Runtime-setup overlay ("Setting Things Up" / HarnessInitOverlay) now appears only on genuine first-run provisioning, not on every launch.
  • Root cause: Python/Node is_done probes used the process-local try_cached() memo (empty after every restart), so the orchestrator re-entered a visible running state each launch.
  • Added durable, non-downloading on-disk readiness probes; classified steps as provisioning vs routine startup so only real install/download work surfaces the overlay.
  • Persisted the "Run in background" dismissal per provisioning run so a remount/reload can't reopen it.

Problem

HarnessInitOverlay is meant to block only during first-run provisioning, but it flashed the full-screen setup page on every app restart while the core re-checked Python/spaCy/TokenJuice/Node. The Python and Node is_done probes in src/openhuman/harness_init/registry.rs used try_cached(), whose state is process-local and empty at boot. The orchestrator therefore published overall = running before the on-disk managed install was resolved, and the overlay rendered for any running snapshot.

Solution

  • Durable probes — new probe_installed() on PythonBootstrap (system detect, else scan the managed cache root for an already-extracted install) and NodeBootstrap (system detect, else stat the deterministic managed install dir). Both are network-free and memoise a hit into the same cache resolve() uses. python_is_done / node_is_done now call these instead of try_cached().
  • Provisioning vs routine startupHarnessInitStep gains provisioning: bool. run_harness_init_with probes on-disk readiness of provisioning steps before publishing any Running state, and only surfaces the overlay when a download/install/repair is genuinely required. runtime_python_server is routine service-start (provisioning: false), so relaunching an already-installed server on a warm host runs silently in the background.
  • Dismissal persistence — the "Run in background" choice is keyed to the run's startedAt and stored in sessionStorage (plus a module-level mirror), so a remount/reload does not reopen the overlay during the same provisioning run; a genuinely new run is still allowed to surface.
  • Debug logging added on the warm-start / provisioning decision and each durable probe outcome (no PII/secrets).

Submission Checklist

If a section does not apply to this change, mark the item as N/A with a one-line reason. Do not delete items.

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
  • Diff coverage ≥ 80% — changed lines (Vitest + cargo-llvm-cov merged via diff-cover) meet the gate. New logic is covered by Rust probe tests + the Vitest overlay suite; ran targeted cargo test and vitest run locally (full pnpm test:coverage/pnpm test:rust matrix skipped locally to avoid filling disk — CI runs the gate).
  • Coverage matrix updated — N/A: behaviour-only fix to an existing feature (harness-init); no feature row added/removed/renamed.
  • All affected feature IDs from the matrix are listed under ## RelatedN/A: no matrix feature ID for this bug fix.
  • No new external network dependencies introduced (the durable probes are stat/subprocess only and never hit the network).
  • Manual smoke checklist updated if this touches release-cut surfaces — N/A: no new manual smoke step; behaviour is covered by automated regression tests.
  • Linked issue closed via Closes #NNN in the ## Related section

Impact

  • Desktop (all platforms): warm launches go straight to the app instead of flashing the runtime-setup overlay; fresh installs still show setup progress; a missing/corrupt runtime still triggers the repair/retry UI. Also resolves the "CMD windows on every launch" symptom for the server-start step by keeping routine startup silent.
  • Performance: on a warm start the durable probe runs a couple of cheap stat/version checks in the background instead of an unconditional visible run. No security/migration/compat implications; no new deps.

Related


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

Keep this section for AI-authored PRs. For human-only PRs, mark each field N/A.

Linear Issue

  • Key: N/A (GitHub-tracked)
  • URL: N/A

Commit & Branch

  • Branch: fix/GH-5047-runtime-setup-first-run-only
  • Commit SHA: d1e8799

Validation Run

  • pnpm --filter openhuman-app format:check — prettier applied to touched files
  • pnpm typecheck
  • Focused tests: cargo test --lib probe_installed (6 ✅), cargo test --lib harness_init (10 ✅), vitest run src/components/InitProgressScreen/ (7 ✅)
  • Rust fmt/check (if changed): cargo fmt applied; cargo check --bin openhuman-core
  • Tauri fmt/check (if changed): N/A: no app/src-tauri changes.

Validation Blocked

  • command: N/A
  • error: N/A
  • impact: N/A

Behavior Changes

  • Intended behavior change: runtime-setup overlay surfaces only when provisioning (download/install/repair) is genuinely required, not on every launch.
  • User-visible effect: warm restarts open the app directly; no repeated setup popup / foreground CMD windows for already-installed runtimes.

Parity Contract

  • Legacy behavior preserved: fresh-install first-run still shows full setup progress; failed required-step handling and Retry/Continue UI unchanged; resolve()/download paths untouched.
  • Guard/fallback/dispatch parity checks: durable probe mirrors resolve()'s system/managed resolution minus the download fallback; non-provisioning steps still run (silently) every launch as before.

Duplicate / Superseded PR Handling

Summary by CodeRabbit

  • New Features

    • Startup now detects previously installed Python and Node runtimes across restarts.
    • Warm starts can proceed silently when provisioning is already complete.
    • The setup overlay appears only when installation or repair work is required.
  • Bug Fixes

    • “Run in background” dismissal now persists for the active setup run.
    • The overlay reappears automatically when a new setup run begins.

…humansaiGH-5047)

The Python/Node `is_done` probes used `try_cached()`, whose memo is
process-local and empty after every restart. The orchestrator therefore
re-entered a visible `running` state on every launch and the full-screen
HarnessInitOverlay flashed even when everything was already installed.

- Add durable, non-downloading `probe_installed()` to PythonBootstrap and
  NodeBootstrap (system detect / on-disk managed scan; never hits network)
  and use them for the harness-init readiness probes.
- Classify each step `provisioning` (download/install) vs routine startup;
  only publish a visible `Running` state when a provisioning step genuinely
  needs work, so relaunching the already-installed Python server stays silent.
- Persist the "Run in background" dismissal per provisioning run (sessionStorage
  + module mirror) so a remount/reload cannot reopen the overlay.
- Regression tests: durable probe recovers readiness from disk after a
  simulated restart (Rust, Python+Node); provisioning classification;
  overlay warm-start / dismissal-persistence / new-run reopen (Vitest).
@M3gA-Mind
M3gA-Mind requested a review from a team July 20, 2026 13:19
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 33 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3198ae89-0269-4e45-a06b-7d1c05c9edcb

📥 Commits

Reviewing files that changed from the base of the PR and between 17a7e7e and 4e2c70c.

📒 Files selected for processing (4)
  • app/src/components/InitProgressScreen/HarnessInitOverlay.tsx
  • src/openhuman/runtime_node/bootstrap.rs
  • src/openhuman/runtime_node/bootstrap_tests.rs
  • src/openhuman/runtime_python/bootstrap.rs
📝 Walkthrough

Walkthrough

Harness initialization now uses durable Python and Node readiness probes, distinguishes provisioning from routine startup, suppresses the blocking overlay on warm starts, and persists “Run in background” dismissal for each provisioning run.

Changes

Harness initialization readiness

Layer / File(s) Summary
Durable runtime readiness probes
src/openhuman/runtime_python/bootstrap.rs, src/openhuman/runtime_python/bootstrap_tests.rs, src/openhuman/runtime_node/bootstrap.rs, src/openhuman/harness_init/registry.rs
Python and Node runtimes probe existing installations without downloading, memoize successful results, and use those probes for restart-durable completion checks.
Provisioning classification and startup gating
src/openhuman/harness_init/registry.rs, src/openhuman/harness_init/ops.rs
Install-related steps are marked as provisioning, while routine server startup is excluded; Running is published only when provisioning remains required.
Per-run overlay dismissal persistence
app/src/components/InitProgressScreen/HarnessInitOverlay.tsx, app/src/components/InitProgressScreen/HarnessInitOverlay.test.tsx
Overlay dismissal is stored by run timestamp, polling stops for dismissed runs, and tests cover remount persistence and reopening for a new run.

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

Sequence Diagram(s)

sequenceDiagram
  participant HarnessInit
  participant RuntimeProbes
  participant OverallState
  participant HarnessInitOverlay
  HarnessInit->>RuntimeProbes: probe installed Python and Node runtimes
  RuntimeProbes-->>HarnessInit: readiness results
  HarnessInit->>OverallState: publish Running only when provisioning is required
  OverallState-->>HarnessInitOverlay: provisioning snapshot
  HarnessInitOverlay->>HarnessInitOverlay: persist dismissal for current run
  HarnessInitOverlay-->>HarnessInit: stop polling for dismissed run
Loading

Suggested labels: bug

Suggested reviewers: senamakel

Poem

I’m a rabbit with a startup tune,
Probes find runtimes by the light of the moon.
Warm launches hop past the setup display,
New runs bring the overlay back to play.
“Run in background” stays tucked away.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main first-run runtime overlay fix and matches the changed behavior.
Linked Issues check ✅ Passed Durable readiness probes, provisioning gating, and dismissal persistence address the first-launch and warm-restart requirements, with regression tests added.
Out of Scope Changes check ✅ Passed The changes stay focused on runtime-init logic, overlay behavior, and related tests, with no unrelated feature work.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d1e8799828

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/openhuman/harness_init/registry.rs
@M3gA-Mind

Copy link
Copy Markdown
Collaborator Author

Synced onto current main (now includes #5070 feature-gate allowlist + #5003/#5025 learning-subscriber decouple). Re-running CI.

@M3gA-Mind

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
app/src/components/InitProgressScreen/HarnessInitOverlay.test.tsx (1)

38-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

beforeEach doesn't reset HarnessInitOverlay's module-level dismissal mirror.

window.sessionStorage.clear() only clears the storage fallback; readDismissedRun() in HarnessInitOverlay.tsx checks the module-level dismissedRunMirror first, which persists across tests within this file and isn't reset here. Current tests happen to pass because each dismissal uses a run key not re-checked incompatibly by a later test, but this makes the suite order-dependent rather than genuinely isolated.

♻️ Suggested fix

Export a test-only reset (e.g. __resetDismissedRunForTests()) from HarnessInitOverlay.tsx and call it in beforeEach, or use vi.resetModules() + dynamic re-import per test so the module singleton starts clean alongside sessionStorage.clear().

🤖 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 `@app/src/components/InitProgressScreen/HarnessInitOverlay.test.tsx` around
lines 38 - 42, Reset the module-level dismissed-run state between tests. Add a
test-only reset export alongside the `dismissedRunMirror`/`readDismissedRun`
logic in `HarnessInitOverlay`, then invoke it in `HarnessInitOverlay.test.tsx`’s
`beforeEach` together with `sessionStorage.clear()` so each test starts with an
isolated dismissal state.
🤖 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 `@app/src/components/InitProgressScreen/HarnessInitOverlay.tsx`:
- Around line 15-55: Add namespaced debug logging to the dismissal-persistence
flow: instrument readDismissedRun and writeDismissedRun, including
sessionStorage access and failures, and log the stay-hidden early-return and
render-guard branches near the polling and rendering logic. Include relevant run
keys and state transitions while preserving existing behavior.

In `@src/openhuman/runtime_node/bootstrap.rs`:
- Around line 413-500: Extract the inline probe_installed_tests module from
NodeBootstrap’s bootstrap module into a new bootstrap_tests.rs, preserving all
helpers and test cases unchanged. Wire the external test module alongside the
existing runtime_node module declarations using the same pattern as
runtime_python/bootstrap_tests.rs, with test-only configuration.

In `@src/openhuman/runtime_python/bootstrap.rs`:
- Around line 261-288: Update probe_any_managed_install to reject symlinked
entries before the is_dir check and before calling probe_managed_install.
Inspect each directory entry’s file type without following links, skip symbolic
links, and preserve probing only for real immediate subdirectories under
cache_root.

---

Nitpick comments:
In `@app/src/components/InitProgressScreen/HarnessInitOverlay.test.tsx`:
- Around line 38-42: Reset the module-level dismissed-run state between tests.
Add a test-only reset export alongside the
`dismissedRunMirror`/`readDismissedRun` logic in `HarnessInitOverlay`, then
invoke it in `HarnessInitOverlay.test.tsx`’s `beforeEach` together with
`sessionStorage.clear()` so each test starts with an isolated dismissal state.
🪄 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: fb1008d5-dd26-443a-9ff3-cd0f71fda930

📥 Commits

Reviewing files that changed from the base of the PR and between 2df98d8 and 17a7e7e.

📒 Files selected for processing (7)
  • app/src/components/InitProgressScreen/HarnessInitOverlay.test.tsx
  • app/src/components/InitProgressScreen/HarnessInitOverlay.tsx
  • src/openhuman/harness_init/ops.rs
  • src/openhuman/harness_init/registry.rs
  • src/openhuman/runtime_node/bootstrap.rs
  • src/openhuman/runtime_python/bootstrap.rs
  • src/openhuman/runtime_python/bootstrap_tests.rs

Comment thread app/src/components/InitProgressScreen/HarnessInitOverlay.tsx
Comment thread src/openhuman/runtime_node/bootstrap.rs Outdated
Comment thread src/openhuman/runtime_python/bootstrap.rs
…ismissal logging

Address CodeRabbit review on tinyhumansai#5066 (tinyhumansai#5047):
- security(runtime_python): reject symlinked cache entries in
  probe_any_managed_install before the is_dir() check. path.is_dir()
  follows symlinks, so a link under cache_root could point the durable
  probe at a dir outside the root and be trusted as managed Python;
  entry.file_type() reports the link itself and is skipped. (The Node
  bootstrap already gets equivalent protection from its canonicalize +
  starts_with(cache_root) guard.)
- refactor(runtime_node): extract the inline probe_installed_tests module
  into runtime_node/bootstrap_tests.rs, wired via
  #[cfg(test)] #[path = "bootstrap_tests.rs"] mod tests; — matching the
  sibling runtime_python pattern and taking bootstrap.rs back under the
  ~500-line ceiling.
- logging(HarnessInitOverlay): add namespaced harness-init debug
  diagnostics across the dismissal-persistence flow — read/write helpers
  (incl. sessionStorage-failure paths), the warm-poll stay-hidden branch,
  and the handleContinue dismissal action.

Targeted: cargo fmt; cargo test probe_installed (6 passed, incl. the moved
node module); prettier on the overlay.
@M3gA-Mind

Copy link
Copy Markdown
Collaborator Author

Addressed the CodeRabbit review in 4e2c70c (3/3 items, none declined):

  1. [Major · Security] runtime_python::probe_any_managed_install now skips symlinked cache entries before is_dir() (via entry.file_type(), which doesn't follow the link), closing the symlink-escape vector where a link under cache_root could be trusted as a managed Python install. Node already had equivalent canonicalize+starts_with protection — this brings Python to parity.
  2. [Major · Maintainability] Extracted the inline probe_installed_tests into runtime_node/bootstrap_tests.rs (#[path]-wired mod tests;, matching runtime_python), taking bootstrap.rs back under the ~500-line ceiling (now 416).
  3. [Major · Logging] Added namespaced harness-init diagnostics across the dismissal-persistence flow (read/write helpers incl. sessionStorage-failure paths, warm-poll stay-hidden branch, handleContinue).

Validation (targeted): cargo fmt ✅; cargo test probe_installed → 6 passed incl. the moved node module ✅; prettier on the overlay ✅.

@M3gA-Mind

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@sanil-23 sanil-23 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Walkthrough

The change replaces the process-local try_cached() memo in the Python/Node is_done probes with durable on-disk probe_installed() probes, adds a provisioning: bool classifier to HarnessInitStep, and gates store::set_overall(Running) behind a pre-flight sweep of the provisioning probes so a warm start never publishes a visible running state. The frontend persists the "Run in background" dismissal in sessionStorage keyed by snapshot.startedAt.

The core diagnosis and the Rust warm-start fix are correct and well-scoped — deriving "first run" from on-disk runtime presence rather than a persisted flag is the right design, and it makes the fresh-profile / Clear-App-Data / multi-user axes fall out correctly for free (see Looks good). Two things don't hold up: the Python durable probe accepts any interpreter under the cache root with no version-bound check (breaks the upgrade path), and the dismissal-persistence design rests on a startedAt that the core stamps once per process, not once per run — so the "reopens for a genuinely new run" guarantee, and its two tests, are unreachable in production.

Changed files

File Δ Notes
src/openhuman/runtime_python/bootstrap.rs +88 probe_installed() + probe_any_managed_install()no version-bound check
src/openhuman/runtime_node/bootstrap.rs +55 probe_installed() — version-pinned install dir, upgrade-safe ✅
src/openhuman/harness_init/registry.rs +47/-4 provisioning flag; probes switched off try_cached
src/openhuman/harness_init/ops.rs +43/-3 Warm-start guard; double-probes every provisioning step
app/src/components/InitProgressScreen/HarnessInitOverlay.tsx +66/-2 sessionStorage dismissal keyed on startedAt
*_tests.rs (×2), HarnessInitOverlay.test.tsx +243 Good probe coverage; two overlay tests assert unreachable behavior

Findings

🔴 Major

1. probe_any_managed_install ignores minimum_version/maximum_version — the overlay is suppressed exactly when an upgrade needs a new Python

src/openhuman/runtime_python/bootstrap.rs:267-311

probe_managed_install parses the version but never compares it against the config bounds, while resolve() enforces them via select_distribution(&release, &self.config.minimum_version, &self.config.maximum_version) (bootstrap.rs:180). Old installs are never pruned from the cache root.

Failure scenario: a user on managed CPython 3.12 upgrades to a build whose default_minimum_version() moves to 3.13.0. probe_any_managed_install finds the stale cpython-3.12.*-install_only/ dir, returns Somepython_is_done = trueprovisioning_required = falseno overlay, and python_run is skipped entirely. The 3.13 download then happens lazily and silently at whatever moment the first resolve() caller runs, with no progress UI — the exact regression this PR is meant to prevent, inverted. Node is immune because its install_dir is derived from config.version.

 fn probe_any_managed_install(cache_root: &Path) -> Option<ResolvedPython> {
+    // Only a dir laid down by `install_managed_from_api` (asset name minus
+    // `.tar.gz`) can be a managed install — scanning arbitrary siblings both
+    // widens the trust boundary and is needlessly expensive (see finding 3).
     let entries = std::fs::read_dir(cache_root).ok()?;
     for entry in entries.flatten() {
         let path = entry.path();
         ...
+        if !path.file_name().and_then(|n| n.to_str())
+            .is_some_and(|n| n.starts_with("cpython-"))
+        {
+            continue;
+        }
         if let Some(resolved) = probe_managed_install(&path) {
             return Some(resolved);
         }
     }
     None
 }

and, in probe_managed_install, reject an out-of-bounds interpreter so a version bump re-provisions:

 fn probe_managed_install(install_dir: &Path) -> Option<ResolvedPython> {
     let python_bin = find_python_binary(install_dir)?;
     let version = super::resolver::probe_python_version_public(&python_bin)?;
     let version_info = super::resolver::parse_python_version(&version)?;
     Some(ResolvedPython { ... })
 }
+
+// …and give the durable probe a bounds-checking wrapper it calls instead:
+fn probe_managed_install_within(
+    install_dir: &Path,
+    minimum: &str,
+    maximum: &str,
+) -> Option<ResolvedPython> {
+    let resolved = probe_managed_install(install_dir)?;
+    let found = super::resolver::parse_python_version(&resolved.version)?;
+    if super::resolver::parse_python_version(minimum).is_some_and(|min| found < min) {
+        return None; // stale install from before a minimum-version bump
+    }
+    if !maximum.is_empty()
+        && super::resolver::parse_python_version(maximum).is_some_and(|max| found >= max)
+    {
+        return None;
+    }
+    Some(resolved)
+}

probe_managed_install is also called from the install path at bootstrap.rs:188/218, where the dir is already version-selected — leave that call site on the unchecked helper.

Please add a regression test: lay down a cpython-3.11.* install, set minimum_version = "3.12.0", assert probe_installed() is None.


2. The dismissal key can never change within a core process, and the overlay stops polling forever — the "reopen for a new run" guarantee is unreachable

app/src/components/InitProgressScreen/HarnessInitOverlay.tsx:97-105, src/openhuman/harness_init/store.rs:53-56

Two compounding issues:

  • store::set_overall stamps started_at only if guard.started_at.is_none(), and nothing ever clears it. startedAt is therefore a process marker, not a run marker: a second provisioning run in the same core process (Retry via harness_init_run, or a repair triggered after a runtime is deleted) reuses the identical startedAt, so isRunDismissed(next) stays true and the new run is silently suppressed.
  • Even if startedAt did rotate, the poll loop sets dismissedRef.current = true and returns without rescheduling (line 104), the effect has [] deps, and HarnessInitOverlay is mounted exactly once in App.tsx:163. Once dismissed, no further status is ever fetched, so a new run cannot be observed.

The two new tests pass only because they call first.unmount() and remount, giving a fresh effect — a lifecycle the real app never performs. Acceptance criterion "A missing or corrupted runtime can still trigger repair/retry UI" does not hold within a session after a dismissal.

Minimal fix — stamp started_at per run in the core, and keep polling after a dismissal so a genuinely new run can re-surface:

// store.rs
 pub fn set_overall(overall: OverallState) {
     let mut guard = state().lock().unwrap();
     let now = Utc::now().to_rfc3339();
     match overall {
-        OverallState::Running if guard.started_at.is_none() => {
-            guard.started_at = Some(now.clone());
-        }
+        // Stamp every transition *into* Running so each run is distinguishable
+        // — the frontend keys its "Run in background" dismissal on this value.
+        OverallState::Running if guard.overall != OverallState::Running => {
+            guard.started_at = Some(now.clone());
+            guard.finished_at = None;
+        }
         OverallState::Done | OverallState::Failed => {
             guard.finished_at = Some(now.clone());
         }
         _ => {}
     }
     guard.overall = overall;
 }
// HarnessInitOverlay.tsx — keep polling; let render-time `isRunDismissed` do the hiding
           if (isRunDismissed(next)) {
             log('run %s already dismissed — staying hidden', runKey(next));
-            dismissedRef.current = true;
             setDismissed(true);
-            return;
+          } else if (dismissed) {
+            // A new run (fresh startedAt) is allowed to surface again.
+            setDismissed(false);
           }

dismissedRef should then only short-circuit on unmount, not on dismissal; the render-time guard at line 160 already suppresses the overlay. Then rewrite the "reopens for a genuinely new provisioning run" test to drive it through the same mount with two successive fetchHarnessInitStatus resolutions, which is what production does.

If instead the intended semantics are "dismissal is permanent for the session", please say so in the doc comment and delete the reopen test rather than leaving an assertion production can't satisfy.


3. probe_any_managed_install can recursively walk an unrelated multi-GB cache when cache_dir is configured

src/openhuman/runtime_python/bootstrap.rs:267-299, 320-351

python_server_cache_root (runtime_python_server/spacy.rs:216-225) resolves to PathBuf::from(cache_dir).join("runtime-python-server") — a child of the Python cache root whenever runtime_python.cache_dir is set. The scan will therefore treat runtime-python-server/ as a candidate install dir; find_python_binary's fallback (bootstrap.rs:335) then WalkDirs the entire tree, including the torch/spaCy venvs, and may return a venv interpreter as source: Managed. read_dir order is unspecified, so this can happen before the real cpython-* dir is reached — on the startup hot path, before any state is published.

With the default empty cache_dir the two roots are siblings and this doesn't trigger, but it's a live hazard for anyone setting cache_dir (config/schema/load_tests.rs:878 shows it's a supported knob). The cpython- prefix filter from finding 1 fixes this too.


🟡 Minor

4. Every provisioning step's is_done runs twice per launch

src/openhuman/harness_init/ops.rs:70-84 then 86-96

provisioning_required probes all four provisioning steps, then run_one probes each again. Because python_is_done/node_is_done construct a fresh PythonBootstrap/NodeBootstrap per call (registry.rs:85, 229-235), each with its own Arc<Mutex<Option<_>>>, the memoisation probe_installed performs is discarded immediately — the doc claim "A hit is memoised into the same cache resolve() uses" (bootstrap.rs:111) is not true on this path. A warm start therefore does 8 probes, each potentially spawning python --version / node --version subprocesses (with a 5s timeout apiece). Thread the results through instead:

-    let needs_provisioning = force || provisioning_required(&config, &steps).await;
+    // Probe once; reuse the verdict in the loop so a warm start doesn't pay
+    // for two full sweeps of subprocess-spawning probes.
+    let mut satisfied: HashSet<&'static str> = HashSet::new();
+    let needs_provisioning = if force {
+        true
+    } else {
+        provisioning_required(&config, &steps, &mut satisfied).await
+    };
     ...
     for step in &steps {
-        run_one(&config, step, force, &mut failed_required).await;
+        run_one(&config, step, force, satisfied.contains(step.id), &mut failed_required).await;
     }

provisioning_required short-circuits on the first miss, so on a cold start only the already-satisfied prefix populates satisfied — which is exactly right.

5. Warm start produces a snapshot with finishedAt set but startedAt null

src/openhuman/harness_init/ops.rs:33-40, 53

When needs_provisioning is false, set_overall(Running) is never called, so started_at stays None while set_overall(Done) stamps finished_at. The wire snapshot then reads "finished without ever starting", and runKey() falls to the UNKEYED_RUN = 'pending' sentinel. Harmless today (a done snapshot never renders), but it makes the 'pending' sentinel collidable and the snapshot semantically inconsistent. Consider stamping started_at unconditionally at the top of run_harness_init_with and only gating the overall transition.

6. Python symlink guard is weaker than the comment claims

src/openhuman/runtime_python/bootstrap.rs:271-282

The comment asserts parity with "the Node bootstrap's canonicalize + starts_with(cache_root) guard", but the guard here only rejects symlinked top-level entries. find_python_binary uses candidate.is_file(), which follows symlinks, so <install_dir>/bin/python3 may point anywhere outside the cache root. Node's probe_managed_install (runtime_node/bootstrap.rs:371-400) canonicalises and containment-checks. Cache roots are user-owned so the practical risk is low, but either add the equivalent check or soften the comment so a future reader doesn't trust a guarantee that isn't there:

+        // Mirror the Node guard: containment-check the resolved binary.
+        let canon_root = std::fs::canonicalize(cache_root).ok()?;
         if let Some(resolved) = probe_managed_install(&path) {
+            let Ok(canon_bin) = std::fs::canonicalize(&resolved.python_bin) else { continue };
+            if !canon_bin.starts_with(&canon_root) {
+                tracing::warn!(bin = %canon_bin.display(), "[runtime_python::bootstrap] refusing interpreter outside cache root");
+                continue;
+            }
             return Some(resolved);
         }

Nitpicks

  • bootstrap.rs:163-166install_managed() is now a thin unused wrapper over install_managed_from_api; pre-existing, but the diff touches its neighbourhood. Worth deleting if nothing but tests call it.
  • HarnessInitOverlay.tsx:33-35readDismissedRun short-circuits on the module mirror and never re-reads sessionStorage. Correct given single-window, but a one-line comment saying the mirror is authoritative once set would help.
  • runtime_node/bootstrap_tests.rs:479-487managed_config uses NodeConfig::default().version while constructing the rest by hand; NodeConfig { prefer_system: false, cache_dir: …, ..Default::default() } is tighter and won't need editing when a field is added.
  • registry.rs:293-302provisioning_classification_excludes_service_startup encodes "everything except runtime_python_server" as a negation, so a newly added non-provisioning step silently fails the test with a confusing message. An explicit expected map keyed by id would fail more legibly.

Questions

  1. handle_run (ops.rs:125-135) has no guard against a Retry racing the boot run — both mutate the shared store, and whichever finishes first publishes Done, hiding the overlay while the other is still downloading. Pre-existing, but the warm-start guard makes the state machine more asymmetric. Worth a tokio::sync::Mutex around run_harness_init_with, or is it deliberately out of scope?
  2. Was the startedAt-per-process behavior (finding 2) noticed? Specifically: what is the intended behavior when a user dismisses to background and a required runtime is subsequently deleted mid-session?
  3. runtime_python_server is provisioning: false, but its runensure_startedensure_spacy/ensure_kompress can install torch into the shared venv (registry.rs:116-118 acknowledges this). On a host where spacy_provisioned is true but the shared venv lacks torch, does that step ever do heavy install work while classified non-provisioning — i.e. a silent multi-GB download with no UI?

Looks good

  • Diagnosis is exactly right, and the try_cached() → durable-probe swap is the correct minimal fix. The doc comment added to HarnessInitStep::is_done (registry.rs:43-46) warning future authors off process-local memos is a genuinely good piece of institutional memory.
  • Deciding visibility before publishing any Running state, rather than debouncing the overlay client-side, is the right layer.
  • Fresh profile / Clear App Data / multi-user all fall out correctly. Both cache roots default to dirs::cache_dir()/openhuman/*, which is per-OS-user and outside the workspace: a wiped ~/.openhuman leaves runtimes intact (warm start, no overlay — correct, nothing to provision), a wiped OS cache re-provisions with the overlay (correct), and two OS users get independent probes. Not persisting a "first run" flag at all and deriving it from on-disk state is what makes this robust — worth keeping.
  • Node's probe is upgrade-safe by construction (version-derived install_dir) and correctly reuses the existing canonicalize + containment guard.
  • probe_installed returning None for a disabled runtime, with callers treating "disabled" separately in *_is_done, is a clean separation — and it's tested on both bootstraps.
  • Rust test coverage of the probes (cold cache / on-disk install / disabled) is solid, and the simulated-restart test asserting try_cached().is_none() as a precondition is a nice touch.
  • Confirmed the Windows console-flash concern is a non-issue: both version probes already set CREATE_NO_WINDOW (runtime_python/resolver.rs:218, runtime_node/resolver.rs:219), so the durable probes don't reintroduce it.

Requesting changes on findings 1 (upgrade path) and 2 (dismissal key + polling stop). The Rust warm-start architecture is sound and should survive both fixes largely intact.

@senamakel
senamakel merged commit d3660b2 into tinyhumansai:main Jul 20, 2026
19 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in Team Openhuman Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

3 participants