fix: repeat doctor state migration repairs by RomneyDa · Pull Request #89281 · openclaw/openclaw

RomneyDa · 2026-06-02T01:59:26Z

Summary

make the legacy plugin-state sidecar migrator import legacy-only rows before reporting remaining conflicts
let expired legacy plugin-state rows drop during migration so stale cache rows do not keep doctor --fix noisy
let richer shared SQLite plugin install records supersede stale legacy JSON records when they describe the same install identity, while still warning on same-version package-name conflicts

Verification

node scripts/run-vitest.mjs src/commands/doctor-state-migrations.test.ts
pnpm tsgo:core
pnpm tsgo:core:test
git diff --check
.agents/skills/autoreview/scripts/autoreview --mode local

clawsweeper · 2026-06-02T02:00:47Z

Codex review: needs maintainer review before merge. Reviewed June 2, 2026, 3:11 AM ET / 07:11 UTC.

Summary
This PR relaxes doctor legacy state migrations so repeated repairs can import or drop covered plugin-state rows and archive covered legacy plugin install JSON, with focused regression tests.

PR surface: Source +79, Tests +182. Total +261 across 2 files.

Reproducibility: yes. Source inspection shows current main can stop plugin-state sidecar imports before legacy-only rows are inserted and can keep richer same-identity install records in conflict; the PR adds focused regression fixtures for those states.

Review metrics: 1 noteworthy metric.

Migration compatibility paths: 2 relaxed. Doctor can now archive plugin-state sidecars and plugin install JSON in additional upgrade states, so maintainers should notice the upgrade behavior before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

Get explicit maintainer sign-off on the changed doctor upgrade/archive semantics before landing.

Risk before merge

[P1] Merging changes upgrade behavior: doctor may archive legacy plugin-state sidecars after dropping expired rows or importing legacy-only rows, and may archive legacy plugin install JSON when SQLite records cover the identity.
[P1] This read-only review did not rerun the reported Vitest/typecheck gates, so validation confidence comes from source inspection, added tests, and the author-reported commands.

Maintainer options:

Accept the migration semantics (recommended)
A maintainer can land this after explicitly accepting that covered legacy install JSON and expired plugin-state cache rows may be archived during doctor repair.
Tighten identity proof first
If maintainers want a narrower safety margin, require another guard or regression around misleading legacy npm identity metadata before archiving install JSON.
Pause if legacy retention is required
Pause or close this PR if the desired policy is to keep legacy sidecars until every current and legacy field is byte-identical.

Next step before merge

[P2] Human maintainer review is the next action because the PR is maintainer-labeled and intentionally changes upgrade migration behavior; no narrow automated repair remains.

Security
Cleared: No concrete security or supply-chain regression was found; the diff adds no dependencies, workflows, permissions, or external execution paths and stays within local state migration/archive code.

Review details

Best possible solution:

Land after a maintainer explicitly accepts the doctor migration/archive compatibility behavior and the reported validation remains green.

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

Yes. Source inspection shows current main can stop plugin-state sidecar imports before legacy-only rows are inserted and can keep richer same-identity install records in conflict; the PR adds focused regression fixtures for those states.

Is this the best way to solve the issue?

Yes. The fix is in the right owner boundary, doctor legacy state migrations, and the remaining question is maintainer acceptance of the upgrade semantics rather than a narrower code defect.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 0552ec899f38.

Label changes

Label justifications:

P2: This is a bounded doctor migration bugfix with upgrade impact but no evidence of a P0/P1 runtime outage.
merge-risk: 🚨 compatibility: The diff changes when legacy state files are archived during upgrades, which can affect existing installations running doctor repair.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external contributor real-behavior-proof gate does not apply to this maintainer/member PR; the body reports focused migration tests and typecheck commands.

Evidence reviewed

PR surface:

Source +79, Tests +182. Total +261 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	93	14	+79
Tests	1	219	37	+182
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	312	51	+261

What I checked:

