feat: add rescue lifecycle and primary recovery in Doctor#15
feat: add rescue lifecycle and primary recovery in Doctor#15Keith-CY merged 12 commits intolay2dev:mainfrom
Conversation
eaf255a to
d6703a9
Compare
dev01lay2
left a comment
There was a problem hiding this comment.
Review Summary
Solid feature addition — Rescue Bot lifecycle + primary recovery diagnostics are well-structured. The local/remote symmetry is clean. A few things to address:
Issues
1. run_openclaw_dynamic duplicates OpenclawCommandOutput fields unnecessarily
commands.rs — run_openclaw_dynamic takes the output from run_openclaw (which already returns OpenclawCommandOutput) and reconstructs it field-by-field. If the types match, just return it directly.
2. json.syntax fix is a no-op pretending to be a fix
build_primary_issue_fix_command maps json.syntax to the same command as field.agents (set agents.defaults.model). The original remote_fix_issues just marks it applied without doing anything meaningful either. If the intent is "normalize the config by rewriting it," consider a dedicated config normalize command or at least add a comment explaining why this works.
3. collect_model_summary moved but unchanged — noisy diff
The function and several hundred lines of helpers (RescueBotAction, utility fns) were inserted in the middle of existing code, causing a large delete+reinsert of fix_issues / collect_model_summary. The actual logic is unchanged — consider reordering to minimize churn (new code at end of file, before tests).
4. Missing use_effect cleanup / abort for async operations in Doctor.tsx
refreshRescueStatus fires on mount via useEffect but has no abort/cleanup. If the component unmounts mid-flight (instance switch), the setState calls will fire on an unmounted component. Consider an abort flag or AbortController.
5. State explosion in Doctor.tsx
15+ individual useState hooks for rescue/primary state. Consider grouping into a reducer or at least an object state to make resets cleaner and reduce the reset list in the instance-change effect.
Minor / Nits
let _ = new_text;inremote_fix_issuesis dead code (was there before, but worth cleaning up since you're touching the file)rescue_portinRescuePrimaryDiagnosisResultisOption<u16>on Rust side butrescuePort?: numberin TS — fine, but document the mapping for optional fields vs undefined- Test
test_suggest_rescue_port_prefers_large_gaponly tests one case — consider edge cases (port near u16::MAX, port 0) cargo test rescue_bot_testsblocked by DNS — noted in PR description, but ensure CI has the fix before merge
| .and_then(Value::as_str) | ||
| .or_else(|| obj.get("fix_hint").and_then(Value::as_str)) | ||
| .map(str::to_string); | ||
| items.push(RescuePrimaryIssue { |
There was a problem hiding this comment.
run_openclaw_dynamic reconstructs OpenclawCommandOutput field-by-field from the return of run_openclaw, which already returns the same struct. If the types truly match, just return it directly instead of destructuring and rebuilding.
| if !output.stdout.trim().is_empty() { | ||
| return trim_for_detail(&output.stdout); | ||
| } | ||
| "no output".into() |
There was a problem hiding this comment.
This maps json.syntax to the exact same command as field.agents (config set agents.defaults.model). If the intent is "rewrite the config file to normalize JSON syntax," that's a side-effect of config set — worth a comment explaining why, otherwise it looks like a copy-paste error.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6703a9173
ℹ️ 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".
|
Addressed review feedback in commits
Validation re-run:
|
dev01lay2
left a comment
There was a problem hiding this comment.
All review feedback addressed:
- ✅
run_openclaw_dynamicnow usesFromtrait (.map(Into::into)) - ✅
json.syntaxfix has explanatory comment about canonical rewrite - ✅ Dead
let _ = new_textremoved fromremote_fix_issues - ✅ State grouped into
RescueUiState+PrimaryRecoveryStatewithupdateRescueState/updatePrimaryStatehelpers - ✅
useEffectcleanup with cancelled flag forrefreshRescueStatus - ✅ Bonus: removed
rescueConfigured === trueguard on Activate button (allows reactivation after deactivation)
LGTM 🚀
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d06370a40a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bece945f7d
ℹ️ 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".
| if ch == closer { | ||
| depth += 1; | ||
| } else if ch == opener { | ||
| depth -= 1; | ||
| if depth == 0 { |
There was a problem hiding this comment.
Handle braces in JSON strings when slicing trailing payload
The backward depth scan treats every }/{ byte as structural, so outputs like "[warn] ...\n{\"message\":\"expected } at ...\",...}" never reach depth 0 and extract_json_from_output returns None even though valid JSON is present. This is a regression from the previous find('{') behavior and causes parse_json_loose callers (including rescue diagnosis/status parsing) to silently drop valid reports whenever a JSON string contains brace characters after prefixed log lines.
Useful? React with 👍 / 👎.
Summary
set | activate | status | deactivate | unsetrestart->stop/start)openclaw --profile <profile> doctor --jsontoopenclaw --profile <profile> doctorwhen--jsonis unsupportedActivate,Deactivate,UnsetTest Plan
cargo check --quiet(insrc-tauri/)bun run tsc --noEmitcargo test rescue_bot_tests --quietblocked in this environment by DNS resolution tostatic.crates.ioNotes