Port openframe-client updates from openframe-oss-tenant (round 3: Jul 23 – Aug 7) - #1725
Conversation
…2156) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: denys-gif <denys@flamingo.cx>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…s (#2167) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…2184) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ooping (#2185) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…te (#2187) Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Danylo Babenko <danylo@flamingo.cx>
… hang tool restart (#2186) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…nts heal (#2203) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…oop on reinstall (#2221) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… .msh on every heal path (#2225) Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: denys-gif <denys@flamingo.cx>
…iles The test-split port (tenant 63da3658e) replaced whole files, dropping this repo's extraction-era clippy deltas (-D warnings gate): platform allows, DirectoryManager Default impl, truncate/is_some_and fixes, and the CI-safety test attributes (#1352). Also two fixes new lints require on freshly ported code: too_many_arguments allow on MeshSelfHealService::new (8th arg from the deactivation port) and inspect_err instead of a cfg-windows map_err that is an identity map on unix (same fix applies upstream). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oc rollout deactivation_service, result_store and result_outbox_run_manager were created by commits ported after #1560 generated docs here; their .md files come from the tenant writer run that covered them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesTenant deactivation and lifecycle
Durable scheduled execution
Platform resilience
Diagnostics and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
clients/openframe-client/src/platform/permissions_tests.rs-93-96 (1)
93-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the verification result.
Permissions::verifycan returnOk(false). The current assertion accepts that result although the test states that verification must pass. Assert the contained boolean.Proposed fix
let verify_result = perms.verify(&test_path); - assert!(verify_result.is_ok()); + assert!(verify_result.unwrap());🤖 Prompt for 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. In `@clients/openframe-client/src/platform/permissions_tests.rs` around lines 93 - 96, Update the assertion for Permissions::verify in the permissions test to require both a successful result and a true contained boolean, rather than only checking is_ok(). Preserve the existing test_path and verify call while asserting the actual verification outcome.clients/openframe-client/src/services/deactivation_service.rs-56-61 (1)
56-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the stale "24h" wording in the comments.
UNINSTALL_AFTERis 2 hours (line 22), but three comments still describe a 24h deadline: line 57 (process_starteddoc), line 256 (uninstall_duedoc is fine, but the wording at line 309 inload_marker), and line 309 (A corrupt marker silently resets the 24h clock). The wording misleads anyone reasoning about a destructive path.📝 Proposed comment fixes
- /// Monotonic start of this run. The 24h deadline itself is wall-clock ([`State::gone_since`]) + /// Monotonic start of this run. The uninstall deadline itself is wall-clock ([`State::gone_since`]) /// because it must track real time across restarts; this monotonic grace floor bounds it so a /// wrong or fast-forwarded system clock can't shortcut the uninstall. `Instant` pauses during /// OS suspend on macOS/Linux (not Windows), which here can only ever delay uninstall.- // A corrupt marker silently resets the 24h clock — leave a trail. + // A corrupt marker silently resets the uninstall clock — leave a trail.🤖 Prompt for 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. In `@clients/openframe-client/src/services/deactivation_service.rs` around lines 56 - 61, Update the stale “24h” references in the documentation around process_started and load_marker to describe the actual UNINSTALL_AFTER duration of 2 hours, including the corrupt-marker reset wording; leave the uninstall_due documentation unchanged if it is already accurate.clients/openframe-client/src/lib.rs-578-583 (1)
578-583: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSkip tool startup while suspended.
ToolRunManager::run()does not checkDeactivationService::is_suspended(). Itsshutting_downcheck runs only after the supervisor processesStopTools, so startup can launch tools first. Check suspension before callingrun().🤖 Prompt for 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. In `@clients/openframe-client/src/lib.rs` around lines 578 - 583, Update the startup flow around DeactivationService::start and ToolRunManager::run to check DeactivationService::is_suspended() before invoking run(), preventing tools from launching while suspended; preserve the existing supervisor startup and normal run behavior when the service is not suspended.clients/openframe-client/src/services/device_data_fetcher.rs-35-52 (1)
35-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject
localhost.localas an invalid hostname.If
LocalHostNameislocalhost, Line 38 createslocalhost.local. Lines 65-66 accept that value. The client then registers a loopback hostname instead of usingComputerNameor returningNone.Reject
localhost.localand normalized trailing-dot variants.Proposed fix
- let lower = name.to_lowercase(); - lower != "localhost" && lower != "localhost.localdomain" + let lower = name.trim_end_matches('.').to_ascii_lowercase(); + !matches!( + lower.as_str(), + "localhost" | "localhost.local" | "localhost.localdomain" + )Also applies to: 58-67
🤖 Prompt for 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. In `@clients/openframe-client/src/services/device_data_fetcher.rs` around lines 35 - 52, Update the hostname validation used by the macOS resolution flow, including the path around is_valid_hostname, to reject localhost.local case-insensitively and with any normalized trailing-dot variant. Ensure this rejection causes LocalHostName resolution to fall back to ComputerName or None rather than returning the loopback hostname.clients/openframe-client/src/services/tool_connection_service.rs-25-33 (1)
25-33: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSerialize reads with the connection-file lock.
Line 33 and Line 62 lock mutations, but
get_allremains unlocked. On a multi-thread Tokio runtime, a concurrent read can observetool_connections.jsonafterfs::writetruncates it and before the write completes. The read then fails JSON deserialization and rejects an otherwise valid connection operation.Use a private unlocked read helper from
saveanddelete_by_tool_agent_id. Lock the publicget_allmethod with the same mutex. An atomic write-and-rename strategy also protects readers outside this service instance.Proposed fix
pub async fn save(&self, connection: ToolConnection) -> Result<()> { let _guard = self.writer.lock().await; - let mut list = self.get_all().await?; + let mut list = self.get_all_unlocked()?; // ... } pub async fn get_all(&self) -> Result<Vec<ToolConnection>> { + let _guard = self.writer.lock().await; + self.get_all_unlocked() +} + +fn get_all_unlocked(&self) -> Result<Vec<ToolConnection>> { if !self.file_path.exists() { return Ok(Vec::new()); } // existing read and deserialization } pub async fn delete_by_tool_agent_id(&self, tool_agent_id: &str) -> Result<bool> { let _guard = self.writer.lock().await; - let mut list = self.get_all().await?; + let mut list = self.get_all_unlocked()?; // ... }Also applies to: 60-68
🤖 Prompt for 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. In `@clients/openframe-client/src/services/tool_connection_service.rs` around lines 25 - 33, Update the connection-file access methods so reads use the same writer mutex as mutations: add a private unlocked read helper, have save and delete_by_tool_agent_id call it while holding their existing lock, and lock public get_all before delegating to the helper. Preserve the current parsing and return behavior, and use an atomic write-and-rename approach if protecting readers outside this service instance is required.clients/openframe-client/src/platform/tool_updater/service.rs-61-87 (1)
61-87: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a short settle delay before the lock re-check.
stop_installed_toolandstop_toolreturn before Windows releases the file handles of the terminated process.clear_aside_binarycan therefore report the.oldfile as locked while the process is already exiting. In that caseis_installed_tool_runningcan also still reporttrue, and the method returns without starting the service. The service then stays stopped until the next update message.
preparealready waits 2 seconds after the same two kill calls (Line 123). Apply the same wait here.🛠️ Proposed fix
if let Err(e) = self.deps.tool_kill_service.stop_tool(tool_agent_id).await { warn!(tool_id = %tool_agent_id, "Orphan remediation: process kill failed: {:#}", e); } + // Handles are released asynchronously after the process exits; give the OS a moment + // before judging the .old lock, as prepare() already does after the same kills. + tokio::time::sleep(tokio::time::Duration::from_secs(2)).await; + // Only abstain when a tool process truly survives; a non-process lock holder (e.g. AV scan) must not block the start. if !clear_aside_binary(exec_path, tool_agent_id).await {🤖 Prompt for 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. In `@clients/openframe-client/src/platform/tool_updater/service.rs` around lines 61 - 87, In the orphan-remediation path before the clear_aside_binary lock re-check, add the same 2-second settle delay already used in prepare after stop_installed_tool and stop_tool. Ensure both awaited kill calls complete before sleeping, then preserve the existing lock and is_installed_tool_running handling.clients/openframe-client/src/services/tool_connection_processing_manager.rs-335-338 (1)
335-338: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove both entries under one
running_toolsguard.The cleanup takes the two locks sequentially. Between Line 337 and Line 338 a concurrent
run_new_toolobserves the tool as still running (try_mark_runningreturnsNone) but finds no wake entry. It then logs "already running - skipping" and returns. The reinstalled tool is never republished until the next process start.Hold the
running_toolswrite guard across both removals.try_mark_runningalready takes that guard first, so the ordering stays consistent and no deadlock is introduced.🛠️ Proposed fix
- // Drop the wake entry before the mark so a successor loop's fresh entry can't be clobbered. - wake_signals.write().await.remove(&tool.tool_id); - running_tools.write().await.remove(&tool.tool_id); + // Remove both under the running_tools guard (same order try_mark_running takes them), + // so a racing run_new_tool never sees "running" without a wake handle. + let mut running = running_tools.write().await; + wake_signals.write().await.remove(&tool.tool_id); + running.remove(&tool.tool_id);🤖 Prompt for 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. In `@clients/openframe-client/src/services/tool_connection_processing_manager.rs` around lines 335 - 338, Update the cleanup block around the running-tools removal to acquire the running_tools write guard first and keep it held while removing both the wake_signals and running_tools entries. Preserve the existing cleanup behavior while ensuring try_mark_running cannot observe an inconsistent intermediate state.clients/openframe-client/src/services/tool_run_manager_tests.rs-26-34 (1)
26-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove wall-clock timing from this test.
ClientUpdatePendingFlagusesstd::time::Instant::now(), so Tokio’s paused clock cannot control this test. Use a sufficiently long TTL, or change the flag to usetokio::time::Instantand enable Tokio’stest-utilfeature before usingstart_pausedandadvance.🤖 Prompt for 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. In `@clients/openframe-client/src/services/tool_run_manager_tests.rs` around lines 26 - 34, Update remark_refreshes_the_ttl to avoid wall-clock-dependent sleeps and timing assertions. Use a sufficiently long TTL that remains valid across the test’s immediate mark calls, or migrate ClientUpdatePendingFlag from std::time::Instant to tokio::time::Instant and configure Tokio test-util with paused time and advance; preserve verification that the second mark refreshes the pending TTL.clients/openframe-client/src/services/.tool_connection_service.md-43-43 (1)
43-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not describe the constructor as asynchronous.
ToolConnectionService::new()is synchronous. The CRUD methods are asynchronous. Change the final sentence so it does not contradict the usage example. (raw.githubusercontent.com)Suggested wording
- The service uses file-based JSON storage in a secured directory, automatically handling serialization/deserialization and directory creation. All operations are async and return `Result<T>` for proper error handling. + The service uses file-based JSON storage in a secured directory, automatically handling serialization/deserialization and directory creation. CRUD methods are async and return `Result<T>` for proper error handling; `new()` creates the service synchronously.🤖 Prompt for 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. In `@clients/openframe-client/src/services/.tool_connection_service.md` at line 43, Update the service documentation around ToolConnectionService::new() to state that construction is synchronous while CRUD operations are asynchronous, removing any wording that implies the constructor is async and keeping the Result<T> behavior description accurate.clients/openframe-client/src/services/.mesh_self_heal_service.md-14-14 (1)
14-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument all conditions for
current_msh_missing_serverid().The description lists only a missing
ServerID. The implementation also returnstruewhen no.mshfile exists or when the file cannot be read. Update the description to include these conditions. (raw.githubusercontent.com)Suggested wording
- Returns `true` when the on-disk `.msh` lacks a `ServerID` (agent cannot authenticate the server) + Returns `true` when no `.msh` is found, it cannot be read, or it lacks a `ServerID` (the agent cannot authenticate the server)🤖 Prompt for 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. In `@clients/openframe-client/src/services/.mesh_self_heal_service.md` at line 14, Update the documentation entry for current_msh_missing_serverid() to state that it returns true when the .msh file is missing, unreadable, or lacks a ServerID, preserving the existing authentication context.clients/openframe-client/src/services/.result_outbox_run_manager.md-51-51 (1)
51-51: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the source link path.
The current target returns 404. The source file is located at
clients/openframe-client/src/services/result_outbox_run_manager.rs. ()Proposed fix
- [`result_outbox_run_manager.rs`](https://github.com/flamingo-stack/openframe-oss-tenant/blob/main/result_outbox_run_manager.rs) + [`result_outbox_run_manager.rs`](https://github.com/flamingo-stack/openframe-oss-tenant/blob/main/clients/openframe-client/src/services/result_outbox_run_manager.rs)🤖 Prompt for 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. In `@clients/openframe-client/src/services/.result_outbox_run_manager.md` at line 51, Update the source link in result_outbox_run_manager.md to target the valid clients/openframe-client/src/services/result_outbox_run_manager.rs location instead of the current broken path.clients/openframe-client/src/services/.mesh_self_heal_service.md-43-43 (1)
43-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the watcher lifecycle description.
run()returns after spawning a detached task. That task logs both watcher exits and panics, waits, and respawns the watcher. Therun()caller does not log the panic. Update this sentence. (raw.githubusercontent.com)Suggested wording
- The watcher exits only on an unexpected panic; the `run()` caller logs an error if that occurs. + The detached watcher logs and respawns when `watch()` exits or panics; `run()` returns after scheduling it.🤖 Prompt for 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. In `@clients/openframe-client/src/services/.mesh_self_heal_service.md` at line 43, Update the watcher lifecycle description to state that run() spawns a detached task and returns, while that task logs watcher exits and panics, waits, and respawns the watcher. Remove the claim that the run() caller logs unexpected panics, and preserve the existing descriptions of suspension handling and timer resets.clients/openframe-client/src/services/.tool_connection_processing_manager.md-58-58 (1)
58-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the degraded backoff as fixed.
The implementation uses 15 seconds for the fast retries and a fixed 300-second delay after the threshold. It does not increase the delay exponentially. Replace “backs off exponentially” with “switches to a fixed degraded interval.” (raw.githubusercontent.com)
Suggested wording
- The agent ID resolution loop defers processing while a tool update is in progress, backs off exponentially after `AGENT_ID_MAX_FAST_RETRIES` consecutive failures, and transitions to a 5-minute degraded interval to avoid tight spinning on persistently unhealthy agents. + The agent ID resolution loop defers processing while a tool update is in progress, switches to the fixed `AGENT_ID_DEGRADED_BACKOFF_SECONDS` interval after `AGENT_ID_MAX_FAST_RETRIES` consecutive failures, and avoids tight spinning on persistently unhealthy agents.🤖 Prompt for 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. In `@clients/openframe-client/src/services/.tool_connection_processing_manager.md` at line 58, Update the description of the agent ID resolution loop to state that it switches to a fixed degraded interval after AGENT_ID_MAX_FAST_RETRIES consecutive failures, rather than backing off exponentially. Preserve the existing 15-second fast-retry and 5-minute degraded-interval details.clients/openframe-client/src/services/result_store.rs-44-49 (1)
44-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
journal_contains_batchcan reject a valid batch when an execution ID contains a colon.
entry_keyjoins the execution ID and the script ID with:.journal_contains_batchthen infers batch membership from the string prefix. If a server-issued execution ID contains:, the prefix test matches keys from a different batch.journal_batchthen returnsfalse,handle_durablelogs "Batch already in flight, skipping redelivery", and the batch never runs and never produces a result.Compare the stored
execution_idfield instead of the key prefix. The scan already reads every entry.🐛 Proposed fix
fn journal_contains_batch( journal: &impl ReadableTable<&'static str, &'static [u8]>, execution_id: &str, ) -> Result<bool> { - let prefix = format!("{}:", execution_id); - for entry in journal.iter()? { - let (key, _) = entry?; - let key = key.value(); - if key == execution_id || key.starts_with(&prefix) { + for entry in journal.iter()? { + let (_, value) = entry?; + let record: JournalRecord = serde_json::from_slice(value.value())?; + if record.execution_id == execution_id { return Ok(true); } } Ok(false) }Also applies to: 378-391
🤖 Prompt for 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. In `@clients/openframe-client/src/services/result_store.rs` around lines 44 - 49, Update journal_contains_batch to determine membership by comparing each scanned entry’s stored execution_id field with the requested execution ID, rather than inferring it from the entry_key string prefix. Preserve the existing scan behavior and ensure colon-containing execution IDs cannot match entries from another batch.clients/openframe-client/src/models/execution.rs-73-79 (1)
73-79: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not log malformed environment variables
apply_env_varslogs the completevarstring atclients/openframe-client/src/executor/env.rs:14. A secret with an empty name becomes=secretand is exposed by this warning. Log only non-sensitive validation metadata.🤖 Prompt for 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. In `@clients/openframe-client/src/models/execution.rs` around lines 73 - 79, Update the environment-variable validation and warning flow used by apply_env_vars so malformed entries with empty names are not logged as complete name-value strings. Replace the var string in the warning with non-sensitive validation metadata, while preserving the existing environment-variable mapping behavior in the execution model.
🧹 Nitpick comments (10)
clients/openframe-client/src/utils/timed_permit_pool.rs (1)
22-53: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe effective deadline is up to twice
timeout.
callappliestimeoutto permit acquisition and again to the blocking task. When all permits are busy and one frees late, the caller can wait almost2 * timeout. Callers document a single cap:scm_call_timedinclients/openframe-client/src/platform/system_service.rspassesSCM_QUERY_TIMEOUT_SECSand describes it as the cap on the whole SCM call.Use one deadline for both phases, or document the two-phase bound.
♻️ Proposed single-deadline implementation
pub async fn call<T, F>(&self, what: &str, timeout: Duration, f: F) -> Result<T> where F: FnOnce() -> T + Send + 'static, T: Send + 'static, { - let permit = match tokio::time::timeout(timeout, self.permits.clone().acquire_owned()).await + let deadline = tokio::time::Instant::now() + timeout; + let permit = match tokio::time::timeout_at(deadline, self.permits.clone().acquire_owned()) + .await { @@ match tokio::time::timeout( - timeout, + timeout, tokio::task::spawn_blocking(move || {Replace the second
timeoutwithtokio::time::timeout_at(deadline, ...)if you want a strict overall cap.🤖 Prompt for 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. In `@clients/openframe-client/src/utils/timed_permit_pool.rs` around lines 22 - 53, Update timed_permit_pool::call to establish one deadline at the start and use it for both permit acquisition and blocking-task execution, replacing the second relative timeout with deadline-based timeout handling. Preserve the existing permit-pool and task error behavior while ensuring the total call duration never exceeds the supplied timeout.clients/openframe-client/src/utils/timed_permit_pool_tests.rs (1)
26-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe tests depend on wall-clock margins and can flake on loaded CI.
permit_is_held_past_timeout_and_freed_when_the_call_returnsassumes a 400ms blocking sleep completes inside the 600ms wait.call_over_capacity_fails_fast_while_slots_are_busyassumes fourspawn_blockingtasks all acquire permits inside 200ms. A slow runner breaks both assumptions.Replace the fixed sleeps with explicit synchronization. Signal from inside each holder closure when it starts, and poll for permit release instead of sleeping a fixed 600ms.
Note also that each holder locks the shared
Mutex<Receiver>beforerecv(), so the holders serialize on the lock. The permits stay occupied, so the assertion still holds, but the test does not exercise four concurrent receivers.♻️ Sketch for deterministic slot occupancy
let mut holders = Vec::new(); + let started = Arc::new(std::sync::atomic::AtomicUsize::new(0)); for _ in 0..4 { let pool = pool.clone(); let rx = rx.clone(); + let started = started.clone(); holders.push(tokio::spawn(async move { pool.call("holder", Duration::from_secs(10), move || { + started.fetch_add(1, std::sync::atomic::Ordering::SeqCst); let _ = rx.lock().unwrap().recv(); }) .await })); } - // Give the four holders time to occupy every slot. - tokio::time::sleep(Duration::from_millis(200)).await; + while started.load(std::sync::atomic::Ordering::SeqCst) < 4 { + tokio::time::sleep(Duration::from_millis(10)).await; + }The same pattern removes the fixed 600ms wait in the previous test.
🤖 Prompt for 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. In `@clients/openframe-client/src/utils/timed_permit_pool_tests.rs` around lines 26 - 90, Replace timing-based sleeps in permit_is_held_past_timeout_and_freed_when_the_call_returns and call_over_capacity_fails_fast_while_slots_are_busy with explicit synchronization: signal from each holder closure after it starts, wait for all signals before asserting capacity exhaustion, and poll until the timed-out call’s permit is released before verifying recovery. Avoid serializing holder startup through the shared Receiver mutex; use per-holder start notifications or equivalent synchronization while preserving the existing release and completion assertions.clients/openframe-client/src/services/initial_key_service.rs (1)
96-100: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider reporting the response status to the deactivation service.
fetch_registration_secretnever callsself.deactivation.on_gateway_status(status). For a deleted tenant this loop keeps retrying every 60 seconds until some other caller establishes suspension. Reporting the status here makes this path consistent withAuthClientand lets the loop suspend on its own evidence.♻️ Proposed change
- if !response.status().is_success() { - let status = response.status(); + let status = response.status(); + self.deactivation.on_gateway_status(status).await; + + if !status.is_success() { let body = response.text().await.unwrap_or_default(); anyhow::bail!("HTTP {} - {}", status, body); }🤖 Prompt for 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. In `@clients/openframe-client/src/services/initial_key_service.rs` around lines 96 - 100, Update fetch_registration_secret to call self.deactivation.on_gateway_status(status) after obtaining the unsuccessful HTTP response status and before returning the error, preserving the existing body extraction and anyhow::bail behavior.clients/openframe-client/src/platform/uninstall.rs (1)
76-80: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNull the child stdio on Windows, as the macOS launcher does.
The macOS launcher sets
stdin,stdout, andstderrtoStdio::null()(lines 48-50). The Windows launcher does not. The detached child therefore inherits the service process stdio handles. After the SCM stops the service, those handles can become invalid, and the child can keep them open. Set them toStdio::null()for the same isolation.♻️ Proposed fix
#[cfg(target_os = "windows")] fn spawn_detached_uninstall_windows(install_path: &Path) -> Result<()> { use std::os::windows::process::CommandExt; - use std::process::Command; + use std::process::{Command, Stdio}; const CREATE_NO_WINDOW: u32 = 0x0800_0000; const DETACHED_PROCESS: u32 = 0x0000_0008; let child = Command::new(install_path) .arg(UNINSTALL_SUBCOMMAND) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) .creation_flags(CREATE_NO_WINDOW | DETACHED_PROCESS) .spawn() .context("Failed to spawn detached self-uninstall process")?;🤖 Prompt for 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. In `@clients/openframe-client/src/platform/uninstall.rs` around lines 76 - 80, Update the Windows self-uninstall Command construction around UNINSTALL_SUBCOMMAND to set stdin, stdout, and stderr to Stdio::null(), matching the macOS launcher’s isolation behavior before spawning the detached process.clients/openframe-client/src/platform/tool_updater/service.rs (1)
81-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the imported
error!macro for consistency.The file imports
warn!andinfo!and uses them unqualified. These two call sites usetracing::error!. Importerrorand use it unqualified.🤖 Prompt for 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. In `@clients/openframe-client/src/platform/tool_updater/service.rs` around lines 81 - 95, Import the tracing error macro alongside warn! and info!, then update both error call sites in the orphan remediation flow to use unqualified error! consistently.clients/openframe-client/src/models/execution_tests.rs (1)
131-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd assertions for the
DURABLEconstant.This test locks
KINDandRESULT_KIND, but notDURABLE.ExecutionListener::handle_messageselects the durable journaling path fromM::DURABLE. A regression that flips this constant changes result durability without failing any test.♻️ Proposed addition
assert_eq!(ScriptMessage::RESULT_KIND, ScriptMessage::KIND); assert_eq!(CommandMessage::RESULT_KIND, CommandMessage::KIND); + assert!(ScriptScheduleExecutionMessage::DURABLE); + assert!(!ScriptMessage::DURABLE); + assert!(!CommandMessage::DURABLE); }🤖 Prompt for 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. In `@clients/openframe-client/src/models/execution_tests.rs` around lines 131 - 143, Extend schedule_results_reuse_the_script_execution_subject to assert the expected DURABLE values for ScriptScheduleExecutionMessage, ScriptMessage, and CommandMessage, covering the constants used by ExecutionListener::handle_message for durable journaling.clients/openframe-client/src/services/result_store_tests.rs (1)
169-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test does not verify the pruning order.
Line 173 sets
r.1.created_at_secs = i as u64, but thatJournalRecordis never stored. The test callscompletedirectly, andcompletewrites its ownOutboxMetawithcreated_at_secs: now_secs(). All five entries therefore carry the same timestamp, and the mutation on line 173 has no effect.The assertions only check the dropped count and the remaining length. They pass for any three victims, so the "keeps newest, drops oldest" behavior is untested.
Assert which keys survive, and give
completea timestamp seam so the test can control ordering. If a seam is not wanted, remove line 173 and rename the test toprune_reduces_outbox_to_cap.🤖 Prompt for 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. In `@clients/openframe-client/src/services/result_store_tests.rs` around lines 169 - 180, Fix prune_keeps_newest_drops_oldest so it controls entry timestamps through a complete timestamp seam, rather than mutating the unused JournalRecord; then assert that the newest keys remain and the oldest keys are removed in addition to count checks. If no timestamp seam is introduced, remove the ineffective created_at_secs assignment and rename the test to prune_reduces_outbox_to_cap.clients/openframe-client/src/listener/execution_listener.rs (1)
144-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
publish_directlyfor the non-durable branch.The loop at lines 144-150 and the body of
publish_directlyat lines 263-274 perform the same steps: derivescript_id, execute, calllog_finished, then publish and log a publish failure. The only difference is that one callspublish_resultand the other inlines the same publish and error log.Call
publish_directlyfrom the non-durable branch, and letpublish_directlyusepublish_result.♻️ Proposed refactor
} else { - for request in requests { - let script_id = request.script_id.unwrap_or("-").to_string(); - let result = self.execution_service.execute(&request, machine_id).await; - log_finished(&execution_id, &schedule_id, &script_id, &result); - self.publish_result(&result_subject, &result, &execution_id, &script_id) - .await; - } + self.publish_directly( + requests, + machine_id, + &result_subject, + &execution_id, + &schedule_id, + ) + .await; }for request in requests { let script_id = request.script_id.unwrap_or("-").to_string(); let result = self.execution_service.execute(&request, machine_id).await; log_finished(execution_id, schedule_id, &script_id, &result); - if let Err(e) = self - .nats_message_publisher - .publish(result_subject, &result) - .await - { - error!(kind = M::KIND, execution_id = %execution_id, script_id = %script_id, error = %e, "Failed to publish result"); - } + self.publish_result(result_subject, &result, execution_id, &script_id) + .await; }Also applies to: 263-275
🤖 Prompt for 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. In `@clients/openframe-client/src/listener/execution_listener.rs` around lines 144 - 150, Refactor the non-durable request loop to call publish_directly instead of duplicating script ID derivation, execution, completion logging, and publishing. Update publish_directly to delegate publishing through publish_result while preserving its existing failure handling and the current execution behavior.clients/openframe-client/src/services/result_store.rs (1)
106-126: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce the repeated serialization in
encode_result.The loop re-serializes the whole
RmmResulton every iteration and shrinks the output by only about 25% each time. For a payload near the 960 KiB limit with a multi-megabytestdout, this performs dozens of full serializations of a large struct.encode_resultis a synchronous function, andExecutionListener::handle_durablecalls it directly on the runtime thread at line 228 ofclients/openframe-client/src/listener/execution_listener.rs. The work is not moved tospawn_blocking.Compute the target length once from the serialized overhead, then truncate in a single step. Keep the current loop only as a final correction.
♻️ Proposed direction
let mut capped = result.clone(); capped.error = Some(TRUNCATION_MARKER.to_string()); + // Drop the bulk of the overflow in one step before the corrective loop. + let overflow = full.len().saturating_sub(OUTBOX_MAX_PAYLOAD_BYTES); + if capped.stdout.len() > overflow { + let mut n = capped.stdout.len() - overflow; + while n > 0 && !capped.stdout.is_char_boundary(n) { + n -= 1; + } + capped.stdout.truncate(n); + } else { + capped.stdout.clear(); + } loop {🤖 Prompt for 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. In `@clients/openframe-client/src/services/result_store.rs` around lines 106 - 126, Update encode_result to serialize the result once, calculate the available payload length from the serialized overhead and OUTBOX_MAX_PAYLOAD_BYTES, then truncate stdout or stderr in a single step rather than repeatedly shrinking by roughly 25%. Retain the existing serialization loop only as a final correction for any remaining size overflow, preserving the current truncation marker and field-priority behavior.clients/openframe-client/src/listener/execution_listener_tests.rs (1)
1-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the durable branch of
handle_message.These tests cover
run_unboundedwell. They do not coverhandle_durable, which is the most complex new logic in this file: journal-then-execute ordering, the "batch already in flight" skip, thepublish_directlyfallback whenjournal_batchfails, and the best-effort publish plusjournal_removewhencompletefails.
ResultStoreaccepts a plainPathBufandExecutionListenerholds anArc<ResultStore>, so atempdir-backed store can drive these paths without NATS. Add tests that assert the journal contains the batch before execution and that the outbox holds one entry per script afterwards.🤖 Prompt for 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. In `@clients/openframe-client/src/listener/execution_listener_tests.rs` around lines 1 - 122, Extend execution_listener_tests.rs with tempdir-backed ResultStore tests covering handle_message/handle_durable: verify journal_batch completes before script execution, skip processing when a batch is already in flight, fall back to publish_directly when journal_batch fails, and perform best-effort publish plus journal_remove when complete fails. Assert the durable outbox contains exactly one entry per script after processing, using ExecutionListener with Arc<ResultStore> and no NATS dependency.
🤖 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/listener/execution_listener.rs`:
- Around line 98-101: Replace the unbounded task spawning in the execution
listener’s run_unbounded flow with a high-cap concurrency mechanism, such as a
semaphore or capped JoinSet, while preserving concurrent handling of independent
messages. Track spawned handlers so listen shutdown or reconnect does not detach
them indefinitely, and ensure the cap limits concurrent
handle_message/ExecutionService::execute work without blocking message intake
unnecessarily.
In `@clients/openframe-client/src/services/deactivation_service.rs`:
- Around line 116-125: Update DeactivationService, including on_gateway_status,
to skip all deactivation handling when local_mode is enabled, rather than
relying only on the ENABLED target-OS constant. Ensure local-mode clients cannot
record GONE or healthy statuses or trigger suspension and self-uninstall, while
preserving existing behavior for non-local mode.
In `@clients/openframe-client/src/services/mesh_self_heal_service.rs`:
- Around line 211-219: Update heal and the related watch/restart paths to first
verify the MeshCentral tool record exists and is in ToolRecordState::Installed;
return without calling try_refresh_msh or restart_agent when it is missing or
not installed. Ensure this guard occurs before mesh_msh_path can trigger refresh
and before restart_agent can fall back to stop_tool.
In `@clients/openframe-client/src/services/nats_message_publisher.rs`:
- Around line 34-46: Update NatsMessagePublisher::publish_raw and the related
NatsMessagePublisher::publish path so each client.flush().await is bounded by
FLUSH_PUBLISH_TIMEOUT_SECS. Preserve the existing publish and error-context
behavior while ensuring stalled NATS connections return after the configured
timeout.
In `@clients/openframe-client/src/services/result_outbox_run_manager.rs`:
- Around line 69-76: Update the oversize-entry branch in the outbox flush logic
to delete the entry after the existing warning instead of only continuing. Use
the same outbox storage/removal mechanism used by the surrounding result
management code, then continue processing subsequent entries.
In `@clients/openframe-client/src/services/result_store.rs`:
- Around line 277-290: Update all three ResultStore scans in
clients/openframe-client/src/services/result_store.rs:232-244 (recover), 277-290
(pending_keys), and 355-359 (prune_oldest) to match JSON deserialization
failures instead of propagating them; increment a shared failure count, log a
warning, and continue scanning. In recover, remove undecodable JOURNAL keys so
they no longer reject redelivered batches; in pending_keys, skip invalid
OutboxMeta records; and in prune_oldest, treat an undecodable entry as the
oldest candidate so capacity can still be reclaimed.
- Around line 63-88: Update ResultStore::open_or_degrade to classify the
Self::open error before quarantining: return a degraded store without renaming
for DatabaseError::DatabaseAlreadyOpen and StorageError::Io, while preserving
the existing error logging. Only rename the database to the .redb.corrupt path
and retry when the error is StorageError::Corrupted; keep the final retry
failure behavior unchanged.
In `@clients/openframe-client/src/services/tool_agent_update_service.rs`:
- Around line 297-311: Persist the updated installation metadata before the
early return in the clear_aside_binary failure path. In the update flow around
installed_tools_service.save, save installed_tool with the new installation and
existing version, then return Ok(()); keep the normal path’s subsequent version
update and save behavior unchanged.
In `@clients/openframe-client/src/services/tool_run_manager.rs`:
- Around line 484-508: Add a supervision epoch field to the tool-run manager,
increment it in stop_all, and capture the current epoch when each supervisor
loop starts. Update the loop’s shutdown and running-tools checks to also exit
whenever its captured epoch differs from the manager’s current epoch, preventing
loops left behind by stop_all from supervising after restart_all.
---
Minor comments:
In `@clients/openframe-client/src/lib.rs`:
- Around line 578-583: Update the startup flow around DeactivationService::start
and ToolRunManager::run to check DeactivationService::is_suspended() before
invoking run(), preventing tools from launching while suspended; preserve the
existing supervisor startup and normal run behavior when the service is not
suspended.
In `@clients/openframe-client/src/models/execution.rs`:
- Around line 73-79: Update the environment-variable validation and warning flow
used by apply_env_vars so malformed entries with empty names are not logged as
complete name-value strings. Replace the var string in the warning with
non-sensitive validation metadata, while preserving the existing
environment-variable mapping behavior in the execution model.
In `@clients/openframe-client/src/platform/permissions_tests.rs`:
- Around line 93-96: Update the assertion for Permissions::verify in the
permissions test to require both a successful result and a true contained
boolean, rather than only checking is_ok(). Preserve the existing test_path and
verify call while asserting the actual verification outcome.
In `@clients/openframe-client/src/platform/tool_updater/service.rs`:
- Around line 61-87: In the orphan-remediation path before the
clear_aside_binary lock re-check, add the same 2-second settle delay already
used in prepare after stop_installed_tool and stop_tool. Ensure both awaited
kill calls complete before sleeping, then preserve the existing lock and
is_installed_tool_running handling.
In `@clients/openframe-client/src/services/.mesh_self_heal_service.md`:
- Line 14: Update the documentation entry for current_msh_missing_serverid() to
state that it returns true when the .msh file is missing, unreadable, or lacks a
ServerID, preserving the existing authentication context.
- Line 43: Update the watcher lifecycle description to state that run() spawns a
detached task and returns, while that task logs watcher exits and panics, waits,
and respawns the watcher. Remove the claim that the run() caller logs unexpected
panics, and preserve the existing descriptions of suspension handling and timer
resets.
In `@clients/openframe-client/src/services/.result_outbox_run_manager.md`:
- Line 51: Update the source link in result_outbox_run_manager.md to target the
valid clients/openframe-client/src/services/result_outbox_run_manager.rs
location instead of the current broken path.
In
`@clients/openframe-client/src/services/.tool_connection_processing_manager.md`:
- Line 58: Update the description of the agent ID resolution loop to state that
it switches to a fixed degraded interval after AGENT_ID_MAX_FAST_RETRIES
consecutive failures, rather than backing off exponentially. Preserve the
existing 15-second fast-retry and 5-minute degraded-interval details.
In `@clients/openframe-client/src/services/.tool_connection_service.md`:
- Line 43: Update the service documentation around ToolConnectionService::new()
to state that construction is synchronous while CRUD operations are
asynchronous, removing any wording that implies the constructor is async and
keeping the Result<T> behavior description accurate.
In `@clients/openframe-client/src/services/deactivation_service.rs`:
- Around line 56-61: Update the stale “24h” references in the documentation
around process_started and load_marker to describe the actual UNINSTALL_AFTER
duration of 2 hours, including the corrupt-marker reset wording; leave the
uninstall_due documentation unchanged if it is already accurate.
In `@clients/openframe-client/src/services/device_data_fetcher.rs`:
- Around line 35-52: Update the hostname validation used by the macOS resolution
flow, including the path around is_valid_hostname, to reject localhost.local
case-insensitively and with any normalized trailing-dot variant. Ensure this
rejection causes LocalHostName resolution to fall back to ComputerName or None
rather than returning the loopback hostname.
In `@clients/openframe-client/src/services/result_store.rs`:
- Around line 44-49: Update journal_contains_batch to determine membership by
comparing each scanned entry’s stored execution_id field with the requested
execution ID, rather than inferring it from the entry_key string prefix.
Preserve the existing scan behavior and ensure colon-containing execution IDs
cannot match entries from another batch.
In `@clients/openframe-client/src/services/tool_connection_processing_manager.rs`:
- Around line 335-338: Update the cleanup block around the running-tools removal
to acquire the running_tools write guard first and keep it held while removing
both the wake_signals and running_tools entries. Preserve the existing cleanup
behavior while ensuring try_mark_running cannot observe an inconsistent
intermediate state.
In `@clients/openframe-client/src/services/tool_connection_service.rs`:
- Around line 25-33: Update the connection-file access methods so reads use the
same writer mutex as mutations: add a private unlocked read helper, have save
and delete_by_tool_agent_id call it while holding their existing lock, and lock
public get_all before delegating to the helper. Preserve the current parsing and
return behavior, and use an atomic write-and-rename approach if protecting
readers outside this service instance is required.
In `@clients/openframe-client/src/services/tool_run_manager_tests.rs`:
- Around line 26-34: Update remark_refreshes_the_ttl to avoid
wall-clock-dependent sleeps and timing assertions. Use a sufficiently long TTL
that remains valid across the test’s immediate mark calls, or migrate
ClientUpdatePendingFlag from std::time::Instant to tokio::time::Instant and
configure Tokio test-util with paused time and advance; preserve verification
that the second mark refreshes the pending TTL.
---
Nitpick comments:
In `@clients/openframe-client/src/listener/execution_listener_tests.rs`:
- Around line 1-122: Extend execution_listener_tests.rs with tempdir-backed
ResultStore tests covering handle_message/handle_durable: verify journal_batch
completes before script execution, skip processing when a batch is already in
flight, fall back to publish_directly when journal_batch fails, and perform
best-effort publish plus journal_remove when complete fails. Assert the durable
outbox contains exactly one entry per script after processing, using
ExecutionListener with Arc<ResultStore> and no NATS dependency.
In `@clients/openframe-client/src/listener/execution_listener.rs`:
- Around line 144-150: Refactor the non-durable request loop to call
publish_directly instead of duplicating script ID derivation, execution,
completion logging, and publishing. Update publish_directly to delegate
publishing through publish_result while preserving its existing failure handling
and the current execution behavior.
In `@clients/openframe-client/src/models/execution_tests.rs`:
- Around line 131-143: Extend
schedule_results_reuse_the_script_execution_subject to assert the expected
DURABLE values for ScriptScheduleExecutionMessage, ScriptMessage, and
CommandMessage, covering the constants used by ExecutionListener::handle_message
for durable journaling.
In `@clients/openframe-client/src/platform/tool_updater/service.rs`:
- Around line 81-95: Import the tracing error macro alongside warn! and info!,
then update both error call sites in the orphan remediation flow to use
unqualified error! consistently.
In `@clients/openframe-client/src/platform/uninstall.rs`:
- Around line 76-80: Update the Windows self-uninstall Command construction
around UNINSTALL_SUBCOMMAND to set stdin, stdout, and stderr to Stdio::null(),
matching the macOS launcher’s isolation behavior before spawning the detached
process.
In `@clients/openframe-client/src/services/initial_key_service.rs`:
- Around line 96-100: Update fetch_registration_secret to call
self.deactivation.on_gateway_status(status) after obtaining the unsuccessful
HTTP response status and before returning the error, preserving the existing
body extraction and anyhow::bail behavior.
In `@clients/openframe-client/src/services/result_store_tests.rs`:
- Around line 169-180: Fix prune_keeps_newest_drops_oldest so it controls entry
timestamps through a complete timestamp seam, rather than mutating the unused
JournalRecord; then assert that the newest keys remain and the oldest keys are
removed in addition to count checks. If no timestamp seam is introduced, remove
the ineffective created_at_secs assignment and rename the test to
prune_reduces_outbox_to_cap.
In `@clients/openframe-client/src/services/result_store.rs`:
- Around line 106-126: Update encode_result to serialize the result once,
calculate the available payload length from the serialized overhead and
OUTBOX_MAX_PAYLOAD_BYTES, then truncate stdout or stderr in a single step rather
than repeatedly shrinking by roughly 25%. Retain the existing serialization loop
only as a final correction for any remaining size overflow, preserving the
current truncation marker and field-priority behavior.
In `@clients/openframe-client/src/utils/timed_permit_pool_tests.rs`:
- Around line 26-90: Replace timing-based sleeps in
permit_is_held_past_timeout_and_freed_when_the_call_returns and
call_over_capacity_fails_fast_while_slots_are_busy with explicit
synchronization: signal from each holder closure after it starts, wait for all
signals before asserting capacity exhaustion, and poll until the timed-out
call’s permit is released before verifying recovery. Avoid serializing holder
startup through the shared Receiver mutex; use per-holder start notifications or
equivalent synchronization while preserving the existing release and completion
assertions.
In `@clients/openframe-client/src/utils/timed_permit_pool.rs`:
- Around line 22-53: Update timed_permit_pool::call to establish one deadline at
the start and use it for both permit acquisition and blocking-task execution,
replacing the second relative timeout with deadline-based timeout handling.
Preserve the existing permit-pool and task error behavior while ensuring the
total call duration never exceeds the supplied timeout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
3 participants
What & why
Third sync round of the shared Rust agent from
openframe-oss-tenant. The previous port (#1527) covered tenant history through922f664d7(Jul 22); the Technical-Writer commit was represented separately by this repo's own rollout (#1560). This PR ports every client commit merged to tenantmainsince, through17ebc0cdc(Aug 7) — 19 commits, 1:1, original authorship and messages preserved.Ported commits (tenant → this branch)
42d2ed8c67b2725b2b9b221a02873e81b3d17be6ace654f6f84005c70a411ac677b76bf68c830676700ce32c1be2b52170d14b9ae4f20c8b741ca1e1563895c4e02a0850fde6602ca25f0fb58604812c0674c6981b26478bcd793b0f0e72678165462763da3658e311e968ecca8f6c98c05febb5737aa0e7f439da2dcdde04f8dcf58c3462a4e3873ce7e64ee86a0f3a815fdff909f8ad671970e226a7a215c353c817ebc0cdcdf8c756d1Deliberately not ported:
b77770fab(🦩 Technical Writer #2198) — each repo runs its own doc rollout (#1560 here); tenant's per-commit doc updates ride along inside the ported commits instead.Plus two follow-up commits:
28108758e— re-applies this repo's clippy deltas (-D warningsgate) that whole-file conflict resolutions in the test-split port had dropped (platform allows,DirectoryManagerDefaultimpl,truncate(true),is_some_and, and the test(client): ignore privileged/interactive tests that fail or hang in headless CI #1352 CI-safety test attributes), and adds two fixes fresh tenant code needs under this repo's gate:#[allow(clippy::too_many_arguments)]onMeshSelfHealService::new(8th arg from the deactivation port) andinspect_errreplacing a cfg-windowsmap_errthat is an identity map on unix (same fix applies upstream).cd0233bb0— adds tenant's writer docs fordeactivation_service,result_store,result_outbox_run_manager: these services were created after 🦩 Flamingo AI Technical Writer #1560 generated docs here, so their.mdfiles come from the tenant writer run that covered them.Deviations from verbatim tenant code
Same policy as #1359/#1527:
cargo fmtfolded per commit (pre-commit hook), extraction-era idioms preserved (sorted module/import lists,bin-feature Cargo.toml, thinsrc/bin+cli.rs), and the clippy follow-up above. The CI-safety test attributes from #1352 survive the tenant test-file split:test_ensure_adminkeeps its#[ignore]in the newpermissions_tests.rs, and both root-required directory tests keep theircfg_attr(not(windows), ignore)guards indirectories_tests.rs(tenant's split files carried the other four ignores already).Verification
17ebc0cdc) vs this branch: every difference is a known lib-specific delta (Cargo lib config + lock, Makefile, README, thin-bin/cli files, the follow-up commits above) or a.mddoc-lineage difference..rsfiles, the per-file delta against fmt(tenant@922f664d7) vs currentmainis byte-identical outside the documented follow-ups: 0 violations. The port introduced no new drift.DeactivationService,ResultStore+ outbox, scheduled-scripts listener,scm_call_timed,clear_aside_binary,FailureLogBackoff, WebView2 doctor check); removed code absent (EXECUTION_MIN_CONCURRENCYsemaphore,clear_running_toolcall).service-managerresolves to0.11.0in bothCargo.tomlandCargo.lock.Gates
cargo clippy --all-targets --features bin -- -D warnings✅ (and the no-features hook variant ✅)cargo fmt --all -- --check✅ every commitcargo test --features bin: 125 passed, 0 failed, 3 ignored (CI-safe ignores intact)Notes for open PRs
tool_agent_update_service/tool_uninstall_service, both reshaped again here — it needs another rebase after this merges; tenant still has neither fix.hotfix/machine-id-header(Rust-client machine-id work, no tenant PR yet) is not in tenantmainand is deliberately not part of this round; its Java-side counterpart is feat: machine-id firewall header — bundled mesh core (Host fix) + openframe-client #1688 here.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes