feat(opencode): per-session agent isolation (RFC-0026) - #18
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces RFC-0023, which proposes a session-scoped interrupt routing mechanism to replace shared instance-level state with a per-session registry, ensuring concurrent agent safety. The review feedback highlights several logical errors and internal inconsistencies within the RFC document. Specifically, the pseudo-code contains bugs regarding session ID fallbacks and flawed Python expression evaluation. Additionally, the reviewer pointed out that the document needs to be updated to remove references to a fallback heuristic that was explicitly rejected in the final decision record to maintain consistency across the proposal.
Derivative RFC of RFC-0021 addressing concurrent safety regression from PR #17. Proposes dict[session_id, AgentRunContext] registry for cross-task interrupt routing, replacing instance variables _active_run_ctx and _iteration_task that violate per-run isolation. Key decisions: - self.session_id internal reads migrated to run_ctx (all agent types) - self._cancelled retained for background runs only - No fallback heuristic; interrupt() requires session_id or run_ctx - self.parent_session_id also migrated to AgentRunContext Reviewed by Metis (pre-planning) and Oracle (2 rounds). Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Skills were returned with source='skill' from GET /command, but the OpenCode TUI autocomplete explicitly filters out source==='skill' commands (autocomplete.tsx:363). Native OpenCode's oh-my-openagent plugin injects skills with source='command', bypassing the filter. Changes: - Change skill command source from 'skill' to 'command' in GET /command endpoint (agent_routes.py) to match native OpenCode behavior - Expand Command model to match native SDK Command.Info type: add agent, model, subtask, hints fields; make description and source optional - Add _extract_hints() helper to extract / placeholders from skill templates, matching native Command.hints() utility - Fix server.py CommandStore update: replace broken add_commands() call with register_command(cmd, replace=True) loop - Add fallback skill lookup via pool.skill_commands in session_routes when CommandStore misses dynamically-added skills
bd6db06 to
70232fb
Compare
Replace the OpenCode server's single shared agent usage with per-session agent instances so concurrent clients stop clobbering each other's mutable state. Key changes: - ServerState: add _session_agents dict + _session_agent_locks for per-session agent registry with double-check locking pattern - get_or_create_agent(session_id): race-free creation using NativeAgentConfig.get_agent() (creates NEW instances, not cached) - _create_session_agent(): wraps agent creation in ConfigContextManager to resolve relative tool schema paths - cleanup_all_session_agents() / remove_session_agent(): graceful shutdown for per-session agents with partial-init safety - Cache _pool, _storage, _agent_config in __post_init__ so non- session-scoped deps remain accessible without shared agent - session_routes: all session flows migrated to get_or_create_agent() - Removed agent_lock / bind_agent_to_session() usage - Removed original_model save/restore in init_session() - abort_session() targets correct session agent - fork_session() creates independent agent instance - message_routes: _process_message_locked() uses per-session agent directly, no agent_lock, no model save/restore - config_routes: model propagation to _session_agents on set_model - 8 new test files with ~55 tests covering: - Session agent registry isolation and race-free creation - Server lifecycle cleanup on shutdown - Session isolation (cross-talk, fork divergence, abort targeting) - Message isolation (concurrent processing, model scoping) - Route discovery and module loading - Session cleanup (delete-session, partial-init safety) - Concurrency (independent sessions, interrupt isolation, FIFO) - Model/fork/input-provider isolation regression Fixes: FileNotFoundError in QuestionProvider when creating session agents at runtime (ConfigContextManager exited before request handlers run, leaving _config_dir_global = None)
Leoyzen
changed the title
When abort_session was called while the agent had a pending question (e.g., question_for_user / get_elicitation), the question's Future remained unresolved. The agent would continue streaming after the user answered the question, even though the session was marked idle. Root cause: abort_session interrupted the agent and cancelled background tasks, but did not cancel pending question Futures. The LLM call had already completed and created a tool call event for a question. When the user answered, the Future resolved and the agent resumed. Fix: Add cancel_session_pending_questions(session_id) to ServerState and call it at the top of abort_session(), BEFORE cancelling background tasks and interrupting the agent. Cancelling the Future causes get_elicitation() to catch CancelledError and return ElicitResult(action='cancel'), which raises RunAbortedError through the stream, properly finalizing the assistant message.
- Register current task in _process_message_locked so abort_session can cancel it even for sync send_message path - Add cancel_active_message_task() to ServerState for direct task cancellation - Re-cancel pending questions after interrupt to close TOCTOU race (agent may ask questions between initial cancel and interrupt)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements per-session agent isolation to resolve concurrency issues and deadlocks caused by the previous shared-agent architecture. By introducing a per-session agent registry in ServerState, each session now operates with its own agent instance, eliminating the need for a global agent_lock. My review identified critical issues regarding agent lifecycle management, potential race conditions with subagent delegation, and unsafe dictionary iteration during model updates. I have provided suggestions to address these risks, including proper agent initialization, snapshotting the registry for iteration, and ensuring forked sessions correctly synchronize their conversation history.
- Call __aenter__ on per-session agents in get_or_create_agent so MCP subprocesses and tool schemas are initialized (only for agents created from config; test mocks skip this path) - Load copied conversation history into fork agent so the LLM sees the full context on the first message after forking - Use list() snapshot when iterating _session_agents.items() with await inside the loop body to prevent RuntimeError on concurrent mutation - Add NOTE comment about subagent shared-instance limitation in _process_message_locked
|
/gemini review |
Move RFC-0023 (session-scoped interrupt routing) from draft/ to implemented/ and RFC-0026 (per-session agent isolation) from accepted/ to implemented/. Both have been fully implemented in the current PR.
There was a problem hiding this comment.
Code Review
This pull request implements per-session agent isolation to resolve concurrency issues and deadlocks in the OpenCode server. By replacing the shared agent instance and global agent lock with a per-session agent registry, multiple sessions can now run concurrently without cross-contamination. My feedback addresses a missing await in the RFC documentation and a redundant registry cleanup call in the implementation.
I am having trouble creating individual review comments. Click here to see my feedback.
docs/rfcs/accepted/RFC-0026-per-session-agent-isolation.md (197)
The function get_or_create_agent is an async function, so it should be awaited. This applies to other similar examples in this document as well.
agent = await state.get_or_create_agent(session_id)
src/agentpool_server/opencode_server/state.py (310-314)
The line self._session_agents.pop(session_id, None) is intended to clean up if agent.aenter() fails. However, the agent is only added to self._session_agents on line 315, after this block. Therefore, this pop call is ineffective as the agent is not yet in the dictionary.
You can safely remove this line and update the comment to reflect that the exception propagation prevents the agent from being added to the registry.
except Exception:
# If init fails, the partially-created agent is not yet
# in the registry, so no cleanup is needed here. The
# exception will propagate and prevent it from being added.
raise
…tigation 5 diagnostic logging points to confirm two bug hypotheses: - Bug 1 (cross-session): shared state.agent leaks conversation across sessions - Bug 2 (duplicate): user msg appears in both message_history and prompts Points: _create_session_agent, _process_message_locked agent resolution, _stream_events history vs prompts overlap. Search 'DIAG:' to find/remove.
Bug1a: _agent_config.name was None because manifest stores original config references (name set from dict key, not YAML). Pool's model_copy only affects local copy. Fix: model_copy in __post_init__ to set name. Bug1c: request.agent matching default agent name replaced per-session agent with shared pool singleton. Fix: skip delegation when request.agent matches the session's default agent config name. Bug2: User message appeared in both message_history and prompts of agentlet.iter() because _run_stream_once() adds user_msg to conversation before _stream_events() calls get_history(). Fix: exclude last history entry when it matches user_msg (identity check). Also removes all 5 DIAG: diagnostic logging statements.
|
/gemini review Applied fixes for all 5 review comments (round 2):
Additional fixes (cross-session contamination bugs found during testing):
RFCs updated: RFC-0023 and RFC-0026 moved to |
There was a problem hiding this comment.
Code Review
This pull request implements per-session agent instances to remove the global agent_lock and enable concurrent multi-client access. It introduces a session agent registry in ServerState and updates message and session routes to use isolated agent instances. The review identifies critical concurrency risks where shared subagent singletons and fallback mechanisms could lead to state clobbering and data leakage across sessions, violating the core isolation goals of the refactor.
| # NOTE: Subagents from state.pool.all_agents are shared singleton | ||
| # instances. Mutating session_id/_input_provider on them is safe ONLY | ||
| # because same-session serialization (via get_session_lock) prevents | ||
| # concurrent access. Per-session subagent instances are NOT feasible | ||
| # due to MCP subprocess overhead. If OpenCode ever supports direct | ||
| # multi-agent selection, this must be redesigned via AgentPool's | ||
| # delegation/team mechanism instead. |
There was a problem hiding this comment.
The assumption that subagents from state.pool.all_agents are safe to mutate because of get_session_lock is incorrect. get_session_lock is per-session and only serializes access within a single session. It does not prevent concurrent access to the same shared subagent instance from different sessions. If Session A and Session B both use the same subagent (e.g., 'researcher'), they will concurrently mutate its session_id and _input_provider at line 427, leading to race conditions and data leakage. This violates the per-session isolation goal of this PR. Note that lock acquisition decisions should remain here in the routing layer to maintain architectural boundaries.
References
- Avoid moving routing-layer logic (e.g., lock acquisition decisions) into the state management layer (ServerState) to maintain clear architectural boundaries.
| if agent.session_id != session_id: | ||
| agent.session_id = session_id | ||
| agent._input_provider = state.ensure_input_provider(session_id) | ||
|
|
There was a problem hiding this comment.
This mutation of shared subagent state is unsafe under cross-session concurrency. Since the global agent_lock has been removed, multiple sessions can reach this block simultaneously for the same subagent instance. To maintain isolation, subagents should also be instantiated per-session, or access to shared instances must be protected by a global lock specific to that agent instance. Note that the if check at line 426 does not protect against a race condition where another session changes the session_id immediately after the check. Synchronization logic should be handled here in the routing layer.
References
- Avoid moving routing-layer logic (e.g., lock acquisition decisions) into the state management layer (ServerState) to maintain clear architectural boundaries.
| # Fallback for test environments where no config is available. | ||
| # Bind the shared agent and return it. | ||
| return self.bind_agent_to_session(session_id) |
There was a problem hiding this comment.
Falling back to the shared self.agent when _agent_config is missing breaks per-session isolation. In this scenario, get_or_create_agent will return the same self.agent instance for every session, leading to state clobbering. To maintain architectural boundaries, do not implement lock acquisition logic within ServerState. Instead, ensure the routing layer handles synchronization when shared instances are returned, or ensure the state layer provides isolated instances.
References
- Avoid moving routing-layer logic (e.g., lock acquisition decisions) into the state management layer (ServerState) to maintain clear architectural boundaries.
…pencode_to_chat_message When a question (elicitation) is cancelled by user (ESC) and abort_session cancels both pending questions and the active message task, the abort handler adds the partial assistant message to agent.conversation via opencode_to_chat_message(). This conversion was only adding ToolReturnPart for ToolStateCompleted and ToolStateError, leaving ToolStatePending and ToolStateRunning tool calls without corresponding results. Pydantic-AI validates that every ModelResponse.tool_calls has a matching ToolReturnPart in the next ModelRequest. Without this, the next user message triggers: 'Cannot provide a new user prompt when the message history contains unprocessed tool calls.' Fix: Add a synthetic ToolReturnPart with 'Tool call was aborted before completion' for Pending/Running tool states.
* docs(rfc): add RFC-0034 ACP Session Config Options 统一化 新增 RFC-0034，提案升级 AgentPool ACP Server 的 Session Config Options 透出逻辑，使 Zed 等 ACP 兼容 IDE 能够选择模型和切换 Agent Role。 主要内容： - 识别 4 个 GAP：Agent Role 未透出（P0）、ACP/OpenCode model list 数据来源不一致（P1）、/mode 路由硬编码（P1）、get_session_mode_state 过滤过严（P2） - 分析 3 个方案，推荐选项 2（三阶段统一化） - 技术设计：build_model_state_for_acp()、get_agent_role_config_option()、 _swap_session_agent() 及 OpenCode /mode 路由动态修复 🤖 Generated with [Qoder][https://qoder.com] * docs(rfc): 根据 review 反馈修正 RFC-0034 - 修正 model fallback 逻辑：strict fallback（configured 存在时只用 configured） - 修正 agent_role current_value：使用 agent.name 而非 pool.main_agent.name - 重写 _swap_session_agent()：委托 session.switch_active_agent() + _session_agent_locks 保护 - 增加 session._task_lock 协调：拒绝 active prompt 期间的 swap - 增加 pool.manifest null check 保护 - 修正 list_modes() null safety：state.agent 为 None 时返回默认值 - 明确开放问题 Q2/Q3 的决策：对话历史不继承、current_value 已修复 - 更新决策记录：增加 session mutation 复用、锁保护、task_lock 协调、对话历史决策 - 更新 Phase 2 实施计划：增加 Zed 预验证、并发测试、current_value 测试 - 调整工作量估算：~260 行 → ~240 行 * docs(rfc): RFC-0034 新增 Phase 0 — ACP Configurable LLM Providers 适配 ACP PR #648 (Configurable LLM Providers) 已 MERGED，引入 providers/list、providers/set、providers/disable 三个方法族， 允许客户端发现和覆盖 agent 的 LLM 请求路由。 主要更新： - 新增 GAP 5 (P0): providers/* 完全未实现 - 新增目标 G7: 实现 ACP providers/* 协议方法 - 新增 Phase 0: ProviderRouter 实现 + schema 类型定义 + ACP 请求处理器 + AgentCapabilities.providers 声明 - 修订 Phase 1: build_model_state_for_acp() 接受 provider_router 参数，过滤被禁用 provider 下的模型 - 更新架构概览图: 传输层(providers)与应用层(session config)分层 - 更新里程碑: 四阶段实施，Phase 0 优先于 Phase 1 - 新增开放问题 6/7/8: providers 对已运行 session 的影响、 provider 路由覆盖与 agent 初始化兼容、SessionModelState 中是否携带 provider 关联信息 - 新增决策记录: providers/set 保守策略、从 model_variants 派生 ProviderInfo、provider_router 参数解耦 🤖 Generated with [Qoder][https://qoder.com] * docs(rfc): 优化 RFC-0034 — 补充 Zed 源码级兼容性分析 基于 Zed 源码调研（crates/agent_ui/src/config_options.rs、profile_selector.rs、 agent_servers/src/acp.rs）的关键发现： 1. Zed 渲染所有 config_options 为独立 UI 按钮，agent_role 可正确显示和点击 2. first_config_option_id() 仅返回同 category 的第一个 option，键盘快捷键 可能冲突 — 标记为已知限制（NG7） 3. Zed ProfileSelector 完全独立于 ACP，使用本地 AgentSettings.profiles 4. Zed 当前完全不支持 providers/* 协议（Phase 0 暂无 Zed UI 入口） RFC 更新内容： - 新增 Zed IDE 渲染行为小节（源码级证据） - 新增 Zed 兼容性分析总结表 - 更新非目标 NG7：键盘快捷键冲突为已知限制 - 更新开放问题 5/6/7/8/9，标记 Zed 调研结论 - 更新 Phase 2 预验证：明确键盘限制和排序建议 - 更新向后兼容保证表：添加 category 冲突行 - 更新决策记录：补充 Zed 调研证据 🤖 Generated with [Qoder][https://qoder.com] * feat(acp): implement RFC-0034 ACP Session Config Options unification Phase 0: ACP Configurable LLM Providers - Add providers/* protocol methods (providers/list, providers/set, providers/disable) - Add ProviderRouter with override/disable/capability tracking - Add providers field to AgentCapabilities and InitializeResponse Phase 1: Shared Model List Logic - Add build_model_state_for_acp() with configured-first, tokonomics-fallback - Invert get_session_model_state() to use configured variants first Phase 2: Agent Role Config Option - Add get_agent_role_config_option() exposing pool.all_agents - Add _swap_session_agent() with lock protection - Extend set_session_config_option() with agent_role handling Phase 3: OpenCode /mode Route Fix - Dynamic /mode route using agent.get_modes() Also includes RFC-0033 MCP over ACP support: - Add AcpMcpServer type and acp field to McpCapabilities - Add acp_mcp_servers parameter to AgentCapabilities.create() Tests: - 35 new tests across provider_router, model_state, agent_role, config_routes, and cross-protocol integration - Snapshot tests re-baselined * chore: remove RFC-0033 code from RFC-0034 branch Remove accidentally included RFC-0033 MCP-over-ACP implementation: - Delete acp_mcp_manager.py, acp_mcp_transport.py - Delete RFC-0033 tests (test_mcp.py, test_acp_mcp_*, test_mcp_integration) - Remove AcpMcpServer from mcp.py - Remove acp field from McpCapabilities - Remove acp_mcp_servers parameter from AgentCapabilities.create() - Remove acp_mcp_servers parameter from InitializeResponse.create() - Remove RFC-0033 handler code from acp_agent.py Keep RFC-0034 changes intact: - providers/* protocol methods - ProviderRouter with override/disable - build_model_state_for_acp() configured-first logic - agent_role config option and swap - Dynamic /mode route * fix: address PR review comments for RFC-0034 - _swap_session_agent: Update session agent registry after swap (review #5) - get_agent_role_config_option: Use display_name with type-safe fallback, add description (review #7) - list_modes: Use mode.id for programmatic identifiers, add explicit None guard (review #4, #8) - Update tests to match new behavior * fix(agent): use model_variants in get_modes() instead of tokonomics Agent.get_modes() was calling get_available_models() which returns all tokonomics-discovered models (2000+). Now it checks configured model_variants first and only falls back to tokonomics when no variants are configured. Fixes the issue where config_options model selector showed thousands of models instead of the configured variants. * fix(agent): track model variant name to fix Zed Unknown display When using model_variants, get_modes() returned variant names as option ids but current_mode_id was the raw model identifier (e.g. openai:svc/glm-4.7). This caused Zed to display 'Unknown' because current_mode_id didn't match any available mode id. Fix: Add _current_model_variant field to Agent. When _set_mode() is called with a variant name, store it. get_modes() now uses _current_model_variant as current_mode_id so it matches the option ids. * fix(agent): set _current_model_variant on init when model is variant name Agent.__init__ resolves model string via _resolve_model_string(), but was not setting _current_model_variant. This caused get_modes() to fall back to self.model_name (the raw model identifier) on initial load, showing 'Unknown' in Zed until _set_mode() was called. Fix: Also track variant name in __init__ when model string matches a model_variants key. * fix(agent): use actual model identifier as mode id, variant name as display name Redesign model config option to use actual model identifiers: - id/value: actual model identifier (e.g. openai:svc/glm-4.7) - name: variant name (e.g. glm47) for display - current_mode_id: actual model identifier This ensures currentValue matches option values in Zed's config option selector, fixing the 'Unknown' display issue. _set_mode() now supports both actual model identifiers and variant names by reverse-lookup from manifest model_variants. * fix(agent): align get_modes() id format with model_name Use config.get_model().system:model_name for option ids instead of config.identifier, ensuring currentValue matches option values. Root cause: model_name returns pydantic-ai system:model_name format (e.g., 'openai:svc/glm-4.7') while config.identifier returns full provider format (e.g., 'openai-chat:svc/glm-4.7'), causing mismatch in Zed's model selector dropdown. * fix: address PR #37 review comments (round 2) - providers/set & providers/disable: 兼容 id 字段（Comment #12, #13） - provider_router: 防御性初始化 + 未知 provider 静默禁用（Comment #14, #15） - model_utils: 先过滤 raw toko_models（更准确），current_model 不在列表时插入（Comment #16） - .gitignore: 添加 .omo/（Comment #18）
1 participant
Summary
Replace the OpenCode server's single shared agent usage with per-session agent instances so concurrent clients stop clobbering each other's mutable state (RFC-0026).
Key Changes
Core Architecture
_session_agentsdict) with double-check locking via_session_agent_locksget_or_create_agent(session_id): race-free creation usingNativeAgentConfig.get_agent()(creates NEW instances, not cached ones fromAgentPool.get_agent())_create_session_agent(): wraps agent creation inConfigContextManagerto resolve relative tool schema paths at runtimecleanup_all_session_agents()/remove_session_agent(): graceful shutdown with partial-init safetyRoute Migration
get_or_create_agent()agent_lock/bind_agent_to_session()usageoriginal_modelsave/restore ininit_session()(model changes are session-local now)abort_session()targets correct session agent via_session_agents.get()fork_session()creates independent agent instance_process_message_locked()uses per-session agent directly, noagent_lock, no model save/restore_session_agentsonset_model()Bug Fix
QuestionProvider:ConfigContextManagerexits before request handlers run, leaving_config_dir_global = None. Fixed by re-entering the context during_create_session_agent().Test Coverage
8 new test files with ~55 tests:
test_session_agent_registry.py(14): isolation, race-free creation, cleanuptest_server_lifecycle.py(3): startup/shutdown cleanuptest_session_isolation.py(8): cross-talk, fork divergence, abort targetingtest_message_isolation.py(3): concurrent processing, model scopingtest_route_discovery.py(14): route discovery, module loading, model propagationtest_session_cleanup.py(5): delete-session, partial-init safetytest_concurrency_isolation.py(4): independent sessions, interrupt isolation, FIFOtest_isolation_regression.py(7): model/fork/input-provider isolationAll 565 opencode_server tests pass.
Guardrails Preserved