Skip to content

fix(migrate): reject external plugin paths during adoption - #110

Open
MertBasar0 wants to merge 2 commits into
openclaw:mainfrom
MertBasar0:fix/adopt-external-plugin-containment
Open

fix(migrate): reject external plugin paths during adoption#110
MertBasar0 wants to merge 2 commits into
openclaw:mainfrom
MertBasar0:fix/adopt-external-plugin-containment

Conversation

@MertBasar0

@MertBasar0 MertBasar0 commented Aug 26, 2026

Copy link
Copy Markdown

What Problem This Solves

ocm adopt import can retain absolute sourcePath or installPath values for path plugins outside the adopted environment. The import currently succeeds with a warning, so a candidate runtime can still receive a writable reference to source-owned state even though the environment is intended to be disposable.

Why This Change Was Made

Containment is a safety boundary, not a compatibility warning. The import now fails before publishing relocated plugin records when any plugin path cannot be isolated inside the target state root. The existing migration transaction then removes the partially created environment.

User Impact

Adoption no longer produces a runnable environment with external plugin references. Operators must relocate or remove external path plugins before importing. Managed ClawHub and npm plugin records continue to relocate into the adopted state as before.

Verification

Targeted regression test

$ cargo test --test migrate_command_tests adopt_import_rejects_external_plugin_paths_without_mutating_the_source -- --exact --nocapture
running 1 test
test adopt_import_rejects_external_plugin_paths_without_mutating_the_source ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 39 filtered out

Real CLI behavior with a safe temporary fixture

The fixture contains a path plugin outside the source OpenClaw home. Paths are redacted below. The source database and external plugin marker were hashed before and after the production ocm command.

$ ocm adopt import --name proof-external-plugin <fixture>/source/.openclaw --json
ocm: imported OpenClaw plugin path(s) could not be isolated inside the env state: external-dev
Run "ocm help" for usage.

exit: 1
target environment removed: yes
source sqlite unchanged: yes (6a5285e2a14ca9cf4df66734c93ef52404fa218b7a8b2fe2f912e3672ad87bc9)
external plugin unchanged: yes (09d7425c9be6e7eae3c094a5884bba879aef74adb6193f54175898965ed44393)

Additional CI-equivalent verification on commit 9695362:

  • cargo fmt --check passes.
  • cargo test --locked passes in a clean WSL-native Linux checkout.
  • cargo +1.88.0 check --workspace --all-targets --locked passes.
  • git diff --check upstream/main...HEAD passes.

Maintainer Decision Requested

Please confirm that fail-closed adoption is the intended policy when a path plugin cannot be contained. This intentionally changes the previous warning-and-continue behavior: operators must relocate or remove the external path plugin before retrying adoption.

Related: #98
Related: openclaw/openclaw#129673

@clawsweeper

clawsweeper Bot commented Aug 26, 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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 26, 2026
@clawsweeper

clawsweeper Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 25, 2026, 10:23 PM ET / August 26, 2026, 02:23 UTC.

ClawSweeper review

What this changes

Makes ocm adopt import reject and remove an adopted environment when a path-plugin record points outside its state root, with regression coverage for source immutability.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

Keep open for a maintainer policy decision: the patch is focused and proven, but intentionally replaces an established warning-and-continue adoption path with a hard failure.

Priority: P1
Reviewed head: 9695362041f5ea8ec4e84d5e4c0f3afdec9c4ba6
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The PR has strong real-command proof and a focused patch; merge readiness depends on the intentional compatibility policy.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR body includes a redacted after-fix production CLI transcript showing the expected failure, target cleanup, and unchanged source artifacts.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body includes a redacted after-fix production CLI transcript showing the expected failure, target cleanup, and unchanged source artifacts.
Evidence reviewed 6 items Current compatibility behavior: Current main collects external plugin IDs and completes import; the CLI then emits a warning that the external location was preserved.
Established contract test: Current main's adoption test expects success and retains the external plugin install path, confirming this PR changes an existing operator workflow.
Proposed containment guard: The PR returns an error whenever external plugin IDs are found, before writing relocated plugin records.
Findings None None.
Security None None.

Live Verification

Command: cargo run -- help adopt import

Result: FAIL (failed) — execution before step 1 run: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.24.0.tgz

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.24.0.tgz

Assertions:

  • FAIL expect_output: Import a plain OpenClaw home
  • FAIL expect_output: ocm adopt import --name ‹env›

How this fits together

OCM imports an existing OpenClaw home into a disposable environment before candidate runtimes use it. Plugin records are relocated during import; this change determines whether an external path is retained with a warning or aborts adoption.

flowchart LR
A[Source OpenClaw home] --> B[Adopt import]
B --> C[Plugin path relocation]
C --> D{Path contained in target state?}
D -->|Yes| E[Publish adopted environment]
D -->|No| F[Abort migration]
F --> G[Remove partial environment]
E --> H[Candidate runtime]
Loading

Decision needed

Question Recommendation
Should OCM adoption fail and remove the target environment whenever an external path plugin cannot be relocated into the adopted state root? Adopt fail-closed containment: Merge the guard so operators relocate or remove external path plugins before an adopted environment is created.

Why: Source and real-command proof establish the implementation, but cannot decide whether replacing the existing warning-and-continue workflow with an operator-blocking failure is the intended product policy.