Repository policy applied: Root policy treats state migrations, persisted state, config loading, and fallback behavior as compatibility-sensitive and says legacy state/cache file reads belong in doctor migrations rather than steady-state runtime. (AGENTS.md:31, 0552ec899f38)
Current main plugin-state conflict path: Current main records plugin-state sidecar conflicts and throws before inserting legacy-only rows, so a mixed sidecar can keep doctor repair noisy across repeats. (src/infra/state-migrations.ts:1350, 0552ec899f38)
Current main install-index conflict path: Current main treats any non-identical current and legacy install records for the same plugin id as a conflict, even when the current SQLite record is richer but covers the same install identity. (src/infra/state-migrations.ts:391, 0552ec899f38)
PR plugin-state migration behavior: The PR head skips expired legacy rows, imports non-conflicting legacy-only rows, and leaves the sidecar only when live conflicts remain. (src/infra/state-migrations.ts:1410, 3a6046419248)
PR install identity behavior: The PR head allows a current install record to cover legacy spec/timestamp fields only when source matches and resolved spec or npm package name plus version prove the same identity. (src/infra/state-migrations.ts:394, 3a6046419248)
Regression coverage: The PR adds tests for archiving richer matching SQLite install records, preserving same-version records with different or unparseable npm identities, and importing legacy-only plugin-state rows while dropping expired conflicts. (src/commands/doctor-state-migrations.test.ts:1370, 3a6046419248)

Likely related people:

