fix: isolate runtime install lifecycle state - #131
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. |
|
Codex review: needs maintainer review before merge. Reviewed September 14, 2026, 6:10 AM ET / 10:10 UTC (Revision 10). ClawSweeper reviewWhat this changesRuntime and companion installation redirect OpenClaw lifecycle state into disposable directories while preserving npm settings, with regression tests and a published-package reproduction workflow. Merge readiness✅ Ready for maintainer review This PR remains necessary: current main and v0.2.46 retain the installation-state leak. The updated patch resolves the earlier npm-configuration and reproduction-lane findings, with convincing published-package proof and no remaining actionable defect found. Priority: P1 Review scores
Verification
How this fits togetherOCM prepares OpenClaw packages before publishing runtimes for managed environments. Its npm installation boundary controls which state package lifecycle scripts discover and therefore whether preparation can affect an existing gateway. flowchart TD
A[Runtime installation request] --> B[Package and companion installer]
C[Caller npm settings] --> B
B --> D[Redirect OpenClaw state paths]
D --> E[Run npm lifecycle scripts]
E --> F[Remove disposable state]
F --> G[Publish successful runtime]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep npm's existing configuration contract while confining supported OpenClaw lifecycle state discovery to disposable installation paths. Do we have a high-confidence way to reproduce the issue? Yes: current-main source exposes caller state to npm lifecycle scripts, and the published-package comparison demonstrates the resulting mutation with the legacy installer. This review did not execute a current-main reproduction. Is this the best way to solve the issue? Yes: redirecting OpenClaw's supported state selectors at both npm installation boundaries addresses the first write while preserving existing npm policy and configuration. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 3f50e7c4a9a7. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (9 earlier review cycles; latest 8 shown)
|
|
@steipete ClawSweeper accepted the supplied behavior proof but raised a P1 compatibility decision: the current isolation boundary changes npm's HOME, so npm no longer discovers an operator's home-level Could you confirm the intended contract for this PR?
I lean toward option 1, but forwarding the original user-config path wholesale could also expose authentication material to package lifecycle scripts, so I do not want to assume that security/compatibility tradeoff without maintainer direction. Once confirmed, I can align the implementation and add focused regression coverage. ClawSweeper finding: #131 (comment) |
|
@steipete A quick follow-up on the npm configuration question above: could you confirm the preferred approach for preserving supported npm transport settings while keeping caller-owned OpenClaw state isolated? I'm ready to update the implementation and add focused compatibility coverage once the direction is clear. If another maintainer owns this decision, happy to follow up with them. Thanks! |
|
@fuller-stack-dev Could you take a look at the maintainer decision requested in the ClawSweeper review? The isolation fix and regression proof are complete; the remaining question is how OCM should preserve supported npm transport configuration without exposing caller-owned OpenClaw state to lifecycle scripts. Your guidance on the intended contract would unblock the final implementation update. |
|
Corrected caller-mode real-package proof is complete on exact head
Formatting, all-target checks, the focused host/managed-npm regression, the full test suite, both release builds, the corrected published-package reproduction, and evidence upload all passed in https://github.com/MertBasar0/ocm/actions/runs/34544350861. The PR body now records the corrected command boundary and redacted result matrix. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
8848cd1 to
7f1785b
Compare
|
Fixed: runtime installation isolates OpenClaw lifecycle state while preserving the caller's npm configuration and script policy. With thanks to @MertBasar0. |
Related: #98
What Problem This Solves
Resolves a problem where preparing an OpenClaw runtime lets npm installation scripts discover and modify the caller's existing OpenClaw state before the new runtime is recorded.
Why This Change Was Made
Package and companion installation now direct OpenClaw's home, state, and config selectors into disposable installation state and clear inherited service-state and active-environment selectors. The npm process retains the caller's home, configuration, and script policy. Installation state is removed when npm returns, including a normal failure.
User Impact
Preparing a runtime preserves caller OpenClaw state through the supported lifecycle path selectors. User npm settings, including disabled scripts, explicit configuration files, and paths expanded from HOME, continue to apply.
Evidence
Validation on Crabbox with Rust 1.88.0, Node.js 24.18.1, and npm 11.16.0:
The reproduction uses the unmodified schema-6 database created by published OpenClaw
2026.8.1-beta.1and installs2026.8.1-beta.2. Database inspection is read-only, and the explicit-path control uses the legacy installer.Config bytes remained unchanged in all three cases. The hosted reproduction also passed.
All required CI jobs passed. The two optional macOS npm jobs reproduce the unchanged-main empty team-ID failure tracked by #216.