Feature/kaifeng.yan/fix opencode version - #16
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 implements the GlobalEvent protocol for OpenCode v1.4.4+, introducing a GlobalEvent envelope for SSE routing and a GlobalEventFactory for event wrapping. It also enhances the ServerState broadcast mechanism to be more robust against full or broken subscriber queues. Feedback indicates that _extract_session_id is missing several event types that include a session_id, potentially breaking subagent tracking, and generates excessive log noise for known no-session events. Furthermore, a performance improvement was suggested to avoid redundant JSON processing during event wrapping.
465789c to
9099065
Compare
|
/gimini review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant changes to support the OpenCode v1.4.4+ GlobalEvent protocol, including new models for workspace and diagnostic information, updated event serialization, and a new diagnostic script. Several critical issues were identified: UserMessage and MessageRequest models are missing the model field declaration, which will cause runtime errors; the ServerState implementation introduces a race condition by modifying shared agent state for session-specific data; and a variable diff is used in converters.py without being defined. Additionally, there are performance concerns regarding redundant JSON serialization in the event stream and encapsulation violations in question_routes.py.
cc83aa4 to
9abbe6a
Compare
…ration for v1.4.4+ SSE protocol
…add protocol tests
…exhaustiveness warning - Add global_router to conftest.py app fixture for SSE integration testing - Remove unused MemoryStorageProvider import from conftest.py - Add logger.warning for unhandled event types in _extract_session_id wildcard case
…ption isolation - Add SSEStream class and global_event_stream fixture to conftest.py - Add parse_sse_event helper for SSE data line parsing - Add exception isolation to broadcast_event: iterate copy, put_nowait, catch QueueFull - Safe queue removal in _event_generator finally block with contextlib.suppress - Add race condition analysis comment for on_first_subscriber callback
…iff.patch, workspace routing, session_id extraction - Migrate top-level variant into model.variant for UserMessage/MessageRequest (backward-compat) - Replace FileDiff.before/after with FileDiff.patch matching OpenCode v1.4.0+ schema - Add WorkspaceInfo/WorkspaceConnectionStatus models and /experimental/workspace endpoints - Add workspace field to GlobalEvent envelope for TUI routing - Fix base_path to anchor on server working_dir, not agent env.cwd - Extend _extract_session_id for 6 additional event types (22 total) - Add /global/diagnostic and /global/routing-check endpoints - Add DiagnosticResponse model - Update tests for new models and routing behavior
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Keep OpenCode question prompts on the que_* namespace while preserving the legacy permission bridge through /question. The regression tests cover both the new question prefix and the fallback permission reply path. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Keep /global/event fully envelope-shaped so 1.4.4+ clients keep consuming after idle timeouts. Normalizing the routing directory in ServerState keeps the envelope aligned with the TUI filter. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Refresh the compliance suite for wrapped heartbeat events and normalized routing metadata. This keeps the regression coverage aligned with the updated /global/event contract. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
67bbb1a to
798cf79
Compare
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces compatibility for OpenCode 1.4.4+ by adding OTLP and global routes, implementing per-turn completion signaling for queued prompts, and establishing a snapshot-based mechanism for concurrent session isolation. It also removes restrictive route-level timeouts on synchronous message processing. Feedback identifies critical TypeError risks in run_stream calls due to signature mismatches, potential concurrency issues when mutating shared agent state, and opportunities to improve resource management and path normalization consistency.
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request enhances OpenCode 1.4.4+ compatibility by adding OTLP telemetry sinks, global configuration routes, and per-turn completion signaling. It also introduces a RunSnapshot mechanism for concurrent session isolation and removes restrictive route-level timeouts. Feedback highlights several critical runtime issues, including missing parameters in _process_message_locked, undefined logger and message_history variables, and invalid keyword arguments passed to agent methods. A suggestion was also provided to improve encapsulation by using the new has_pending_permission method.
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive updates for OpenCode 1.4.4+ compatibility, including OTLP telemetry sinks, global compatibility routes, and a new async prompt queuing system. A key feature is the implementation of session isolation through a snapshot mechanism. Feedback identifies critical improvements for the snapshot logic to correctly handle multi-agent configurations and state capture timing. Additionally, suggestions were provided to prevent session status flickering during async transitions and to improve the reliability of the TUI routing filter using path resolution.
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request implements OpenCode 1.4.4+ compatibility by addressing protocol gaps, improving queued prompt handling, and introducing a RunSnapshot mechanism for concurrent session isolation. It adds OTLP telemetry sinks, global compatibility routes, and per-session async prompt workers. Feedback identifies critical bugs including signature mismatches in _process_message_locked and agent.run_stream calls, and potential stale data in snapshots. Further improvements are needed regarding the logic for marking sessions as idle, exception handling in the async queue worker, and optimizing project lookup performance in storage.
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request implements OpenCode 1.4.4+ compatibility and improves session isolation using a new RunSnapshot mechanism. Key changes include adding OTLP telemetry sinks, global compatibility routes, and a per-session async prompt worker to handle queued prompts. Feedback points out several critical TypeError bugs caused by calling run_stream and _stream_events with unsupported arguments. The reviewer also recommends restoring agent modes after turns and prioritizing snapshot data in AssistantMessage to ensure robust concurrency.
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request implements OpenCode 1.4.4+ compatibility and refactors async prompt handling to use a per-session FIFO queue and a RunSnapshot mechanism for better concurrency isolation. Key additions include OTLP telemetry sinks, global compatibility routes, and enhanced SSE event metadata for TUI routing. Feedback identifies several critical issues, including a TypeError in the run_stream call due to an unhandled parameter and multiple race conditions where shared agent state is mutated or restored outside of appropriate locks. Furthermore, some agent implementations still incorrectly rely on shared instance attributes rather than the provided snapshot, which risks state contamination between concurrent sessions.
| iterator = agent.run_stream( | ||
| *user_prompt, | ||
| session_id=snapshot.session_id, | ||
| message_history=snapshot.conversation, | ||
| input_provider=snapshot.input_provider, | ||
| snapshot=snapshot, | ||
| ) |
There was a problem hiding this comment.
The call to agent.run_stream includes a message_history keyword argument, but the BaseAgent.run_stream method (and its implementations in subclasses) has not been updated to accept this parameter. This will result in a TypeError: run_stream() got an unexpected keyword argument 'message_history' at runtime.
Since the RunSnapshot already contains the conversation history, the agent implementations should be updated to extract the history from the snapshot object instead of passing it as a separate argument, or the run_stream signature must be updated to include message_history.
| resolved._input_provider = self.input_providers[session_id] | ||
| resolved.session_id = session_id |
There was a problem hiding this comment.
Mutating the shared agent instance's _input_provider and session_id attributes outside of a persistent lock (or during a long-running async operation) introduces a race condition. While snapshot_for_session is called under agent_lock, the lock is released before the agent's run_stream completes. If another session starts a turn, it will overwrite these attributes on the shared agent, causing the first session's ongoing run to see incorrect state if it accesses self.session_id or self._input_provider.
To achieve true concurrent session isolation, the agent implementations must be refactored to avoid relying on self for per-run state, instead using the values provided in the snapshot or run_ctx.
References
- Avoid moving routing-layer logic (e.g., lock acquisition decisions) into the state management layer (ServerState) to maintain clear architectural boundaries.
| # Restore original model if we changed it | ||
| if original_model is not None: | ||
| with contextlib.suppress(Exception): | ||
| await agent.set_model(original_model) | ||
| logger.info("Restored original model", model=original_model) | ||
|
|
||
| # Restore original variant/mode if we changed it | ||
| if original_variant is not None: | ||
| with contextlib.suppress(Exception): | ||
| await agent.set_mode(original_variant, category_id="thought_level") | ||
| logger.info("Restored original variant mode", variant=original_variant) |
There was a problem hiding this comment.
The restoration of original_model and original_variant in the finally block happens after the agent_lock has been released. This creates a race condition where one session might be restoring the agent's state while another session is simultaneously mutating it under the lock. This can lead to the shared agent being left in an inconsistent or incorrect state. Restoration of shared state should ideally happen under the same lock that protected the mutation, or the mutations should be avoided entirely in favor of passing parameters to the run call.
References
- Avoid moving routing-layer logic (e.g., lock acquisition decisions) into the state management layer (ServerState) to maintain clear architectural boundaries.
| session_id=effective_session_id, | ||
| parent_id=user_msg.message_id, | ||
| model_name=self.model_name, |
There was a problem hiding this comment.
The model_name is being read from self.model_name during event generation. In a concurrent environment where the shared agent's model can be switched by another session (as seen in message_routes.py), this will lead to incorrect model metadata being reported in stream events for overlapping runs. This should be updated to use snapshot.model_name if a snapshot is available.
| # Set thread_id from session_id (needed for AG-UI protocol) | ||
| if self._sdk_session_id is None: | ||
| self._sdk_session_id = self.session_id | ||
| self._sdk_session_id = effective_session_id |
There was a problem hiding this comment.
Mutating self._sdk_session_id on a shared agent instance is not concurrency-safe. If multiple sessions run concurrently, they will overwrite this shared attribute. This state should be managed within the RunSnapshot or AgentRunContext to ensure isolation between sessions.
* 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）
2 participants
修复opencode更新导致的通信问题