config: fix silent failures and misleading log in config recovery#62665
config: fix silent failures and misleading log in config recovery#62665pruthvishetty wants to merge 1 commit intoopenclaw:mainfrom
Conversation
… in config recovery Replace empty catch blocks in io.observe-recovery.ts with diagnostic logger.warn() calls so failures in config health tracking, clobbered snapshot persistence, and backup restoration are visible in logs. Fix misleading 'Config auto-restored from backup' warning that fired even when the copyFile restore actually failed — the message now reflects the actual outcome.
Greptile SummaryAdds diagnostic Confidence Score: 5/5This PR is safe to merge; all changes are limited to log output with no control-flow impact. All findings are P2 or lower. The fixes are accurate, well-scoped, and match the PR description. No new dependencies, no behavioral regressions, and the existing test suite passes. No files require special attention. Reviews (1): Last reviewed commit: "config: log diagnostic warnings instead ..." | Re-trigger Greptile |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as duplicate/superseded. Current main still has the config restore false-success bug, but #70515 is an open, more complete implementation for the same remaining work; this PR's logging-only patch would still leave audit and caller state misleading on failed restores. So I’m closing this here and keeping the remaining discussion on the canonical linked item. Review detailsBest possible solution: Use #70515 or an equivalent canonical patch that keeps restore-failure logs, audit validity/error fields, and async/sync regression tests aligned, then close the non-canonical overlapping branch. Do we have a high-confidence way to reproduce the issue? Yes. A high-confidence reproduction path is to inject failing Is this the best way to solve the issue? No. The logging-only approach is not the best fix because it reports failure in the log while leaving the audit record and returned recovery result in a success shape; the narrower maintainable answer is the structured audit-and-test approach in #70515. Security review: Security review cleared: The diff is limited to config recovery logging with no dependency, workflow, lockfile, package, publishing, secret-handling, or external code execution changes. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against ad7fa6c38774. |
1 participant
Summary
maybeRecoverSuspiciousConfigReadand its sync variant logged "Config auto-restored from backup" even when thecopyFilerestore actually failed. The message now reflects the actual outcome (auto-restoredvsbackup restore failed).catch {}blocks in the config health tracking and recovery pipeline now emitlogger.warn()with the error message, making failures visible in logs instead of silently swallowing them.Affected catch blocks
writeConfigHealthState/SyncpersistClobberedConfigSnapshot/SyncmaybeRecoverSuspiciousConfigRead/SyncWhat is NOT changed
The following
catch {}blocks are intentionally left as-is — they handle expected conditions (file not found on first run, optional JSON parse fallbacks):readConfigHealthState/Sync— health state file may not exist yetjson5.parseinreadConfigFingerprintForPath/Sync— deliberate fallback to empty parsed objectreadConfigFingerprintForPath/Sync— target path may not existTest plan
io.observe-recovery.test.tssuite passes (3/3 tests green)pnpm test src/config/io.observe-recovery.test.ts[AI-assisted]
🤖 Generated with Claude Code