Before merge

  • Resolve merge risk (P1) - Existing users with external path plugins will no longer be able to adopt with the current warning-and-continue behavior; they must relocate or remove those plugins first.
  • Resolve merge risk (P1) - The patch covers plugin-record paths only, while the broader isolation report at Adopted environment upgrade mutated the source gateway state database #98 describes other unresolved possible writable references.
  • Complete next step (P2) - A maintainer must choose the intentional compatibility policy before merge; no mechanical repair is currently needed.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 2 files affected; production +9, tests +8/-14 The behavioral change is tightly limited to plugin-path relocation and its migration regression test.

Merge-risk options

Maintainer options:

  1. Confirm fail-closed adoption (recommended)
    Accept the compatibility change and retain the regression coverage proving target cleanup and source immutability.
  2. Pause for a compatible remediation design
    Do not merge until maintainers choose how existing external path-plugin users should migrate without an unexpected hard failure.

Technical review

Best possible solution:

Confirm the permanent adoption policy, then merge this narrow containment guard if fail-closed behavior is accepted; otherwise design an explicit compatible remediation path before changing the default.

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

Yes, source-reproducible: current main's regression test explicitly exercises successful import with a preserved external path, while the PR body supplies an after-fix real CLI run for the opposite behavior.

Is this the best way to solve the issue?

Unclear: failing before publishing the adopted environment is a narrow containment implementation, but maintainers must decide whether the deliberate compatibility break is the best permanent policy.

AGENTS.md: not found in the target repository.

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

Labels

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes a redacted after-fix production CLI transcript showing the expected failure, target cleanup, and unchanged source artifacts.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes a redacted after-fix production CLI transcript showing the expected failure, target cleanup, and unchanged source artifacts.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P1: This changes the safety of release-validation adoption, where a disposable environment can retain references outside its intended state root.
  • merge-risk: 🚨 compatibility: The patch replaces a tested warning-and-continue adoption behavior with a hard failure for existing external path-plugin setups.
  • merge-risk: 🚨 security-boundary: It changes whether an adopted candidate can retain a writable reference to state outside its containment boundary.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes a redacted after-fix production CLI transcript showing the expected failure, target cleanup, and unchanged source artifacts.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes a redacted after-fix production CLI transcript showing the expected failure, target cleanup, and unchanged source artifacts.

Evidence

What I checked:

  • Current compatibility behavior: Current main collects external plugin IDs and completes import; the CLI then emits a warning that the external location was preserved. (src/store/openclaw_state.rs:405, cd8decaf04f2)
  • Established contract test: Current main's adoption test expects success and retains the external plugin install path, confirming this PR changes an existing operator workflow. (tests/migrate_command_tests.rs:1243, cd8decaf04f2)
  • Proposed containment guard: The PR returns an error whenever external plugin IDs are found, before writing relocated plugin records. (src/store/openclaw_state.rs:405, 9695362041f5)
  • Rollback path: A failure from runtime-state preparation removes the partially created environment through the existing migration rollback path. (src/migrate.rs:311, 9695362041f5)
  • Feature provenance: The relocation helper and warning behavior trace to the shipped v0.2.33 source snapshot; its author has also carried the migration area through subsequent history. (src/store/openclaw_state.rs:311, 7e110ae25c82)
  • After-fix real behavior proof: The PR body supplies a redacted production ocm adopt import transcript showing exit 1, removal of the target environment, and unchanged source SQLite and external-plugin artifacts. (9695362041f5)

Likely related people:

  • Shakker: Current main blames the plugin relocation implementation and warning behavior to the v0.2.33 source snapshot authored by Shakker, with extensive later migration history. (role: original migration-area contributor; confidence: high; commits: 7e110ae25c82, 332178976722; files: src/store/openclaw_state.rs, src/migrate.rs, tests/migrate_command_tests.rs)
  • Patrick-Erichsen: The contributor explicitly requested Patrick-Erichsen's confirmation of the fail-closed compatibility policy in the PR discussion. (role: likely policy decision owner; confidence: medium; files: src/store/openclaw_state.rs, tests/migrate_command_tests.rs)

Rank-up moves

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

  • Obtain explicit maintainer confirmation that fail-closed adoption is the intended default for external path-plugin setups.

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 (2 earlier review cycles)
  • reviewed 2026-08-26T01:02:52.031Z sha a4f91fc :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-26T01:40:51.892Z sha 9695362 :: needs maintainer review before merge. :: none

@MertBasar0

Copy link
Copy Markdown
Author

@clawsweeper re-review

Added the requested after-fix production-command proof from a safe temporary fixture. The targeted regression test passes; the real CLI run exits nonzero, removes the partial target environment, and leaves both the source SQLite database and external plugin marker byte-for-byte unchanged. The PR body now also calls out the remaining maintainer policy decision explicitly.

@clawsweeper

clawsweeper Bot commented Aug 26, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

@MertBasar0

Copy link
Copy Markdown
Author

@Patrick-Erichsen The requested real-command proof and CI-equivalent local verification are now in the PR body. The remaining maintainer decision is the compatibility policy: this patch intentionally fails closed when an external path plugin cannot be contained, instead of preserving the current warning-and-continue behavior. Could you confirm that fail-closed adoption is the intended policy?

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 26, 2026
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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant