Port openframe-client updates from openframe-oss-tenant (post-extraction commits)#1359
Conversation
…msh target) (#1981) Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
…(#2030) Co-authored-by: Danylo Babenko <[email protected]>
- drop dead trailing assignment to reinstall_dir_cleared (unused_assignments; the flag is never read after the no-registry-record repair block) - mesh self-heal: map_or(false, ..) -> is_some_and(..) (clippy::unnecessary_map_or) Both warnings originate in code ported verbatim from openframe-oss-tenant; this keeps the repo clippy gate (-D warnings) green. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_011z7MGH2qYeKt2ZQV88EaD8
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds uninstall handling, explicit tool record state, per-tool coordination, service stop/delete hardening, mesh self-heal ServerID support, and lower-verbosity logging. ChangesTool Uninstall Feature
Tool State and Coordination
Service Stop and Reinstall Hardening
Mesh Self-Heal and Logging
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant NATS
participant ToolUninstallMessageListener
participant ToolRunManager
participant ToolUninstallService
participant InstalledToolsService
NATS->>ToolUninstallMessageListener: deliver ToolUninstallMessage
ToolUninstallMessageListener->>ToolRunManager: tool_lock(tool_id)
ToolUninstallMessageListener->>ToolRunManager: mark_updating(tool_id)
ToolUninstallMessageListener->>ToolUninstallService: uninstall_by_tool_agent_id(id)
ToolUninstallService->>InstalledToolsService: delete_by_tool_agent_id
ToolUninstallService-->>ToolUninstallMessageListener: UninstallOutcome
ToolUninstallMessageListener->>ToolRunManager: clear_updating(tool_id)
ToolUninstallMessageListener->>NATS: ack (if Removed/NotInstalled)
Related issues: None found. Related PRs: None found. Suggested labels: rust, client, feature Suggested reviewers: None found. 🐰 A tool once stuck, now knows to bow, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clients/openframe-client/src/platform/system_service.rs`:
- Around line 582-599: The `.exe` detection in parse_exe_from_image_path should
use ASCII-only case folding because the current to_lowercase() can change string
length and break the byte slice used for the returned PathBuf. Update the
unquoted path handling in parse_exe_from_image_path to search against
to_ascii_lowercase() so the idx from find(".exe") still aligns with
&trimmed[..idx + 4] and the parsed executable path stays correct.
In `@clients/openframe-client/src/services/tool_agent_update_service.rs`:
- Around line 80-122: The update path in tool_agent_update_service is not
serialized with the per-tool lock, so an uninstall can race with process_update
while binaries or state are being modified. Update tool_agent_update_listener or
process_update to acquire tool_run_manager.tool_lock(tool_agent_id) before doing
any update work, and hold it for the full duration of the update flow so it
matches the uninstall listener’s locking behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 78cc5207-9f79-47f6-be3b-1ff12f2c90ca
📒 Files selected for processing (25)
clients/openframe-client/src/config/update_config.rsclients/openframe-client/src/lib.rsclients/openframe-client/src/listener/mod.rsclients/openframe-client/src/listener/tool_uninstall_message_listener.rsclients/openframe-client/src/logging/log_source.rsclients/openframe-client/src/logging/mod.rsclients/openframe-client/src/logging/nats_streaming.rsclients/openframe-client/src/models/installed_tool.rsclients/openframe-client/src/models/mod.rsclients/openframe-client/src/models/tool_uninstall_message.rsclients/openframe-client/src/platform/directories.rsclients/openframe-client/src/platform/system_service.rsclients/openframe-client/src/platform/tool_updater/gui_app.rsclients/openframe-client/src/platform/tool_updater/service.rsclients/openframe-client/src/platform/uninstall.rsclients/openframe-client/src/services/installed_tools_service.rsclients/openframe-client/src/services/mesh_self_heal_service.rsclients/openframe-client/src/services/mod.rsclients/openframe-client/src/services/openframe_client_update_service.rsclients/openframe-client/src/services/tool_agent_update_service.rsclients/openframe-client/src/services/tool_connection_processing_manager.rsclients/openframe-client/src/services/tool_installation_service.rsclients/openframe-client/src/services/tool_kill_service.rsclients/openframe-client/src/services/tool_run_manager.rsclients/openframe-client/src/services/tool_uninstall_service.rs
|
CI status note: the |
What & why
The shared Rust agent (
openframe-agent-lib, PR #1265) was extracted fromopenframe-oss-tenantat tenant commit6105a10bb(#1967, Jun 23). Since then 8 client commits were merged to tenantmain. This PR ports all of them, 1:1, preserving original authorship and messages.Ported commits (tenant → this branch)
37eddc66e7b92b9f1246084c181d5b2afb34d167edb221ea2c8af331ce09e5a96bd72d76d4ef4372082e13fa93dd0bd79eb2add28f600dba362d0aa9e2be28019691ebaeb3168872Plus one follow-up commit resolving the two clippy findings the ported code trips in this repo's
-D warningsgate (see below).Deviations from verbatim tenant code (all intentional)
cargo fmtfolded into each commit — this repo's pre-commit hook enforces fmt; tenant does not.derive(Default)instead of manualimpl Default(incl. the newToolRecordStatefield),is_none_or(..)instead ofmap_or(true, ..), sorted module/import lists, no needless.clone()on abool.map_err(|e| { #[cfg(windows)] log…; e })into explicit match arms: on non-Windows the cfg-stripped closure is|e| e, which failsclippy::map_identity. The match form keeps both the 300s timeout and the Windows file-lock logging (which the original extraction had dropped at this call site).reinstall_dir_cleared = true;(dead in tenant too) andmap_or(false, ..)→is_some_and(..).Verification (source ↔ target, 4 independent methods)
fmt(tenant@6105a10bb)vs258fa87c). Result: 231 files identical, 45 files with byte-identical constant extraction delta, 5 flagged — each explained by the deviations above.Default-idiom diffs and themain()→run()hoist.ToolRecordState,TOOL_COMMAND_TIMEOUT_SECS,service_clear_for_install,tool_lock,orbit-spawned osqueryd); removed code confirmed absent (ReinstallBackupGuard,ToolUninstallResultPublisher,NOOP_COOLDOWN— superseded upstream by #1986/#1993/delete-tool).Gates
cargo clippy --all-targets --features bin -- -D warnings✅ (and hook variant without--features bin✅)cargo fmt --all -- --check✅ (every commit)cargo test --features bin: 82 passed; the 2 failures (platform::directories::tests::{test_directory_permissions,test_health_check}) fail identically onmain— pre-existing, local-permissions-dependent, unrelated to this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_011z7MGH2qYeKt2ZQV88EaD8
Summary by CodeRabbit
New Features
Bug Fixes
Improvements