fix: load packaged native addons from the isolated session - #75
Merged
paulcam206 merged 5 commits intoSep 19, 2026
Merged
paulcam206 merged 5 commits into
paulcam206 merged 5 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P1
|
Codex review: blocked before merge. Reviewed September 18, 2026, 8:51 PM ET / September 19, 2026, 00:51 UTC (Revision 4). ClawSweeper reviewWhat this changesSetup copies native dependency packages into the isolated agent’s profile and redirects foreground commands, shells, and the gateway to those copies while preserving the agent’s Node.js options. Merge readiness⛔ Blocked before merge - 3 items remain This remains a useful fix absent from current main. The three latest review findings are addressed; no remaining blocking code defect was established. Installed upgrade and architecture validation remain incomplete. Priority: P1 Review scores
Verification
How this fits togetherThe Windows launcher starts OpenClaw inside an isolated agent session. Setup prepares native dependencies, and Node.js resolution redirects their loads from the installed MSIX into the agent-owned copies. flowchart TD
A[Installed MSIX dependencies] --> B[Setup scans native packages]
B --> C[Agent profile copies]
C --> D[Recorded staging root]
D --> E[Guest launch environment and lease]
E --> F[Node module resolution]
F --> G[Foreground commands and gateway]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain narrowly scoped agent-owned staging, with demonstrated fresh and state-preserving upgraded operation on both supported architectures. Do we have a high-confidence way to reproduce the issue? Yes: the reported installed-session gateway/TUI path and direct loader error identify a concrete failure, corroborated by the existing helper-staging constraint. This read-only review did not execute Windows reproduction. Is this the best way to solve the issue? Yes: scanning and redirecting native-bearing packages fits the existing nested payload and agent-owned runtime model; installed upgrade validation is still needed to establish readiness. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against 26e22e4657a2. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
clawsweeper
Bot
added
the
proof: 📸 screenshot
|
@anna-dingler FYI I hit this issue that you're fixing when updating the plugin UI for the latest POR changes for gateway isolation |
This was referenced Sep 18, 2026
Closed
The isolated-session agent identity may read package content but may not map it as an executable image. Loading any packaged `.node` therefore fails with ERR_DLOPEN_FAILED / access denied, even though the same bytes load from a writable location and the file is present and intact. This surfaced as an unrelated-looking message. Koffi's loader swallows the real error in a bare catch and reports only "Cannot find the native Koffi module; did you bundle it correctly?", which OpenClaw then wraps in disk-space and XDG_CACHE_HOME advice. The package was never at fault. The limitation is long-standing, and `SessionHelperStager` already documents it for this package's own helper executable. What changed is reachability: the pinned upstream revision moved Windows private-directory and process-start handling onto koffi, putting a native load on the path that opens the shared state database. `setup` still worked because it touches only JSON, while `tui`, `doctor`, and `onboard` failed. Verified on the affected build: all eight win32-x64 addons fail identically, so koffi was simply the first to be called unguarded. Setup now mirrors the dependency packages that carry native artifacts into the agent's own LocalState and redirects resolution to them. Whole owning package directories are copied, because a package locates its sibling libraries and helper executables relative to its own directory; that is what keeps sqlite-vec's vec0.dll and node-pty's OpenConsole.exe correct without intercepting each one. A packaged preload redirects both CommonJS and ESM resolution, delivered through NODE_OPTIONS so the Node.js workers OpenClaw starts inherit it, and gated on the staged file existing. The staged set is discovered by scanning `app\node_modules`, never hard-coded, so a future upstream revision that adds a native dependency is staged automatically rather than failing once something calls it. Application code still executes from the immutable package: only ~55 MB of 1,074 native-bearing files are mirrored, out of 426 MB and 36,463 files. The copies live in the agent profile that already holds the extracted Node.js runtime and is written and read by the identity that executes them, not in the guest-writable shared workspace, so the trust boundary is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 24c979ef-3fc7-4ec2-8fef-451bf67263d6
Addresses three defects found in review of the staging change. Inherited NODE_OPTIONS was replaced rather than extended. Both production callers left the existing value unread, so an agent configured with options such as --max-old-space-size lost them the first time setup staged native packages. Foreground and gateway launches now pass the inherited value through, and an agent-launch test asserts the redirect is appended to it. Reclaiming a superseded staged root deleted it recursively while a gateway or foreground process could still be loading from it. Ordinary setup reuses a running session, and a recursive delete removes the unlocked files before it fails on the first mapped image, so catching the failure could not undo the damage: measured locally, a sibling JavaScript file was already gone once the exception surfaced. Each root is now renamed before deletion. Windows refuses to rename a directory holding an open file, so a root still in use fails the rename intact and is reclaimed by a later setup instead. Staged content was identified by path, length, and write time. That does not distinguish revisions: an npm package republished at the same length carries its tarball's timestamps, and a Developer Mode layout is rewritten in place, so changed executable content could be served from an identifier already on disk. The identifier now covers each file's bytes. Hashing the staged set costs one pass over roughly 55 MB during setup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 24c979ef-3fc7-4ec2-8fef-451bf67263d6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 24c979ef-3fc7-4ec2-8fef-451bf67263d6
Host-supplied launch environment values are assigned over the agent account's own, so composing NODE_OPTIONS on the host replaced whatever the agent had set and could push a host-only preload path into the agent's Node.js processes. The launcher now names the preload option in the launch request and the guest appends it to the agent's own value, the same way PathPrefix already works for PATH. Reclaiming a superseded native root treated a successful rename as proof that nothing was using it. A consumer need not hold any staged file open: an agent shell sitting at a prompt carries the redirect in its environment and resolves through that root on every later OpenClaw run. Each launch now holds the root's marker open for its whole lifetime, so the rename is a real lifetime check, and reclamation also runs on the unchanged-content reuse path so releasing a consumer actually frees the root it was holding. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The gateway launch is a second guest boundary and had no coverage for the named native redirect, so a regression there would only surface as native addons failing to load in a supervised session. Assert the delivered launch request names the preload and the staged root without setting NODE_OPTIONS, that a session with no staged natives names neither, and that both fields survive the launch protocol round trip. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
paulcam206
force-pushed
the
fix/stage-native-modules-for-isolated-session
branch
from
September 19, 2026 00:47
a6a849b to
32df93e
Compare
Collaborator
|
Thanks — all three findings are addressed, and the branch is rebased onto [P1] Append the preload to the guest's
|
clawsweeper
Bot
added
status: 👀 ready for maintainer look
paulcam206
marked this pull request as ready for review
September 19, 2026 01:05
Collaborator
|
I don't love it, but we don't have another workaround at the moment |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix: load packaged native addons from the isolated session This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
openclaw tui,openclaw doctor, andopenclaw onboard --classicfail inside the isolated session with:The package is not at fault. The file is present and intact — bypassing koffi's loader with
process.dlopengives the real error:The isolated-session agent identity may read packaged content but may not map it as an executable image. Copying the same bytes to a writable location loads fine (
OK 3.1.6).The message misleads twice over: koffi's
loadStaticswallows the real error in a barecatch {}, and OpenClaw then wraps the result in disk-space /XDG_CACHE_HOMEadvice that has nothing to do with the cause.Why now
The limitation is long-standing, and
SessionHelperStageralready documents it for this package's own helper executable. What changed is reachability:koffi@3.1.6is pinned identically at the old and new upstream revisions, but the new revision moved Windows private-directory and process-start handling onto koffi, putting a native load on the path that opens the shared state database. #15 removed the payload staging that had been masking it, and #36's pin bump made it reachable.clawctl setupstill succeeded because it touches only JSON.Measured on the affected build, all eight win32-x64 addons fail identically — pi-tui, node-pty (×2), fs-safe, cua-driver (×2), koffi, tree-sitter-bash. koffi was simply the first called without a guard.
sqlite-vec'svec0.dllis in the same state.Fix
Setup mirrors the dependency packages carrying native artifacts into the agent's own LocalState and redirects resolution to them.
vec0.dlland node-pty'sOpenConsole.execorrect without intercepting each loading mechanism separately (there are three: CJSdlopen, ESMimport.meta.resolve, and derived sibling paths).NODE_OPTIONSrather than argv so the Node.js workers OpenClaw spawns inherit it, and gated on the staged file existing. The inheritedNODE_OPTIONSis preserved and the redirect appended.app\node_modules, never hard-coded, so a future upstream revision that adds a native dependency is staged automatically instead of failing when something first calls it.Trust boundary
Application code still executes from the immutable package; this is deliberately not a payload stager. Only ~55 MB / 1,074 native-bearing files are mirrored, out of 426 MB / 36,463. The copies live in the agent profile that already holds the extracted Node.js runtime and is written and read by the identity that executes them — not the guest-writable shared workspace.
README.md,CONTRIBUTING.md, and.github/copilot-instructions.mdare updated, since the existing "do not copy the payload" guidance needed to name this exception.Evidence: installed package on a test VM
A test-signed x64 MSIX built from this branch was installed on a clean VM. The commands that previously died on the Koffi load now work.
clawctl gateway-service status— the gateway starts and listens, which is the path that opens the shared state database through koffi:openclaw tui— previously the hard failure, since it loads koffi unguarded. It now connects to the session and round-trips a message:The Control UI served by that gateway, reached from the browser:
Scope of this evidence, stated precisely: the VM build is commit
9696a50, which carries the staging fix but not the three review fixes that follow it. It therefore demonstrates that staging plus redirection resolves the reported failure on an installed package; it does not exercise inheritedNODE_OPTIONS, in-use root reclamation, or byte-based content identity, which are covered by unit tests.Known gaps
Stated plainly, because several are not closed by this PR:
agentNativeRootfield behaves upgrading from a build that never wrote it is untested.process.dlopena path it built by string concatenation bypasses it. No such call is known in the current payload, but this is not enforced.app\node_modulesnode_modulesonly. Verified that all 19 native entries in the current package live there, so there is no blind spot at this revision — but a future payload that ships one elsewhere would regress silently.node\native-redirect.mjsis actually present in the composed MSIX. Given that metadata files are a release trust boundary here, that check is probably worth adding.Module._resolveFilenameValidation
Local
dotnet restorecannot reach the NuGet feed from my sandbox, so build and test coverage is CI's.Before the VM run, the redirect was verified against the real MSIX in isolation: two koffi trees extracted from the artifact, the "package" copy's
koffi.nodedeliberately corrupted to 7 bytes. Without the preload both CJS and ESM reproduce the exact user-facing error; with it,CJS OK 3.1.6/ESM OK 3.1.6.The partial-delete hazard behind the reclamation fix was reproduced directly: with a read handle open on one file,
Directory.Delete(recursive: true)removed a sibling file before throwing, whileDirectory.MovethrewIOExceptionand left the tree intact.