fix(config): avoid plugin-local last-good rollback#71367
fix(config): avoid plugin-local last-good rollback#71367steipete merged 2 commits intoopenclaw:mainfrom
Conversation
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟡 Last-known-good recovery skipped for plugin-entry-only invalid configs can cause persistent startup DoS
DescriptionThe new recovery policy suppresses last-known-good (LKG) rollback when validation issues are only under Impact:
Vulnerable logic (policy + enforcement):
Vulnerable code: const canRecoverFromLastKnownGood = shouldAttemptLastKnownGoodRecovery(configSnapshot);
const recovered = canRecoverFromLastKnownGood
? await recoverConfigFromLastKnownGood({ snapshot: configSnapshot, reason: "startup-invalid-config" })
: false;
...
assertValidGatewayStartupConfigSnapshot(configSnapshot, { includeDoctorHint: true });RecommendationReconsider skipping LKG recovery entirely for plugin-scoped invalidity on startup. Safer options:
Example (startup): if (!configSnapshot.valid) {
const recovered = await recoverConfigFromLastKnownGood({
snapshot: configSnapshot,
reason: "startup-invalid-config",
});
if (recovered) {
configSnapshot = await readConfigFileSnapshot();
}
}
assertValidGatewayStartupConfigSnapshot(configSnapshot);If preserving other non-plugin edits is required, implement targeted plugin-entry cleanup before falling back to full LKG rollback. Analyzed PR: #71367 at commit Last updated on: 2026-04-25T03:08:28Z |
Greptile SummaryThis PR introduces a shared Confidence Score: 4/5Safe to merge; the recovery policy logic is correct and no critical paths are broken. Two minor P2 issues in a log message guard and a test mock remain. Only P2 findings present: a mildly inconsistent warning log in config-reload.ts and a small mock/implementation divergence in a test. No behavioral bugs or data-loss risk introduced. src/gateway/config-reload.ts (warning message guard) and src/gateway/server-startup-config.recovery.test.ts (mock fidelity) Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/config-reload.ts
Line: 226-230
Comment:
**Misleading warning when snapshot is unexpectedly valid**
`shouldAttemptLastKnownGoodRecovery` returns `false` for two distinct cases: (1) the snapshot is valid (no recovery needed) and (2) the snapshot is plugin-locally invalid. Both cases reach the `opts.log.warn(...)` call here, so if somehow a valid snapshot is passed into `recoverAndReadSnapshot`, the log would say "invalidity is scoped to plugin entries" when there is no invalidity at all. The `io.observe-recovery.ts` version correctly guards with an inner `isPluginLocalInvalidConfigSnapshot` check before logging — the same pattern would make this warning more precise.
```suggestion
if (!shouldAttemptLastKnownGoodRecovery(snapshot)) {
if (isPluginLocalInvalidConfigSnapshot(snapshot)) {
opts.log.warn(
`config reload recovery skipped after ${reason}: invalidity is scoped to plugin entries`,
);
}
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/gateway/server-startup-config.recovery.test.ts
Line: 11-20
Comment:
**Test mock diverges from real implementation**
The inline mock accepts a path of exactly `"plugins.entries."` (trailing dot, empty plugin name) as a plugin-local issue — `issue.path.startsWith("plugins.entries.")` is `true` — while the real `isPluginEntryIssue` in `recovery-policy.ts` additionally requires something after the prefix (`path.slice(prefix.length).trim().length > 0`), so it would return `false` for that same path and allow recovery. This means the mock is a slight over-approximation: a snapshot with an issue path of `"plugins.entries."` would skip recovery in tests but actually trigger recovery at runtime. The gap is unlikely to surface in practice, but it means a regression in that edge case would not be caught by these tests.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: avoid plugin-local config recovery ..." | Re-trigger Greptile |
| if (!shouldAttemptLastKnownGoodRecovery(snapshot)) { | ||
| opts.log.warn( | ||
| `config reload recovery skipped after ${reason}: invalidity is scoped to plugin entries`, | ||
| ); | ||
| return null; |
There was a problem hiding this comment.
shouldAttemptLastKnownGoodRecovery returns false for two distinct cases: (1) the snapshot is valid (no recovery needed) and (2) the snapshot is plugin-locally invalid. Both cases reach the opts.log.warn(...) call here, so if somehow a valid snapshot is passed into recoverAndReadSnapshot, the log would say "invalidity is scoped to plugin entries" when there is no invalidity at all. The io.observe-recovery.ts version correctly guards with an inner isPluginLocalInvalidConfigSnapshot check before logging — the same pattern would make this warning more precise.
| if (!shouldAttemptLastKnownGoodRecovery(snapshot)) { | |
| opts.log.warn( | |
| `config reload recovery skipped after ${reason}: invalidity is scoped to plugin entries`, | |
| ); | |
| return null; | |
| if (!shouldAttemptLastKnownGoodRecovery(snapshot)) { | |
| if (isPluginLocalInvalidConfigSnapshot(snapshot)) { | |
| opts.log.warn( | |
| `config reload recovery skipped after ${reason}: invalidity is scoped to plugin entries`, | |
| ); | |
| } | |
| return null; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/config-reload.ts
Line: 226-230
Comment:
**Misleading warning when snapshot is unexpectedly valid**
`shouldAttemptLastKnownGoodRecovery` returns `false` for two distinct cases: (1) the snapshot is valid (no recovery needed) and (2) the snapshot is plugin-locally invalid. Both cases reach the `opts.log.warn(...)` call here, so if somehow a valid snapshot is passed into `recoverAndReadSnapshot`, the log would say "invalidity is scoped to plugin entries" when there is no invalidity at all. The `io.observe-recovery.ts` version correctly guards with an inner `isPluginLocalInvalidConfigSnapshot` check before logging — the same pattern would make this warning more precise.
```suggestion
if (!shouldAttemptLastKnownGoodRecovery(snapshot)) {
if (isPluginLocalInvalidConfigSnapshot(snapshot)) {
opts.log.warn(
`config reload recovery skipped after ${reason}: invalidity is scoped to plugin entries`,
);
}
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.| shouldAttemptLastKnownGoodRecovery: vi.fn((snapshot: ConfigFileSnapshot) => { | ||
| if (snapshot.valid) { | ||
| return false; | ||
| } | ||
| return !( | ||
| snapshot.legacyIssues.length === 0 && | ||
| snapshot.issues.length > 0 && | ||
| snapshot.issues.every((issue) => issue.path.startsWith("plugins.entries.")) | ||
| ); | ||
| }), |
There was a problem hiding this comment.
The inline mock accepts a path of exactly "plugins.entries." (trailing dot, empty plugin name) as a plugin-local issue — issue.path.startsWith("plugins.entries.") is true — while the real isPluginEntryIssue in recovery-policy.ts additionally requires something after the prefix (path.slice(prefix.length).trim().length > 0), so it would return false for that same path and allow recovery. This means the mock is a slight over-approximation: a snapshot with an issue path of "plugins.entries." would skip recovery in tests but actually trigger recovery at runtime. The gap is unlikely to surface in practice, but it means a regression in that edge case would not be caught by these tests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-startup-config.recovery.test.ts
Line: 11-20
Comment:
**Test mock diverges from real implementation**
The inline mock accepts a path of exactly `"plugins.entries."` (trailing dot, empty plugin name) as a plugin-local issue — `issue.path.startsWith("plugins.entries.")` is `true` — while the real `isPluginEntryIssue` in `recovery-policy.ts` additionally requires something after the prefix (`path.slice(prefix.length).trim().length > 0`), so it would return `false` for that same path and allow recovery. This means the mock is a slight over-approximation: a snapshot with an issue path of `"plugins.entries."` would skip recovery in tests but actually trigger recovery at runtime. The gap is unlikely to surface in practice, but it means a regression in that edge case would not be caught by these tests.
How can I resolve this? If you propose a fix, please make it concise.8d693cd to
d4cef0c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5684e0476
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function isPluginLocalInvalidConfigSnapshot( | ||
| snapshot: Pick<ConfigFileSnapshot, "valid" | "issues" | "legacyIssues">, | ||
| ): boolean { | ||
| if (snapshot.valid || snapshot.legacyIssues.length > 0 || snapshot.issues.length === 0) { |
There was a problem hiding this comment.
Remove legacy warning gate from plugin-local recovery check
The new classifier returns false whenever legacyIssues.length > 0, so a snapshot that is invalid only because of plugins.entries.* (the case this commit targets) still goes through whole-file last-known-good rollback if the config also contains any migrated legacy keys. Because legacy issues are warning-style migration metadata rather than the invalidity root cause, this can still clobber current openclaw.json during plugin schema/minHostVersion skew windows for users with legacy keys present.
Useful? React with 👍 / 👎.
d4cef0c to
5e9d33f
Compare
|
Landed via rebase onto
Thanks @jalehman. |
Summary
Fixes #71289.
Prevents plugin-local validation failures from triggering whole-file last-known-good recovery. This preserves
openclaw.jsonduring plugin schema evolution /minHostVersionskew windows while keeping last-known-good recovery intact for true base config corruption.What changed
plugins.entries.*as plugin-local..last-good.minHostVersionskew so stale.last-goodsnapshots do not clobber active config.Verification
pnpm tsgopnpm check:test-typespnpm test src/config/io.observe-recovery.test.ts src/gateway/config-reload.test.ts src/gateway/server-startup-config.recovery.test.tspnpm test src/config/config.plugin-validation.test.ts src/config/io.observe-recovery.test.ts src/gateway/config-reload.test.ts src/gateway/server-startup-config.recovery.test.ts