vincentkoc: Current-main blame covers the central state migration functions, and recent commit history includes legacy migration cold-path work in the same migration area. (role: recent area contributor; confidence: medium; commits: baade28397bc, 97ee0c6fd339; files: src/infra/state-migrations.ts, src/commands/doctor-state-migrations.test.ts)
steipete: Recent GitHub commit metadata shows doctor state migration test work around channel migration setup surfaces, adjacent to this PR's regression coverage. (role: adjacent area contributor; confidence: medium; commits: 8742e8fae3b7; files: src/commands/doctor-state-migrations.test.ts)
gumadeiras: Recent GitHub commit metadata shows legacy state migration discovery changes in the same core migration module and setup-entry boundary. (role: adjacent area contributor; confidence: low; commits: 5ae059db16c6; files: src/infra/state-migrations.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.

byungskers · 2026-06-02T05:09:02Z

Nice direction overall — especially the extra migration coverage. One edge case still looks risky in legacyInstallRecordHasCurrentResolvedIdentity(): sameNpmPackage becomes true when both readInstallRecordNpmName(...) calls return undefined, because undefined === undefined. Since parseRegistryNpmSpec() can return null for unsupported specs, two same-version but unparseable npm records could get treated as the same package and suppress a real conflict. I think this shortcut should require both parsed package names to be present before it counts as identity coverage, plus a regression for two unparseable same-version specs.

RomneyDa · 2026-06-02T05:37:21Z

Addressed the ClawSweeper feedback.

Changes:

legacyInstallRecordHasCurrentResolvedIdentity now requires both npm package names to be present and equal before the same-version shortcut can suppress a conflict.
Added a regression test where current and legacy same-version npm records both have unparseable specs; doctor now keeps plugins/installs.json and reports the conflict.

Verification:

node scripts/run-vitest.mjs src/commands/doctor-state-migrations.test.ts
pnpm tsgo:core
pnpm tsgo:core:test
git diff --check
.agents/skills/autoreview/scripts/autoreview --mode local

RomneyDa · 2026-06-02T05:37:24Z

@clawsweeper re-review

clawsweeper · 2026-06-02T05:37:27Z

🦞🧹
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:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26800724954
Updated: 2026-06-02T05:44:56.546Z

RomneyDa · 2026-06-02T05:50:06Z

Simplified the diff after review.

What changed:

factored repeated plugin install-index fixtures into small local helpers
table-drove the two same-version conflict cases
flattened the install-record identity helper so the production path is easier to audit

Diff size is down from +351/-14 to +312/-51 while keeping the same behavior coverage.

Verification:

node scripts/run-vitest.mjs src/commands/doctor-state-migrations.test.ts
pnpm tsgo:core
pnpm tsgo:core:test
git diff --check
.agents/skills/autoreview/scripts/autoreview --mode local

RomneyDa · 2026-06-02T05:50:12Z

@clawsweeper re-review

clawsweeper · 2026-06-02T05:50:14Z

🦞🧹
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:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26801189732
Updated: 2026-06-02T05:56:42.904Z

RomneyDa · 2026-06-02T06:39:55Z

Rebased onto latest origin/main and force-pushed with lease.

Verification after rebase:

node scripts/run-vitest.mjs src/commands/doctor-state-migrations.test.ts
pnpm tsgo:core
pnpm tsgo:core:test
git diff --check origin/main...HEAD

RomneyDa · 2026-06-02T07:02:04Z

@clawsweeper re-review

clawsweeper · 2026-06-02T07:02:07Z

🦞🧹
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:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26804041092
Updated: 2026-06-02T07:11:41.195Z

steipete · 2026-06-02T11:12:49Z

Maintainer pre-merge proof for head 5f3a7e0749372a40cabd7a090cae155997481b71.

Work done:

Accepted and fixed autoreview findings around npm selector preservation, malformed legacy install specs, and repeated expired sidecar reporting.
Kept the fix in doctor state migration code and expanded focused migration regressions.

Local proof:

node scripts/run-vitest.mjs src/commands/doctor-state-migrations.test.ts — passed, 51 tests.
git diff --check origin/main...HEAD — clean.
.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main — clean, no accepted/actionable findings.

CI proof:

PR rollup for this head is MERGEABLE / CLEAN with 0 pending checks and no failing checks.
Relevant lanes include CI, CodeQL/security, dependency guard, real behavior proof, OpenGrep, and workflow sanity all passing or intentionally skipped by path selection.

Known proof gaps:

No separate live user-environment doctor run beyond the focused SQLite migration regression tests and PR CI.

openclaw-barnacle Bot added commands Command implementations size: M maintainer Maintainer-authored PR labels Jun 2, 2026

RomneyDa added 3 commits June 1, 2026 23:38

fix: stabilize doctor state migrations

e10079c

fix: require npm identity for legacy install match

a132fa5

test: simplify doctor migration fixtures

3a60464

RomneyDa force-pushed the fix/doctor-state-migration-repeats branch from 85b6b25 to 3a60464 Compare June 2, 2026 06:39

RomneyDa changed the title ~~Fix repeat doctor state migration repairs~~ fix: repeat doctor state migration repairs Jun 2, 2026

steipete self-assigned this Jun 2, 2026

steipete added 2 commits June 2, 2026 11:44

fix: preserve npm selector conflicts in state migrations

8f0ee59

fix: guard legacy install spec parsing

e1b7205

steipete added 3 commits June 2, 2026 11:54

fix: keep legacy npm selectors during migration

c02d7ec

fix: respect legacy npm spec during migration

a29213d

fix: avoid false expired sidecar migration reports

5f3a7e0

steipete merged commit 30b9e12 into main Jun 2, 2026
161 checks passed

steipete deleted the fix/doctor-state-migration-repeats branch June 2, 2026 11:13

Bartok9 mentioned this pull request Jun 4, 2026

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

Open

Uh oh!

Conversation

RomneyDa commented Jun 2, 2026

Summary

Verification

Uh oh!

clawsweeper Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

byungskers commented Jun 2, 2026

Uh oh!

RomneyDa commented Jun 2, 2026

Uh oh!

RomneyDa commented Jun 2, 2026

Uh oh!

clawsweeper Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RomneyDa commented Jun 2, 2026

Uh oh!

RomneyDa commented Jun 2, 2026

Uh oh!

clawsweeper Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RomneyDa commented Jun 2, 2026

Uh oh!

RomneyDa commented Jun 2, 2026

Uh oh!

clawsweeper Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented Jun 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clawsweeper Bot commented Jun 2, 2026 •

edited

Loading

clawsweeper Bot commented Jun 2, 2026 •

edited

Loading

clawsweeper Bot commented Jun 2, 2026 •

edited

Loading

clawsweeper Bot commented Jun 2, 2026 •

edited

Loading