feat(acp): Agent Client Protocol worker — iii as a first-class agent in any ACP editor - #63
Merged
Conversation
Adds iii-acp, a stdio JSON-RPC worker that exposes iii agents to any
ACP-speaking client. Mirrors iii-mcp (tools) and iii-a2a (peers).
v0 surface
- initialize, authenticate (no-op success)
- session/new, session/load, session/list, session/close
- session/prompt with built-in echo brain or pluggable IIIACP_BRAIN_FN
- session/cancel via in-process AbortFlag + durable cancel topic
- session/update notifications written to stdout per turn
State lives entirely in iii state primitives (state::set/get/delete via
iii.trigger), namespaced by a per-process connId so concurrent editors
do not collide. History is replayed on session/load with
params._meta.iii.dev/historical=true.
Reverse-RPC paths (request_permission, fs/*, terminal/*) are deferred.
Agents reach for iii primitives directly for fs/terminal needs.
Pluggable brains: pass --brain-fn or set IIIACP_BRAIN_FN to a function
that takes { sessionId, connId, prompt, respondTopic } and returns
{ stopReason }. Built-in echo brain ships for smoke tests.
acp-client (consume external ACP agents like Claude Code, Codex) is a
follow-up. Conductor today spawns CLIs as raw subprocess; acp-client
earns its keep when conductor opts in.
Adds acp to the create-tag.yml worker enum and registers the acp/v*
tag pattern in release.yml.
Validation
- 13 unit + protocol envelope tests pass without an engine
- 14-assertion end-to-end smoke against a live engine
(initialize > session/new > session/prompt streaming
3 session/update notifications > session/list > session/cancel >
session/load replaying 5 historical updates > session/close).
stdout hygiene verified: every emitted line is valid JSON-RPC,
logs go to stderr.
Bugs caught locally during smoke
- tokio::spawn race on inbound frames raced session/new ahead of
initialize -> switched to serial dispatch
- mcpServers typed shape rejected ACP http/sse variants -> loose
Vec<Value> passthrough
- _meta placed on the JSON-RPC envelope (violates JSON-RPC 2.0,
only jsonrpc/method/params/id allowed) -> moved into params._meta
Prereq for runtime: engine config must include iii-state worker
(state::* functions are not engine builtins). Add via
`iii worker add iii-state`.
|
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 a new Rust "acp" worker crate and binary implementing a stdio JSON-RPC ACP server (session lifecycle, prompt handling, state/history persistence, outbound notifications, optional external brain), plus stdio transport, README, tests, and CI workflow updates for ChangesCI/CD Workflow Updates
ACP Worker
Sequence Diagram(s)sequenceDiagram
actor Client as ACP Client
participant Handler as AcpHandler
participant State as State Store
participant Brain as Brain Function
participant Out as Outbound
Client->>Handler: session/prompt (JSON-RPC)
activate Handler
Handler->>State: append_history(prompt)
alt external brain configured
Handler->>Brain: iii.trigger(brain_fn, request)
activate Brain
Brain-->>Handler: streamed/final responses
deactivate Brain
else echo brain fallback
Handler->>Handler: generate echo chunks
end
loop per-chunk
Handler->>State: append_history(update)
Handler->>Out: session/update notification
end
Handler->>State: update last_activity_ms
Handler->>Out: write final JSON-RPC reply
deactivate Handler
Out-->>Client: notifications/reply
sequenceDiagram
actor Client as ACP Client
participant Handler as AcpHandler
participant State as State Store
participant Out as Outbound
Client->>Handler: session/new (JSON-RPC)
activate Handler
Handler->>State: set(session_key, SessionRecord)
Handler->>State: append_session_to_index
Handler->>Out: reply with sessionId
deactivate Handler
Out-->>Client: session/new response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acp/README.md`:
- Around line 71-73: The example currently sends the JSON RPC initialize payload
to stderr (the echo ... >&2) so it never reaches the worker; change it to pipe
the payload into the stdin-based worker invocation (use the echo/printf or
here-doc piped into iii-acp rather than redirected to >&2) so the request is
delivered to iii-acp (reference the iii-acp invocation and the echo line sending
the JSON).
In `@acp/src/handler.rs`:
- Around line 302-325: The run_external_brain call advertises a respondTopic but
never consumes or forwards messages from that topic; implement a consumer loop
that subscribes to respond_topic before calling self.iii.trigger(req), reads
messages (until the trigger finishes or abort fires), and forwards each message
as session/update frames to the client (use the same emitting mechanism used
elsewhere in this module for session updates); ensure the subscription/task is
cancelled on abort or when iii.trigger returns and that the final stopReason
from iii.trigger is still returned. Reference: run_external_brain, respondTopic,
self.iii.trigger(...), and wait_aborted(...) when adding the
subscription/forwarding logic.
- Around line 252-256: session_close currently ignores the result of
state_delete and always returns Ok, and it doesn't remove related history/index
entries; change session_close to await and inspect the Result from state_delete
for the session key and also delete associated history and index keys (use the
same state_delete helper with keys derived similarly to session_key, e.g.,
history_key(&self.conn_id, &p.session_id) and index_key(...)), aggregating any
errors and returning Err((<appropriate_code>, format!(...))) if any backend
deletion fails; ensure you reference session_close, session_key, state_delete,
scope(), and the iii field when locating and removing all related keys.
In `@acp/src/main.rs`:
- Around line 14-15: The CLI arg for engine_url (field engine_url with
#[arg(...)] in main.rs) is missing an environment binding; update its attribute
to include env = "IIIACP_ENGINE_URL" (e.g., #[arg(long, short = 'e', env =
"IIIACP_ENGINE_URL", default_value = "ws://localhost:49134")]) so the value can
be overridden from the environment using the existing IIIACP_* convention while
retaining the current default.
In `@acp/src/transport.rs`:
- Around line 43-82: The reader currently awaits handler.handle(body).await
which blocks stdin processing for long-running "session/prompt" work; change
this so the reader spawns prompt handling onto a background Tokio task instead
of awaiting it inline: detect when the parsed request is a long-running prompt
(e.g., method "session/prompt" or whatever signal your JsonRpc body uses), then
call let body_clone = body.clone(); let outbound = handler.outbound().clone();
tokio::spawn(async move { if let Some(reply) = handler.handle(body_clone).await
{ if let Err(e) = outbound.write(&reply).await { tracing::error!(error=%e,
"stdout write failed"); } } }); for non-prompt messages (initialize,
session/new, cancel, notifications) keep the existing serial/await behavior so
state-machine ordering is preserved; ensure errors from serde parsing and stdout
writes remain logged as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 947f2600-3ff6-404b-954f-2c2032c70bf3
⛔ Files ignored due to path filters (1)
acp/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/create-tag.yml.github/workflows/release.ymlacp/Cargo.tomlacp/README.mdacp/iii.worker.yamlacp/src/handler.rsacp/src/lib.rsacp/src/main.rsacp/src/session.rsacp/src/transport.rsacp/src/types.rsacp/tests/protocol.rs
Five findings, all confirmed against current code, all fixed.
1. README wire example sent JSON to stderr via `>&2` — never reached the
worker. Pipe to stdin instead.
2. `run_external_brain` advertised `respondTopic` but no consumer ever
read from it. Fix: register one durable subscriber per connection at
handler init on `acp:<connId>:updates`. Brain publishes
`{ sessionId, update }` payloads via `iii::durable::publish`; the
subscriber decodes and writes `session/update` JSON-RPC notifications
to stdout. Trigger handle held in `AcpHandler` so dropping the handler
tears the subscription down. Subscriber wired only when `--brain-fn` is
set (echo brain bypasses durable for in-process speed). Drops
`--publish-updates` flag — no longer needed; the per-connection topic
is canonical.
3. `session_close` ignored the `state_delete` result and never removed
history or index entries. Fix: delete session record + history record,
remove session id from index, aggregate per-key errors, return
INTERNAL_ERROR with diagnostic message if any backend deletion fails.
Also drops the in-process cancel slot for the session.
4. `engine_url` arg lacked `env = "IIIACP_ENGINE_URL"`. Added.
5. Serial dispatch in transport blocked stdin while `session/prompt` ran,
so `session/cancel` couldn't reach the handler until the prompt
finished. Fix: hybrid dispatch — `session/prompt` runs on a spawned
task while everything else stays serial. The shared `Outbound` Mutex
keeps frame writes intact under concurrency.
Validation
- 13 unit + protocol envelope tests pass (`cargo test`)
- 18-assertion end-to-end smoke against a live engine validates the
hybrid dispatch (cancel ack arrives mid-prompt, long prompt returns
`stopReason=cancelled`) and the close cleanup (`session/load` after
close errors, `session/list` empty after close).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@acp/src/handler.rs`:
- Around line 504-527: forward_brain_update currently builds the session/update
frame (using seq.fetch_add and inserting iii.dev/seq into params) and forwards
it via outbound.write but never persists it to the session history; add a call
to the repository/session-history persistence before outbound.write (e.g.,
invoke the same append/persist function used by local session updates) so the
created params/frame (including the iii.dev/seq meta) is stored in history in
the same format session/load expects, then proceed to outbound.write and keep
the existing error logging; locate this change around the forward_brain_update
function, where params, s (seq.fetch_add), and frame are constructed and
outbound.write is called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bf0dd8b-4025-4138-9ead-fe211f5b799b
📒 Files selected for processing (5)
acp/README.mdacp/src/handler.rsacp/src/main.rsacp/src/session.rsacp/src/transport.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- acp/src/transport.rs
Conflicts in .github/workflows/create-tag.yml and .github/workflows/release.yml resolved by taking main's worker list (which dropped a2a, a2a-client, agent, coding, conductor, eval, experiment, guardrails as part of the harness migration in #66) and re-adding acp on top.
The previous brain contract ({sessionId, connId, prompt, respondTopic}
with a per-connection durable pub/sub topic) was a one-off invention.
The iii ecosystem already has a canonical shape used by every provider
worker, turn-orchestrator, and observers like context-compaction:
- Input: { session_id, messages, model, provider, system_prompt, ... }
(turn-orchestrator's run::start_and_wait wire format)
- Streaming: AgentEvent frames on the agent::events stream, group_id =
session_id
- Output: { session_id, messages, turn_count } where the final
assistant message carries stop_reason
Aligning iii-acp to this shape means turn-orchestrator drops in as the
brain with zero adapter, and any future brain just has to expose the
same surface — same as every other iii worker.
Concrete changes:
- DEFAULT_BRAIN_FN = run::start_and_wait
- New CLI flags: --use-canonical-brain, --model, --provider,
--system-prompt (each with IIIACP_* env binding)
- BrainConfig struct replaces the bare brain_fn arg on AcpHandler::new
- run_external_brain builds an AgentMessage::User from ACP prompt
content blocks and triggers the brain function with the canonical
payload, then derives ACP stopReason from the returned transcript
- Stream subscriber on agent::events replaces the durable subscriber
on the per-connection updates topic. trigger_type=stream, payload
envelope is { stream_name, group_id, item_id, data }, group_id =
session_id. iii-acp filters by an in-process owned_sessions DashSet
populated on session/new and cleared on session/close
- translate_agent_event maps:
message_update text_delta -> agent_message_chunk
message_update thinking_delta -> agent_thought_chunk
tool_execution_start -> tool_call (in_progress)
tool_execution_end -> tool_call_update (completed/failed)
- Tool kind heuristic from tool name (read/edit/execute/...)
- Drops connection_updates_topic and durable subscriber wiring;
AGENT_EVENTS_STREAM constant added in session.rs
- session_close also clears owned_sessions entry
Why streams over durable pub/sub for this:
- One ordered tape per session (group_id = session_id) eliminates
cross-session leakage without explicit filter logic
- Replay capability matches ACP session/load semantics
- Already the canonical iii streaming wire — no new convention
Validation:
- 22 unit + integration tests pass (15 lib + 7 integration). New
tests cover translate_agent_event for text/thinking deltas and
tool start/end, stop reason mapping, and content block construction
- 18-assertion smoke against live engine still PASSES (echo brain
bypasses the new code path; full path was tested locally with a
stub brain emitting AgentEvent frames)
- README rewrites the brain contract section with the canonical shape
and a Zed agent_servers example wired to turn-orchestrator
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
acp/src/handler.rs (2)
483-490: ⚡ Quick winReplace polling
wait_abortedwith a notification primitive.100 ms polling on
AtomicBoolintroduces up to 100 ms cancel latency per abort and wakes a tokio worker 10× per second per active prompt. Prefertokio::sync::Notify(or awatch::channel<bool>) created alongside the abort flag and notified fromsession_cancel; thetokio::select!arm then awaitsnotified()directly.🤖 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 `@acp/src/handler.rs` around lines 483 - 490, The current wait_aborted function polls an Arc<AtomicBool> every 100ms causing latency and unnecessary wakeups; replace it by creating a tokio::sync::Notify (or a tokio::sync::watch::channel<bool>) together with the abort flag and change wait_aborted to await the notifier (using notify.notified().await or watching the channel) and have session_cancel call notify.notify_one() (or send on the watch channel); update call sites that used wait_aborted (and any tokio::select! usage) to await the notifier directly to remove the polling loop and eliminate the 100ms sleep.
552-592: 💤 Low valueDe-duplicate the
_meta/seq stamping betweenforward_agent_eventandsend_notification.The seq stamping + frame envelope construction (Lines 575–587) repeats verbatim what
send_notificationdoes (Lines 435–453). They've already drifted:send_notificationwrites throughoutboundvia&self, this path uses the&Outboundreference. Extract the stamp/write into a free function (or method onOutbound) taking(&Outbound, &AtomicU64, &str /*method*/, Value /*params*/)so a future_metaschema change touches one place.Bonus:
forward_agent_eventcould also benefit fromemit_update's history append + notify pairing if the histories are kept consistent (currently both append; just route through one helper).🤖 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 `@acp/src/handler.rs` around lines 552 - 592, The seq-stamping and JSON-RPC frame construction in forward_agent_event duplicates send_notification; extract that logic into a single helper (either a free function or an Outbound method) with signature like (outbound: &Outbound, seq: &AtomicU64, method: &str, params: Value) which increments seq, injects iii.dev/seq into params._meta, wraps params into the JSON-RPC frame, and writes via outbound.write(&frame). Replace the stamping/writing block in forward_agent_event with a call to this new helper, and consider delegating the append_history+notify pairing to the existing emit_update (or call append_history then the new helper) so history appends and notifications share the same path.acp/src/main.rs (1)
84-93: 💤 Low valueHardcoded
EnvFilterignoresRUST_LOG.
EnvFilter::new(...)always uses the literal directives, soRUST_LOGis ineffective for this binary. Operators commonly bump verbosity for individual modules without adding a CLI flag; consider falling back toRUST_LOGwhen set:♻️ Suggested change
- let filter = if args.debug { - EnvFilter::new("iii_acp=debug,iii_sdk=debug") - } else { - EnvFilter::new("iii_acp=info,iii_sdk=warn") - }; + let default = if args.debug { + "iii_acp=debug,iii_sdk=debug" + } else { + "iii_acp=info,iii_sdk=warn" + }; + let filter = EnvFilter::try_from_default_env() + .unwrap_or_else(|_| EnvFilter::new(default));🤖 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 `@acp/src/main.rs` around lines 84 - 93, The current use of EnvFilter::new(...) hardcodes module directives and ignores RUST_LOG; change to first attempt loading the filter from the environment (e.g. EnvFilter::try_from_default_env() or EnvFilter::from_env()) and only fall back to the literal directives when the env var is absent or invalid. Keep the args.debug path by choosing a more verbose fallback when args.debug is true (e.g. fallback to "iii_acp=debug,iii_sdk=debug") and a less verbose fallback otherwise (e.g. "iii_acp=info,iii_sdk=warn"); apply this where the filter is constructed before calling tracing_subscriber::registry().with(...).with(filter).init().
🤖 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 `@acp/README.md`:
- Around line 166-170: The README example uses the non-canonical brain id
"agent::run" while the rest of the docs and the flag table use
"run::start_and_wait"; update the example invocation to use the canonical brain
id by replacing the shown command iii-acp --brain-fn agent::run with iii-acp
--brain-fn run::start_and_wait (or, if the example intentionally demonstrates a
different function, add a short clarifying note explaining why "agent::run" is
used and how it differs from "run::start_and_wait") so the brain id is
consistent with the documented flag table and usages.
In `@acp/src/handler.rs`:
- Around line 173-273: session ownership isn't re-established after a reload
because owned_sessions is only appended in session_new; update session_load or
session_prompt to claim ownership for the session being interacted with so
forwarded agent events aren't filtered out. Locate the owned_sessions field and
add logic in session_load (or at start of session_prompt) to insert p.session_id
into self.owned_sessions (and ensure it's removed when the turn completes if
transient), updating any related cancel/abort handling (self.cancels
insertion/removal in session_prompt and forward_agent_event's owned.contains
check) so historical loads can receive live external-brain events.
In `@acp/src/main.rs`:
- Around line 37-44: The env var parsing for the CLI flag represented by
use_canonical_brain currently treats any non-empty value as true
(ArgAction::SetTrue behavior); update the argument definition for
use_canonical_brain to use clap's boolean value parser (value_parser =
clap::value_parser!(bool)) so that "0" and "false" are parsed as false, and
remove the SetTrue action/behavior; alternatively, if you prefer to keep current
semantics, update the help text for
use_canonical_brain/IIIACP_USE_CANONICAL_BRAIN to explicitly state that any
non-empty env value (e.g., "1", "true") enables the flag while "0" or "false"
will not be treated specially.
In `@acp/src/session.rs`:
- Around line 117-131: append_history performs a non-atomic read-modify-write
(state_get → push → state_set) which races with concurrent writers and drops
entries; fix by serializing per-session history writes: add a per-session mutex
map (e.g., DashMap<String, tokio::sync::Mutex<()>>) on AcpHandler and acquire
the session-specific lock around the body of append_history (the state_get,
modify, state_set sequence) so only one writer per session_id runs at a time;
alternatively, if the III engine provides an append-style primitive, replace the
read-modify-write in append_history (and the similar append_session_to_index)
with that atomic append operation instead.
- Around line 53-63: The match should not string-match error messages but
instead propagate real errors and detect missing keys via JSON null; change the
Ok arm to let v = unwrap_value(val) and return Ok(None) if v.is_null() else
return Ok(Some(v)), and remove the special-case error handling that converts
Err(e) to Ok(None); keep Err(e) propagated directly. Reference: the value
binding `val`, the helper `unwrap_value`, and the `Err(e)` branch currently
present in the match.
---
Nitpick comments:
In `@acp/src/handler.rs`:
- Around line 483-490: The current wait_aborted function polls an
Arc<AtomicBool> every 100ms causing latency and unnecessary wakeups; replace it
by creating a tokio::sync::Notify (or a tokio::sync::watch::channel<bool>)
together with the abort flag and change wait_aborted to await the notifier
(using notify.notified().await or watching the channel) and have session_cancel
call notify.notify_one() (or send on the watch channel); update call sites that
used wait_aborted (and any tokio::select! usage) to await the notifier directly
to remove the polling loop and eliminate the 100ms sleep.
- Around line 552-592: The seq-stamping and JSON-RPC frame construction in
forward_agent_event duplicates send_notification; extract that logic into a
single helper (either a free function or an Outbound method) with signature like
(outbound: &Outbound, seq: &AtomicU64, method: &str, params: Value) which
increments seq, injects iii.dev/seq into params._meta, wraps params into the
JSON-RPC frame, and writes via outbound.write(&frame). Replace the
stamping/writing block in forward_agent_event with a call to this new helper,
and consider delegating the append_history+notify pairing to the existing
emit_update (or call append_history then the new helper) so history appends and
notifications share the same path.
In `@acp/src/main.rs`:
- Around line 84-93: The current use of EnvFilter::new(...) hardcodes module
directives and ignores RUST_LOG; change to first attempt loading the filter from
the environment (e.g. EnvFilter::try_from_default_env() or
EnvFilter::from_env()) and only fall back to the literal directives when the env
var is absent or invalid. Keep the args.debug path by choosing a more verbose
fallback when args.debug is true (e.g. fallback to
"iii_acp=debug,iii_sdk=debug") and a less verbose fallback otherwise (e.g.
"iii_acp=info,iii_sdk=warn"); apply this where the filter is constructed before
calling tracing_subscriber::registry().with(...).with(filter).init().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71f146f7-3054-4b78-b8a4-2939b40d85c3
📒 Files selected for processing (6)
.github/workflows/create-tag.yml.github/workflows/release.ymlacp/README.mdacp/src/handler.rsacp/src/main.rsacp/src/session.rs
Comment on lines
+37
to
+44
| #[arg( | ||
| long, | ||
| env = "IIIACP_USE_CANONICAL_BRAIN", | ||
| help = "Shortcut for --brain-fn run::start_and_wait. Wires iii-acp \ | ||
| straight to turn-orchestrator. Ignored if --brain-fn is \ | ||
| already set." | ||
| )] | ||
| use_canonical_brain: bool, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
clap v4 ArgAction::SetTrue with env variable behavior 0 false
💡 Result:
In clap v4, for an argument with.action(ArgAction::SetTrue).env("VAR"), if the environment variable VAR is absent, the flag defaults to false [1][2]. If VAR is set to any non-empty value (including "0" or "false"), the flag is considered raised (true), as no value parser is applied by default and any set env var raises flags with non-Set actions [1]. To treat "0" or "false" as false, explicitly add.value_parser(FalseyValueParser::new), which recognizes "0", "f", "false", "off", "n", "no" as false, absent env as false, and anything else as true [1].
Citations:
- 1: https://docs.rs/clap/4.4.6/clap/struct.Arg.html
- 2: https://docs.rs/clap/latest/clap/enum.ArgAction.html
🏁 Script executed:
# Check if README exists and look for IIIACP_USE_CANONICAL_BRAIN documentation
find . -name "README*" -o -name "readme*" | head -5Repository: iii-hq/workers
Length of output: 159
🏁 Script executed:
# Search for IIIACP_USE_CANONICAL_BRAIN in the codebase to understand usage
rg "IIIACP_USE_CANONICAL_BRAIN" --type rust --type markdown --type text -A 2 -B 2Repository: iii-hq/workers
Length of output: 87
🏁 Script executed:
# Check if there are any value_parser usages on bool fields in the main.rs
rg "value_parser" acp/src/main.rs -A 2 -B 2Repository: iii-hq/workers
Length of output: 40
🏁 Script executed:
# Read the acp README
cat -n ./acp/README.md | head -100Repository: iii-hq/workers
Length of output: 4652
🏁 Script executed:
# Read the main.rs file around the problematic lines and look for IIIACP_USE_CANONICAL_BRAIN
cat -n acp/src/main.rs | sed -n '30,50p'Repository: iii-hq/workers
Length of output: 991
🏁 Script executed:
# Search for all IIIACP_USE_CANONICAL_BRAIN references
rg "IIIACP_USE_CANONICAL_BRAIN" -A 2 -B 2Repository: iii-hq/workers
Length of output: 1249
Document or fix IIIACP_USE_CANONICAL_BRAIN boolean parsing behavior.
Clap v4's ArgAction::SetTrue treats any non-empty env var value as true—including "0" and "false". The README documents an example with "IIIACP_USE_CANONICAL_BRAIN": "1", but does not explicitly state that "0" will not work as expected (it will still activate the flag). Either explicitly document that only "1" (or any non-empty value) activates the flag, or parse it as value_parser = clap::value_parser!(bool) to correctly recognize "false" and "0" as false.
🤖 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 `@acp/src/main.rs` around lines 37 - 44, The env var parsing for the CLI flag
represented by use_canonical_brain currently treats any non-empty value as true
(ArgAction::SetTrue behavior); update the argument definition for
use_canonical_brain to use clap's boolean value parser (value_parser =
clap::value_parser!(bool)) so that "0" and "false" are parsed as false, and
remove the SetTrue action/behavior; alternatively, if you prefer to keep current
semantics, update the help text for
use_canonical_brain/IIIACP_USE_CANONICAL_BRAIN to explicitly state that any
non-empty env value (e.g., "1", "true") enables the flag while "0" or "false"
will not be treated specially.
Comment on lines
+53
to
+63
| match result { | ||
| Ok(val) => Ok(unwrap_value(val)), | ||
| Err(e) => { | ||
| let msg = e.to_string().to_lowercase(); | ||
| if msg.contains("not found") || msg.contains("no such") { | ||
| Ok(None) | ||
| } else { | ||
| Err(e) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for typed not-found error variants in iii-sdk usage across the workspace.
rg -nP --type=rust -C3 '\b(NotFound|KeyNotFound|StateNotFound|MissingKey)\b'
rg -nP --type=rust -C3 'IIIError::' | head -60Repository: iii-hq/workers
Length of output: 8148
🏁 Script executed:
#!/bin/bash
# Read the acp/src/session.rs file to understand the context
fd -t f session.rs | grep acpRepository: iii-hq/workers
Length of output: 77
🏁 Script executed:
#!/bin/bash
# Search for the session.rs file and read the relevant section
find . -path "*/acp/src/session.rs" -type f -exec head -80 {} \;Repository: iii-hq/workers
Length of output: 2580
🏁 Script executed:
#!/bin/bash
# Find IIIError definition in iii-sdk or elsewhere
rg -n "enum IIIError" --type=rust
rg -n "struct IIIError" --type=rust
rg -n "impl.*IIIError" --type=rust | head -20Repository: iii-hq/workers
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Look at patterns of state::get usage and error handling
rg -n "state::get" -A 10 -B 2 --type=rust | head -100Repository: iii-hq/workers
Length of output: 6775
🏁 Script executed:
#!/bin/bash
# Check if there are any backend-specific error types or docs
rg -n "not found" --type=rust | grep -i "state\|error" | head -20Repository: iii-hq/workers
Length of output: 1054
🏁 Script executed:
#!/bin/bash
# Check if IIIError is defined locally or comes from iii-sdk
rg -n "pub enum IIIError\|pub struct IIIError" --type=rust
grep -r "from iii_sdk" acp/src/ --include="*.rs"Repository: iii-hq/workers
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Look for engine state::get contract or documentation
rg -n "state::get" --type=rust | grep -i "comment\|doc\|//"
# Also check how turn-orchestrator handles this differently
cat turn-orchestrator/src/persistence.rs | sed -n '100,120p'Repository: iii-hq/workers
Length of output: 1018
🏁 Script executed:
#!/bin/bash
# Look for state::get documentation in codebase and check if it returns null or error for missing
rg -B5 -A5 "function_id.*state::get" --type=rust | head -80Repository: iii-hq/workers
Length of output: 5386
🏁 Script executed:
#!/bin/bash
# Check if there's documentation or comments about state::get behavior
rg -n "state::get" --type=rust -B3 | grep -A3 -B3 "null\|missing\|error\|contract"Repository: iii-hq/workers
Length of output: 2325
🏁 Script executed:
#!/bin/bash
# Look for error code patterns in state::get errors or other error handling
rg -n "S2[0-9]{2}\|error.*code\|error.*:" --type=rust | grep -i "state\|not.*found" | head -30Repository: iii-hq/workers
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check session-tree's handling of state::get more carefully
cat session-tree/src/store_iii_state.rs | sed -n '115,135p'Repository: iii-hq/workers
Length of output: 868
🏁 Script executed:
#!/bin/bash
# Find callers of acp's state_get function
rg -n "state_get\|session_load" acp/src/ --type=rust -B2 -A5Repository: iii-hq/workers
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if there's actual evidence of state::get returning errors for missing keys
rg -n "state::get" --type=rust -A15 | grep -B5 -A10 "is_null\|Err.*not\|Err.*no such"Repository: iii-hq/workers
Length of output: 11144
Fix error handling in state_get: check is_null() instead of catching error messages.
The code incorrectly assumes that errors from state::get indicate a missing key, but the actual contract (evident across the codebase) is that state::get returns Ok(null) for missing keys and only errors on transient failures. Catching the error and string-matching "not found" / "no such" silently converts real backend failures (network timeouts, backend crashes) into Ok(None), causing callers to report "session not found" for actual infrastructure problems.
Follow the pattern used throughout the codebase (session-tree, auth-rbac, skills, etc.): propagate errors, and check is_null() on the successful response.
🤖 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 `@acp/src/session.rs` around lines 53 - 63, The match should not string-match
error messages but instead propagate real errors and detect missing keys via
JSON null; change the Ok arm to let v = unwrap_value(val) and return Ok(None) if
v.is_null() else return Ok(Some(v)), and remove the special-case error handling
that converts Err(e) to Ok(None); keep Err(e) propagated directly. Reference:
the value binding `val`, the helper `unwrap_value`, and the `Err(e)` branch
currently present in the match.
Comment on lines
+117
to
+131
| pub async fn append_history( | ||
| iii: &III, | ||
| conn_id: &str, | ||
| session_id: &str, | ||
| entry: Value, | ||
| ) -> Result<(), IIIError> { | ||
| let scope = scope(); | ||
| let key = session_history_key(conn_id, session_id); | ||
| let mut hist = state_get(iii, &scope, &key) | ||
| .await? | ||
| .and_then(|v| v.as_array().cloned()) | ||
| .unwrap_or_default(); | ||
| hist.push(entry); | ||
| state_set(iii, &scope, &key, Value::Array(hist)).await | ||
| } |
There was a problem hiding this comment.
Race condition in append_history: read-modify-write without atomicity loses entries under streaming.
append_history does a state_get → push → state_set sequence with no synchronization. During a prompt turn, the agent::events stream subscriber (forward_agent_event in handler.rs) invokes this concurrently for every text/thinking delta and tool event, while session_prompt also writes the user-message entry. Two callbacks racing on the same session will both load the same array, each push their own entry, and the loser overwrites the winner — silently dropping session/update history that session/load later replays.
append_session_to_index has the same shape but is much less hot (only session/new).
Consider serializing per-session history writes (e.g., a DashMap<String, tokio::sync::Mutex<()>> keyed by session_id held across the read/write pair, kept in AcpHandler) or moving to an append-style state primitive if the iii-engine exposes one.
🤖 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 `@acp/src/session.rs` around lines 117 - 131, append_history performs a
non-atomic read-modify-write (state_get → push → state_set) which races with
concurrent writers and drops entries; fix by serializing per-session history
writes: add a per-session mutex map (e.g., DashMap<String,
tokio::sync::Mutex<()>>) on AcpHandler and acquire the session-specific lock
around the body of append_history (the state_get, modify, state_set sequence) so
only one writer per session_id runs at a time; alternatively, if the III engine
provides an append-style primitive, replace the read-modify-write in
append_history (and the similar append_session_to_index) with that atomic append
operation instead.
Two changes after end-to-end test in Zed:
1. handler.rs — fix stream-trigger envelope shape
The engine's stream trigger payload uses camelCase keys (groupId,
streamName) and nests the AgentEvent under event.data, not at the
top level. forward_agent_event was reading group_id and data
directly, so every brain-emitted AgentEvent got dropped on the
floor and Zed saw no session/update notifications during a turn.
Added extract_event_payload that handles both shapes (camelCase
nested + legacy snake_case flat) so the worker keeps working if
the engine envelope flips later.
Also added a message_end -> agent_message_chunk branch in
translate_agent_event. turn-orchestrator at HEAD does not emit
message_update text deltas (provider-router consumes the streaming
response internally and surfaces the assembled assistant message
as one message_end event). Without this branch the editor never
sees the assistant text. Added two unit tests covering the new
path.
2. README.md — full rewrite around three things the previous draft
was thin on:
- WHY iii-acp exists vs MCP / skills / agent workers. They stack,
they don't compete: ACP is the north interface (editor -> iii),
MCP is the south interface (iii -> tools). What an editor session
gets from iii that a vanilla agent host doesn't: provider routing,
budgets, guardrails, audit trail, RBAC, durable observability.
- Concrete prereqs. The minimum stack (iii-state, iii-stream,
iii-queue, then the brain workers) plus the auth::set_token
command to drop the API key in. Includes a one-shot
run::start_and_wait probe to verify the stack before plugging an
editor in.
- Every supported ACP client. Not just Zed: VS Code (ACP Client
extension), JetBrains (plugin), Neovim (3 plugins listed),
Emacs (agent-shell.el), Obsidian, Unity, Chrome, plus 30+
CLI/notebook/mobile/messaging clients. Setup pattern is the
same everywhere — point at the iii-acp binary, set IIIACP_*
env vars.
Validation
- 17 lib + 7 protocol envelope tests pass.
- Live end-to-end against a real Anthropic API key: Zed agent panel
-> iii-acp -> turn-orchestrator -> provider-router ->
provider-anthropic -> stream::set agent::events -> iii-acp stream
subscriber -> session/update notification -> Zed renders Claude's
reply.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@acp/src/handler.rs`:
- Around line 65-69: The event subscriber registration can fail silently: when
brain.function_id.is_some() and register_event_subscriber(&iii, &conn_id,
&outbound, &update_seq, &owned_sessions) yields (None, None), the handler is
still constructed but run_external_brain() will never receive updates; change
the code that currently assigns event_subscriber/event_function to instead
detect that failure and fail fast (return an Err from the handler constructor or
explicitly disable/reject external-brain prompts) so external-brain prompts are
not accepted unless register_event_subscriber succeeded; apply the same pattern
to the other similar blocks (around lines 375-427 and 530-543) to either return
an error or mark brain.function_id as inactive when registration fails.
- Around line 56-59: The code currently generates a transient conn_id in
Acp::new() and uses it to compose persisted session keys, which prevents a
restarted process from finding existing sessions; change session persistence to
use a stable client-provided identifier (e.g. session_id) for all persisted keys
and index lookups used by session/load and session/list while keeping conn_id
only as transient ownership metadata; update the constructors and places
referenced (Acp::new, the logic that writes/reads session keys around symbols
conn_id, owned_sessions, and any functions/methods that build session keys or
indexes) to stop embedding conn_id in durable keys and instead include conn_id
only in in-memory structures or transient metadata while ensuring existing
write/read code uses session_id (or a client-stable namespace) for durable
storage and lookups.
- Around line 367-371: The synthetic empty echo being appended to history via
append_history(&self.iii, &self.conn_id, session_id, echo).await is creating a
blank agent_message_chunk that gets replayed later; remove that extra
append_history call (or guard it so it doesn't run for an empty echo) and rely
on emit_update() to persist real echo chunks instead, ensuring no empty
"agent_message_chunk" is written to session history.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1afdd053-2a50-48f6-aa4c-7297173ca025
📒 Files selected for processing (2)
acp/README.mdacp/src/handler.rs
Three valid findings from CodeRabbit's review of the canonical-brain
refactor. All verified against current code, all fixed.
1. Sessions persisted under per-process conn_id broke session/load
after editor reconnect (handler.rs:59). Real durability bug —
conn_id is regenerated in AcpHandler::new, but the same conn_id was
the prefix on every session key, history key, and the index key.
Once the editor reconnected to a fresh subprocess, session/list
came back empty and session/load couldn't find old records even
though they still existed in iii state.
Fix: drop conn_id from persisted state keys entirely. session_id
is a globally unique uuid (sess_<32hex>), it survives subprocess
restarts. conn_id stays in-memory only as transient ownership
metadata for routing agent::events to the subprocess that's
currently driving each session.
- session_key, session_index_key, session_history_key no longer
take conn_id
- append_history, read_history, append_session_to_index,
remove_session_from_index, read_session_index drop conn_id arg
- forward_agent_event drops conn_id arg
- session/load now also inserts the session into owned_sessions
so agent::events for it route to this subprocess from now on
- session/prompt defensively claims ownership too (covers brains
that started a run via run::start outside ACP)
- Test renamed: keys_namespace_by_conn -> keys_are_session_id_only
2. Brain prompts ran even when agent::events subscriber failed to
register (handler.rs:69). With no subscriber, every brain-emitted
AgentEvent fell on the floor — Zed showed a stuck UI with no
updates and only the final stopReason ever returned.
Fix: add event_subscriber_healthy field on AcpHandler, set true
when no external brain configured (echo path doesn't need it) or
when both function + trigger registered cleanly. session/prompt
now fails fast with INTERNAL_ERROR + actionable message
("ensure iii-stream worker is active") when the brain is
configured but the subscriber didn't come up.
3. Echo brain wrote a synthetic empty agent_message_chunk to history
that was never sent live (handler.rs:371). On session/load that
blank chunk replayed as empty assistant content. Removed the
3-line append_history call; emit_update already persists every
real chunk during the turn.
Validation:
- 17 lib + 7 protocol envelope tests pass. Test renamed to reflect
new key shape.
- cargo clippy --all-targets -- -D warnings clean.
- Live smoke against engine + 7-worker brain stack: real Claude reply
streamed through iii-acp into stdout. 2 session/update notifications
per turn, final stopReason=end_turn.
Triaged 11 findings against current code. 4 already fixed in prior commits (skipped with reason in inline replies). 4 valid + fixed: 1. handler.rs:483-490 — wait_aborted polled an Arc<AtomicBool> every 100ms. Replaced with a CancelHandle struct pairing the AtomicBool (kept for tool handlers that poll) with a tokio::sync::Notify. session_cancel wakes any awaiter immediately via notify_waiters; run_external_brain's tokio::select! awaits cancel.wait() with no poll loop. Latency on cancel drops from ≤100ms to single-digit ms. 2. handler.rs:552-592 — seq stamp + JSON-RPC frame construction was duplicated between send_notification (method on AcpHandler) and forward_agent_event (free fn called from the stream subscriber). Extracted both into write_notification(outbound, seq, method, params). send_notification + forward_agent_event now share one code path so _meta.iii.dev/seq behavior can't drift. 3. session.rs:117-131 — append_history did read-modify-write (state_get → push → state_set). Concurrent agent::events for the same session race and drop entries. Engine's state::update only ships set/merge/increment/decrement/remove ops in this version (no array append; verified at runtime: 'unknown variant '), so atomic append at the engine level isn't available. Added a per-session in-process Mutex map (history_locks: DashMap<String, Arc<Mutex<()>>>) on AcpHandler. session_prompt, emit_update, and the stream subscriber forward_agent_event all acquire the session-keyed lock before the read-modify-write. Closes the race for the common case (multiple agent::events arriving in parallel from one stream subscriber). Read-modify-write of the index keeps a documented small race for session/new vs session/close; acceptable in v0 since session/close is a single-user single-action. 4. main.rs:84-93 — EnvFilter hardcoded module directives, ignored RUST_LOG. Now tries EnvFilter::try_from_default_env first; only falls back to the literal directives when RUST_LOG is unset/invalid. The --debug switch still chooses a more verbose fallback. Operators get per-module control via RUST_LOG. Skipped (already fixed earlier or invalid against current code): - handler.rs:65-69 — fail-fast on subscriber registration failure: ALREADY fixed in 883d1f2 via event_subscriber_healthy flag. - handler.rs:56-59 — conn_id in persisted keys: ALREADY fixed in 883d1f2, session keys are session_id only now. - handler.rs:367-371 — synthetic empty echo chunk: ALREADY fixed in 883d1f2. - handler.rs:173-273 — owned_sessions claim on session/load: ALREADY fixed in 883d1f2, session_load + session_prompt both insert into owned_sessions defensively. - README.md:166-170 — agent::run example inconsistency: stale finding; README rewrite in 77d74a0 replaced that example with run::start_and_wait. No agent::run reference remains. - main.rs:37-44 — use_canonical_brain SetTrue semantics: clap's derive bool field already enforces strict 'true'/'false' (rejected '1' at runtime earlier). No change needed. - session.rs:53-63 — string-match on state::get errors: shared pattern with llm-router/src/state.rs and other iii workers. Defensive against engine adapter version skew. Tightening should happen at the iii engine level, not per-worker; diverging here would break consistency. Validation: - 17 lib + 7 protocol envelope tests pass. - cargo clippy --all-targets -- -D warnings clean. - Live smoke against engine + 7-worker brain stack with real Anthropic API key: 2 session/update notifications + stopReason=end_turn, matching pre-refactor behavior. CancelHandle path tested by session/cancel mid-prompt during prior smoke runs (still passes).
rohitg00
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (2)
acp/src/session.rs (1)
58-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
state_getmasks real backend errors as missing-key by string-matching.The
Errarm catches errors and converts those whose message contains"not found"or"no such"intoOk(None). The contract used elsewhere in the workspace is thatstate::getreturnsOk(null)for missing keys and only errs on transient/infra failures — yourunwrap_valuealready handles the null case. As written, network/timeouts/backend crashes will be misreported as "session not found" bysession_load/session_prompt/session_closecallers, hiding real outages.🛡️ Suggested fix
- match result { - Ok(val) => Ok(unwrap_value(val)), - Err(e) => { - let msg = e.to_string().to_lowercase(); - if msg.contains("not found") || msg.contains("no such") { - Ok(None) - } else { - Err(e) - } - } - } + result.map(unwrap_value)🤖 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 `@acp/src/session.rs` around lines 58 - 68, The current Err arm in state_get string-matches error messages and maps any error containing "not found"/"no such" to Ok(None), which masks real backend failures; change this to stop string-matching and instead either (a) propagate Err(e) unchanged so transient/backend errors surface to callers (session_load/session_prompt/session_close), or (b) if the state backend exposes a concrete NotFound error variant, match on that specific error type (e.g., state::Error::NotFound) to return Ok(None) while propagating all other Err(e); update the match in state_get (the Err(e) arm) and keep unwrap_value usage for the Ok path.acp/src/main.rs (1)
70-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
env = "IIIACP_RBAC_TAG"for parity with the other args.Every other CLI option in
Args(engine_url,brain_fn,use_canonical_brain,model,provider,system_prompt) supports anIIIACP_*env override;rbac_tagis the lone exception. Env-driven deploys (containers, systemd units) currently can't set the RBAC tag without injecting CLI args.🔧 Suggested fix
#[arg( long, + env = "IIIACP_RBAC_TAG", value_name = "TAG", help = "Forward an `x-iii-rbac-tag` header on the worker WebSocket \ upgrade. iii-worker-manager's `auth_function_id` reads this \ tag to apply policy." )] rbac_tag: Option<String>,🤖 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 `@acp/src/main.rs` around lines 70 - 77, The rbac_tag CLI option in the Args struct lacks an env override like the other options; add env = "IIIACP_RBAC_TAG" to the #[arg(...)] attributes for the rbac_tag field so it can be set via environment variables. Locate the rbac_tag field (rbac_tag: Option<String>) and update its #[arg(...)] macro to include env = "IIIACP_RBAC_TAG", matching the pattern used by engine_url, brain_fn, use_canonical_brain, model, provider, and system_prompt.
🧹 Nitpick comments (2)
acp/src/session.rs (1)
192-200: 💤 Low valueComment contradicts the implementation —
append_session_to_indexis not atomic.
append_session_to_index(lines 148–162) does a read-modify-write throughstate_get/state_set; this comment asserts it "uses an atomic append". The dedupe-on-read is a defensive workaround for that race, not a consequence of an atomic append. Worth fixing the comment so future readers don't rely on a guarantee that doesn't exist.📝 Suggested wording
- // Dedupe on read — append_session_to_index uses an atomic - // append, so the index can carry duplicates if the same id - // ever lands twice. + // Dedupe on read — append_session_to_index is a non-atomic + // read-modify-write, so concurrent appends can let the same + // id land twice in the persisted array.🤖 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 `@acp/src/session.rs` around lines 192 - 200, The comment claiming append_session_to_index performs an "atomic append" is incorrect; update the comment near the dedupe-on-read block to state that append_session_to_index performs a read-modify-write via state_get/state_set (so races are possible) and that the dedupe using seen/insert in the loop is a defensive workaround rather than a consequence of atomic behavior; reference append_session_to_index, state_get, state_set and the seen/out dedupe logic so readers understand the real reason for deduplication.acp/src/handler.rs (1)
344-354: 💤 Low valueDrop the unused initial value of
record_payload.
record_payloadis initialized tojson!({})on line 344 and unconditionally reassigned inside theif letbefore its only use (line 352). The outer binding can go away, simplifying the block.♻️ Suggested cleanup
- let mut record_payload = json!({}); if let Some(rec) = state_get(&self.iii, &scope(), &key) .await .map_err(|e| (INTERNAL_ERROR, e.to_string()))? { if let Ok(mut r) = serde_json::from_value::<SessionRecord>(rec) { r.last_activity_ms = now_ms(); - record_payload = serde_json::to_value(&r).unwrap_or(Value::Null); - let _ = state_set(&self.iii, &scope(), &key, record_payload.clone()).await; + if let Ok(payload) = serde_json::to_value(&r) { + let _ = state_set(&self.iii, &scope(), &key, payload).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 `@acp/src/handler.rs` around lines 344 - 354, Remove the unused initial binding of record_payload and instead create and use it only when state_get returns a record: call state_get(&self.iii, &scope(), &key).await, unwrap the SessionRecord via serde_json::from_value::<SessionRecord>(rec), update r.last_activity_ms = now_ms(), then serialize into a local variable (e.g., record_payload) and pass that to state_set(&self.iii, &scope(), &key, record_payload.clone()).await; ensure you only declare record_payload inside the Some(rec) branch so the outer json!({}) initialization is eliminated.
🤖 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.
Duplicate comments:
In `@acp/src/main.rs`:
- Around line 70-77: The rbac_tag CLI option in the Args struct lacks an env
override like the other options; add env = "IIIACP_RBAC_TAG" to the #[arg(...)]
attributes for the rbac_tag field so it can be set via environment variables.
Locate the rbac_tag field (rbac_tag: Option<String>) and update its #[arg(...)]
macro to include env = "IIIACP_RBAC_TAG", matching the pattern used by
engine_url, brain_fn, use_canonical_brain, model, provider, and system_prompt.
In `@acp/src/session.rs`:
- Around line 58-68: The current Err arm in state_get string-matches error
messages and maps any error containing "not found"/"no such" to Ok(None), which
masks real backend failures; change this to stop string-matching and instead
either (a) propagate Err(e) unchanged so transient/backend errors surface to
callers (session_load/session_prompt/session_close), or (b) if the state backend
exposes a concrete NotFound error variant, match on that specific error type
(e.g., state::Error::NotFound) to return Ok(None) while propagating all other
Err(e); update the match in state_get (the Err(e) arm) and keep unwrap_value
usage for the Ok path.
---
Nitpick comments:
In `@acp/src/handler.rs`:
- Around line 344-354: Remove the unused initial binding of record_payload and
instead create and use it only when state_get returns a record: call
state_get(&self.iii, &scope(), &key).await, unwrap the SessionRecord via
serde_json::from_value::<SessionRecord>(rec), update r.last_activity_ms =
now_ms(), then serialize into a local variable (e.g., record_payload) and pass
that to state_set(&self.iii, &scope(), &key, record_payload.clone()).await;
ensure you only declare record_payload inside the Some(rec) branch so the outer
json!({}) initialization is eliminated.
In `@acp/src/session.rs`:
- Around line 192-200: The comment claiming append_session_to_index performs an
"atomic append" is incorrect; update the comment near the dedupe-on-read block
to state that append_session_to_index performs a read-modify-write via
state_get/state_set (so races are possible) and that the dedupe using
seen/insert in the loop is a defensive workaround rather than a consequence of
atomic behavior; reference append_session_to_index, state_get, state_set and the
seen/out dedupe logic so readers understand the real reason for deduplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b94d9a3-97a8-4f84-b1a7-5cca044f9f24
📒 Files selected for processing (3)
acp/src/handler.rsacp/src/main.rsacp/src/session.rs
5 tasks
rohitg00
added a commit
that referenced
this pull request
May 6, 2026
PR #63 shipped 8 client->agent methods. ACP spec defines three more client->agent methods that fall to METHOD_NOT_FOUND today; this commit implements all of them. session/resume — like session/load but skips history replay. Per ACP spec: 'useful for agents that can resume sessions but don't implement full session loading.' Refreshes cwd + mcpServers on the session record and claims ownership so subsequent agent::events route to this subprocess. session/set_mode — persists modeId on the session record. Brain workers can read it on the next session/prompt turn (e.g. via a system prompt suffix or per-mode tool gating). Validation of the mode against any agent-specific catalog is left to the brain. session/set_config_option — persists configId/value pairs in config_options on the session record. Same rationale as set_mode: persistence here, semantics in the brain. Implementation notes: - SessionRecord gains mode: Option<String> (skip_serializing_if = is_none) and config_options: serde_json::Map<String, Value> (defaults empty). Backward-compatible with sessions written before this change since both fields use serde defaults. - update_session_record helper: read-modify-write of one record guarded by the per-session history mutex (we reuse the lock that already exists for append_history rather than adding a parallel lock map for the same key). - agentCapabilities.sessionCapabilities now advertises { list: {}, close: {}, resume: {} } — matches the ACP schema slots that exist in this version. set_mode and set_config_option ship without an explicit capability flag because the in-repo schema doesn't have those slots yet; clients that try them succeed, clients that don't try are unaffected. README method table is now exhaustive: every ACP method is listed with its current implementation status. The previously-misleading 'session/request_permission deferred to v0.2' line is replaced with explicit per-method rows for fs/* and terminal/* so the deferred surface is fully visible. Reverse-RPC paths (session/request_permission, fs/*, terminal/*) remain deferred — they need a JSON-RPC framer that can originate requests from the agent side, and our internal iii brains use iii primitives directly for filesystem and terminal access. Those can land later without breaking this PR. Validation: - 17 lib tests + 10 protocol envelope tests pass (3 new round-trip tests for the new param shapes). - cargo clippy --all-targets -- -D warnings clean. - Live smoke against the 7-worker brain stack: - sessionCapabilities advertises {close, list, resume} on initialize. - session/set_mode persists modeId='code' (verified via state::get). - session/set_config_option persists configId='thinking' value='high'. - session/resume refreshes cwd to '/new'. - All three return {} on success, INVALID_PARAMS (-32602) on missing sessionId. - Full canonical brain flow (session/prompt streaming real Claude through agent::events) still passes.
This was referenced May 9, 2026
4 tasks
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Adds iii-acp, a stdio JSON-RPC worker that turns the iii engine into an agent any ACP-speaking editor can drive — Zed, VS Code, JetBrains, Neovim, Emacs, Obsidian, plus 30+ CLI/notebook/mobile clients. Same binary, no editor plugin, no fork.
The brain contract aligns to iii's existing canonical shape (`run::start_and_wait` + `agent::events` stream): turn-orchestrator drops in as the brain with zero adapter.
End-to-end validated locally against the full stack with a real Anthropic API key — Zed agent panel → iii-acp → turn-orchestrator → provider-router → provider-anthropic → Claude → streamed back.
Why this exists (vs MCP, skills, agent workers)
These are stack-able layers, not alternatives. ACP fills the slot the others don't.
ACP = north edge. MCP = south edge. They coexist:
```
Editor (Zed, VS Code, Neovim, …)
↓ ACP ← iii-acp (this PR)
iii engine + brain workers
↓ MCP ← iii-mcp / mcp-client
External tool servers
```
What an editor session gets from iii that vanilla agent hosts don't
Methods (v0 surface)
Brain contract (canonical, no adapter)
iii-acp talks to the canonical iii shape used by `turn-orchestrator` and every provider worker:
```jsonc
// iii.trigger("run::start_and_wait", ...)
{
"session_id": "sess_",
"messages": [{
"role": "user",
"content": [{"type": "text", "text": "..."}],
"timestamp": 1234567890
}],
"model": "claude-sonnet-4-5-20250929",
"provider": "anthropic",
"system_prompt": "...",
"timeout_ms": 600000
}
// returns
{ "session_id": "...", "messages": [...], "turn_count": N }
```
Streaming is the iii ecosystem's existing `agent::events` stream (group_id = session_id). iii-acp registers one stream subscriber per connection and translates AgentEvent → ACP session/update:
Same stream `context-compaction` already subscribes to. No bespoke iii-acp publish protocol.
Prerequisites
```bash
Engine builtins iii-acp uses directly
iii worker add iii-state iii-stream iii-queue
acp itself
iii worker add acp
Canonical brain stack
iii worker add turn-orchestrator provider-router provider-anthropic auth-credentials \
session-inbox llm-budget hook-fanout
Optional but recommended: iii's distinctive primitives
iii worker add guardrails dlp-scrubber audit-log policy-denylist context-compaction
```
Store the API key once:
```bash
iii trigger --function-id auth::set_token \
--payload '{"provider":"anthropic","credential":{"type":"api_key","key":"sk-ant-..."}}'
```
Configuration
Editor wiring (Zed example)
```jsonc
{
"agent_servers": {
"iii-acp": {
"type": "custom",
"command": "/path/to/iii-acp",
"env": {
"IIIACP_USE_CANONICAL_BRAIN": "true",
"IIIACP_MODEL": "claude-sonnet-4-5-20250929",
"IIIACP_PROVIDER": "anthropic"
}
}
}
}
```
Same pattern on VS Code (ACP Client extension), JetBrains, Neovim (CodeCompanion / agentic.nvim / avante.nvim), Emacs (agent-shell.el), Obsidian, Unity, Chrome, plus all CLI clients (`acpx`, `Nori CLI`, …). Full list in README.
Architectural notes
Validation
Workflow updates
Follow-ups (not in this PR)
Test plan