OpenClaw agent lifecycle and explicit microvm selection#556
OpenClaw agent lifecycle and explicit microvm selection#556justinmoon merged 12 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds agent-kind and marketplace support across client, core, server, spawner, and CLI: new AgentKind/AgentStartupPhase types, showAgentMarketplace flag, EnsureAgentKind/SetShowAgentMarketplace actions/APIs, VM guest_ready markers and a GET /vms/{id} status endpoint, UI flows for selecting Pi/OpenClaw, and removal of poll-based progress. Changes
Sequence Diagram(s)sequenceDiagram
actor User as User (UI)
participant AppMgr as AppManager (Mobile)
participant Core as AppCore (Rust)
participant API as Pika Server (Agent API)
participant Spawner as VM Spawner (microvm)
participant Guest as Guest VM (Agent)
User->>AppMgr: ensureAgent(kind: Pi)
AppMgr->>Core: dispatch EnsureAgentKind
Core->>Core: set provisioning.agent_kind = Pi\rphase = Requested
Core->>API: POST /agents/ensure (body.kind=Pi)
API->>Spawner: create_vm (kind=Pi, backend=...)
Spawner->>Spawner: build_create_vm_request (inject PIKA_AGENT_KIND=pi)
Spawner->>Guest: launch VM with autostart script
Guest->>Guest: boot agent service -> write READY_MARKER
Spawner->>Spawner: VmManager.status(vm_id) detects guest_ready
API->>Spawner: GET /vms/{id}
Spawner-->>API: VmResponse { guest_ready: true }
API-->>Core: AgentStateResponse (startup_phase=Ready)
Core-->>AppMgr: update provisioning phase -> Ready
AppMgr-->>User: UI shows Ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fn resolve_kind(kind: Option<MicrovmAgentKind>) -> ResolvedMicrovmAgentKind { | ||
| match kind.unwrap_or(MicrovmAgentKind::Pi) { | ||
| MicrovmAgentKind::Pi => ResolvedMicrovmAgentKind::Pi, | ||
| MicrovmAgentKind::Openclaw => ResolvedMicrovmAgentKind::Openclaw, | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Default resolve_kind chooses Pi which immediately fails validation against default Native backend
The new resolve_kind function defaults to MicrovmAgentKind::Pi when no kind is specified (crates/pika-agent-microvm/src/lib.rs:312), while resolve_backend defaults to Native when no backend is specified (crates/pika-agent-microvm/src/lib.rs:320). The new validate_resolved_params at crates/pika-agent-microvm/src/lib.rs:300-308 explicitly rejects Pi + Native as an invalid combination. This means any code path that calls resolved_spawner_params(None) — with no client-provided overrides — will fail validation.
This breaks agent_api_healthcheck() at crates/pika-server/src/agent_api.rs:817 (which calls resolved_spawner_params(None)), causing it to fail on server startup unless PIKA_AGENT_MICROVM_KIND=openclaw is explicitly set in the server environment. It also breaks refresh_agent_from_spawner() at crates/pika-server/src/agent_api.rs:352, silently preventing startup-phase enrichment on every GET /v1/agents/me call.
The app's own default is Openclaw (rust/src/core/agent.rs:513), and the CLI demos also default to Openclaw+Native. The microvm-level default should match to avoid breaking existing server deployments that have no PIKA_AGENT_MICROVM_KIND env var set.
| fn resolve_kind(kind: Option<MicrovmAgentKind>) -> ResolvedMicrovmAgentKind { | |
| match kind.unwrap_or(MicrovmAgentKind::Pi) { | |
| MicrovmAgentKind::Pi => ResolvedMicrovmAgentKind::Pi, | |
| MicrovmAgentKind::Openclaw => ResolvedMicrovmAgentKind::Openclaw, | |
| } | |
| } | |
| fn resolve_kind(kind: Option<MicrovmAgentKind>) -> ResolvedMicrovmAgentKind { | |
| match kind.unwrap_or(MicrovmAgentKind::Openclaw) { | |
| MicrovmAgentKind::Pi => ResolvedMicrovmAgentKind::Pi, | |
| MicrovmAgentKind::Openclaw => ResolvedMicrovmAgentKind::Openclaw, | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
justfile (1)
995-1001: Factor the kind/backend mapping into shared helper recipes.These four recipes duplicate the same
PIKA_AGENT_MICROVM_KIND+PIKA_AGENT_MICROVM_BACKENDpairing. Keeping the mapping in one place will reduce drift when one backend changes later.♻️ Possible cleanup
+agent-ensure KIND BACKEND *ARGS="": + PIKA_AGENT_MICROVM_KIND={{ KIND }} PIKA_AGENT_MICROVM_BACKEND={{ BACKEND }} ./scripts/demo-agent-microvm.sh {{ ARGS }} + +agent-chat KIND BACKEND MESSAGE="CLI demo check: reply with ACK and one short sentence.": + PIKA_AGENT_MICROVM_KIND={{ KIND }} PIKA_AGENT_MICROVM_BACKEND={{ BACKEND }} ./scripts/agent-demo.sh "{{ MESSAGE }}" + # Run the HTTP agent ensure demo for a Pi ACP-backed guest. agent-pi-ensure *ARGS="": - PIKA_AGENT_MICROVM_KIND=pi PIKA_AGENT_MICROVM_BACKEND=acp ./scripts/demo-agent-microvm.sh {{ ARGS }} + just agent-ensure pi acp {{ ARGS }} # Run the HTTP agent ensure demo for an OpenClaw guest over the daemon protocol. agent-claw-ensure *ARGS="": - PIKA_AGENT_MICROVM_KIND=openclaw PIKA_AGENT_MICROVM_BACKEND=native ./scripts/demo-agent-microvm.sh {{ ARGS }} + just agent-ensure openclaw native {{ ARGS }} # Reset the current test account's VM on pika-build, recover/create a Pi ACP guest, then chat. agent-pi MESSAGE="CLI demo check: reply with ACK and one short sentence.": - PIKA_AGENT_MICROVM_KIND=pi PIKA_AGENT_MICROVM_BACKEND=acp ./scripts/agent-demo.sh "{{ MESSAGE }}" + just agent-chat pi acp "{{ MESSAGE }}" # Reset the current test account's VM on pika-build, recover/create an OpenClaw guest, then chat. agent-claw MESSAGE="CLI demo check: reply with ACK and one short sentence.": - PIKA_AGENT_MICROVM_KIND=openclaw PIKA_AGENT_MICROVM_BACKEND=native ./scripts/agent-demo.sh "{{ MESSAGE }}" + just agent-chat openclaw native "{{ MESSAGE }}"Also applies to: 1043-1049
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 995 - 1001, Consolidate the duplicated PIKA_AGENT_MICROVM_KIND/PIKA_AGENT_MICROVM_BACKEND mappings into a shared helper recipe (e.g., a recipe called agent-env or agent-ensure-helper) and have agent-pi-ensure and agent-claw-ensure invoke that helper with their specific kind/backend values; specifically remove the repeated inline environment variable pairs from agent-pi-ensure and agent-claw-ensure and instead call the helper with the appropriate KIND and BACKEND for each (referencing the existing recipe names agent-pi-ensure, agent-claw-ensure and the env vars PIKA_AGENT_MICROVM_KIND and PIKA_AGENT_MICROVM_BACKEND so you can find where to replace the duplications).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/pika-agent-microvm/src/lib.rs`:
- Around line 513-554: Both wait_for_pi_ready and wait_for_openclaw_ready bail
out immediately when the child process dies but never emit the failed marker;
update both functions so that in the branch where kill -0 fails you capture the
child's exit status via wait "$agent_pid", call write_failed_marker with a clear
reason (e.g. "agent_exited_before_ready" or "openclaw_exited_before_ready")
before returning the exit code, ensuring write_failed_marker and wait are
invoked in that branch to mark hard failures.
In `@crates/pika-server/src/agent_api.rs`:
- Around line 489-497: The code currently overlays requested.kind/backend into
params then resolves and validates after AgentInstance::create is called in
provision_agent_for_owner, which can leave a persisted errored agent; change the
flow so that resolve_params(¶ms) and validate_resolved_params(&resolved) are
called and succeed before any persistent mutation: compute the final params by
merging requested into params, call resolve_params(...) and
validate_resolved_params(...) immediately (failing the request on error), and
only after validation call AgentInstance::create(...) (or the create path used
in provision_agent_for_owner) to persist the new agent instance.
- Around line 615-617: The response currently hard-codes
AgentStartupPhase::ProvisioningVm for reprovision paths; instead call
startup_phase_from_spawner_vm on the normalized spawner row so the returned
startup_phase matches the spawner-derived state. Update the json_response calls
(e.g., the branch handling Ok(reprovisioned) and the similar branch at lines
665-669) to pass startup_phase_from_spawner_vm(&reprovisioned) (or &recovered
where appropriate) rather than AgentStartupPhase::ProvisioningVm; locate these
in the provison_agent_for_owner() response paths and make the same change as the
recover path that uses startup_phase_from_spawner_vm(&recovered).
- Around line 335-420: The function refresh_agent_from_spawner currently holds a
&mut PgConnection (conn) across the await to
MicrovmSpawnerClient::get_vm_with_request_id, which blocks a DB pool slot;
refactor so no mutable DB connection is borrowed during the spawner call: change
the function to not accept &mut PgConnection (e.g., accept a reference to the
PgPool or a connection factory instead), capture row.agent_id and vm_id (clone
strings) and call spawner.get_vm_with_request_id(...) without any DB borrow in
scope, then after the await reacquire a fresh &mut PgConnection to perform
mark_agent_errored and AgentInstance::update_phase; ensure you update call sites
to supply the pool/factory and keep logic around phase_from_spawner_vm,
startup_phase_from_spawner_vm, and the error-handling branches intact.
In `@crates/vm-spawner/src/manager.rs`:
- Around line 65-66: The readiness marker constants GUEST_READY_MARKER_PATH and
GUEST_FAILED_MARKER_PATH currently point to "workspace/pika-agent/..." but the
guest writes markers under the persisted read-write mount at ./home (guest-side
symlink makes /workspace -> /root), so update those constants to
"home/pika-agent/service-ready.json" and "home/pika-agent/service-failed.json"
(so status() and any checks that reference GUEST_READY_MARKER_PATH /
GUEST_FAILED_MARKER_PATH will look for the markers in the correct persisted
mount).
- Around line 281-289: The match currently maps
unit_active_state(...).await.as_deref() None (and unknown states) to "starting",
which hides lookup failures; change the mapping in manager.rs so
unit_active_state(...) returning None yields an explicit error/unknown status
(e.g., "unknown" or "missing") instead of "starting", and reserve "starting" for
actual transitional states (activating/reloading). Update the match arms for
Some("active") -> "running", Some("failed") -> "failed",
Some("activating")|Some("reloading") -> "starting", Some(_) -> "unknown" (or
another explicit error label), and None -> "unknown" so callers of the status
API can distinguish backend/unit lookup failures from a healthy transitional VM.
In `@rust/src/core/agent.rs`:
- Around line 154-164: The Pi provisioning request currently only sets kind and
leaves backend as None, causing the server to default/reject Pi; in the
AgentProvisionRequest/MicrovmProvisionParams builder (where agent_kind is
matched to MicrovmAgentKind::Pi), set backend to Some(MicrovmBackend::Acp) when
agent_kind == crate::state::AgentKind::Pi (update both occurrences around the
shown block and the similar block at lines ~247-257) so the Pi request includes
the ACP backend; if backend selection should come from app state instead, plumb
that value into the MicrovmProvisionParams backend field and use it here.
In `@rust/src/core/mod.rs`:
- Around line 3075-3078: When clearing persisted app settings in the logout
branch (the block that calls profile_db::clear_all(conn) and
profile_db::clear_app_settings(conn) on self.profile_db.as_ref()), also reset
the in-memory flag self.state.show_agent_marketplace to the default (e.g.,
false) so the next same-process login doesn't inherit the previous user's
preference; mirror the behavior performed by the wipe_local_data() function by
assigning the same default to self.state.show_agent_marketplace in that branch.
---
Nitpick comments:
In `@justfile`:
- Around line 995-1001: Consolidate the duplicated
PIKA_AGENT_MICROVM_KIND/PIKA_AGENT_MICROVM_BACKEND mappings into a shared helper
recipe (e.g., a recipe called agent-env or agent-ensure-helper) and have
agent-pi-ensure and agent-claw-ensure invoke that helper with their specific
kind/backend values; specifically remove the repeated inline environment
variable pairs from agent-pi-ensure and agent-claw-ensure and instead call the
helper with the appropriate KIND and BACKEND for each (referencing the existing
recipe names agent-pi-ensure, agent-claw-ensure and the env vars
PIKA_AGENT_MICROVM_KIND and PIKA_AGENT_MICROVM_BACKEND so you can find where to
replace the duplications).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7f846fb-6c8a-41a7-895a-89cf9da25dea
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
android/app/src/main/java/com/pika/app/AppManager.ktandroid/app/src/main/java/com/pika/app/rust/pika_core.ktandroid/app/src/main/java/com/pika/app/ui/PikaApp.ktandroid/app/src/main/java/com/pika/app/ui/screens/ChatListScreen.ktandroid/app/src/main/java/com/pika/app/ui/screens/MyProfileScreen.ktcli/src/main.rscrates/pika-agent-control-plane/src/lib.rscrates/pika-agent-microvm/src/lib.rscrates/pika-server/src/agent_api.rscrates/vm-spawner/src/main.rscrates/vm-spawner/src/manager.rsios/Sources/AppManager.swiftios/Sources/ContentView.swiftios/Sources/ViewState.swiftios/Sources/Views/AgentProvisioningView.swiftios/Sources/Views/ChatListView.swiftios/Sources/Views/MyNpubQrSheet.swiftjustfilerust/Cargo.tomlrust/src/actions.rsrust/src/core/agent.rsrust/src/core/mod.rsrust/src/core/profile_db.rsrust/src/state.rsrust/src/updates.rsscripts/agent-demo.shscripts/demo-agent-microvm.sh
💤 Files with no reviewable changes (3)
- rust/src/updates.rs
- android/app/src/main/java/com/pika/app/ui/PikaApp.kt
- ios/Sources/Views/AgentProvisioningView.swift
| async fn refresh_agent_from_spawner( | ||
| conn: &mut PgConnection, | ||
| row: AgentInstance, | ||
| request_id: &str, | ||
| ) -> Result<RefreshedAgentStatus, AgentApiError> { | ||
| if row.phase == AGENT_PHASE_READY { | ||
| return Ok(RefreshedAgentStatus { | ||
| row, | ||
| startup_phase: AgentStartupPhase::Ready, | ||
| }); | ||
| } | ||
| let Some(vm_id) = row.vm_id.as_deref() else { | ||
| return Ok(RefreshedAgentStatus { | ||
| row, | ||
| startup_phase: AgentStartupPhase::Requested, | ||
| }); | ||
| }; | ||
| let resolved = match resolved_spawner_params(None) { | ||
| Ok(resolved) => resolved, | ||
| Err(err) => { | ||
| tracing::warn!( | ||
| request_id, | ||
| agent_id = %row.agent_id, | ||
| vm_id, | ||
| error = %err, | ||
| "failed to resolve spawner params while refreshing agent readiness" | ||
| ); | ||
| return Ok(RefreshedAgentStatus { | ||
| startup_phase: startup_phase_from_row_phase(&row.phase) | ||
| .unwrap_or(AgentStartupPhase::ProvisioningVm), | ||
| row, | ||
| }); | ||
| } | ||
| }; | ||
| let spawner = MicrovmSpawnerClient::new(resolved.spawner_url); | ||
| let vm = match spawner | ||
| .get_vm_with_request_id(vm_id, Some(request_id)) | ||
| .await | ||
| { | ||
| Ok(vm) => vm, | ||
| Err(err) if is_vm_not_found_error(&err) => { | ||
| tracing::warn!( | ||
| request_id, | ||
| agent_id = %row.agent_id, | ||
| vm_id, | ||
| error = %err, | ||
| "agent vm missing during readiness refresh; marking row errored" | ||
| ); | ||
| let errored = mark_agent_errored(conn, &row.agent_id) | ||
| .map_err(|err| err.with_request_id(request_id.to_string()))?; | ||
| return Ok(RefreshedAgentStatus { | ||
| row: errored, | ||
| startup_phase: AgentStartupPhase::Failed, | ||
| }); | ||
| } | ||
| Err(err) => { | ||
| tracing::warn!( | ||
| request_id, | ||
| agent_id = %row.agent_id, | ||
| vm_id, | ||
| error = %err, | ||
| "failed to refresh agent readiness from spawner; keeping existing phase" | ||
| ); | ||
| return Ok(RefreshedAgentStatus { | ||
| startup_phase: startup_phase_from_row_phase(&row.phase) | ||
| .unwrap_or(AgentStartupPhase::ProvisioningVm), | ||
| row, | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| let next_phase = phase_from_spawner_vm(&vm); | ||
| let startup_phase = startup_phase_from_spawner_vm(&vm); | ||
| if row.phase == next_phase && row.vm_id.as_deref() == Some(vm.id.as_str()) { | ||
| return Ok(RefreshedAgentStatus { row, startup_phase }); | ||
| } | ||
|
|
||
| let updated = AgentInstance::update_phase(conn, &row.agent_id, next_phase, Some(&vm.id)) | ||
| .map_err(|_| { | ||
| AgentApiError::from_code(AgentApiErrorCode::Internal) | ||
| .with_request_id(request_id.to_string()) | ||
| })?; | ||
| Ok(RefreshedAgentStatus { | ||
| row: updated, | ||
| startup_phase, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A40 -B5 'async fn refresh_agent_from_spawner|get_vm_with_request_id|mark_agent_errored|update_phase\(' crates/pika-server/src/agent_api.rsRepository: sledtools/pika
Length of output: 12790
Release the DB connection before polling vm-spawner.
This function holds &mut PgConnection across get_vm_with_request_id(...).await, blocking a pool connection slot for the entire spawner timeout duration. Under concurrent agent launches on the hot polling path (/v1/agents/me), this can exhaust the connection pool. Refactor to fetch VM status without the borrowed connection, then reacquire it only for mark_agent_errored and update_phase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pika-server/src/agent_api.rs` around lines 335 - 420, The function
refresh_agent_from_spawner currently holds a &mut PgConnection (conn) across the
await to MicrovmSpawnerClient::get_vm_with_request_id, which blocks a DB pool
slot; refactor so no mutable DB connection is borrowed during the spawner call:
change the function to not accept &mut PgConnection (e.g., accept a reference to
the PgPool or a connection factory instead), capture row.agent_id and vm_id
(clone strings) and call spawner.get_vm_with_request_id(...) without any DB
borrow in scope, then after the await reacquire a fresh &mut PgConnection to
perform mark_agent_errored and AgentInstance::update_phase; ensure you update
call sites to supply the pool/factory and keep logic around
phase_from_spawner_vm, startup_phase_from_spawner_vm, and the error-handling
branches intact.
| Ok(reprovisioned) => { | ||
| json_response(reprovisioned, AgentStartupPhase::ProvisioningVm, request_id) | ||
| } |
There was a problem hiding this comment.
Return the spawner-derived startup phase here.
Both response sites hard-code AgentStartupPhase::ProvisioningVm, even though provision_agent_for_owner() has already normalized the row from the spawner's actual status and guest_ready. If VM creation comes back running + guest_ready or failed, the API can emit contradictory state / startup_phase pairs. The recover path already uses startup_phase_from_spawner_vm(&recovered); these two paths should do the same.
Also applies to: 665-669
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pika-server/src/agent_api.rs` around lines 615 - 617, The response
currently hard-codes AgentStartupPhase::ProvisioningVm for reprovision paths;
instead call startup_phase_from_spawner_vm on the normalized spawner row so the
returned startup_phase matches the spawner-derived state. Update the
json_response calls (e.g., the branch handling Ok(reprovisioned) and the similar
branch at lines 665-669) to pass startup_phase_from_spawner_vm(&reprovisioned)
(or &recovered where appropriate) rather than AgentStartupPhase::ProvisioningVm;
locate these in the provison_agent_for_owner() response paths and make the same
change as the recover path that uses startup_phase_from_spawner_vm(&recovered).
| const GUEST_READY_MARKER_PATH: &str = "workspace/pika-agent/service-ready.json"; | ||
| const GUEST_FAILED_MARKER_PATH: &str = "workspace/pika-agent/service-failed.json"; |
There was a problem hiding this comment.
Point readiness markers at the persisted home mount.
./home is the only host path mounted read-write into the guest, and /workspace is created as a guest-side symlink to /root. With these constants under workspace/..., status() looks for state/<vm>/workspace/..., but the guest will actually write state/<vm>/home/.... In production that keeps guest_ready false even after the service writes its ready marker.
🛠️ Proposed fix
-const GUEST_READY_MARKER_PATH: &str = "workspace/pika-agent/service-ready.json";
-const GUEST_FAILED_MARKER_PATH: &str = "workspace/pika-agent/service-failed.json";
+const GUEST_READY_MARKER_PATH: &str = "home/pika-agent/service-ready.json";
+const GUEST_FAILED_MARKER_PATH: &str = "home/pika-agent/service-failed.json";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const GUEST_READY_MARKER_PATH: &str = "workspace/pika-agent/service-ready.json"; | |
| const GUEST_FAILED_MARKER_PATH: &str = "workspace/pika-agent/service-failed.json"; | |
| const GUEST_READY_MARKER_PATH: &str = "home/pika-agent/service-ready.json"; | |
| const GUEST_FAILED_MARKER_PATH: &str = "home/pika-agent/service-failed.json"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm-spawner/src/manager.rs` around lines 65 - 66, The readiness marker
constants GUEST_READY_MARKER_PATH and GUEST_FAILED_MARKER_PATH currently point
to "workspace/pika-agent/..." but the guest writes markers under the persisted
read-write mount at ./home (guest-side symlink makes /workspace -> /root), so
update those constants to "home/pika-agent/service-ready.json" and
"home/pika-agent/service-failed.json" (so status() and any checks that reference
GUEST_READY_MARKER_PATH / GUEST_FAILED_MARKER_PATH will look for the markers in
the correct persisted mount).
| let status = match unit_active_state(&self.cfg.systemctl_cmd, &unit_name) | ||
| .await | ||
| .as_deref() | ||
| { | ||
| Some("active") => "running", | ||
| Some("failed") => "failed", | ||
| Some("activating") | Some("reloading") => "starting", | ||
| Some(_) | None => "starting", | ||
| }; |
There was a problem hiding this comment.
Don’t collapse status lookup failures into "starting".
unit_active_state() returns None when systemctl show fails or the unit is missing, and Line 288 turns that into a transitional "starting" state. That makes the status API report a healthy-looking poll target for VMs that are already gone or for backend lookup failures.
🛠️ Tighten the mapping
- let status = match unit_active_state(&self.cfg.systemctl_cmd, &unit_name)
- .await
- .as_deref()
- {
- Some("active") => "running",
- Some("failed") => "failed",
- Some("activating") | Some("reloading") => "starting",
- Some(_) | None => "starting",
- };
+ let active_state = unit_active_state(&self.cfg.systemctl_cmd, &unit_name)
+ .await
+ .ok_or_else(|| anyhow!("failed to query ActiveState for {unit_name}"))?;
+ let status = match active_state.as_str() {
+ "active" => "running",
+ "failed" => "failed",
+ "activating" | "reloading" => "starting",
+ "inactive" | "deactivating" => "failed",
+ other => return Err(anyhow!("unexpected ActiveState `{other}` for {unit_name}")),
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm-spawner/src/manager.rs` around lines 281 - 289, The match currently
maps unit_active_state(...).await.as_deref() None (and unknown states) to
"starting", which hides lookup failures; change the mapping in manager.rs so
unit_active_state(...) returning None yields an explicit error/unknown status
(e.g., "unknown" or "missing") instead of "starting", and reserve "starting" for
actual transitional states (activating/reloading). Update the match arms for
Some("active") -> "running", Some("failed") -> "failed",
Some("activating")|Some("reloading") -> "starting", Some(_) -> "unknown" (or
another explicit error label), and None -> "unknown" so callers of the status
API can distinguish backend/unit lookup failures from a healthy transitional VM.
| let body = serde_json::to_value(AgentProvisionRequest { | ||
| microvm: Some(MicrovmProvisionParams { | ||
| spawner_url: None, | ||
| kind: Some(match agent_kind { | ||
| crate::state::AgentKind::Openclaw => MicrovmAgentKind::Openclaw, | ||
| crate::state::AgentKind::Pi => MicrovmAgentKind::Pi, | ||
| }), | ||
| backend: None, | ||
| }), | ||
| }) | ||
| .map_err(|_| AgentFlowError::InvalidResponse)?; |
There was a problem hiding this comment.
Pi provisioning from the app is still missing the required ACP backend.
These bodies only set kind. When agent_kind == Pi, the server resolves the missing backend to its default/native mode and rejects the request, so Pi selection cannot ensure or recover successfully from the app. Set ACP here for Pi, or plumb backend choice through the app state as well.
Minimal fix
- backend: None,
+ backend: matches!(agent_kind, crate::state::AgentKind::Pi).then_some(
+ pika_agent_control_plane::MicrovmAgentBackend::Acp {
+ exec_command: None,
+ cwd: None,
+ },
+ ),Apply the same change in both request builders.
Also applies to: 247-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/core/agent.rs` around lines 154 - 164, The Pi provisioning request
currently only sets kind and leaves backend as None, causing the server to
default/reject Pi; in the AgentProvisionRequest/MicrovmProvisionParams builder
(where agent_kind is matched to MicrovmAgentKind::Pi), set backend to
Some(MicrovmBackend::Acp) when agent_kind == crate::state::AgentKind::Pi (update
both occurrences around the shown block and the similar block at lines ~247-257)
so the Pi request includes the ACP backend; if backend selection should come
from app state instead, plumb that value into the MicrovmProvisionParams backend
field and use it here.
| if let Some(conn) = self.profile_db.as_ref() { | ||
| profile_db::clear_all(conn); | ||
| profile_db::clear_app_settings(conn); | ||
| } |
There was a problem hiding this comment.
Reset show_agent_marketplace when clearing app settings on logout.
Line 3077 clears the persisted setting, but this branch leaves self.state.show_agent_marketplace untouched. After a same-process logout/login, the next session can still inherit the previous user's marketplace preference, and Line 4870 will then early-return before the setting is written again. wipe_local_data() already resets this field at Line 3104, so logout should stay in sync here too.
Suggested fix
if let Some(conn) = self.profile_db.as_ref() {
profile_db::clear_all(conn);
profile_db::clear_app_settings(conn);
}
+ self.state.show_agent_marketplace = false;
profile_pics::clear_cache(&self.data_dir);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(conn) = self.profile_db.as_ref() { | |
| profile_db::clear_all(conn); | |
| profile_db::clear_app_settings(conn); | |
| } | |
| if let Some(conn) = self.profile_db.as_ref() { | |
| profile_db::clear_all(conn); | |
| profile_db::clear_app_settings(conn); | |
| } | |
| self.state.show_agent_marketplace = false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/core/mod.rs` around lines 3075 - 3078, When clearing persisted app
settings in the logout branch (the block that calls profile_db::clear_all(conn)
and profile_db::clear_app_settings(conn) on self.profile_db.as_ref()), also
reset the in-memory flag self.state.show_agent_marketplace to the default (e.g.,
false) so the next same-process login doesn't inherit the previous user's
preference; mirror the behavior performed by the wipe_local_data() function by
assigning the same default to self.state.show_agent_marketplace in that branch.
| self.profiles.clear(); | ||
| if let Some(conn) = self.profile_db.as_ref() { | ||
| profile_db::clear_all(conn); | ||
| profile_db::clear_app_settings(conn); |
There was a problem hiding this comment.
🟡 Logout now inadvertently clears developer_mode from database via new clear_app_settings call
The new profile_db::clear_app_settings(conn) call was added to the logout code path (set_session(false)) at rust/src/core/mod.rs:3077 alongside the pre-existing clear_all call. clear_app_settings deletes the entire app_settings table, which contains both the new show_agent_marketplace key and the pre-existing developer_mode key. Before this PR, developer_mode was intentionally preserved across logouts (only cleared on explicit "Wipe All Local Data" via wipe_local_data()). Now, every logout silently clears the developer_mode DB row. Additionally, the in-memory self.state.developer_mode is not reset in this code path (unlike wipe_local_data at rust/src/core/mod.rs:3104 which does set it to false), creating an inconsistency where developer mode appears active until the app restarts, at which point it's gone.
Affected code paths
The logout branch clears the DB but not the in-memory flag:
// rust/src/core/mod.rs — set_session(false) branch
if let Some(conn) = self.profile_db.as_ref() {
profile_db::clear_all(conn);
profile_db::clear_app_settings(conn); // NEW — clears developer_mode too
}
// self.state.developer_mode is NOT set to false hereCompare with wipe_local_data which correctly resets both:
self.state.developer_mode = false;
self.state.show_agent_marketplace = false;Prompt for agents
In rust/src/core/mod.rs at line 3077, the call to profile_db::clear_app_settings(conn) in the logout (set_session false) code path inadvertently clears developer_mode from the database. Instead of clearing the entire app_settings table on logout, only clear the show_agent_marketplace setting. You can either:
1. Replace clear_app_settings with a targeted delete: add a new function like clear_show_agent_marketplace(conn) that only deletes the show_agent_marketplace key from app_settings.
2. Or simply remove the clear_app_settings call from the logout path entirely, since show_agent_marketplace is a developer-mode feature that arguably should also persist across logouts (it's already cleared in wipe_local_data).
Also ensure that the wipe_local_data path in rust/src/core/mod.rs continues to clear both settings (it already does via file deletion + DB recreation).
Was this helpful? React with 👍 or 👎 to provide feedback.
| microvm: Some(MicrovmProvisionParams { | ||
| spawner_url: None, | ||
| kind: Some(match agent_kind { | ||
| crate::state::AgentKind::Openclaw => MicrovmAgentKind::Openclaw, | ||
| crate::state::AgentKind::Pi => MicrovmAgentKind::Pi, | ||
| }), | ||
| backend: None, | ||
| }), |
There was a problem hiding this comment.
🔴 Mobile Pi agent provisioning always fails: request sends kind=Pi but no backend, server resolves to Pi+Native which fails validation
When a user selects "Pi" from the agent marketplace on mobile, ensure_agent in rust/src/core/agent.rs sends an AgentProvisionRequest with kind: Some(Pi) and backend: None. On the server at crates/pika-server/src/agent_api.rs:489-494, the overlay logic only applies the kind (since requested.backend.is_none(), the backend is not overlaid). After resolve_params, the resolved kind is Pi and the resolved backend defaults to Native (crates/pika-agent-microvm/src/lib.rs:320). Then validate_resolved_params at crates/pika-agent-microvm/src/lib.rs:301-307 rejects Pi + Native with an error. The same issue affects recover_my_agent at rust/src/core/agent.rs:248-257. The Pi agent marketplace feature is completely non-functional from mobile apps — provisioning and recovery both fail with a validation error.
Affected code path
Mobile sends (rust/src/core/agent.rs:155-162):
let body = serde_json::to_value(AgentProvisionRequest {
microvm: Some(MicrovmProvisionParams {
spawner_url: None,
kind: Some(MicrovmAgentKind::Pi),
backend: None, // ← no backend sent
}),
})Server resolves (crates/pika-agent-microvm/src/lib.rs:318-320):
fn resolve_backend(backend: Option<&MicrovmAgentBackend>) -> ... {
match backend {
Some(MicrovmAgentBackend::Native) | None => ResolvedMicrovmAgentBackend::Native,
// ↑ None defaults to Native
}
}Validation rejects Pi+Native (crates/pika-agent-microvm/src/lib.rs:301-307).
Prompt for agents
The mobile app sends kind=Pi but backend=None for Pi agent provisioning. The server's resolve_backend defaults None to Native, and validate_resolved_params rejects Pi+Native. Fix this by having the mobile app also send the correct backend for Pi agents. In rust/src/core/agent.rs, in both the ensure_agent and recover_my_agent functions, change the AgentProvisionRequest construction to infer backend from kind:
backend: match agent_kind {
crate::state::AgentKind::Pi => Some(MicrovmAgentBackend::Acp { exec_command: None, cwd: None }),
crate::state::AgentKind::Openclaw => None,
},
This needs to be applied in both ensure_agent (around line 155) and recover_my_agent (around line 248) in rust/src/core/agent.rs.
Was this helpful? React with 👍 or 👎 to provide feedback.
The new validation rejects Pi+Native, but test fixtures weren't setting PIKA_AGENT_MICROVM_KIND. Default it to openclaw so pika-server starts correctly in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
justinmoon
force-pushed
the
openclaw-landed
branch
from
2bbb488 to
b55f2b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
crates/pika-agent-microvm/src/lib.rs (1)
513-554:⚠️ Potential issue | 🟠 MajorWrite the failed marker when the guest process dies before readiness.
Both wait loops still return immediately when the child exits before becoming ready, but they never call
write_failed_marker(...)on that path. That leaves startup polling without a hard-failure marker and can misclassify a crash-on-boot as a stuck launch.Representative fix
if ! kill -0 "$agent_pid" 2>/dev/null; then - wait "$agent_pid" - return $? + wait "$agent_pid" || true + write_failed_marker "pi_agent_exited_before_ready" + return 1 fi @@ if ! kill -0 "$agent_pid" 2>/dev/null; then - wait "$agent_pid" - return $? + wait "$agent_pid" || true + write_failed_marker "openclaw_exited_before_ready" + return 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pika-agent-microvm/src/lib.rs` around lines 513 - 554, Both wait_for_pi_ready and wait_for_openclaw_ready bail out immediately if the child process dies (the ! kill -0 "$agent_pid" branch) without writing a failure marker; modify those branches to call write_failed_marker with a descriptive key (e.g., "agent_exited_before_ready" or "openclaw_exited_before_ready") before returning the child's exit code so a crash-on-boot is recorded as a hard failure (ensure you still wait "$agent_pid" to capture its exit status and return that status after writing the marker).crates/pika-server/src/agent_api.rs (3)
615-617:⚠️ Potential issue | 🟠 MajorReturn the spawner-derived startup phase here.
These response paths still hard-code
AgentStartupPhase::ProvisioningVm, even though the provisioning call has already normalized the row from the spawner's actualstatusandguest_readyvalues. That can send contradictorystate/startup_phasepairs back to clients.Also applies to: 665-669
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pika-server/src/agent_api.rs` around lines 615 - 617, The response currently hard-codes AgentStartupPhase::ProvisioningVm instead of using the spawner-normalized phase; change the json_response call to pass the startup phase from the reprovisioned result (e.g., use reprovisioned.startup_phase or the appropriate field/method on the reprovisioned value, cloning/handling Option as needed) so the returned state/startup_phase pair matches the spawner-derived normalization; make the same change for the other analogous response path that also uses AgentStartupPhase::ProvisioningVm.
525-577:⚠️ Potential issue | 🟠 MajorValidate microVM params before inserting the agent row.
AgentInstance::create()still happens before the microVM selection and spawner URL are fully resolved/validated inprovision_vm_for_owner(). A bad request therefore leaves behind a fresh errored agent row instead of failing cleanly without mutating state.
335-421:⚠️ Potential issue | 🟠 MajorRelease the DB connection before polling vm-spawner.
refresh_agent_from_spawner()still holds&mut PgConnectionacrossget_vm_with_request_id(...).await. On the hotGET /v1/agents/mepolling path, that burns a pool slot for the full spawner timeout and can starve unrelated requests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pika-server/src/agent_api.rs` around lines 335 - 421, The function refresh_agent_from_spawner currently holds a &mut PgConnection across an await (spawner.get_vm_with_request_id) which ties up a DB pool slot; refactor so the spawner call does not hold the DB connection: change refresh_agent_from_spawner to accept either an owned AgentInstance (or clone the needed vm_id string) plus a PgPool/connection factory instead of &mut PgConnection, then (1) perform all DB reads necessary up front, drop the connection/return it to the pool, (2) call MicrovmSpawnerClient::get_vm_with_request_id(...) and await the result, and only (3) reacquire a new connection from the pool when calling mark_agent_errored(...) or AgentInstance::update_phase(...). This removes the &mut PgConnection borrow across the await while preserving the logic around vm_id, mark_agent_errored, and AgentInstance::update_phase.rust/src/core/mod.rs (1)
3075-3077:⚠️ Potential issue | 🟠 MajorReset in-memory app settings during logout too.
Line 3077 clears persisted
app_settings, but this branch still leavesself.state.developer_modeandself.state.show_agent_marketplaceuntouched. A same-process logout/login can therefore inherit the previous user’s flags, and the early returns inEnableDeveloperMode/SetShowAgentMarketplacethen prevent the defaults from being re-saved.Suggested fix
if let Some(conn) = self.profile_db.as_ref() { profile_db::clear_all(conn); profile_db::clear_app_settings(conn); } + self.state.developer_mode = false; + self.state.show_agent_marketplace = false; profile_pics::clear_cache(&self.data_dir);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/core/mod.rs` around lines 3075 - 3077, The logout path currently clears persisted app settings via profile_db::clear_app_settings(conn) but does not reset in-memory flags, so ensure you also reset self.state.developer_mode and self.state.show_agent_marketplace to their default values in the same branch where profile_db::clear_all and profile_db::clear_app_settings are called; update the logout code in mod.rs to explicitly set self.state.developer_mode = false (or the appropriate default) and self.state.show_agent_marketplace = <default> so that subsequent same-process login won't inherit previous flags and the early-return checks in EnableDeveloperMode and SetShowAgentMarketplace behave correctly.rust/src/core/agent.rs (1)
154-164:⚠️ Potential issue | 🟠 MajorPi requests still omit the required ACP backend.
Line 161 and Line 254 only set
kind. Whenagent_kind == Pi, leavingbackendasNonestill allows the server to resolve/reject the request as the default/native backend, so app-driven Pi ensure/recover can fail even though Pi is now selectable.Minimal fix
kind: Some(match agent_kind { crate::state::AgentKind::Openclaw => MicrovmAgentKind::Openclaw, crate::state::AgentKind::Pi => MicrovmAgentKind::Pi, }), - backend: None, + backend: matches!(agent_kind, crate::state::AgentKind::Pi).then_some( + pika_agent_control_plane::MicrovmAgentBackend::Acp { + exec_command: None, + cwd: None, + }, + ),Please apply the same change in both builders.
Also applies to: 247-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/core/agent.rs` around lines 154 - 164, The Pi agent requests omit the required ACP backend: when constructing AgentProvisionRequest / MicrovmProvisionParams (the builders that set spawner_url, kind, backend) you must set backend = Some(MicrovmAgentBackend::Acp) whenever agent_kind == crate::state::AgentKind::Pi (instead of leaving backend as None); update both builder sites that create MicrovmProvisionParams (the block using AgentProvisionRequest and the duplicate block around lines ~247-257) to conditionally assign MicrovmAgentBackend::Acp for Pi requests and keep the existing backend logic for other kinds.
🧹 Nitpick comments (1)
ios/Sources/ContentView.swift (1)
258-259: Make the OpenClaw branch use the typed agent-kind path too.In
ios/Sources/AppManager.swift, Lines 376-382,ensureAgent()andensureAgent(kind:)dispatch different actions. WiringonEnsureOpenclawAgentto the legacy default path keeps the “OpenClaw” choice coupled to whatever the app default is, while the Pi branch already goes through the explicit kind flow this PR is introducing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Sources/ContentView.swift` around lines 258 - 259, The onEnsureOpenclawAgent callback is wired to the legacy manager.ensureAgent() default path; change it to call the typed path manager.ensureAgent(kind: .openclaw) so the OpenClaw branch dispatches the explicit agent-kind action like the Pi branch does (update the binding for onEnsureOpenclawAgent in ContentView to use ensureAgent(kind:) with the .openclaw enum case).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/pika-server/src/agent_api.rs`:
- Around line 112-129: The code leaves MicrovmProvisionParams.kind as None when
PIKA_AGENT_MICROVM_KIND is unset, causing resolved_spawner_params(None) to pick
an invalid Pi+Native combo; change default_microvm_params_from_env to provide a
valid default kind (e.g. set kind:
microvm_kind_from_env().or(Some(MicrovmAgentKind::Openclaw))) so
MicrovmProvisionParams returned by default_microvm_params_from_env yields a
resolver/validate_resolved_params-acceptable configuration; update the
assignment in default_microvm_params_from_env (referencing
MicrovmProvisionParams, microvm_kind_from_env, resolved_spawner_params, and
validate_resolved_params) accordingly.
In `@ios/Sources/Views/ChatListView.swift`:
- Around line 147-160: The confirmation dialog message in ChatListView (the
.confirmationDialog tied to showAgentMarketplaceDialog and the action buttons
onEnsureOpenclawAgent/onEnsurePiAgent) currently reads like a settings
description; change its Text(...) to a selection prompt that makes the intent
clear (e.g., "Select an agent to start a chat" or similar) so the OpenClaw/Pi
buttons read as immediate choices rather than toggles.
In `@justfile`:
- Around line 1072-1078: The recipe comments for agent-pi and agent-claw are
stale (they claim the recipe "Reset the current test account's VM" and
"recover/create a guest") even though scripts/agent-demo.sh no longer performs
VM reset or guest recovery; update the two comment lines above the agent-pi and
agent-claw targets to accurately describe the current flow (e.g., that the
recipe runs the agent demo/chat using environment variables
PIKA_AGENT_MICROVM_KIND and PIKA_AGENT_MICROVM_BACKEND without resetting VMs),
referencing the agent-pi and agent-claw targets and scripts/agent-demo.sh so
readers aren’t misled.
In `@scripts/agent-demo.sh`:
- Around line 20-29: The script currently only normalizes a missing
MICROVM_BACKEND but doesn't reject explicit incompatible overrides; add a
compatibility check that runs when MICROVM_BACKEND is non-empty: after the
existing defaulting logic (or before CLI/network use), validate the pair of
AGENT_KIND and MICROVM_BACKEND (e.g., disallow AGENT_KIND="pi" with
MICROVM_BACKEND="native" and AGENT_KIND="openclaw" with MICROVM_BACKEND="acp"),
and if incompatible print a clear error to stderr and exit non‑zero; reference
the MICROVM_BACKEND and AGENT_KIND variables in your check so the script fails
early with a helpful message instead of letting the CLI/server fail later.
In `@scripts/demo-agent-microvm.sh`:
- Around line 12-21: Add an explicit validation that rejects incompatible
explicit kind/backend pairs when MICROVM_BACKEND is already set: check
AGENT_KIND and MICROVM_BACKEND and if AGENT_KIND=pi and MICROVM_BACKEND!="acp"
or AGENT_KIND=openclaw and MICROVM_BACKEND!="native" print an error (mention
both variables) to stderr and exit 1; place this check alongside the existing
derivation logic so an explicit bad pair (e.g. PIKA_AGENT_MICROVM_KIND=pi with
PIKA_AGENT_MICROVM_BACKEND=native) fails fast instead of later during server
validation.
---
Duplicate comments:
In `@crates/pika-agent-microvm/src/lib.rs`:
- Around line 513-554: Both wait_for_pi_ready and wait_for_openclaw_ready bail
out immediately if the child process dies (the ! kill -0 "$agent_pid" branch)
without writing a failure marker; modify those branches to call
write_failed_marker with a descriptive key (e.g., "agent_exited_before_ready" or
"openclaw_exited_before_ready") before returning the child's exit code so a
crash-on-boot is recorded as a hard failure (ensure you still wait "$agent_pid"
to capture its exit status and return that status after writing the marker).
In `@crates/pika-server/src/agent_api.rs`:
- Around line 615-617: The response currently hard-codes
AgentStartupPhase::ProvisioningVm instead of using the spawner-normalized phase;
change the json_response call to pass the startup phase from the reprovisioned
result (e.g., use reprovisioned.startup_phase or the appropriate field/method on
the reprovisioned value, cloning/handling Option as needed) so the returned
state/startup_phase pair matches the spawner-derived normalization; make the
same change for the other analogous response path that also uses
AgentStartupPhase::ProvisioningVm.
- Around line 335-421: The function refresh_agent_from_spawner currently holds a
&mut PgConnection across an await (spawner.get_vm_with_request_id) which ties up
a DB pool slot; refactor so the spawner call does not hold the DB connection:
change refresh_agent_from_spawner to accept either an owned AgentInstance (or
clone the needed vm_id string) plus a PgPool/connection factory instead of &mut
PgConnection, then (1) perform all DB reads necessary up front, drop the
connection/return it to the pool, (2) call
MicrovmSpawnerClient::get_vm_with_request_id(...) and await the result, and only
(3) reacquire a new connection from the pool when calling
mark_agent_errored(...) or AgentInstance::update_phase(...). This removes the
&mut PgConnection borrow across the await while preserving the logic around
vm_id, mark_agent_errored, and AgentInstance::update_phase.
In `@rust/src/core/agent.rs`:
- Around line 154-164: The Pi agent requests omit the required ACP backend: when
constructing AgentProvisionRequest / MicrovmProvisionParams (the builders that
set spawner_url, kind, backend) you must set backend =
Some(MicrovmAgentBackend::Acp) whenever agent_kind ==
crate::state::AgentKind::Pi (instead of leaving backend as None); update both
builder sites that create MicrovmProvisionParams (the block using
AgentProvisionRequest and the duplicate block around lines ~247-257) to
conditionally assign MicrovmAgentBackend::Acp for Pi requests and keep the
existing backend logic for other kinds.
In `@rust/src/core/mod.rs`:
- Around line 3075-3077: The logout path currently clears persisted app settings
via profile_db::clear_app_settings(conn) but does not reset in-memory flags, so
ensure you also reset self.state.developer_mode and
self.state.show_agent_marketplace to their default values in the same branch
where profile_db::clear_all and profile_db::clear_app_settings are called;
update the logout code in mod.rs to explicitly set self.state.developer_mode =
false (or the appropriate default) and self.state.show_agent_marketplace =
<default> so that subsequent same-process login won't inherit previous flags and
the early-return checks in EnableDeveloperMode and SetShowAgentMarketplace
behave correctly.
---
Nitpick comments:
In `@ios/Sources/ContentView.swift`:
- Around line 258-259: The onEnsureOpenclawAgent callback is wired to the legacy
manager.ensureAgent() default path; change it to call the typed path
manager.ensureAgent(kind: .openclaw) so the OpenClaw branch dispatches the
explicit agent-kind action like the Pi branch does (update the binding for
onEnsureOpenclawAgent in ContentView to use ensureAgent(kind:) with the
.openclaw enum case).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78acccd7-c268-4fa2-a120-09041dbb3f9a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
android/app/src/main/java/com/pika/app/AppManager.ktandroid/app/src/main/java/com/pika/app/rust/pika_core.ktandroid/app/src/main/java/com/pika/app/ui/PikaApp.ktandroid/app/src/main/java/com/pika/app/ui/screens/ChatListScreen.ktandroid/app/src/main/java/com/pika/app/ui/screens/MyProfileScreen.ktcli/src/main.rscrates/pika-agent-control-plane/src/lib.rscrates/pika-agent-microvm/src/lib.rscrates/pika-server/src/agent_api.rscrates/pikahut/src/component.rscrates/vm-spawner/src/main.rscrates/vm-spawner/src/manager.rsios/Sources/AppManager.swiftios/Sources/ContentView.swiftios/Sources/ViewState.swiftios/Sources/Views/AgentProvisioningView.swiftios/Sources/Views/ChatListView.swiftios/Sources/Views/MyNpubQrSheet.swiftjustfilerust/Cargo.tomlrust/src/actions.rsrust/src/core/agent.rsrust/src/core/mod.rsrust/src/core/profile_db.rsrust/src/state.rsrust/src/updates.rsscripts/agent-demo.shscripts/demo-agent-microvm.sh
💤 Files with no reviewable changes (3)
- rust/src/updates.rs
- ios/Sources/Views/AgentProvisioningView.swift
- android/app/src/main/java/com/pika/app/ui/PikaApp.kt
🚧 Files skipped from review as they are similar to previous changes (11)
- ios/Sources/ViewState.swift
- rust/src/actions.rs
- crates/pika-agent-control-plane/src/lib.rs
- android/app/src/main/java/com/pika/app/ui/screens/ChatListScreen.kt
- crates/pikahut/src/component.rs
- ios/Sources/Views/MyNpubQrSheet.swift
- rust/Cargo.toml
- android/app/src/main/java/com/pika/app/ui/screens/MyProfileScreen.kt
- rust/src/core/profile_db.rs
- ios/Sources/AppManager.swift
- android/app/src/main/java/com/pika/app/AppManager.kt
| fn microvm_kind_from_env() -> Option<MicrovmAgentKind> { | ||
| match std::env::var("PIKA_AGENT_MICROVM_KIND") | ||
| .ok() | ||
| .as_deref() | ||
| .map(str::trim) | ||
| .filter(|v| !v.is_empty()) | ||
| { | ||
| Some("openclaw") => Some(MicrovmAgentKind::Openclaw), | ||
| Some("pi") => Some(MicrovmAgentKind::Pi), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| fn default_microvm_params_from_env() -> anyhow::Result<MicrovmProvisionParams> { | ||
| Ok(MicrovmProvisionParams { | ||
| spawner_url: Some(required_microvm_spawner_url_from_env()?), | ||
| kind: microvm_kind_from_env(), | ||
| ..MicrovmProvisionParams::default() |
There was a problem hiding this comment.
Choose a valid default microVM selection when the env var is unset.
If PIKA_AGENT_MICROVM_KIND is absent, this leaves kind=None; resolved_spawner_params(None) then resolves to Pi + Native, which validate_resolved_params() rejects. That makes the healthcheck and any ensure/recover call without explicit microVM settings fail unless every environment sets the kind up front.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pika-server/src/agent_api.rs` around lines 112 - 129, The code leaves
MicrovmProvisionParams.kind as None when PIKA_AGENT_MICROVM_KIND is unset,
causing resolved_spawner_params(None) to pick an invalid Pi+Native combo; change
default_microvm_params_from_env to provide a valid default kind (e.g. set kind:
microvm_kind_from_env().or(Some(MicrovmAgentKind::Openclaw))) so
MicrovmProvisionParams returned by default_microvm_params_from_env yields a
resolver/validate_resolved_params-acceptable configuration; update the
assignment in default_microvm_params_from_env (referencing
MicrovmProvisionParams, microvm_kind_from_env, resolved_spawner_params, and
validate_resolved_params) accordingly.
| .confirmationDialog( | ||
| "Choose Agent", | ||
| isPresented: $showAgentMarketplaceDialog, | ||
| titleVisibility: .visible | ||
| ) { | ||
| Button("OpenClaw") { | ||
| onEnsureOpenclawAgent() | ||
| } | ||
| Button("Pi") { | ||
| onEnsurePiAgent() | ||
| } | ||
| Button("Cancel", role: .cancel) {} | ||
| } message: { | ||
| Text("Show experimental agent choices when creating a new agent.") |
There was a problem hiding this comment.
Dialog copy is describing the setting, not the selection.
"Show experimental agent choices when creating a new agent." reads like the profile-sheet toggle label, not a chooser message. In this dialog it makes the OpenClaw/Pi buttons feel like they change a setting instead of selecting which agent to start.
Suggested copy
- } message: {
- Text("Show experimental agent choices when creating a new agent.")
+ } message: {
+ Text("Choose which agent to start.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .confirmationDialog( | |
| "Choose Agent", | |
| isPresented: $showAgentMarketplaceDialog, | |
| titleVisibility: .visible | |
| ) { | |
| Button("OpenClaw") { | |
| onEnsureOpenclawAgent() | |
| } | |
| Button("Pi") { | |
| onEnsurePiAgent() | |
| } | |
| Button("Cancel", role: .cancel) {} | |
| } message: { | |
| Text("Show experimental agent choices when creating a new agent.") | |
| .confirmationDialog( | |
| "Choose Agent", | |
| isPresented: $showAgentMarketplaceDialog, | |
| titleVisibility: .visible | |
| ) { | |
| Button("OpenClaw") { | |
| onEnsureOpenclawAgent() | |
| } | |
| Button("Pi") { | |
| onEnsurePiAgent() | |
| } | |
| Button("Cancel", role: .cancel) {} | |
| } message: { | |
| Text("Choose which agent to start.") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Sources/Views/ChatListView.swift` around lines 147 - 160, The
confirmation dialog message in ChatListView (the .confirmationDialog tied to
showAgentMarketplaceDialog and the action buttons
onEnsureOpenclawAgent/onEnsurePiAgent) currently reads like a settings
description; change its Text(...) to a selection prompt that makes the intent
clear (e.g., "Select an agent to start a chat" or similar) so the OpenClaw/Pi
buttons read as immediate choices rather than toggles.
| # Reset the current test account's VM on pika-build, recover/create a Pi ACP guest, then chat. | ||
| agent-pi MESSAGE="CLI demo check: reply with ACK and one short sentence.": | ||
| PIKA_AGENT_MICROVM_KIND=pi PIKA_AGENT_MICROVM_BACKEND=acp ./scripts/agent-demo.sh "{{ MESSAGE }}" | ||
|
|
||
| # Reset/create/recover the current test account's VM, then chat through an ACP-backed guest daemon. | ||
| agent-demo-acp MESSAGE="CLI demo check: reply with ACK and one short sentence.": | ||
| PIKA_AGENT_MICROVM_BACKEND=acp ./scripts/agent-demo.sh "{{ MESSAGE }}" | ||
| # Reset the current test account's VM on pika-build, recover/create an OpenClaw guest, then chat. | ||
| agent-claw MESSAGE="CLI demo check: reply with ACK and one short sentence.": | ||
| PIKA_AGENT_MICROVM_KIND=openclaw PIKA_AGENT_MICROVM_BACKEND=native ./scripts/agent-demo.sh "{{ MESSAGE }}" |
There was a problem hiding this comment.
Update the recipe comments to match the new flow.
These comments still say the recipes reset the VM and recover/create a guest, but scripts/agent-demo.sh no longer does that. The stale description will send people debugging the wrong path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@justfile` around lines 1072 - 1078, The recipe comments for agent-pi and
agent-claw are stale (they claim the recipe "Reset the current test account's
VM" and "recover/create a guest") even though scripts/agent-demo.sh no longer
performs VM reset or guest recovery; update the two comment lines above the
agent-pi and agent-claw targets to accurately describe the current flow (e.g.,
that the recipe runs the agent demo/chat using environment variables
PIKA_AGENT_MICROVM_KIND and PIKA_AGENT_MICROVM_BACKEND without resetting VMs),
referencing the agent-pi and agent-claw targets and scripts/agent-demo.sh so
readers aren’t misled.
| if [[ -z "$MICROVM_BACKEND" ]]; then | ||
| case "$AGENT_KIND" in | ||
| pi) MICROVM_BACKEND="acp" ;; | ||
| openclaw) MICROVM_BACKEND="native" ;; | ||
| *) | ||
| echo "Unsupported PIKA_AGENT_MICROVM_KIND: $AGENT_KIND (expected pi or openclaw)" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| fi |
There was a problem hiding this comment.
Validate explicit kind/backend overrides, not just the defaulting path.
This branch normalizes empty backend values, but it still accepts incompatible explicit overrides. For example, pi + native reaches the CLI/server and fails there instead of stopping immediately with a clear demo-side error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/agent-demo.sh` around lines 20 - 29, The script currently only
normalizes a missing MICROVM_BACKEND but doesn't reject explicit incompatible
overrides; add a compatibility check that runs when MICROVM_BACKEND is
non-empty: after the existing defaulting logic (or before CLI/network use),
validate the pair of AGENT_KIND and MICROVM_BACKEND (e.g., disallow
AGENT_KIND="pi" with MICROVM_BACKEND="native" and AGENT_KIND="openclaw" with
MICROVM_BACKEND="acp"), and if incompatible print a clear error to stderr and
exit non‑zero; reference the MICROVM_BACKEND and AGENT_KIND variables in your
check so the script fails early with a helpful message instead of letting the
CLI/server fail later.
| if [[ -z "$MICROVM_BACKEND" ]]; then | ||
| case "$AGENT_KIND" in | ||
| pi) MICROVM_BACKEND="acp" ;; | ||
| openclaw) MICROVM_BACKEND="native" ;; | ||
| *) | ||
| echo "Unsupported PIKA_AGENT_MICROVM_KIND: $AGENT_KIND (expected pi or openclaw)" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| fi |
There was a problem hiding this comment.
Reject incompatible explicit kind/backend pairs here.
This only derives a backend when PIKA_AGENT_MICROVM_BACKEND is empty. An explicit invalid pair like PIKA_AGENT_MICROVM_KIND=pi plus PIKA_AGENT_MICROVM_BACKEND=native still gets through and now fails much later in the server validation path, which makes the demo look broken instead of surfacing a local usage error.
Suggested guard
if [[ -z "$MICROVM_BACKEND" ]]; then
case "$AGENT_KIND" in
pi) MICROVM_BACKEND="acp" ;;
openclaw) MICROVM_BACKEND="native" ;;
@@
esac
fi
+
+case "$AGENT_KIND:$MICROVM_BACKEND" in
+ pi:acp|openclaw:native) ;;
+ *)
+ echo "Unsupported microVM selection: kind=$AGENT_KIND backend=$MICROVM_BACKEND" >&2
+ exit 1
+ ;;
+esac📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -z "$MICROVM_BACKEND" ]]; then | |
| case "$AGENT_KIND" in | |
| pi) MICROVM_BACKEND="acp" ;; | |
| openclaw) MICROVM_BACKEND="native" ;; | |
| *) | |
| echo "Unsupported PIKA_AGENT_MICROVM_KIND: $AGENT_KIND (expected pi or openclaw)" >&2 | |
| exit 1 | |
| ;; | |
| esac | |
| fi | |
| if [[ -z "$MICROVM_BACKEND" ]]; then | |
| case "$AGENT_KIND" in | |
| pi) MICROVM_BACKEND="acp" ;; | |
| openclaw) MICROVM_BACKEND="native" ;; | |
| *) | |
| echo "Unsupported PIKA_AGENT_MICROVM_KIND: $AGENT_KIND (expected pi or openclaw)" >&2 | |
| exit 1 | |
| ;; | |
| esac | |
| fi | |
| case "$AGENT_KIND:$MICROVM_BACKEND" in | |
| pi:acp|openclaw:native) ;; | |
| *) | |
| echo "Unsupported microVM selection: kind=$AGENT_KIND backend=$MICROVM_BACKEND" >&2 | |
| exit 1 | |
| ;; | |
| esac |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/demo-agent-microvm.sh` around lines 12 - 21, Add an explicit
validation that rejects incompatible explicit kind/backend pairs when
MICROVM_BACKEND is already set: check AGENT_KIND and MICROVM_BACKEND and if
AGENT_KIND=pi and MICROVM_BACKEND!="acp" or AGENT_KIND=openclaw and
MICROVM_BACKEND!="native" print an error (mention both variables) to stderr and
exit 1; place this check alongside the existing derivation logic so an explicit
bad pair (e.g. PIKA_AGENT_MICROVM_KIND=pi with
PIKA_AGENT_MICROVM_BACKEND=native) fails fast instead of later during server
validation.
- Add missing `import com.pika.app.rust.AgentKind` to AppManager.kt
- Handle GET /vms/{id} in mock vm-spawner (the refresh_agent_from_spawner
logic now polls VM status during /me responses)
- Bump expected_requests from 2 to 3 for tests that call /me
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements