From b4e416286e60745e4b53743f2e1534d467a03ea4 Mon Sep 17 00:00:00 2001 From: Vernon Stinebaker Date: Mon, 28 Sep 2026 12:07:36 +0800 Subject: [PATCH 1/4] fix(installer): auto-install declared UI modules 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//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. --- src/installer/orchestrator.zig | 64 ++++++++++++++++++++++++++++++++++ src/server.zig | 1 + 2 files changed, 65 insertions(+) diff --git a/src/installer/orchestrator.zig b/src/installer/orchestrator.zig index 4725209d..2c7fc80a 100644 --- a/src/installer/orchestrator.zig +++ b/src/installer/orchestrator.zig @@ -384,6 +384,9 @@ pub fn install( error.StartFailed => return error.StartFailed, }; + // Best effort: a failed UI module install must not fail the instance. + ensureComponentUiModules(allocator, p, comp); + return .{ .version = version, .instance_name = opts.instance_name, @@ -1358,6 +1361,35 @@ pub fn syncLocalUiModules(allocator: std.mem.Allocator, p: paths_mod.Paths) void } } +/// Install the component's declared UI modules, best effort. Already-installed +/// modules are skipped and a failed install never fails the caller — the +/// module can still be installed via the API endpoint afterwards. +/// NOTE: No unit test for the download path — tests must not hit the network; +/// the guard below makes this a no-op under `zig build test`. Covered by a +/// manual smoke test against a seeded state. +fn ensureComponentUiModules(allocator: std.mem.Allocator, p: paths_mod.Paths, comp: registry.KnownComponent) void { + if (builtin.is_test) return; + + for (comp.ui_modules) |ui_mod| { + installUiModule(allocator, p, ui_mod, "latest") catch |err| { + std.log.warn("ui module {s} auto-install failed: {s}", .{ ui_mod.name, @errorName(err) }); + }; + } +} + +/// Download declared UI modules for components that have at least one +/// instance. Complements syncLocalUiModules (which builds from a local +/// sibling checkout) by covering release installs, so a fresh binary ends +/// up with a working chat UI without a manual API call. +pub fn syncMissingUiModules(allocator: std.mem.Allocator, p: paths_mod.Paths, s: *state_mod.State) void { + for (®istry.known_components) |*comp| { + if (comp.ui_modules.len == 0) continue; + const instances = s.instances.getPtr(comp.name) orelse continue; + if (instances.count() == 0) continue; + ensureComponentUiModules(allocator, p, comp.*); + } +} + /// Write content to a file at an absolute path, creating the file if needed. fn writeFile(path: []const u8, content: []const u8) !void { const file = try std_compat.fs.createFileAbsolute(path, .{}); @@ -2030,3 +2062,35 @@ test "patchNullclawRuntimeProfileIntoConfig prepares stateless gateway config fo try std.testing.expectEqualStrings(expected_hash, paired_tokens[0].string); try std.testing.expect(!nullclaw_gateway_config.isNullhubGatewayToken(paired_tokens[0].string)); } + +test "nullclaw component declares nullclaw-chat-ui ui module" { + const comp = registry.findKnownComponent("nullclaw") orelse return error.TestUnexpectedResult; + try std.testing.expectEqual(@as(usize, 1), comp.ui_modules.len); + try std.testing.expectEqualStrings("nullclaw-chat-ui", comp.ui_modules[0].name); + try std.testing.expectEqualStrings("nullclaw/nullclaw-chat-ui", comp.ui_modules[0].repo); +} + +test "syncMissingUiModules leaves module uninstalled under test despite qualifying instance" { + const allocator = std.testing.allocator; + var fixture = try test_helpers.TempPaths.init(allocator); + defer fixture.deinit(); + const state_path = try fixture.paths.state(allocator); + defer allocator.free(state_path); + var state = state_mod.State.init(allocator, state_path); + defer state.deinit(); + try state.addInstance("nullclaw", "backfill-target", .{ .version = "v2026.5.29" }); + + syncMissingUiModules(allocator, fixture.paths, &state); + + // The test guard held: the network download never ran, so the module + // destination directory was not created. + const dest = try fixture.paths.uiModule(allocator, "nullclaw-chat-ui", "latest"); + defer allocator.free(dest); + if (std_compat.fs.openDirAbsolute(dest, .{})) |existing| { + var dir = existing; + dir.close(); + return error.TestUnexpectedResult; + } else |err| { + try std.testing.expectEqual(error.FileNotFound, err); + } +} diff --git a/src/server.zig b/src/server.zig index c6cb6c39..84a99295 100644 --- a/src/server.zig +++ b/src/server.zig @@ -129,6 +129,7 @@ pub const Server = struct { }; orchestrator.syncLocalUiModules(allocator, paths); + orchestrator.syncMissingUiModules(allocator, paths, state); return .{ .allocator = allocator, From 2a0cfe0d4d7392d5c22f2781d15265ba0b14e6a4 Mon Sep 17 00:00:00 2001 From: Vernon Stinebaker Date: Sun, 4 Oct 2026 15:50:27 +0800 Subject: [PATCH 2/4] fix(installer): bound startup UI sync, stage builds, dedupe install passes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/api/instances.zig | 8 + src/installer/downloader.zig | 2 +- src/installer/orchestrator.zig | 97 ++++----- src/installer/prereqs.zig | 5 + src/installer/registry.zig | 11 +- src/installer/ui_modules.zig | 351 ++++++++++++++++++++++++++++++++- src/main.zig | 12 ++ src/server.zig | 36 +++- src/test_helpers.zig | 3 +- 9 files changed, 454 insertions(+), 71 deletions(-) diff --git a/src/api/instances.zig b/src/api/instances.zig index 41c76dbc..ae2b4f47 100644 --- a/src/api/instances.zig +++ b/src/api/instances.zig @@ -5,6 +5,7 @@ const state_mod = @import("../core/state.zig"); const manager_mod = @import("../supervisor/manager.zig"); const paths_mod = @import("../core/paths.zig"); const registry = @import("../installer/registry.zig"); +const orchestrator = @import("../installer/orchestrator.zig"); const downloader = @import("../installer/downloader.zig"); const platform = @import("../core/platform.zig"); const helpers = @import("helpers.zig"); @@ -4499,6 +4500,13 @@ pub fn handleImport(allocator: std.mem.Allocator, s: *state_mod.State, paths: pa return helpers.serverError(); }; + // Best effort: registration must also provision the component's declared + // UI modules, otherwise an import into an already-running hub still needs + // a restart (or a manual API call) before the UI is available. + if (registry.findKnownComponent(component)) |comp| { + orchestrator.ensureComponentUiModules(allocator, paths, comp); + } + const response_body = buildImportResponse(allocator, instance_name, source_dir) catch return helpers.serverError(); return jsonOk(response_body); } diff --git a/src/installer/downloader.zig b/src/installer/downloader.zig index 9186b73b..006152a9 100644 --- a/src/installer/downloader.zig +++ b/src/installer/downloader.zig @@ -58,7 +58,7 @@ pub fn download(allocator: std.mem.Allocator, url: []const u8, dest_path: []cons const result = std_compat.process.Child.run(.{ .allocator = allocator, - .argv = &.{ "curl", "-sfL", "-o", tmp_path, url }, + .argv = &.{ "curl", "-sfL", "--max-time", prereqs.curl_max_time_secs, "-o", tmp_path, url }, }) catch return error.DownloadFailed; defer allocator.free(result.stdout); defer allocator.free(result.stderr); diff --git a/src/installer/orchestrator.zig b/src/installer/orchestrator.zig index 2c7fc80a..3a7c36ee 100644 --- a/src/installer/orchestrator.zig +++ b/src/installer/orchestrator.zig @@ -1254,27 +1254,20 @@ pub fn installUiModule( ui_mod: registry.UiModuleRef, version: []const u8, ) !void { + const dest = try p.uiModule(allocator, ui_mod.name, version); + defer allocator.free(dest); + + // A local sibling checkout is the authoritative source in dev setups, so + // refresh `dev-local` from it on every install. The build goes to staging + // and only replaces the live module on success — a failed npm run leaves + // the previously working module in place. if (!builtin.is_test) { if (findLocalUiModuleDir(allocator, ui_mod.name)) |module_dir| { defer allocator.free(module_dir); - - const dev_local_version = "dev-local"; - const dev_local_dest = try p.uiModule(allocator, ui_mod.name, dev_local_version); - defer allocator.free(dev_local_dest); - - std_compat.fs.deleteTreeAbsolute(dev_local_dest) catch |err| switch (err) { - error.FileNotFound => {}, - else => {}, - }; - - if (buildLocalUiModuleFromDir(allocator, module_dir, dev_local_dest)) return; - return error.DownloadFailed; + return installFromLocalSource(allocator, p, ui_mod.name, module_dir); } } - const dest = try p.uiModule(allocator, ui_mod.name, version); - defer allocator.free(dest); - // Skip if already installed if (ui_modules_mod.isModuleInstalled(dest)) return; @@ -1284,6 +1277,26 @@ pub fn installUiModule( }; } +/// Build `module_dir` into staging and promote it over the `dev-local` +/// install. Staging is discarded on failure so an existing `module.js` +/// survives a broken build. +fn installFromLocalSource( + allocator: std.mem.Allocator, + p: paths_mod.Paths, + module_name: []const u8, + module_dir: []const u8, +) !void { + const dest = try p.uiModule(allocator, module_name, "dev-local"); + defer allocator.free(dest); + + // Clears staging only — `dest` stays untouched until the build succeeds. + const staging = try ui_modules_mod.prepareStaging(allocator, dest); + defer allocator.free(staging); + + if (!buildLocalUiModuleFromDir(allocator, module_dir, staging)) return error.DownloadFailed; + try ui_modules_mod.promoteStagedModule(allocator, staging, dest); +} + /// Build a UI module from a local sibling repository. /// Looks for ../{module_name}/ relative to CWD, runs `npm run build:module`, /// and copies the dist/ output to dest_dir. @@ -1361,16 +1374,20 @@ pub fn syncLocalUiModules(allocator: std.mem.Allocator, p: paths_mod.Paths) void } } -/// Install the component's declared UI modules, best effort. Already-installed -/// modules are skipped and a failed install never fails the caller — the -/// module can still be installed via the API endpoint afterwards. -/// NOTE: No unit test for the download path — tests must not hit the network; -/// the guard below makes this a no-op under `zig build test`. Covered by a -/// manual smoke test against a seeded state. -fn ensureComponentUiModules(allocator: std.mem.Allocator, p: paths_mod.Paths, comp: registry.KnownComponent) void { - if (builtin.is_test) return; - +/// Install the component's declared UI modules, best effort. A failed install +/// never fails the caller — the module can still be installed via the API +/// endpoint afterwards. +/// +/// Modules that already exist (in **any** version directory) are skipped, so +/// the two startup passes — `syncLocalUiModules` (local build) and +/// `syncMissingUiModules` (release download) — never build or fetch the same +/// module twice. `builtin.is_test` keeps the npm/network boundary offline; the +/// skip decision and the staged promotion it guards are covered by tests. +pub fn ensureComponentUiModules(allocator: std.mem.Allocator, p: paths_mod.Paths, comp: registry.KnownComponent) void { for (comp.ui_modules) |ui_mod| { + if (ui_modules_mod.isInstalledUnderRoot(allocator, p.root, ui_mod.name)) continue; + if (builtin.is_test) continue; + installUiModule(allocator, p, ui_mod, "latest") catch |err| { std.log.warn("ui module {s} auto-install failed: {s}", .{ ui_mod.name, @errorName(err) }); }; @@ -2062,35 +2079,3 @@ test "patchNullclawRuntimeProfileIntoConfig prepares stateless gateway config fo try std.testing.expectEqualStrings(expected_hash, paired_tokens[0].string); try std.testing.expect(!nullclaw_gateway_config.isNullhubGatewayToken(paired_tokens[0].string)); } - -test "nullclaw component declares nullclaw-chat-ui ui module" { - const comp = registry.findKnownComponent("nullclaw") orelse return error.TestUnexpectedResult; - try std.testing.expectEqual(@as(usize, 1), comp.ui_modules.len); - try std.testing.expectEqualStrings("nullclaw-chat-ui", comp.ui_modules[0].name); - try std.testing.expectEqualStrings("nullclaw/nullclaw-chat-ui", comp.ui_modules[0].repo); -} - -test "syncMissingUiModules leaves module uninstalled under test despite qualifying instance" { - const allocator = std.testing.allocator; - var fixture = try test_helpers.TempPaths.init(allocator); - defer fixture.deinit(); - const state_path = try fixture.paths.state(allocator); - defer allocator.free(state_path); - var state = state_mod.State.init(allocator, state_path); - defer state.deinit(); - try state.addInstance("nullclaw", "backfill-target", .{ .version = "v2026.5.29" }); - - syncMissingUiModules(allocator, fixture.paths, &state); - - // The test guard held: the network download never ran, so the module - // destination directory was not created. - const dest = try fixture.paths.uiModule(allocator, "nullclaw-chat-ui", "latest"); - defer allocator.free(dest); - if (std_compat.fs.openDirAbsolute(dest, .{})) |existing| { - var dir = existing; - dir.close(); - return error.TestUnexpectedResult; - } else |err| { - try std.testing.expectEqual(error.FileNotFound, err); - } -} diff --git a/src/installer/prereqs.zig b/src/installer/prereqs.zig index 627cd457..c685ea0b 100644 --- a/src/installer/prereqs.zig +++ b/src/installer/prereqs.zig @@ -4,6 +4,11 @@ const builtin = @import("builtin"); const log = std.log.scoped(.prereqs); +/// Wall-clock ceiling for every outbound curl invocation, in seconds. +/// Registry metadata fetches and artifact downloads run on the startup path, +/// so an unbounded request must never be able to hang the hub. +pub const curl_max_time_secs = "30"; + pub const EnsureError = error{ ToolInstallFailed, UnsupportedPlatform, diff --git a/src/installer/registry.zig b/src/installer/registry.zig index 9a074ddb..8688a22f 100644 --- a/src/installer/registry.zig +++ b/src/installer/registry.zig @@ -198,7 +198,7 @@ pub fn fetchLatestRelease(allocator: std.mem.Allocator, repo: []const u8) !std.j const result = std_compat.process.Child.run(.{ .allocator = allocator, - .argv = &.{ "curl", "-sfL", "-H", "Accept: application/vnd.github+json", url }, + .argv = &.{ "curl", "-sfL", "--max-time", prereqs.curl_max_time_secs, "-H", "Accept: application/vnd.github+json", url }, }) catch return error.FetchFailed; defer allocator.free(result.stdout); defer allocator.free(result.stderr); @@ -225,7 +225,7 @@ pub fn fetchReleaseByTag(allocator: std.mem.Allocator, repo: []const u8, tag: [] const result = std_compat.process.Child.run(.{ .allocator = allocator, - .argv = &.{ "curl", "-sfL", "-H", "Accept: application/vnd.github+json", url }, + .argv = &.{ "curl", "-sfL", "--max-time", prereqs.curl_max_time_secs, "-H", "Accept: application/vnd.github+json", url }, }) catch return error.FetchFailed; defer allocator.free(result.stdout); defer allocator.free(result.stderr); @@ -390,3 +390,10 @@ test "findAssetForComponentPlatform matches swapped platform order" { try std.testing.expect(asset != null); try std.testing.expectEqualStrings("nullclaw-macos-aarch64.bin", asset.?.name); } + +test "nullclaw component declares nullclaw-chat-ui ui module" { + const comp = findKnownComponent("nullclaw") orelse return error.TestUnexpectedResult; + try std.testing.expectEqual(@as(usize, 1), comp.ui_modules.len); + try std.testing.expectEqualStrings("nullclaw-chat-ui", comp.ui_modules[0].name); + try std.testing.expectEqualStrings("nullclaw/nullclaw-chat-ui", comp.ui_modules[0].repo); +} diff --git a/src/installer/ui_modules.zig b/src/installer/ui_modules.zig index 8e5026f2..ab1522dd 100644 --- a/src/installer/ui_modules.zig +++ b/src/installer/ui_modules.zig @@ -10,6 +10,7 @@ const fs_compat = @import("../fs_compat.zig"); pub const UiModuleError = error{ ExtractionFailed, AssetNotFound, + StagingIncomplete, }; fn findUiModuleArchiveAsset( @@ -82,6 +83,117 @@ pub fn isModuleInstalled(dest_dir: []const u8) bool { return true; } +/// True when **any** installed version of `module_name` under `ui_dir` has a +/// `module.js` entrypoint. +/// +/// Startup runs two passes over the same components (local build, then release +/// download); this is the skip predicate that keeps the second pass from +/// rebuilding or re-downloading a module the first pass already produced. +pub fn isModuleInstalledAnywhere( + allocator: std.mem.Allocator, + ui_dir: []const u8, + module_name: []const u8, +) bool { + var dir = std_compat.fs.openDirAbsolute(ui_dir, .{ .iterate = true }) catch return false; + defer dir.close(); + + var it = dir.iterate(); + while (it.next() catch null) |entry| { + if (entry.kind != .directory) continue; + const at = std.mem.indexOfScalar(u8, entry.name, '@') orelse continue; + if (!std.mem.eql(u8, entry.name[0..at], module_name)) continue; + + const child = std.fs.path.join(allocator, &.{ ui_dir, entry.name }) catch continue; + defer allocator.free(child); + if (isModuleInstalled(child)) return true; + } + return false; +} + +/// True when **any** installed version of `module_name` under `{root}/ui` has +/// a `module.js` entrypoint. +/// +/// This is the skip predicate that keeps the two startup passes — local build +/// (`syncLocalUiModules`) and release download (`syncMissingUiModules`) — from +/// building or fetching the same module twice. Lives here rather than in the +/// orchestrator so it is reachable from the wired unit-test root (#88). +pub fn isInstalledUnderRoot(allocator: std.mem.Allocator, root: []const u8, module_name: []const u8) bool { + const ui_dir = std.fs.path.join(allocator, &.{ root, "ui" }) catch return false; + defer allocator.free(ui_dir); + return isModuleInstalledAnywhere(allocator, ui_dir, module_name); +} + +/// Clear and return the staging directory for `dest_dir`. +/// +/// Deliberately touches **only** staging: the live `dest_dir` is never removed +/// here, so a build or download that fails afterwards cannot take down a +/// working module. (The previous implementation deleted `dest_dir` first.) +pub fn prepareStaging(allocator: std.mem.Allocator, dest_dir: []const u8) ![]u8 { + const staging = try stagingPathFor(allocator, dest_dir); + errdefer allocator.free(staging); + + std_compat.fs.deleteTreeAbsolute(staging) catch |err| switch (err) { + error.FileNotFound => {}, + else => return err, + }; + return staging; +} + +/// `{ui_dir}/.{basename}{suffix}` — a dot-prefixed sibling of `dest_dir`. +/// +/// Dot-prefixing matters: version enumeration and uninstall both split the +/// directory name at `@` and compare the prefix to the module name, so +/// `.nullclaw-chat-ui@latest.staging` is invisible to them while +/// `nullclaw-chat-ui@latest.staging` would be mistaken for a version. +fn siblingPath(allocator: std.mem.Allocator, dest_dir: []const u8, suffix: []const u8) ![]u8 { + const parent = std.fs.path.dirname(dest_dir) orelse return error.FileNotFound; + const base = std.fs.path.basename(dest_dir); + return std.fmt.allocPrint(allocator, "{s}/.{s}{s}", .{ parent, base, suffix }); +} + +/// Staging directory used while building `dest_dir`. +pub fn stagingPathFor(allocator: std.mem.Allocator, dest_dir: []const u8) ![]u8 { + return siblingPath(allocator, dest_dir, ".staging"); +} + +/// Promote a fully built `staging_dir` over `dest_dir`. +/// +/// Refuses to touch `dest_dir` unless staging contains `module.js`, so a failed +/// or partial build/download can never destroy an already working module. The +/// old directory is moved aside first and restored if the rename fails. +pub fn promoteStagedModule(allocator: std.mem.Allocator, staging_dir: []const u8, dest_dir: []const u8) !void { + if (!isModuleInstalled(staging_dir)) return error.StagingIncomplete; + + if (std.fs.path.dirname(dest_dir)) |parent| { + fs_compat.makePath(parent) catch |err| switch (err) { + error.PathAlreadyExists => {}, + else => return err, + }; + } + + const previous = try siblingPath(allocator, dest_dir, ".previous"); + defer allocator.free(previous); + std_compat.fs.deleteTreeAbsolute(previous) catch |err| switch (err) { + error.FileNotFound => {}, + else => return err, + }; + + const dest_existed = blk: { + var existing = std_compat.fs.openDirAbsolute(dest_dir, .{}) catch break :blk false; + existing.close(); + break :blk true; + }; + if (dest_existed) try std_compat.fs.renameAbsolute(dest_dir, previous); + + std_compat.fs.renameAbsolute(staging_dir, dest_dir) catch |err| { + // Put the working module back before surfacing the failure. + if (dest_existed) std_compat.fs.renameAbsolute(previous, dest_dir) catch {}; + return err; + }; + + if (dest_existed) std_compat.fs.deleteTreeAbsolute(previous) catch {}; +} + fn resolveExtractedModuleRoot(allocator: std.mem.Allocator, extract_dir: []const u8) ![]const u8 { var dir = try std_compat.fs.openDirAbsolute(extract_dir, .{ .iterate = true }); defer dir.close(); @@ -117,7 +229,9 @@ fn installExtractedUiModule(allocator: std.mem.Allocator, extract_dir: []const u /// 1. Resolves GitHub release metadata for `version`. /// 2. Selects a compatible `.tar.gz` release asset. /// 3. Downloads the tarball to a temporary path next to `dest_dir`. -/// 4. Extracts and normalizes the archive layout into `dest_dir`. +/// 4. Extracts and normalizes the archive layout into a staging directory. +/// 5. Promotes staging over `dest_dir` only once staging is complete, so a +/// failed download or extract never removes an already working module. pub fn downloadUiModule( allocator: std.mem.Allocator, repo: []const u8, @@ -133,19 +247,24 @@ pub fn downloadUiModule( const asset = findUiModuleArchiveAsset(allocator, release.value, module_name) orelse return error.AssetNotFound; - std_compat.fs.deleteTreeAbsolute(dest_dir) catch |err| switch (err) { + // Build in staging; `dest_dir` is only replaced after staging holds a + // complete module.js (promoteStagedModule enforces that). + const staging = try stagingPathFor(allocator, dest_dir); + defer allocator.free(staging); + + std_compat.fs.deleteTreeAbsolute(staging) catch |err| switch (err) { error.FileNotFound => {}, else => return err, }; - try fs_compat.makePath(dest_dir); + try fs_compat.makePath(staging); - const archive_path = try std.fmt.allocPrint(allocator, "{s}.download.tar.gz", .{dest_dir}); + const archive_path = try std.fmt.allocPrint(allocator, "{s}.download.tar.gz", .{staging}); defer allocator.free(archive_path); try downloader.download(allocator, asset.browser_download_url, archive_path); defer std_compat.fs.deleteFileAbsolute(archive_path) catch {}; - const extract_dir = try std.fmt.allocPrint(allocator, "{s}.extract", .{dest_dir}); + const extract_dir = try std.fmt.allocPrint(allocator, "{s}.extract", .{staging}); defer allocator.free(extract_dir); std_compat.fs.deleteTreeAbsolute(extract_dir) catch |err| switch (err) { error.FileNotFound => {}, @@ -154,7 +273,8 @@ pub fn downloadUiModule( defer std_compat.fs.deleteTreeAbsolute(extract_dir) catch {}; try extractTarGz(allocator, archive_path, extract_dir); - try installExtractedUiModule(allocator, extract_dir, dest_dir); + try installExtractedUiModule(allocator, extract_dir, staging); + try promoteStagedModule(allocator, staging, dest_dir); } // ─── Tests ─────────────────────────────────────────────────────────────────── @@ -263,8 +383,9 @@ test "installExtractedUiModule flattens single top-level archive directory" { const dest_dir = try std.fmt.allocPrint(allocator, "{s}/dest", .{tmp_dir}); defer allocator.free(dest_dir); - try std_compat.fs.makeDirAbsolute(nested_dir); - try std_compat.fs.makeDirAbsolute(dest_dir); + // `nested_dir` lives under `extract_dir`, which does not exist yet. + try fs_compat.makePath(nested_dir); + try fs_compat.makePath(dest_dir); const module_path = try std.fmt.allocPrint(allocator, "{s}/module.js", .{nested_dir}); defer allocator.free(module_path); @@ -314,3 +435,217 @@ test "isModuleInstalled returns false without module entrypoint" { test "isModuleInstalled returns false for non-existing directory" { try std.testing.expect(!isModuleInstalled("/tmp/test-nullhub-ui-nonexistent-dir-xyz")); } + +// ─── Staging / promotion tests ─────────────────────────────────────────────── + +fn testWriteFile(path: []const u8, contents: []const u8) !void { + const file = try std_compat.fs.createFileAbsolute(path, .{}); + defer file.close(); + try file.writeAll(contents); +} + +fn testReadFile(allocator: std.mem.Allocator, path: []const u8) ![]u8 { + const file = try std_compat.fs.openFileAbsolute(path, .{}); + defer file.close(); + return file.readToEndAlloc(allocator, 4096); +} + +fn testDirExists(path: []const u8) bool { + var dir = std_compat.fs.openDirAbsolute(path, .{}) catch return false; + dir.close(); + return true; +} + +test "promoteStagedModule installs a complete staging directory" { + const allocator = std.testing.allocator; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const root = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); + defer allocator.free(root); + + const dest = try std.fs.path.join(allocator, &.{ root, "mod@v1" }); + defer allocator.free(dest); + const staging = try stagingPathFor(allocator, dest); + defer allocator.free(staging); + + try fs_compat.makePath(staging); + const staged_entry = try std.fs.path.join(allocator, &.{ staging, "module.js" }); + defer allocator.free(staged_entry); + try testWriteFile(staged_entry, "new"); + + try promoteStagedModule(allocator, staging, dest); + try std.testing.expect(isModuleInstalled(dest)); + try std.testing.expect(!testDirExists(staging)); +} + +test "promoteStagedModule refuses incomplete staging and leaves dest untouched" { + const allocator = std.testing.allocator; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const root = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); + defer allocator.free(root); + + const dest = try std.fs.path.join(allocator, &.{ root, "mod@v1" }); + defer allocator.free(dest); + const staging = try stagingPathFor(allocator, dest); + defer allocator.free(staging); + + // A working install already exists… + try fs_compat.makePath(dest); + const dest_entry = try std.fs.path.join(allocator, &.{ dest, "module.js" }); + defer allocator.free(dest_entry); + try testWriteFile(dest_entry, "working"); + + // …and staging is a partial/failed build with no entrypoint. + try fs_compat.makePath(staging); + const partial = try std.fs.path.join(allocator, &.{ staging, "partial.txt" }); + defer allocator.free(partial); + try testWriteFile(partial, "incomplete"); + + try std.testing.expectError(error.StagingIncomplete, promoteStagedModule(allocator, staging, dest)); + + // The working module was not removed. + try std.testing.expect(isModuleInstalled(dest)); + const contents = try testReadFile(allocator, dest_entry); + defer allocator.free(contents); + try std.testing.expectEqualStrings("working", contents); +} + +test "promoteStagedModule replaces an existing install only on success" { + const allocator = std.testing.allocator; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const root = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); + defer allocator.free(root); + + const dest = try std.fs.path.join(allocator, &.{ root, "mod@v1" }); + defer allocator.free(dest); + const staging = try stagingPathFor(allocator, dest); + defer allocator.free(staging); + + try fs_compat.makePath(dest); + const old_entry = try std.fs.path.join(allocator, &.{ dest, "module.js" }); + defer allocator.free(old_entry); + try testWriteFile(old_entry, "old"); + + try fs_compat.makePath(staging); + const new_entry = try std.fs.path.join(allocator, &.{ staging, "module.js" }); + defer allocator.free(new_entry); + try testWriteFile(new_entry, "new"); + + try promoteStagedModule(allocator, staging, dest); + + const contents = try testReadFile(allocator, old_entry); + defer allocator.free(contents); + try std.testing.expectEqualStrings("new", contents); + + // No leftover backup directory. + const previous = try siblingPath(allocator, dest, ".previous"); + defer allocator.free(previous); + try std.testing.expect(!testDirExists(previous)); +} + +test "isModuleInstalledAnywhere matches the module name without prefix bleed" { + const allocator = std.testing.allocator; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const ui_dir = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); + defer allocator.free(ui_dir); + + for ([_][]const u8{ "moda@v1", "modb@v2" }) |entry_name| { + const dir_path = try std.fs.path.join(allocator, &.{ ui_dir, entry_name }); + defer allocator.free(dir_path); + try fs_compat.makePath(dir_path); + const entry = try std.fs.path.join(allocator, &.{ dir_path, "module.js" }); + defer allocator.free(entry); + try testWriteFile(entry, "x"); + } + + try std.testing.expect(isModuleInstalledAnywhere(allocator, ui_dir, "moda")); + try std.testing.expect(isModuleInstalledAnywhere(allocator, ui_dir, "modb")); + try std.testing.expect(!isModuleInstalledAnywhere(allocator, ui_dir, "modc")); + // "mod" is a prefix of "moda" but is a different module. + try std.testing.expect(!isModuleInstalledAnywhere(allocator, ui_dir, "mod")); + // A directory without a module.js does not count as installed. + const empty = try std.fs.path.join(allocator, &.{ ui_dir, "modd@v1" }); + defer allocator.free(empty); + try fs_compat.makePath(empty); + try std.testing.expect(!isModuleInstalledAnywhere(allocator, ui_dir, "modd")); +} + +test "stagingPathFor is dot-prefixed so version scans ignore it" { + const allocator = std.testing.allocator; + const staging = try stagingPathFor(allocator, "/tmp/ui/nullclaw-chat-ui@latest"); + defer allocator.free(staging); + try std.testing.expectEqualStrings("/tmp/ui/.nullclaw-chat-ui@latest.staging", staging); + + // The dot prefix keeps `@`-splitting enumeration from treating it as + // `{module}@{version}`: name prefix becomes ".nullclaw-chat-ui". + const slash = std.mem.lastIndexOfScalar(u8, staging, '/').?; + const base = staging[slash + 1 ..]; + try std.testing.expect(base[0] == '.'); + const at = std.mem.indexOfScalar(u8, base, '@').?; + // Enumeration splits at '@' and compares the whole prefix (dot included), + // so the staging dir does not resolve as this module. + try std.testing.expect(!std.mem.eql(u8, base[0..at], "nullclaw-chat-ui")); + try std.testing.expectEqualStrings(".nullclaw-chat-ui", base[0..at]); +} + +test "isInstalledUnderRoot finds a dev-local install under {root}/ui" { + const allocator = std.testing.allocator; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const root = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); + defer allocator.free(root); + + // Nothing installed: the release-download pass is justified. + try std.testing.expect(!isInstalledUnderRoot(allocator, root, "nullclaw-chat-ui")); + + // Seed the dev-local install the local-build pass produces. The download + // pass must then skip instead of building/downloading it a second time. + const dest = try std.fs.path.join(allocator, &.{ root, "ui", "nullclaw-chat-ui@dev-local" }); + defer allocator.free(dest); + try fs_compat.makePath(dest); + const entry = try std.fs.path.join(allocator, &.{ dest, "module.js" }); + defer allocator.free(entry); + try testWriteFile(entry, "x"); + + try std.testing.expect(isInstalledUnderRoot(allocator, root, "nullclaw-chat-ui")); + // A different module name is still not installed. + try std.testing.expect(!isInstalledUnderRoot(allocator, root, "other-ui")); +} + +test "prepareStaging never touches an installed module" { + const allocator = std.testing.allocator; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const root = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); + defer allocator.free(root); + + const dest = try std.fs.path.join(allocator, &.{ root, "ui", "nullclaw-chat-ui@dev-local" }); + defer allocator.free(dest); + try fs_compat.makePath(dest); + const entry = try std.fs.path.join(allocator, &.{ dest, "module.js" }); + defer allocator.free(entry); + try testWriteFile(entry, "working"); + + // Simulate a leftover half-written staging dir from an earlier failed + // build, then clear it. + const staging_path = try stagingPathFor(allocator, dest); + defer allocator.free(staging_path); + try fs_compat.makePath(staging_path); + const junk = try std.fs.path.join(allocator, &.{ staging_path, "junk.txt" }); + defer allocator.free(junk); + try testWriteFile(junk, "partial"); + + const staging = try prepareStaging(allocator, dest); + defer allocator.free(staging); + + // Staging is gone; the live module is untouched. + try std.testing.expectEqualStrings(staging_path, staging); + try std.testing.expect(!testDirExists(staging)); + try std.testing.expect(isModuleInstalled(dest)); + const contents = try testReadFile(allocator, entry); + defer allocator.free(contents); + try std.testing.expectEqualStrings("working", contents); +} diff --git a/src/main.zig b/src/main.zig index 679988ab..ed5d71e8 100644 --- a/src/main.zig +++ b/src/main.zig @@ -406,3 +406,15 @@ fn delayedOpenBrowser( std_compat.thread.sleep(750 * std.time.ns_per_ms); openBrowser(allocator, host, port, publisher.accessOptions()); } + +// TODO(#88): `build.zig` uses `src/main.zig` as the unit-test root, but this +// file had no test wiring at all — `zig build test` therefore executed **zero** +// tests while still reporting success. Wiring the full `root.zig` test block +// surfaces 15 pre-existing compile errors in unrelated modules (supervisor, +// discovery, downloader, managed_skills, compat/fs); those are tracked in #88. +// +// Until #88 lands, wire only the modules this change touches so its regression +// coverage actually runs rather than silently compiling to nothing. +test { + _ = @import("installer/ui_modules.zig"); +} diff --git a/src/server.zig b/src/server.zig index 84a99295..779418b7 100644 --- a/src/server.zig +++ b/src/server.zig @@ -114,6 +114,8 @@ pub const Server = struct { mission_control: mission_control_api.RuntimeStore = .{}, mission_workflow_evidence_cache: MissionWorkflowEvidenceCache = .{}, start_time: i64, + /// Background UI-module backfill started by `startUiModuleSync`. + ui_sync_thread: ?std.Thread = null, pub fn init(allocator: std.mem.Allocator, host: []const u8, port: u16, manager: *manager_mod.Manager, mutex: *std_compat.sync.Mutex) !Server { var paths = try paths_mod.Paths.init(allocator, null); @@ -128,9 +130,9 @@ pub const Server = struct { break :blk state_mod.State.init(allocator, state_path); }; - orchestrator.syncLocalUiModules(allocator, paths); - orchestrator.syncMissingUiModules(allocator, paths, state); - + // UI module sync is deliberately NOT done here: both passes can run + // `npm` and unbounded network requests, and `init` runs before + // `listener.listen()`. See startUiModuleSync(). return .{ .allocator = allocator, .host = host, @@ -160,12 +162,38 @@ pub const Server = struct { } pub fn deinit(self: *Server) void { + // The sync worker borrows `state`/`paths`, so it must finish before + // they are freed. Bounded by the curl --max-time ceiling. + if (self.ui_sync_thread) |thread| { + thread.join(); + self.ui_sync_thread = null; + } self.mission_workflow_evidence_cache.deinit(); self.state.deinit(); self.allocator.destroy(self.state); self.paths.deinit(self.allocator); } + /// Backfill declared UI modules on a background thread. + /// + /// This used to run inline in `init()`, before `listener.listen()`: a + /// stalled registry fetch could delay or prevent the hub from becoming + /// reachable at all. Called from `run()` once we are accepting connections, + /// so the worst case is a late-arriving module rather than an unavailable + /// hub. Joined in `deinit()`. + pub fn startUiModuleSync(self: *Server) void { + if (self.ui_sync_thread != null) return; + self.ui_sync_thread = std.Thread.spawn(.{}, uiModuleSyncWorker, .{self}) catch |err| { + std.log.warn("ui module sync not started: {s}", .{@errorName(err)}); + return; + }; + } + + fn uiModuleSyncWorker(self: *Server) void { + orchestrator.syncLocalUiModules(self.allocator, self.paths); + orchestrator.syncMissingUiModules(self.allocator, self.paths, self.state); + } + pub fn setAccessOptions(self: *Server, options: access.Options) void { self.access_options = options; } @@ -470,6 +498,8 @@ pub const Server = struct { defer listener.deinit(); std.debug.print("listening on http://{s}:{d}\n", .{ self.host, self.port }); + // Only now that we accept connections: best-effort UI module backfill. + self.startUiModuleSync(); var urls = access.buildAccessUrlsWithOptions(self.allocator, self.host, self.port, self.currentAccessOptions()) catch null; defer if (urls) |*u| u.deinit(self.allocator); if (urls) |u| { diff --git a/src/test_helpers.zig b/src/test_helpers.zig index 0b06a633..c7f6a779 100644 --- a/src/test_helpers.zig +++ b/src/test_helpers.zig @@ -9,7 +9,8 @@ pub const TempPaths = struct { paths: paths_mod.Paths, pub fn init(allocator: std.mem.Allocator) !TempPaths { - const tmp = std.testing.tmpDir(.{}); + // `cleanup(self: *TmpDir)` needs a mutable receiver. + var tmp = std.testing.tmpDir(.{}); errdefer tmp.cleanup(); const root = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); From 3cff80ccd2b36e4a9f8b616022281a14a3d536db Mon Sep 17 00:00:00 2001 From: Vernon Stinebaker Date: Sun, 4 Oct 2026 15:58:06 +0800 Subject: [PATCH 3/4] test(ui_modules): use tmpDir instead of hardcoded /tmp paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/installer/ui_modules.zig | 39 +++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/installer/ui_modules.zig b/src/installer/ui_modules.zig index ab1522dd..6f309e2e 100644 --- a/src/installer/ui_modules.zig +++ b/src/installer/ui_modules.zig @@ -322,10 +322,11 @@ test "findUiModuleArchiveAsset does not match a different module prefix" { test "extractTarGz creates dest_dir and extracts contents" { const allocator = std.testing.allocator; - const tmp_dir = "/tmp/test-nullhub-ui-extract"; - std_compat.fs.deleteTreeAbsolute(tmp_dir) catch {}; - try std_compat.fs.makeDirAbsolute(tmp_dir); - defer std_compat.fs.deleteTreeAbsolute(tmp_dir) catch {}; + // A per-test scratch dir: hardcoded /tmp paths do not exist on Windows. + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const tmp_dir = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); + defer allocator.free(tmp_dir); // Create a test file to put in the tarball. const src_dir = try std.fmt.allocPrint(allocator, "{s}/src", .{tmp_dir}); @@ -371,10 +372,10 @@ test "extractTarGz creates dest_dir and extracts contents" { test "installExtractedUiModule flattens single top-level archive directory" { const allocator = std.testing.allocator; - const tmp_dir = "/tmp/test-nullhub-ui-install-extracted"; - std_compat.fs.deleteTreeAbsolute(tmp_dir) catch {}; - try std_compat.fs.makeDirAbsolute(tmp_dir); - defer std_compat.fs.deleteTreeAbsolute(tmp_dir) catch {}; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const tmp_dir = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); + defer allocator.free(tmp_dir); const extract_dir = try std.fmt.allocPrint(allocator, "{s}/extract", .{tmp_dir}); defer allocator.free(extract_dir); @@ -407,13 +408,14 @@ test "installExtractedUiModule flattens single top-level archive directory" { } test "isModuleInstalled returns true when module entrypoint exists" { - const tmp_dir = "/tmp/test-nullhub-ui-installed"; - std_compat.fs.deleteTreeAbsolute(tmp_dir) catch {}; - try std_compat.fs.makeDirAbsolute(tmp_dir); - defer std_compat.fs.deleteTreeAbsolute(tmp_dir) catch {}; + const allocator = std.testing.allocator; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const tmp_dir = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); + defer allocator.free(tmp_dir); - const module_path = try std.fmt.allocPrint(std.testing.allocator, "{s}/module.js", .{tmp_dir}); - defer std.testing.allocator.free(module_path); + const module_path = try std.fmt.allocPrint(allocator, "{s}/module.js", .{tmp_dir}); + defer allocator.free(module_path); { var file = try std_compat.fs.createFileAbsolute(module_path, .{}); defer file.close(); @@ -424,10 +426,11 @@ test "isModuleInstalled returns true when module entrypoint exists" { } test "isModuleInstalled returns false without module entrypoint" { - const tmp_dir = "/tmp/test-nullhub-ui-installed-no-entrypoint"; - std_compat.fs.deleteTreeAbsolute(tmp_dir) catch {}; - try std_compat.fs.makeDirAbsolute(tmp_dir); - defer std_compat.fs.deleteTreeAbsolute(tmp_dir) catch {}; + const allocator = std.testing.allocator; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const tmp_dir = try std_compat.fs.Dir.wrap(tmp.dir).realpathAlloc(allocator, "."); + defer allocator.free(tmp_dir); try std.testing.expect(!isModuleInstalled(tmp_dir)); } From 9b1f95a702a52b62e0d2ea6ab584514e4c1cf483 Mon Sep 17 00:00:00 2001 From: Vernon Stinebaker Date: Sun, 4 Oct 2026 16:03:11 +0800 Subject: [PATCH 4/4] test(ui_modules): skip extractTarGz fixture when tar is unusable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/installer/ui_modules.zig | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/installer/ui_modules.zig b/src/installer/ui_modules.zig index 6f309e2e..1000d505 100644 --- a/src/installer/ui_modules.zig +++ b/src/installer/ui_modules.zig @@ -345,13 +345,25 @@ test "extractTarGz creates dest_dir and extracts contents" { const archive_path = try std.fmt.allocPrint(allocator, "{s}/test-bundle.tar.gz", .{tmp_dir}); defer allocator.free(archive_path); + // Building the fixture needs a working `tar`. Its CLI differs across + // environments (GNU tar vs the bsdtar shipped with Windows), so an + // unusable archive tool is a skip rather than a failure — otherwise the + // extraction assertion below would fail on the fixture, not the code. const tar_result = std_compat.process.Child.run(.{ .allocator = allocator, .argv = &.{ "tar", "-czf", archive_path, "-C", src_dir, "." }, - }) catch return; + }) catch return error.SkipZigTest; defer allocator.free(tar_result.stdout); defer allocator.free(tar_result.stderr); + switch (tar_result.term) { + .exited => |code| if (code != 0) { + std.debug.print("tar -czf unavailable here (exit {d}): {s}\n", .{ code, tar_result.stderr }); + return error.SkipZigTest; + }, + else => return error.SkipZigTest, + } + // Extract to a new directory. const dest_dir = try std.fmt.allocPrint(allocator, "{s}/extracted", .{tmp_dir}); defer allocator.free(dest_dir);