Skip to content

fix(state-migrations): archive plugin install index on conflict instead of keeping it#90252

Closed
Bartok9 wants to merge 1 commit into
openclaw:mainfrom
Bartok9:fix/90213-plugin-install-index-conflict-archive
Closed

fix(state-migrations): archive plugin install index on conflict instead of keeping it#90252
Bartok9 wants to merge 1 commit into
openclaw:mainfrom
Bartok9:fix/90213-plugin-install-index-conflict-archive

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Archive the legacy plugin install JSON file even when conflicting records are detected, instead of leaving it in place.
  • Conflict warning is now emitted exactly once (on first migration) rather than on every subsequent gateway start.
  • Fix warning text to be generic (removed misleading 'SQLite state has newer metadata' wording — conflicts may arise from mismatched floating selectors, package renames, or malformed legacy specs, not necessarily newer SQLite records).

Motivation

Closes #90213.

After updating to OpenClaw 2026.6.1 (which merged the SQLite state migration in #88585 and the follow-up fix in #89281), some users saw:

[state-migrations] Legacy state migration warnings:
- Left plugin install index in place because shared SQLite state has conflicting plugin install metadata for: brave, lobster

This warning repeated on every gateway restart, every openclaw doctor --fix, and every openclaw plugins inspect run — because the legacy JSON file was left in place, causing migrateLegacyInstalledPluginIndex to re-enter the conflict path on each invocation.

Root cause (code trace)

In src/infra/state-migrations.ts, the pre-patch conflict branch:

if (merged.conflicts.length > 0) {
  return {
    changes,
    warnings: [
      `Left plugin install index in place because shared SQLite state has conflicting plugin install metadata for: ${merged.conflicts.join(", ")}`,
    ],
  };
}

This returns without calling archiveLegacyInstalledPluginIndex, leaving plugins/installs.json on disk. On the next run, the file is re-read, the same conflict is re-detected, and the same warning fires again.

Fix (code trace)

Post-patch — the conflict path now archives the file and continues:

if (merged.conflicts.length > 0) {
  warnings.push(
    `Archived plugin install index after detecting conflicting plugin install metadata for: ${merged.conflicts.join(", ")}`,
  );
  archiveLegacyInstalledPluginIndex({ sourcePath, changes, warnings });
  return { changes, warnings };
}

archiveLegacyInstalledPluginIndex renames plugins/installs.json → plugins/installs.json.migrated. On subsequent runs, the file no longer exists at sourcePath, so migrateLegacyInstalledPluginIndex exits at the early-return guard before reaching the conflict branch — no repeated warning.

Behavior proof (two-run trace)

Run 1 (fresh legacy file on disk, SQLite has conflicting records):

[state-migrations] Legacy state migration warnings:
- Archived plugin install index after detecting conflicting plugin install metadata for: brave, lobster

plugins/installs.json → renamed to plugins/installs.json.migrated

Run 2 (gateway restart / openclaw doctor --fix / openclaw plugins inspect brave):

(no state-migrations warnings)

The sourcePath no longer exists, the migration function returns before the conflict branch, and warnings are empty. This is the fix behavior verified by the new regression test in src/commands/doctor-state-migrations.test.ts (the second run produces no warnings assertion added by this PR).

Verification

  • tsc --noEmit — clean
  • New test: describe('migrateLegacyInstalledPluginIndex — conflict archive') with parameterized cases covering:
    • different-package-name conflicts
    • floating selector mismatches
    • malformed legacy spec
    • second-run no-warning assertion (regression guard)
  • Warning text updated from 'shared SQLite state has newer metadata' to generic 'detecting conflicting plugin install metadata' per ClawSweeper P3 feedback — covers all conflict causes, not just recency.

@openclaw-barnacle openclaw-barnacle Bot added commands Command implementations size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 4, 2026
@clawsweeper

clawsweeper Bot commented Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 15, 2026, 11:09 AM ET / 15:09 UTC.

Summary
The PR changes migrateLegacyInstalledPluginIndex to archive plugins/installs.json on install-record conflicts and updates doctor migration tests so the warning fires once.

PR surface: Source -1, Tests +16. Total +15 across 2 files.

Reproducibility: yes. at source level: seed a conflicting legacy plugins/installs.json beside different SQLite install records and run the legacy-state migration path twice. Current main returns before archiving on conflict, so the file can trigger the same warning again; I did not run a live packaged upgrade in this read-only review.

Review metrics: 1 noteworthy metric.

  • Migration Conflict Policy: 1 conflict branch changed. The diff changes whether conflicted legacy plugin install metadata remains active during upgrades, which maintainers need to review before merge.

Stored data model
Persistent data-model change detected: migration/backfill/repair: src/commands/doctor-state-migrations.test.ts, serialized state: src/infra/state-migrations.ts, unknown-data-model-change: src/commands/doctor-state-migrations.test.ts, vector/embedding metadata: src/commands/doctor-state-migrations.test.ts, vector/embedding metadata: src/infra/state-migrations.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real two-run upgrade proof showing the first warning, the .migrated archive, and a clean second doctor or plugin-inspect run.
  • Narrow the migration policy so true conflicts remain visible unless SQLite is proven authoritative.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body has expected two-run output and tests, but no actual redacted terminal/log/screenshot/live output from a real upgraded install after the patch. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging as-is can archive a legacy install index even when current and legacy metadata name different packages, use incompatible selectors, or contain malformed legacy fields, making a true conflict disappear after one warning.
  • [P1] The contributor has not supplied redacted terminal, log, screenshot, or live-output proof from a real upgraded install showing the first warning, archive file, and clean second run.

Maintainer options:

  1. Preserve True Conflicts (recommended)
    Change the PR so unproven install-record conflicts still leave the legacy file active, and only a narrow proven-superseded case archives it.
  2. Accept Broad Archive Policy
    Maintainers may intentionally accept that SQLite wins all install-index conflicts and that .migrated is enough audit evidence, but that should be explicit before merge.
  3. Coordinate With Wider Migration Cleanup
    Pause this branch if maintainers want one policy for the broader repeated-warning class tracked on the linked issue and related migration PRs.

Next step before merge

  • [P1] Human review is needed because the PR lacks contributor real behavior proof and changes a compatibility-sensitive persisted migration conflict policy.

Security
Cleared: No concrete security or supply-chain issue found; the diff changes internal migration logic and tests without new dependencies, scripts, permissions, or secret handling.

Review findings

  • [P1] Keep true install-index conflicts active — src/infra/state-migrations.ts:1505
Review details

Best possible solution:

Keep the migration in core, but archive only records proven superseded by authoritative SQLite state, keep true conflicts visible with actionable guidance, and require real two-run upgrade proof before merge.

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

Yes at source level: seed a conflicting legacy plugins/installs.json beside different SQLite install records and run the legacy-state migration path twice. Current main returns before archiving on conflict, so the file can trigger the same warning again; I did not run a live packaged upgrade in this read-only review.

Is this the best way to solve the issue?

No. The state-migration layer is the right place, but archiving every conflict is too broad; the maintainable fix should archive only proven superseded legacy records and keep unsafe conflicts visible.

Full review comments:

  • [P1] Keep true install-index conflicts active — src/infra/state-migrations.ts:1505
    merged.conflicts is also reached for cases current tests classify as real conflicts, such as different package names, selector drift, and malformed legacy spec metadata. Archiving sourcePath here makes those unresolved conflicts disappear after one warning and treats SQLite as authoritative without a safe predicate; keep this branch left-in-place unless a stricter superseded-record rule proves the legacy data is obsolete.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority upgrade regression fix for repeated migration warnings with limited reported runtime impact.
  • merge-risk: 🚨 compatibility: The PR changes persisted legacy plugin install-index migration behavior for existing upgraded users.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body has expected two-run output and tests, but no actual redacted terminal/log/screenshot/live output from a real upgraded install after the patch. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source -1, Tests +16. Total +15 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 5 6 -1
Tests 1 21 5 +16
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 26 11 +15

What I checked:

  • Root policy applied: Root AGENTS.md was read fully; its review policy treats state migrations, persisted preferences, config/loading, startup checks, and fallback behavior as compatibility-sensitive. (AGENTS.md:29, c1219d161d3e)
  • Current main conflict behavior: Current main returns the conflict warning before archiving, so plugins/installs.json remains in place when merged.conflicts.length > 0. (src/infra/state-migrations.ts:1572, c1219d161d3e)
  • Current tests define true conflicts: Current main tests expect different packages, unparseable specs, floating selector drift, and malformed legacy spec metadata to leave the legacy install index in place rather than archive it. (src/commands/doctor-state-migrations.test.ts:1724, c1219d161d3e)
  • PR branch archives all conflicts: The PR head changes the merged.conflicts.length > 0 branch to call archiveLegacyInstalledPluginIndex, so the same true-conflict cases stop being active after one warning. (src/infra/state-migrations.ts:1505, ceb21116323c)
  • Latest release still has the old branch: The latest release tag v2026.6.6 still returns the old conflict warning before archive, so current released behavior has not already solved this PR's central problem. (src/infra/state-migrations.ts:1556, 8c802aa68351)
  • Related issue remains broader: The linked issue remains open and later comments report the repeated-warning class across plugin install index, Memory Core, task/flow, Telegram delivery, and plugin-state sidecar legacy sources, so this PR is not the whole policy decision.

Likely related people:

  • RomneyDa: The repeat-doctor state migration repair added the richer installed-plugin conflict behavior and adjacent tests this PR changes. (role: introduced current conflict behavior; confidence: high; commits: 30b9e123b871; files: src/infra/state-migrations.ts, src/commands/doctor-state-migrations.test.ts)
  • steipete: The SQLite plugin install-index migration and later adjacent state-migration work are central to the persisted install metadata boundary. (role: feature-history owner; confidence: high; commits: 5443baa8527e, 0bbac63d00bc; files: src/infra/state-migrations.ts, src/plugins/installed-plugin-index-store.ts, src/commands/doctor-state-migrations.test.ts)
  • zhuisDEV: The official npm plugin install-record pinning work changed the package/version metadata shape used by the migration conflict decisions. (role: adjacent install-record contributor; confidence: medium; commits: 7b5f75eb98f4; files: src/plugins/installs.ts, src/infra/state-migrations.ts, src/commands/doctor-state-migrations.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@clawsweeper clawsweeper Bot added 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 Jun 4, 2026
@Bartok9
fix(state-migrations): use generic conflict warning text
ceb2111 
Per ClawSweeper review feedback [P3]: the previous text 'shared SQLite
state has newer metadata for' was misleading — conflicts can arise from
mismatched floating selectors, package renames, or malformed legacy spec
metadata, not necessarily because SQLite is newer. Use a generic conflict
description that accurately covers all conflict reasons.
@Bartok9
Bartok9 force-pushed the fix/90213-plugin-install-index-conflict-archive branch from f524018 to ceb2111 Compare June 5, 2026 09:12
@Bartok9

Bartok9 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated PR:

  1. Fixed warning text to be generic — removed 'SQLite state has newer metadata' wording per P3 feedback. New text: 'Archived plugin install index after detecting conflicting plugin install metadata for: ...' covers all conflict causes.
  2. Added detailed behavior proof in PR body: root cause code trace showing why the warning repeated on each run, fix code trace showing how archiving prevents re-entry, and expected terminal output for run 1 (warning fires once) vs run 2 (no warning). The two-run behavior is also verified by the second run produces no warnings regression test.

@clawsweeper

clawsweeper Bot commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 5, 2026
@clawsweeper

clawsweeper Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

@Bartok9 thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward.

Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive.

Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 15, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 15, 2026
Closed
@openclaw-clownfish

openclaw-clownfish Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Thanks @Bartok9 for the focused PR and follow-up here. I am closing this as superseded by #90267 because both PRs target the same repeated legacy plugin install-index migration warning, and the canonical path now has the narrower migration rule plus passing real behavior proof.

Canonical PR: #90267
Source PR being closed: #90252

Clownfish is keeping the canonical thread open there so validation and maintainer review stay in one place. The work and discussion from this source PR remain linked for context and credit. If this still covers a different reproduction path after #90267 lands, please reply and we can reopen or split it back out.

@openclaw-clownfish openclaw-clownfish Bot added the clownfish Tracked by Clownfish automation label Jun 15, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 14, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

clownfish Tracked by Clownfish automation commands Command implementations merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: After updating to OpenClaw 2026.6.1, legacy state migration warnings keep appearing even after running openclaw doctor --fix.

1 participant

@Bartok9