Feature/icp agent runtime phases 0 2 - #18
Conversation
Embed knolo-agent-core/scheduler in an ICP canister host with deterministic control-plane execution (Phase 1) and pack-gated tools, ic-llm, knowledge retrieval, timers, and cycles/budget observation (Phase 2). Document constraints and ADR-001; ship a local dfx example and CHANGELOG entry.
Add upgrade-safe ic-stable-structures persistence, runtime limits and caller hardening, multi-agent handoff, scripts/templates, TS client, and cost/security docs for the ICP agent canister.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 217780ae52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let resolved = ENGINE.with(|e| std::mem::take(&mut *e.borrow_mut())); | ||
| let mut eng = resolved; | ||
| let result = effects::resolve_one_effect(&mut eng, &execution_id).await; | ||
| ENGINE.with(|e| *e.borrow_mut() = eng); |
There was a problem hiding this comment.
When LLM or knowledge resolution reaches the .await, ICP may process another message while the thread-local contains AgentEngine::default(). During that interval queries report no definition, authorization reads the default open limits, and mutations from an interleaved update are discarded when this code overwrites the entire engine after the await. Preserve shared configuration/state across the await and merge only the resolved execution result.
Useful? React with 👍 / 👎.
| let state = parse_state(state_json)?; | ||
| let id = ExecutionId::from_str(execution_id) | ||
| .map_err(|e| CoreError::Host(format!("invalid execution_id: {e}")))?; | ||
| let report = self.run_from_start(&id, state, None, BTreeMap::new())?; |
There was a problem hiding this comment.
When an envelope narrows max_steps, max_cost_micros, or allowed capabilities, the child is nevertheless started through the normal loaded graph and full pack policy; authority_projection is only copied into the later audit record. A child excluding a tool or granting five steps can therefore use that tool and run up to the graph's larger limits, defeating the handoff's least-authority boundary. Pass the validated child authority into execution and policy enforcement.
Useful? React with 👍 / 👎.
| if let Some(json) = snap.definition_json { | ||
| eng.load_definition(&json) | ||
| .map_err(|err| format!("reload definition: {err}"))?; |
There was a problem hiding this comment.
On every upgrade with a loaded definition, load_definition clears budget and handoffs after those fields were restored on lines 592–593. Executions are reassigned afterward, but the persisted budget counters and handoff audit records remain empty, so get_budget/get_handoff lose durable data and budget accounting resets across upgrades. Reload the definition before applying the snapshot fields that it clears.
Useful? React with 👍 / 👎.
| .and_then(Value::as_str) | ||
| .unwrap_or("Say hello from Knolo ICP agent.") | ||
| .to_string(); | ||
| let (text, tokens, cost) = llm_prompt(&prompt).await?; |
There was a problem hiding this comment.
When a definition sets host.llm_enabled to false but reaches an await_llm node, this branch still invokes ic_llm::prompt on wasm and spends cycles. The flag is currently used only for inspection text, so the advertised disable control has no runtime effect; reject or leave the execution suspended before calling the provider when it is false.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
| async fn continue_effects(execution_id: String) -> RunReportDto { | ||
| if let Err(err) = require_run_auth() { | ||
| return RunReportDto::err(execution_id, err.message); | ||
| } | ||
| let finished = continue_effects_inner(execution_id).await; |
There was a problem hiding this comment.
When the balance falls below a configured min_cycles_reserve after an execution has suspended for LLM or retrieval, callers can invoke continue_effects and initiate the costly operation because this update omits the guard used by start_execution, step, resume, and handoff methods. This bypasses the reserve precisely on the endpoint dedicated to spending cycles; run require_cycles_guard before draining effects.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
| async fn continue_effects_inner(execution_id: String) -> RunReportDto { | ||
| loop { |
There was a problem hiding this comment.
When host.max_effect_rounds is set below the number of pending effects—even zero—this unconditional loop continues resolving until termination or HITL and never reads the configured bound. All public canister paths call this function rather than the separate bounded helper in effects.rs, so the documented per-message instruction guard is ineffective. Count resolutions here and return the suspended record once the configured maximum is reached.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
Land remaining CI-green fixes after #18: clippy cleanups in knolo-agent-icp, hygiene without ripgrep, and FUTURE.md legacy wording.
2 participants
Summary
Validation