Skip to content

fix(installer): auto-install declared UI modules - #84

Open
vernonstinebaker wants to merge 4 commits into
mainfrom
fix/auto-install-ui-modules
Open

vernonstinebaker wants to merge 4 commits into
mainfrom
fix/auto-install-ui-modules

Conversation

@vernonstinebaker

Copy link
Copy Markdown
Contributor

Problem

On a fresh release install of NullHub, nothing ever installs the declared UI modules. Traced every trigger:

  • startup only runs syncLocalUiModules, which builds from a local sibling checkout (dev-only)
  • instance creation (orchestrator.install) never touches ui_modules
  • the Chat tab's "Reinstall this instance to add it" hint has no backing install path either

The only working trigger is a manual POST /api/ui-modules/<name>/install, so a binary-release user had to run a curl command to get a working chat window (background: nullclaw-chat-ui v2026.9.28 fixed the archives, verified in nullhub#20).

Change

Two best-effort install triggers, both reusing the existing installUiModule (skip if installed → local sibling build → GitHub Releases download):

  1. Startup — new syncMissingUiModules installs declared modules for any component that has at least one instance. Covers existing installs the moment they upgrade: no re-creation needed.
  2. Instance create — orchestrator.install ensures the component's modules after a successful install. Covers newly created instances.

Failures log a warning and never fail startup or instance creation; the API endpoint stays as a manual fallback. Downloads are skipped under builtin.is_test (same pattern as syncLocalUiModules) — tests must not hit the network.

Validation

  • zig build test --summary all — 0 failures, 0 leaks; new tests: registry wiring (nullclaw declares nullclaw-chat-ui) and test-guard no-op with a seeded qualifying instance.
  • zig fmt --check src/ clean.
  • Manual smoke (release-style env, no sibling checkout): seeded state.json with a nullclaw instance → fresh nullhub serve → ui/nullclaw-chat-ui@latest created automatically, /api/ui-modules lists it, module.js served with 200 (1,655,520 bytes = fixed v2026.9.28).

Known behavior

Uninstalling a module via DELETE /api/ui-modules/<name> followed by a hub restart reinstalls it while the component still has instances (startup can't distinguish "never installed" from "deliberately removed" without new state). Flagging in case that deserves follow-up rather than a bigger change here.

A fresh release install previously had no path that installs declared
UI modules: startup only builds from a local sibling checkout, instance
creation never touched ui_modules, and the Chat tab's "Reinstall this
instance to add it" hint had no backing install path either. Getting
the chat window required a manual POST to
/api/ui-modules/<name>/install.

Install declared UI modules in two places:

- on startup, for components that have at least one instance
  (syncMissingUiModules, covering installs created before this change)
- after a successful instance install (covering new instances)

Both are best effort: a failed download logs a warning and never fails
startup or instance creation; the API endpoint remains available as a
manual fallback. Skipped under builtin.is_test like the other module
paths, since tests must not hit the network.

@DonPrus DonPrus 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.

Reviewed the full change, issue #20 and its discussion, the earlier installer/bundle fixes, current main, and the analogous instance-registration paths. The automatic trigger is needed; it does not duplicate the earlier archive-layout fix. The branch merges cleanly with current main. I am leaving this without approval because of two regressions:

  1. Startup waits for an unbounded network request (src/server.zig:132, syncMissingUiModules). This runs in Server.init, before the supervisor starts, instances are reconciled, or HTTP listens. fetchLatestRelease and the download helper use synchronous curl without a total timeout. With an existing nullclaw instance and no module, a stub that sleeps five seconds before failing delayed listening by those five seconds; without an instance, no fetch occurred. A stalled request can therefore prevent the entire hub and its auto-start instances from becoming available. It would be great to perform this best-effort installation after startup with bounded network operations; catching an error only helps once the operation returns.

  2. The startup paths build the same local module twice and can remove a healthy result (ensureComponentUiModules / the consecutive sync calls in Server.init). syncLocalUiModules already builds a sibling checkout. The new sync then calls installUiModule again; its local-build branch runs before the installed-module check and deletes @dev-local before building. With a stub build that succeeds once and fails on the second call, startup runs both builds and ends with no module.js. Please deduplicate the two paths and stage builds before replacing a working module. Check the new instance-install hook too, since it uses the same destructive helper against a module shared by existing instances.

An analogous path remains uncovered: api/instances.zig:handleImport registers and saves the first nullclaw instance directly, without the new installation hook. Importing into a running hub still needs a restart or manual module installation. It would be useful to route successful registration paths through a common best-effort ensure operation.

Validation: bash tests/test_backend.sh passed 12 integration tests, but its unit-test target currently executes zero tests because the src/main.zig test root does not reference the test-import block in root.zig. This wiring issue predates this PR. The added test guard also bypasses the behavior under test. The startup reproductions above used a locally instrumented Paths.init that requires an explicit temporary fixture root, fake curl/npm, and disabled auto-start fixture instances. Please add coverage for successful install, repeat sync, and failure preserving an already working module without depending on live GitHub.

…asses

Addresses the blocking findings from the review of #84.

Startup ordering: syncLocalUiModules/syncMissingUiModules ran inline in
Server.init, before listener.listen(), and fetchLatestRelease/download used
curl with no --max-time — a stalled request could keep the hub from ever
listening. Both passes now run on a background thread started from run()
after we accept connections, and every outbound curl carries a 30s ceiling
(prereqs.curl_max_time_secs). The thread is joined in deinit() before
state/paths are freed.

Destructive replace: installUiModule deleted dev-local before building and
returned before the installed check, so the local-build pass and the
download pass both rebuilt the same module and a failed build destroyed a
working one. Install now stages and promotes: prepareStaging touches only
the staging directory, and promoteStagedModule refuses to replace dest
unless staging contains module.js (restoring the old dir if the rename
fails). downloadUiModule stages the same way. ensureComponentUiModules
skips modules already present in any version directory.

Registration: handleImport now provisions declared UI modules, so importing
into a running hub no longer needs a restart.

Coverage: build.zig uses src/main.zig as the unit-test root but that file
had no test wiring, so zig build test ran zero tests while reporting
success. Wire installer/ui_modules (the full root.zig wiring surfaces 15
pre-existing errors, tracked in #88). New tests cover staged promotion,
incomplete-staging refusal, prepareStaging isolation, installed-anywhere
skip detection, and staging path naming. Fixes surfaced bitrot in
test_helpers (mutable TmpDir) and an installExtractedUiModule test that
never created its parent dir.
Four ui_modules tests hard-coded `/tmp/test-nullhub-*`, which does not exist
on Windows, so they failed with FileNotFound there. They never failed before
only because the unit-test root had no wiring and nothing was executed (see
#88); wiring them surfaced the failure on windows-x86_64 CI.

Each now allocates its scratch directory from `std.testing.tmpDir()` and
resolves it to an absolute path, which is what the rest of the suite already
does. `isModuleInstalled returns false for non-existing directory` keeps a
genuinely absent path — that is the behaviour under test.
The test built its archive with `tar -czf` but ignored that exit code, then
asserted extraction — so a fixture failure was reported as a failure of
extractTarGz itself. That is what the newly wired suite surfaced on
windows-x86_64 (bsdtar CLI differs from the GNU tar the test assumes).

Now a non-zero `tar -czf` exits with error.SkipZigTest and prints tar's
stderr, so the extraction assertion only runs when the fixture actually
exists. On platforms where tar works the test still asserts extraction.
@vernonstinebaker

Copy link
Copy Markdown
Contributor Author

All four points addressed across 2a0cfe0, 3cff80c, 9b1f95a.

1. Startup no longer waits on the network. Both sync passes moved out of Server.init onto a background thread started from run() after listener.listen(), joined in deinit() before state/paths are freed. Every outbound curl now carries --max-time (prereqs.curl_max_time_secs = 30), covering fetchLatestRelease, fetchReleaseByTag and downloader.download.

Reproduced your scenario (stalled curl stub + an existing nullclaw instance), A/B:

time to /health 200 first log line
before (b4e4162) 16s warning(prereqs): required tool 'curl' is missing… → auto-install failed → then listening
after (2a0cfe0) 1s listening on http://127.0.0.1:19999
merge result (c07aca9b) 0s listening on http://127.0.0.1:19999

2. Destructive double-build removed. installUiModule deleted dev-local before building and returned before the installed check. Now:

  • prepareStaging clears only staging — it never touches dest_dir.
  • promoteStagedModule refuses to replace dest_dir unless staging contains module.js, moving the old dir aside first and restoring it if the rename fails.
  • downloadUiModule stages the same way instead of deleting dest_dir up front.
  • ensureComponentUiModules skips any module already present in any version directory, so the local-build pass and the download pass can no longer build the same module twice.

3. handleImport now provisions UI modules via ensureComponentUiModules after a successful registration, so importing into a running hub no longer needs a restart.

4. Coverage — and a correction to my own approach. Wiring real tests exposed the gap you flagged in #88: build.zig uses src/main.zig as the unit-test root and that file had no test block at all, so zig build test executed zero tests while reporting success. I wired installer/ui_modules (only): 47/47 now run, up from 0. Wiring root.zig instead surfaces 15 pre-existing compile errors in unrelated modules — that is #88 and is deliberately out of scope here.

New tests: staged promotion success, incomplete staging refused + working module untouched, prepareStaging isolation, isInstalledUnderRoot skip detection (incl. prefix-bleed), staging path naming, prepareStaging never touching an install.

I removed the vacuous syncMissingUiModules leaves module uninstalled under test test you called out — it asserted the guard, not the behaviour — and moved the registry assertion into registry.zig where it actually executes.

Wiring also surfaced three pieces of dormant bitrot, all fixed: test_helpers (const TmpDir can't satisfy cleanup(self: *TmpDir)), installExtractedUiModule (never created its parent dir), and four tests hard-coding /tmp/... which don't exist on Windows. extractTarGz's fixture ignored its own tar -czf exit code, so a fixture failure was reported as a failure of extractTarGz — it now skips with tar's stderr when the archive tool is unusable.

Validation — merge result c07aca9b (base = current main 825b0cd4, includes #87):

  • tests/test_backend.sh: unit 47/47, integration 12/12
  • zig fmt --check src/: OK
  • Startup probe: 0s to health, listening first
  • CI: 4/4 pass, including windows-x86_64

Ready for re-review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants