Skip to content

fix: isolate runtime install lifecycle state - #131

Open
MertBasar0 wants to merge 9 commits into
openclaw:mainfrom
MertBasar0:codex/ocm-98-runtime-install-repro
Open

fix: isolate runtime install lifecycle state#131
MertBasar0 wants to merge 9 commits into
openclaw:mainfrom
MertBasar0:codex/ocm-98-runtime-install-repro

Conversation

@MertBasar0

@MertBasar0 MertBasar0 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • isolate npm package lifecycle scripts from caller-owned OpenClaw state during runtime installation
  • use a disposable install-scoped home, state directory, and config path for both host npm and managed Node/npm
  • remove active OCM environment and OpenClaw profile context from lifecycle children
  • apply the same boundary to local companion package installation and remove the disposable state after npm exits

Fixes #98.

Relation to the requested reproduction

This directly answers Peter's follow-up on #98. PR #110 had already proved that known external plugin records are rejected safely, but it did not identify the original schema-migration path. Peter therefore asked for a credential-free reproduction that preserved the relevant config path fields, plugin install records, symlink targets, environment-variable names, and exact commands while omitting credentials and database contents.

The reproduction added here keeps those requested structural inputs and compares the same published OpenClaw beta installation across OCM v0.2.32, the fixed current build, and an explicit-path control. It identifies ocm runtime install's inherited npm lifecycle environment as the first write boundary: the legacy lane resolves the caller's default OpenClaw state and migrates its database, while the fixed lane leaves the source state and config unchanged.

Root cause

ocm runtime install launched npm with the caller's environment. When an OpenClaw package postinstall resolved its default paths, inherited HOME led it back to $HOME/.openclaw/state/openclaw.sqlite. Installing [email protected] could therefore migrate the source database from schema 7 to 8 before the runtime was recorded.

PR #110 correctly contains known external plugin records during adoption, but it does not own this runtime-package child-process boundary.

Fix

The runtime package installer now gives npm lifecycle scripts a disposable root under the staged install:

  • HOME and USERPROFILE
  • OPENCLAW_HOME
  • OPENCLAW_STATE_DIR
  • OPENCLAW_CONFIG_PATH

It also removes OCM_ACTIVE_ENV, OCM_ACTIVE_ENV_ROOT, and OPENCLAW_PROFILE. The same helper covers direct npm, managed Node/npm, local builds that use the package installer, and local companion installs.

Regression proof

The integration test runs the official runtime-install flow with both host npm and managed Node/npm. Its fake lifecycle script uses OpenClaw's path-resolution order and attempts to mutate caller-owned state/config.

  • pre-fix: run 33440512090 failed because the host-npm lifecycle changed the caller state file
  • fixed head: run 33441331742 passed the same two-path regression test

The credential-free published-package reproduction in the fixed run also proves:

Lane Schema Source DB Source config
OCM v0.2.32 7 -> 8 changed unchanged
fixed current 7 -> 7 unchanged unchanged
explicit-path control 7 -> 7 unchanged unchanged

The fixture preserves the requested redacted config path fields, plugins.installs records, symlink target, environment-variable names, and exact commands without uploading credentials or database contents.

Validation

  • cargo fmt --check
  • cargo check --workspace --all-targets --locked
  • focused host-npm + managed-Node regression
  • cargo test --locked
  • current and v0.2.32 release builds
  • credential-free clean-room reproduction against the published beta packages

All passed in run 33441331742.

Scope

  • production delta: +42 / -2 lines, establishing and cleaning the shared lifecycle isolation boundary
  • no configuration, schema, persistent-store, protocol, or compatibility surface changes
  • no real gateway, user config, credentials, or user database used

Agent Transcript

Redacted Codex session transcript

This is a scoped, sanitized record of the work that produced this PR. System/developer prompts, internal reasoning, raw tool output, credentials, authentication details, local user paths, and unrelated conversation were omitted.

User: Asked whether an OCM release-validation isolation failure matched an earlier report.

Assistant: Matched the incident to OCM issue #98: an adopted disposable environment's beta runtime installation migrated the caller-owned OpenClaw state database from schema 7 to 8.

User: Shared Patrick's explicit approval to investigate and open an OCM PR.

Assistant: Rechecked issue #98 and Peter's follow-up. PR #110 covered known external plugin records, but Peter still needed a credential-free reproduction preserving config path shapes, plugin install records, symlink targets, environment-variable names, and exact commands without credentials or database contents.

Assistant: Reconstructed the original command order and narrowed the first source-state write to ocm runtime install, before the candidate gateway was started. To avoid touching real gateway state, moved the proof to a synthetic fixture and secrets-free GitHub Actions workflow.

Assistant: Built a three-lane reproduction using the published OpenClaw beta package. OCM v0.2.32 changed the synthetic source database from schema 7 to 8; an explicit-path control left it unchanged. This identified npm lifecycle path resolution through the inherited caller environment as the owner boundary.

User: Approved implementing the OCM repair and regression test.

Assistant: Added a production-path regression covering both host npm and managed Node/npm. Before the fix, the test failed because the lifecycle probe changed caller-owned OpenClaw state.

Assistant: Updated runtime installation so npm lifecycle children receive disposable OpenClaw home, state, and config paths, while active OCM/OpenClaw profile context is removed. Applied the same boundary to companion-package installation and cleaned the disposable state after npm exits.

Assistant: Reran the focused regression, full Rust test suite, formatting/checks, release builds, and credential-free clean-room reproduction. The fixed lane remained schema 7 with source database and config unchanged. Opened PR #131 with the red/green runs and evidence matrix.

Assistant: After PR creation, ClawSweeper accepted the supplied behavior proof and raised a separate compatibility decision: replacing npm's home also hides user-level .npmrc transport configuration. That choice was left visible for maintainer decision rather than silently assumed.

@clawsweeper

clawsweeper Bot commented Aug 31, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 31, 2026
@clawsweeper

clawsweeper Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed September 2, 2026, 10:47 AM ET / 14:47 UTC.

ClawSweeper review

What this changes

This PR gives npm runtime-install lifecycle work a disposable OpenClaw home and state path so installing a runtime cannot mutate the caller’s OpenClaw database or config.

Regression provenance

Possible regression — probable (reproduction; reviewed change). No predecessor PR is attributed.

Merge readiness

⚠️ Needs maintainer review before merge - 5 items remain

Keep this PR open: it demonstrates the caller-state isolation fix, but the prior P1 compatibility blocker remains because npm itself now runs with a replacement home directory and cannot discover an operator’s user-level npm transport configuration.

Priority: P1
Reviewed head: 402add405cbdb9e20662e504c3fe3f2781062e4e
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The state-isolation proof is strong, but the unresolved npm transport-configuration contract remains a merge-blocking compatibility concern.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (logs): The changed production owner is the runtime npm installer, exercised by the added host-npm and managed-Node production-path regression; the PR's fixed-head clean-room evidence records that the lifecycle installation leaves the source database at schema 7 and the source config unchanged after installation.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (logs): The changed production owner is the runtime npm installer, exercised by the added host-npm and managed-Node production-path regression; the PR's fixed-head clean-room evidence records that the lifecycle installation leaves the source database at schema 7 and the source config unchanged after installation.
Evidence reviewed 6 items Introduced isolation boundary: The PR changes HOME, USERPROFILE, and the OpenClaw path variables for the npm command, while clearing active environment/profile selectors; this directly prevents lifecycle children from resolving the caller's OpenClaw state.
Regression coverage: The added integration test exercises both host npm and managed Node/npm, runs a lifecycle probe, and verifies the caller state file and config remain unchanged.
Supplied real behavior proof: The PR body records a clean-room published-package comparison: the legacy lane changes schema 7 to 8, while the fixed lane and explicit-path control leave the source database and config unchanged in GitHub Actions run 33441331742.
Findings 1 actionable finding [P1] Preserve npm configuration needed for dependency resolution
Security None None.

How this fits together

OCM installs OpenClaw runtimes through npm, whose package lifecycle scripts inherit the install process environment. Those scripts can otherwise resolve caller-owned OpenClaw paths before OCM records the installed runtime.

flowchart LR
  A[OCM runtime install] --> B[npm install process]
  B --> C[Isolated lifecycle environment]
  C --> D[Package lifecycle script]
  D --> E[Disposable OpenClaw state]
  B --> F[Managed runtime files]
Loading

Decision needed

Question Recommendation
What npm transport configuration, if any, may OCM make available while lifecycle scripts run with isolated OpenClaw state? Pause for an explicit contract: Keep strict isolation unmerged until maintainers define a supported, non-secret transport configuration path and its upgrade behavior.

Why: Preserving a user-level npm configuration can restore private-registry compatibility but can also expose credentials to package lifecycle code; strict isolation avoids that exposure but changes existing operator behavior.

Before merge

  • Preserve npm configuration needed for dependency resolution (P1) - The new HOME is applied to npm itself, not only lifecycle scripts. npm therefore no longer discovers an operator's user-level .npmrc, so private registries, proxies, custom CAs, or authentication configured there can make an existing runtime install fail. This is the still-unfixed P1 from the prior review; define and test the supported transport-config boundary before merge.
  • Resolve merge risk (P1) - Existing installations that rely on a home-level .npmrc for a private registry, proxy, custom CA, or authentication may fail to resolve runtime dependencies after this change.
  • Resolve merge risk (P1) - The regression test proves caller OpenClaw state isolation but does not establish the supported transport-configuration contract for npm.
  • Complete next step (P2) - Obtain maintainer direction on the supported npm transport-configuration contract, then add focused compatibility coverage for that contract.

Findings

  • [P1] Preserve npm configuration needed for dependency resolution — src/store/runtimes.rs:934-935
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch composition production +42/-2, test/reproduction +583 Most of the 625 added lines are regression and clean-room reproduction material around a small production isolation boundary.

Merge-risk options

Maintainer options:

  1. Pause for the transport contract (recommended)
    Do not merge until maintainers choose how private registries, proxies, custom CAs, and authentication are supported without reopening lifecycle access to caller state.
  2. Implement an explicit safe configuration path
    Add focused coverage for the approved npm transport path and prove the lifecycle process still cannot access caller-owned OpenClaw files or credentials.
  3. Accept the strict isolation tradeoff
    Merge only with a maintainer-approved declaration that user-level npm configuration is intentionally unsupported for runtime installs.

Technical review

Best possible solution:

Retain caller-state isolation, then define and test an explicit npm transport-configuration boundary that does not expose caller-owned OpenClaw data or credentials to package lifecycle code.

Do we have a high-confidence way to reproduce the issue?

Yes—source-reproducible with high confidence: the supplied clean-room run demonstrates the legacy mutation and the added production-path regression covers host and managed npm, while current main still lacks this isolation boundary.

Is this the best way to solve the issue?

No—the state-isolation mechanism is appropriate, but applying a replacement HOME to npm itself leaves an unresolved compatibility and credential-exposure contract for user-level npm transport configuration.

Full review comments:

  • [P1] Preserve npm configuration needed for dependency resolution — src/store/runtimes.rs:934-935
    The new HOME is applied to npm itself, not only lifecycle scripts. npm therefore no longer discovers an operator's user-level .npmrc, so private registries, proxies, custom CAs, or authentication configured there can make an existing runtime install fail. This is the still-unfixed P1 from the prior review; define and test the supported transport-config boundary before merge.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against e04c10166d6b.

Labels

Label justifications:

  • P1: Runtime installation can regress immediately for operators whose dependency transport relies on home-level npm configuration.
  • merge-risk: 🚨 compatibility: Replacing npm's HOME changes its discovery of existing user-level registry, proxy, CA, and authentication settings.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (logs): The changed production owner is the runtime npm installer, exercised by the added host-npm and managed-Node production-path regression; the PR's fixed-head clean-room evidence records that the lifecycle installation leaves the source database at schema 7 and the source config unchanged after installation.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the runtime npm installer, exercised by the added host-npm and managed-Node production-path regression; the PR's fixed-head clean-room evidence records that the lifecycle installation leaves the source database at schema 7 and the source config unchanged after installation.

Evidence

What I checked:

  • Introduced isolation boundary: The PR changes HOME, USERPROFILE, and the OpenClaw path variables for the npm command, while clearing active environment/profile selectors; this directly prevents lifecycle children from resolving the caller's OpenClaw state. (src/store/runtimes.rs:933, 402add405cbd)
  • Regression coverage: The added integration test exercises both host npm and managed Node/npm, runs a lifecycle probe, and verifies the caller state file and config remain unchanged. (tests/runtime_command_tests.rs:2102, 402add405cbd)
  • Supplied real behavior proof: The PR body records a clean-room published-package comparison: the legacy lane changes schema 7 to 8, while the fixed lane and explicit-path control leave the source database and config unchanged in GitHub Actions run 33441331742. (402add405cbd)
  • Unresolved npm configuration contract: The author explicitly notes that replacing HOME prevents discovery of a user-level .npmrc, which can carry private-registry, proxy, CA, or authentication configuration, and asks for maintainer direction. (src/store/runtimes.rs:934, 402add405cbd)
  • Current-main check: The PR head is not an ancestor of fetched current main, so the runtime lifecycle isolation helper is not already implemented on current main. (src/store/runtimes.rs:954, e04c10166d6b)
  • Feature-history routing: Runtime npm lifecycle support was previously changed by Shakker in the same production and regression-test paths. (src/store/runtimes.rs, 873c7b2ee68b)

Likely related people:

  • Shakker: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Hannes Rudolph: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Jason (Json): Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Obtain maintainer direction on the supported npm transport-configuration contract.
  • Add focused coverage for the selected registry, proxy, CA, or authentication path while proving lifecycle code cannot access caller-owned state or credentials.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-08-31T21:46:38.547Z sha 402add4 :: found issues before merge. :: [P1] Preserve npm configuration needed for dependency resolution
  • reviewed 2026-08-31T22:02:40.613Z sha 402add4 :: needs changes before merge. :: [P1] Preserve npm configuration needed for dependency resolution
  • reviewed 2026-08-31T23:21:01.059Z sha 402add4 :: found issues before merge. :: [P1] Preserve npm configuration needed for dependency resolution

@MertBasar0

Copy link
Copy Markdown
Contributor Author

@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 .npmrc. That can affect custom registries, proxies, certificates, or authenticated package sources.

Could you confirm the intended contract for this PR?

  1. Preserve supported npm transport configuration through a reviewed boundary while keeping caller-owned OpenClaw paths unavailable to lifecycle scripts; or
  2. Keep strict empty-home isolation and explicitly require/document another configuration path.

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)

@MertBasar0

Copy link
Copy Markdown
Contributor Author

@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!

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

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adopted environment upgrade mutated the source gateway state database

1 participant