Repository navigation
feat: support CodeX agent - #1226
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CodeX execution (agent, config builder, event mapper, session store), extends model compatibility to OpenAI "responses" formats, merges UI reasoning into execution requests, adds WeWork default-team config and wiring, and updates executor routing, packaging, env helpers, and tests. ChangesCodeX and WeWork Integration
Sequence Diagram — high-level CodeX flow: sequenceDiagram
participant API as Backend API
participant Factory as AgentFactory
participant CodeX as CodeXAgent
participant SDK as openai_codex SDK
participant Store as CodeXSessionStore
API->>Factory: request code agent (model_config)
Factory->>CodeX: instantiate CodeXAgent
CodeX->>Store: load(session for task/bot)
CodeX->>SDK: start client / open thread (reasoning, thread_config)
SDK->>CodeX: stream events
CodeX->>API: emit text_delta / done / error via ResponsesAPIEmitter
CodeX->>Store: save thread_id
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
executor/agents/codex/codex_agent.py (1)
32-33: 💤 Low valueAnnotate shared class-level registries with
ClassVar.These mutable class attributes are intentional shared registries, but the bare annotations trip Ruff RUF012 and read as per-instance defaults. Mark them
ClassVarto document intent and silence the warning.♻️ Suggested annotation
- _active_task_ids: set[int] = set() - _active_agents: dict[int, "CodeXAgent"] = {} + _active_task_ids: ClassVar[set[int]] = set() + _active_agents: ClassVar[dict[int, "CodeXAgent"]] = {}(add
ClassVarto thetypingimport)🤖 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 `@executor/agents/codex/codex_agent.py` around lines 32 - 33, The class-level mutable registries _active_task_ids and _active_agents on CodeXAgent are currently annotated as per-instance defaults and should be explicit shared class vars; update their annotations to use typing.ClassVar (e.g., ClassVar[set[int]] and ClassVar[dict[int, "CodeXAgent"]]), and add ClassVar to the imports from typing so Ruff's RUF012 warning is resolved and intent is documented.executor/agents/codex/session_store.py (1)
44-50: 💤 Low valueConsider an atomic write for session persistence.
write_textis non-atomic; an interrupted write can leave a truncated/corrupt JSON file.load()already tolerates parse failures (returnsNone), so impact is limited to losing a resumable thread, but a write-to-temp-then-os.replaceis a cheap hardening.♻️ Atomic write
- try: - path.write_text(json.dumps(payload), encoding="utf-8") - except OSError as exc: + try: + tmp = path.with_suffix(".json.tmp") + tmp.write_text(json.dumps(payload), encoding="utf-8") + tmp.replace(path) + except OSError as exc: logger.warning("Failed to save Codex session file %s: %s", path, exc)🤖 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 `@executor/agents/codex/session_store.py` around lines 44 - 50, The save() method currently uses Path.write_text which is non-atomic; change save(self, task_id: int, bot_id: Optional[int], thread_id: str) to write payload JSON to a temporary file alongside the target (e.g., use the same directory and a suffix like ".tmp" or a UUID), flush and fsync the temp file, then atomically replace the target with os.replace(temp_path, path) so partial writes cannot corrupt the session file; reference the save() method and the _path() helper and ensure exceptions from the temp write are caught/logged similarly to the existing OSError handling.executor/agents/codex/config_builder.py (1)
68-76: ⚡ Quick winAvoid logging Codex bearer token embedded in
config_overrides
experimental_bearer_token={api_key}is included inCodeXConfig.config_overrides(soCodeXConfigrepr would contain the secret).- No Codex agent/runner logging/printing of
CodeXConfig,config_overrides, orexperimental_bearer_tokenwas found; the warning atexecutor/agents/codex/codex_agent.py:299only reports unsupportedcodex_config.effort.- Keep it that way: if adding debug/telemetry later, redact or omit
config_overrides/CodeXConfigfrom log formatting.🤖 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 `@executor/agents/codex/config_builder.py` around lines 68 - 76, The current overrides list in config_builder.py adds the raw API key into CodeXConfig.config_overrides via the entry "model_providers.{model_provider}.experimental_bearer_token={api_key}", which can expose secrets in repr/logs; remove that entry or replace the value with a redacted placeholder (e.g. "<REDACTED>") and instead store the actual api_key in a separate secure field accessible to the runner/agent at runtime (do not include it in config_overrides), updating any code that reads config_overrides accordingly (refer to the overrides list, CodeXConfig.config_overrides, and the usage sites that resolve model_provider/_resolve_provider_name) so logs and reprs no longer contain the bearer token.backend/tests/services/chat/test_reasoning_config.py (1)
104-128: ⚡ Quick winConsider adding edge case tests for _reasoning_from_model_options.
The current tests cover happy paths for object-shaped reasoning. Consider adding tests for edge cases handled by the implementation:
payload=None→ should returnNonepayload.model_options=None→ should returnNonemodel_options={}(empty dict) → should returnNonemodel_options={"reasoning": "high"}(string, not dict) → should return{"effort": "high"}model_options={"reasoning": {"effort": "high"}}(no summary) → should return{"effort": "high"}model_options={"summary": "detailed"}(no reasoning) → should return{"summary": "detailed"}📋 Example edge case tests
def test_model_options_none_returns_none(self): from app.services.chat.trigger.unified import _reasoning_from_model_options assert _reasoning_from_model_options(None) is None def test_model_options_missing_returns_none(self): from app.services.chat.trigger.unified import _reasoning_from_model_options payload = MagicMock() payload.model_options = None assert _reasoning_from_model_options(payload) is None def test_model_options_reasoning_string(self): from app.services.chat.trigger.unified import _reasoning_from_model_options payload = MagicMock() payload.model_options = {"reasoning": "high"} assert _reasoning_from_model_options(payload) == {"effort": "high"}🤖 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 `@backend/tests/services/chat/test_reasoning_config.py` around lines 104 - 128, Add edge-case unit tests for _reasoning_from_model_options: cover payload=None (expect None), payload.model_options=None (expect None), model_options={} (expect None), model_options={"reasoning": "high"} (expect {"effort":"high"}), model_options={"reasoning": {"effort":"high"}} (expect {"effort":"high"}), and model_options={"summary":"detailed"} (expect {"summary":"detailed"}). Place these assertions alongside the existing tests in backend/tests/services/chat/test_reasoning_config.py and import the function from app.services.chat.trigger.unified to validate behavior of _reasoning_from_model_options.backend/app/services/chat/trigger/unified.py (1)
56-56: 💤 Low valueClarify the fallback intent for effort extraction.
The expression
reasoning.get("effort") or reasoning.get("reasoning")usesor, which will evaluate the secondget()if the first returns a falsy value (including0,"",False, orNone). Ifreasoning["effort"]is explicitly set to a falsy value that's semantically valid, this would incorrectly fall back toreasoning["reasoning"].Consider being explicit about the fallback condition if this is intentional:
🔍 Proposed refactor for clarity
- if isinstance(reasoning, dict): - effort = reasoning.get("effort") or reasoning.get("reasoning") - summary = summary or reasoning.get("summary") - else: - effort = reasoning + if isinstance(reasoning, dict): + # Prefer "effort" key, fall back to "reasoning" key if "effort" is absent + effort = reasoning.get("effort") if "effort" in reasoning else reasoning.get("reasoning") + summary = summary or reasoning.get("summary") + else: + effort = reasoning🤖 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 `@backend/app/services/chat/trigger/unified.py` at line 56, The current fallback uses `effort = reasoning.get("effort") or reasoning.get("reasoning")`, which treats any falsy value (0, "", False) as missing; change the logic to only fall back when the "effort" key is truly absent or None. For example, fetch a temp := reasoning.get("effort") and set effort = temp if temp is not None else reasoning.get("reasoning"), or check `"effort" in reasoning` before falling back; update the assignment of effort in unified.py to use this explicit None/key check so valid falsy effort values are preserved.backend/app/api/endpoints/adapter/models.py (1)
314-320: ⚡ Quick winClarify CodeX compatibility detection sources (and avoid logic drift)
ModelSpecdefinesapiFormatandprotocolas CRD spec fields, whilewire_apiis only read frommodel_crd.spec.modelConfig(nowire_apifield in theModelSpecschema).- Since
backend/app/services/model_aggregation_service.pyalready has_is_codex_compatible_model_config()that evaluatesapiFormat/protocol/wire_apifrom a single config dict,backend/app/api/endpoints/adapter/models.py’s parallel logic risks divergence—reuse the helper (or factor into one shared function) instead of mixing spec fields + config keys.🤖 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 `@backend/app/api/endpoints/adapter/models.py` around lines 314 - 320, The current Codex compatibility check in models.py mixes CRD spec fields and model_config keys (using model_crd.spec.apiFormat/protocol and model_config.get("wire_api")), which risks logic drift; instead reuse the existing helper _is_codex_compatible_model_config from model_aggregation_service.py (or extract that logic into a shared util) and call it with a single config dict assembled from model_crd.spec and model_config (e.g., combine apiFormat/protocol into the config) so compatibility is determined by the single canonical function rather than ad-hoc checks in models.py (update imports and replace the api_format/protocol/is_codex_compatible block).executor/agents/factory.py (1)
121-128: ⚡ Quick win
close_all_clientslacks per-agent error isolation.Unlike
cleanup_task_clients, this loop has no try/except, so an exception from one agent'sclose_all_clients()prevents the remaining agents from being closed, leaving clients leaked.♻️ Isolate failures per agent
`@classmethod` async def close_all_clients(cls) -> None: """Close active clients across local code agents.""" for agent_class in (ClaudeCodeAgent, CodeXAgent): closer = getattr(agent_class, "close_all_clients", None) if not closer: continue - await closer() + try: + await closer() + except Exception as exc: + logger.warning( + "Failed to close %s clients: %s", + agent_class.__name__, + exc, + )🤖 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 `@executor/agents/factory.py` around lines 121 - 128, The loop in classmethod close_all_clients currently awaits each agent's close_all_clients (for ClaudeCodeAgent and CodeXAgent) without isolating failures, so an exception in one prevents others from running; update close_all_clients to wrap the await closer() call for each agent_class in a try/except that catches exceptions, logs or records the error including agent_class.__name__ and the exception, and then continues to the next agent—follow the same per-agent error-isolation pattern used in cleanup_task_clients to ensure no client leaks.executor/services/agent_service.py (1)
287-327: 🏗️ Heavy liftFix CodeX teardown concern: CodeX clients are already cleaned up per-task
executor/services/agent_service.py::_close_agent_sessiononly closes"ClaudeCode"and"Agno", so"CodeX"intentionally falls through. However,executor/agents/codex/codex_agent.py::CodeXAgent.execute_async()always callsawait self.cleanup_async()in afinallyblock, andcleanup_async()closes theAsyncCodexclient and removes the agent fromCodeXAgent._active_agents.CodeXAgentalso definesget_name() -> "CodeX"and acleanup_task_clients()hook, so there’s no per-task CodeX client/process leak in the normal path.
Optional: if you want extra defensiveness for edge cases, route"CodeX"teardown throughAgentFactory.cleanup_task_clients(task_id)in_close_agent_session.🤖 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 `@executor/services/agent_service.py` around lines 287 - 327, The comment notes CodeX clients are already cleaned up by CodeXAgent.execute_async -> cleanup_async (which closes AsyncCodex and removes the agent from CodeXAgent._active_agents), so _close_agent_session need not duplicate teardown; if you want extra defensiveness, add an explicit branch for agent_name == "CodeX" that calls AgentFactory.cleanup_task_clients(str(task_id)) (or the existing cleanup hook) instead of falling through—update executor/services/agent_service.py::_close_agent_session to detect "CodeX" and invoke AgentFactory.cleanup_task_clients(task_id) (referencing CodeXAgent.execute_async, CodeXAgent.cleanup_async, and AgentFactory.cleanup_task_clients) so edge-case leftover clients are cleaned without changing normal CodeXAgent behavior.
🤖 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 `@backend/app/services/chat/trigger/unified.py`:
- Around line 303-307: The current assignment uses truthiness and treats an
explicit empty dict as falsy; change it to explicitly prefer any provided
reasoning_config (including {}) over falling back to
_reasoning_from_model_options(payload). Replace the `selected_reasoning_config =
reasoning_config or _reasoning_from_model_options(payload)` logic with an
explicit None check (e.g., if reasoning_config is not None then use
reasoning_config else call _reasoning_from_model_options(payload)) so that
explicit empty dicts are honored.
In `@executor/agents/codex/codex_agent.py`:
- Around line 314-319: The _resolve_bot_id helper in codex_agent.py currently
does int(bot_id) unguarded and can raise ValueError for non-numeric ids; update
the _resolve_bot_id(ExecutionRequest) implementation to attempt a safe
conversion and return None on failure: check if task_data.bot exists, extract
bot_id from task_data.bot[0].get("id"), then wrap the int conversion in a
try/except catching ValueError and TypeError and return None if conversion fails
(otherwise return the int). Keep the function signature and behavior for None
unchanged so agent __init__ doesn't abort on non-numeric ids.
- Around line 113-174: The cancel logic currently schedules interrupt on the
wrong event loop; capture the worker thread's event loop that owns self._turn
(e.g., set self._turn_loop = asyncio.get_running_loop() inside execute_async()
before creating/assigning self._turn) and on cancel_run() use
asyncio.run_coroutine_threadsafe(self._turn.interrupt(), self._turn_loop) (store
the returned Future on the agent, e.g., self._interrupt_future) instead of
loop.create_task or asyncio.run; also guard for missing
self._turn_loop/self._turn and return False if not present.
In `@executor/agents/codex/event_mapper.py`:
- Around line 27-50: Add OpenTelemetry tracing to the public async dispatch
entrypoint by importing trace_async from shared.telemetry.decorators and
annotating CodeXEventMapper.handle with
`@trace_async`(span_name="codex.event_mapper.handle", tracer_name="executor");
inside handle (after reading method/payload) set span attributes for the
incoming event method (e.g.
opentelemetry.trace.get_current_span().set_attribute("codex.event.method",
method)) and update the span with outcome attributes when branching to
_handle_usage (set "codex.event.usage" or usage details), _handle_completed_item
(set "codex.event.completed" and any result status), and when awaiting
_handle_turn_completed (set "codex.event.turn_completed" and the returned
outcome); ensure emitter.text_delta handling still records deltas but also
annotates the span (e.g. "codex.event.delta_seen" or delta length) so tracing
captures method and usage/done outcomes for CodeXEventMapper.handle.
In `@executor/agents/factory.py`:
- Around line 75-82: The routing code is incorrectly guarded by a Pydantic
reserved-name concern even though ExecutionRequest.model_config is a real dict
field; remove any special-case/workaround and treat model_config as a normal
dict. In executor/agents/factory.py keep the routing that calls
is_codex_compatible_model(task_data.model_config) and log the model id via
task_data.model_config.get("model_id"), then return CodeXAgent(task_data,
emitter=emitter) or ClaudeCodeAgent(task_data, emitter=emitter) as before;
ensure no extra runtime checks or renames for Pydantic ConfigDict are present
and rely on ExecutionRequest.model_config being a dict.
---
Nitpick comments:
In `@backend/app/api/endpoints/adapter/models.py`:
- Around line 314-320: The current Codex compatibility check in models.py mixes
CRD spec fields and model_config keys (using model_crd.spec.apiFormat/protocol
and model_config.get("wire_api")), which risks logic drift; instead reuse the
existing helper _is_codex_compatible_model_config from
model_aggregation_service.py (or extract that logic into a shared util) and call
it with a single config dict assembled from model_crd.spec and model_config
(e.g., combine apiFormat/protocol into the config) so compatibility is
determined by the single canonical function rather than ad-hoc checks in
models.py (update imports and replace the
api_format/protocol/is_codex_compatible block).
In `@backend/app/services/chat/trigger/unified.py`:
- Line 56: The current fallback uses `effort = reasoning.get("effort") or
reasoning.get("reasoning")`, which treats any falsy value (0, "", False) as
missing; change the logic to only fall back when the "effort" key is truly
absent or None. For example, fetch a temp := reasoning.get("effort") and set
effort = temp if temp is not None else reasoning.get("reasoning"), or check
`"effort" in reasoning` before falling back; update the assignment of effort in
unified.py to use this explicit None/key check so valid falsy effort values are
preserved.
In `@backend/tests/services/chat/test_reasoning_config.py`:
- Around line 104-128: Add edge-case unit tests for
_reasoning_from_model_options: cover payload=None (expect None),
payload.model_options=None (expect None), model_options={} (expect None),
model_options={"reasoning": "high"} (expect {"effort":"high"}),
model_options={"reasoning": {"effort":"high"}} (expect {"effort":"high"}), and
model_options={"summary":"detailed"} (expect {"summary":"detailed"}). Place
these assertions alongside the existing tests in
backend/tests/services/chat/test_reasoning_config.py and import the function
from app.services.chat.trigger.unified to validate behavior of
_reasoning_from_model_options.
In `@executor/agents/codex/codex_agent.py`:
- Around line 32-33: The class-level mutable registries _active_task_ids and
_active_agents on CodeXAgent are currently annotated as per-instance defaults
and should be explicit shared class vars; update their annotations to use
typing.ClassVar (e.g., ClassVar[set[int]] and ClassVar[dict[int,
"CodeXAgent"]]), and add ClassVar to the imports from typing so Ruff's RUF012
warning is resolved and intent is documented.
In `@executor/agents/codex/config_builder.py`:
- Around line 68-76: The current overrides list in config_builder.py adds the
raw API key into CodeXConfig.config_overrides via the entry
"model_providers.{model_provider}.experimental_bearer_token={api_key}", which
can expose secrets in repr/logs; remove that entry or replace the value with a
redacted placeholder (e.g. "<REDACTED>") and instead store the actual api_key in
a separate secure field accessible to the runner/agent at runtime (do not
include it in config_overrides), updating any code that reads config_overrides
accordingly (refer to the overrides list, CodeXConfig.config_overrides, and the
usage sites that resolve model_provider/_resolve_provider_name) so logs and
reprs no longer contain the bearer token.
In `@executor/agents/codex/session_store.py`:
- Around line 44-50: The save() method currently uses Path.write_text which is
non-atomic; change save(self, task_id: int, bot_id: Optional[int], thread_id:
str) to write payload JSON to a temporary file alongside the target (e.g., use
the same directory and a suffix like ".tmp" or a UUID), flush and fsync the temp
file, then atomically replace the target with os.replace(temp_path, path) so
partial writes cannot corrupt the session file; reference the save() method and
the _path() helper and ensure exceptions from the temp write are caught/logged
similarly to the existing OSError handling.
In `@executor/agents/factory.py`:
- Around line 121-128: The loop in classmethod close_all_clients currently
awaits each agent's close_all_clients (for ClaudeCodeAgent and CodeXAgent)
without isolating failures, so an exception in one prevents others from running;
update close_all_clients to wrap the await closer() call for each agent_class in
a try/except that catches exceptions, logs or records the error including
agent_class.__name__ and the exception, and then continues to the next
agent—follow the same per-agent error-isolation pattern used in
cleanup_task_clients to ensure no client leaks.
In `@executor/services/agent_service.py`:
- Around line 287-327: The comment notes CodeX clients are already cleaned up by
CodeXAgent.execute_async -> cleanup_async (which closes AsyncCodex and removes
the agent from CodeXAgent._active_agents), so _close_agent_session need not
duplicate teardown; if you want extra defensiveness, add an explicit branch for
agent_name == "CodeX" that calls AgentFactory.cleanup_task_clients(str(task_id))
(or the existing cleanup hook) instead of falling through—update
executor/services/agent_service.py::_close_agent_session to detect "CodeX" and
invoke AgentFactory.cleanup_task_clients(task_id) (referencing
CodeXAgent.execute_async, CodeXAgent.cleanup_async, and
AgentFactory.cleanup_task_clients) so edge-case leftover clients are cleaned
without changing normal CodeXAgent behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e46fafc-ed2f-4b7c-a1d3-737275b9f24b
⛔ Files ignored due to path filters (1)
executor/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
backend/.env.examplebackend/app/api/endpoints/adapter/models.pybackend/app/api/endpoints/adapter/teams.pybackend/app/api/endpoints/users.pybackend/app/core/config.pybackend/app/services/chat/trigger/unified.pybackend/app/services/model_aggregation_service.pybackend/tests/api/endpoints/test_adapter_teams_defaults.pybackend/tests/services/chat/test_reasoning_config.pybackend/tests/services/test_model_aggregation_service.pydocker/executor/Dockerfileexecutor/agents/__init__.pyexecutor/agents/codex/__init__.pyexecutor/agents/codex/codex_agent.pyexecutor/agents/codex/config_builder.pyexecutor/agents/codex/event_mapper.pyexecutor/agents/codex/session_store.pyexecutor/agents/factory.pyexecutor/config/config.pyexecutor/executor.specexecutor/modes/local/runner.pyexecutor/modes/local/websocket_client.pyexecutor/pyproject.tomlexecutor/services/agent_service.pyexecutor/tests/agents/test_codex_config_builder.pyexecutor/tests/agents/test_codex_event_mapper.pyexecutor/tests/agents/test_factory.pywework/src/api/adapters.test.tswework/src/api/teams.ts
| # Merge reasoning config from API/model selection into model_config. | ||
| # Priority: explicit API reasoning_config > UI model_options > model think_config. | ||
| selected_reasoning_config = reasoning_config or _reasoning_from_model_options( | ||
| payload | ||
| ) |
There was a problem hiding this comment.
Fix priority logic to respect explicit empty reasoning_config.
The current code uses or, which treats an empty dict {} as falsy:
selected_reasoning_config = reasoning_config or _reasoning_from_model_options(payload)If the API caller explicitly passes reasoning_config={} to indicate "no reasoning," the code incorrectly falls back to extracting reasoning from model_options, violating the documented priority ("explicit API reasoning_config > UI model_options").
🛠️ Proposed fix to honor explicit empty dict
- # Merge reasoning config from API/model selection into model_config.
- # Priority: explicit API reasoning_config > UI model_options > model think_config.
- selected_reasoning_config = reasoning_config or _reasoning_from_model_options(
- payload
- )
+ # Merge reasoning config from API/model selection into model_config.
+ # Priority: explicit API reasoning_config > UI model_options > model think_config.
+ selected_reasoning_config = (
+ reasoning_config
+ if reasoning_config is not None
+ else _reasoning_from_model_options(payload)
+ )🤖 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 `@backend/app/services/chat/trigger/unified.py` around lines 303 - 307, The
current assignment uses truthiness and treats an explicit empty dict as falsy;
change it to explicitly prefer any provided reasoning_config (including {}) over
falling back to _reasoning_from_model_options(payload). Replace the
`selected_reasoning_config = reasoning_config or
_reasoning_from_model_options(payload)` logic with an explicit None check (e.g.,
if reasoning_config is not None then use reasoning_config else call
_reasoning_from_model_options(payload)) so that explicit empty dicts are
honored.
| def execute(self) -> TaskStatus: | ||
| result: dict[str, Any] = {} | ||
|
|
||
| def run() -> None: | ||
| try: | ||
| result["status"] = asyncio.run(self.execute_async()) | ||
| except Exception as exc: | ||
| result["error"] = exc | ||
|
|
||
| thread = threading.Thread(target=run, daemon=True) | ||
| thread.start() | ||
| thread.join() | ||
| if "error" in result: | ||
| raise result["error"] | ||
| return result.get("status", TaskStatus.FAILED) | ||
|
|
||
| async def execute_async(self) -> TaskStatus: | ||
| if self.codex_config is None: | ||
| return TaskStatus.FAILED | ||
|
|
||
| self.__class__._active_task_ids.add(self.task_id) | ||
| self.__class__._active_agents[self.task_id] = self | ||
| mapper = CodeXEventMapper(self.emitter) | ||
|
|
||
| try: | ||
| await self._start_codex_client() | ||
| await self._open_thread() | ||
| await self._notify_client_created() | ||
|
|
||
| input_text = self._build_turn_input() | ||
| effort, summary = self._build_reasoning_params() | ||
| self._turn = await self._thread.turn( | ||
| input_text, | ||
| cwd=self.project_path, | ||
| effort=effort, | ||
| model=self.codex_config.model, | ||
| sandbox=self._sandbox_full_access(), | ||
| summary=summary, | ||
| ) | ||
|
|
||
| async for event in self._turn.stream(): | ||
| status = await mapper.handle(event) | ||
| if status is not None: | ||
| return status | ||
| return TaskStatus.FAILED | ||
| except Exception as exc: | ||
| logger.exception("CodeXAgent execution failed: %s", exc) | ||
| await self.emitter.error(str(exc), "execution_error") | ||
| return TaskStatus.FAILED | ||
| finally: | ||
| await self.cleanup_async() | ||
|
|
||
| def cancel_run(self) -> bool: | ||
| if self._turn is None: | ||
| logger.warning("CodeXAgent has no active turn to cancel: %s", self.task_id) | ||
| return False | ||
| try: | ||
| loop = asyncio.get_running_loop() | ||
| loop.create_task(self._turn.interrupt()) | ||
| except RuntimeError: | ||
| asyncio.run(self._turn.interrupt()) | ||
| return True |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C4 '\b(cancel_run|execute_async|\.execute\s*\()' --type=py -g '!**/tests/**'
rg -nP -C3 'run_coroutine_threadsafe|get_running_loop|asyncio\.run' --type=py -g 'executor/**'Repository: wecode-ai/Wegent
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect CodexAgent around execute/execute_async/cancel_run
sed -n '1,220p' executor/agents/codex/codex_agent.py
# Inspect Base agent execution path
sed -n '1,260p' executor/agents/base.py
# Inspect local runner cancel + execution call sites
sed -n '250,420p' executor/modes/local/runner.py
sed -n '520,640p' executor/modes/local/runner.py
# Inspect agent_service cancel call site
sed -n '330,450p' executor/services/agent_service.py
# Find other call sites of cancel_run/execute for CodexAgent
rg -n --type=py '\b(cancel_run\(\)|execute_async\(\)|\.execute\(\))' executor/ | rg -n 'codex|CodeXAgent|codex_agent' || true
rg -n --type=py '\bexecute\(\)|execute_async\(\)|cancel_run\(\)' executor/agents/codex/codex_agent.pyRepository: wecode-ai/Wegent
Length of output: 35179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where agent.handle() is used (which would call Agent.execute() -> CodeXAgent.execute()).
rg -n --type=py '\.handle\(' executor/ | head -n 50
# Find where execute()/execute_async() are invoked on agents in runner/task processor code.
rg -n --type=py '\bexecute_async\(\)' executor/ | head -n 80
rg -n --type=py '\bexecute\(\)' executor/ | head -n 80
# Focus on CodeXAgent references.
rg -n --type=py 'CodeXAgent|codex_agent' executor/ | head -n 80
# Check other runner modes for calling execute() vs execute_async()
rg -n --type=py 'execute_async\(\)|\.execute\(\)|agent\.execute\(\)|agent\.execute_async\(\)' executor/modes/ | head -n 120Repository: wecode-ai/Wegent
Length of output: 5490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '140,230p' executor/services/agent_service.py
sed -n '1,120p' executor/app.py
sed -n '600,720p' executor/app.py
# Also inspect task_processor + any other mode for calling agent.handle()
sed -n '240,360p' executor/tasks/task_processor.py
# Find non-local runner paths that call agent.handle()
rg -n --type=py 'await agent\.handle\(' executor/ | head -n 50
rg -n --type=py 'return await agent\.handle' executor/ | head -n 50
# Inspect cancel paths
rg -n --type=py '\bcancel_task\b' executor/ | head -n 80
sed -n '320,430p' executor/services/agent_service.pyRepository: wecode-ai/Wegent
Length of output: 24104
Fix cross-event-loop cancellation in CodeXAgent.cancel_run()
Agent.handle()(used byAgentService.execute_task) callsCodeXAgent.execute(), which runsexecute_async()viaasyncio.run(...)inside a worker thread;self._turnis therefore bound to that worker thread’s event loop.- The cancel endpoint calls
CodeXAgent.cancel_run()from the main FastAPI event loop;cancel_run()then usesloop.create_task(self._turn.interrupt())(orasyncio.run(...)), which targets the wrong loop forself._turnand can fail or not interrupt the running turn. - Capture the worker-loop that owns
_turnduringexecute()/execute_async()and useasyncio.run_coroutine_threadsafe(self._turn.interrupt(), that_loop)(and keep the returned future/handle instead of discarding it).
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 119-119: Do not catch blind exception: Exception
(BLE001)
[warning] 171-171: Store a reference to the return value of loop.create_task
(RUF006)
🤖 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 `@executor/agents/codex/codex_agent.py` around lines 113 - 174, The cancel
logic currently schedules interrupt on the wrong event loop; capture the worker
thread's event loop that owns self._turn (e.g., set self._turn_loop =
asyncio.get_running_loop() inside execute_async() before creating/assigning
self._turn) and on cancel_run() use
asyncio.run_coroutine_threadsafe(self._turn.interrupt(), self._turn_loop) (store
the returned Future on the agent, e.g., self._interrupt_future) instead of
loop.create_task or asyncio.run; also guard for missing
self._turn_loop/self._turn and return False if not present.
| @staticmethod | ||
| def _resolve_bot_id(task_data: ExecutionRequest) -> Optional[int]: | ||
| if not task_data.bot: | ||
| return None | ||
| bot_id = task_data.bot[0].get("id") | ||
| return int(bot_id) if bot_id is not None else None |
There was a problem hiding this comment.
int(bot_id) can raise on non-numeric ids.
_resolve_bot_id returns int(bot_id) whenever bot_id is not None, so a non-numeric id (e.g., a UUID string) raises ValueError during __init__, aborting agent construction. Guard the conversion.
🛡️ Proposed guard
- bot_id = task_data.bot[0].get("id")
- return int(bot_id) if bot_id is not None else None
+ bot_id = task_data.bot[0].get("id")
+ if bot_id is None:
+ return None
+ try:
+ return int(bot_id)
+ except (TypeError, ValueError):
+ return None🤖 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 `@executor/agents/codex/codex_agent.py` around lines 314 - 319, The
_resolve_bot_id helper in codex_agent.py currently does int(bot_id) unguarded
and can raise ValueError for non-numeric ids; update the
_resolve_bot_id(ExecutionRequest) implementation to attempt a safe conversion
and return None on failure: check if task_data.bot exists, extract bot_id from
task_data.bot[0].get("id"), then wrap the int conversion in a try/except
catching ValueError and TypeError and return None if conversion fails (otherwise
return the int). Keep the function signature and behavior for None unchanged so
agent __init__ doesn't abort on non-numeric ids.
| async def handle(self, event: Any) -> Optional[TaskStatus]: | ||
| method = getattr(event, "method", "") | ||
| payload = getattr(event, "payload", None) | ||
|
|
||
| if method == "item/agentMessage/delta": | ||
| delta = str(getattr(payload, "delta", "") or "") | ||
| if delta: | ||
| self.final_text += delta | ||
| self._saw_delta = True | ||
| await self.emitter.text_delta(delta) | ||
| return None | ||
|
|
||
| if method == "item/completed": | ||
| self._handle_completed_item(payload) | ||
| return None | ||
|
|
||
| if method == "thread/tokenUsage/updated": | ||
| self._handle_usage(payload) | ||
| return None | ||
|
|
||
| if method == "turn/completed": | ||
| return await self._handle_turn_completed(payload) | ||
|
|
||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '`@trace_async`|`@trace_sync`' -g 'executor/**/*.py' -C1
rg -nP 'def\s+(trace_async|trace_sync)\b' -g '*.py' -C2Repository: wecode-ai/Wegent
Length of output: 1310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and show the relevant section around handle()
FILE="executor/agents/codex/event_mapper.py"
ls -la "$FILE"
nl -ba "$FILE" | sed -n '1,120p'
nl -ba "$FILE" | sed -n '120,260p'
# Check whether handle() is decorated with trace_* in this file
rg -nP '^\s*`@trace_`(async|sync)\b' "$FILE"
rg -nP 'async\s+def\s+handle\s*\(' "$FILE"
# Show how other executor modules import/use the decorators (telemetry decorator import path)
rg -nP 'from\s+shared\.telemetry\.decorators\s+import\s+trace_(async|sync)|import\s+shared\.telemetry\.decorators|trace_(async|sync)\b' executor -g'*.py'Repository: wecode-ai/Wegent
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="executor/agents/codex/event_mapper.py"
echo "== File metadata =="
ls -la "$FILE"
echo "== Imports / top of file =="
cat -n "$FILE" | sed -n '1,120p'
echo "== handle() region =="
rg -nP 'async\s+def\s+handle\s*\(' "$FILE" || true
LINE="$(rg -nP 'async\s+def\s+handle\s*\(' "$FILE" | head -n1 | cut -d: -f1)"
START=$((LINE-15))
END=$((LINE+40))
cat -n "$FILE" | sed -n "${START},${END}p"
echo "== trace decorator usage in this file =="
rg -nP '^\s*`@trace_`(async|sync)\b' "$FILE" || true
echo "== How executors import trace decorators =="
rg -nP 'from\s+shared\.telemetry\.decorators\s+import\s+trace_(async|sync)\b|import\s+shared\.telemetry\.decorators' executor -g'*.py'Repository: wecode-ai/Wegent
Length of output: 6529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect codex event mapper for any tracing imports/usages
FILE="executor/agents/codex/event_mapper.py"
echo "== trace decorator usage in file =="
rg -nP 'trace_(async|sync)\b' "$FILE" || true
echo "== span helpers usage in file =="
rg -nP 'add_span_event\(|set_span_attribute\(' "$FILE" || true
echo "== top imports containing telemetry/logging/tracing =="
rg -nP 'telemetry|tracer|trace_(async|sync)|opentelemetry' "$FILE" || true
# Inspect how other executor code imports/uses trace decorators
echo "== Example usage/imports in other executor files =="
rg -nP 'from\s+shared\.telemetry\.decorators\s+import\s+trace_(async|sync)\b' executor -g'*.py' || true
rg -nP '^\s*`@trace_`(async|sync)\b' executor -g'*.py' -C2 || true
# Open relevant sections for one representative file each (keep small)
for f in \
executor/tasks/task_processor.py \
executor/agents/claude_code/claude_code_agent.py \
executor/agents/agno/agno_agent.py \
shared/telemetry/decorators.py
do
echo "== showing head+decorator area for $f =="
if [ -f "$f" ]; then
head -n 40 "$f" | sed -n '1,40p'
# show around first decorator occurrence
line="$(rg -nP '^\s*`@trace_`(async|sync)\b' "$f" | head -n1 | cut -d: -f1 || true)"
if [ -n "${line:-}" ]; then
start=$((line-10)); end=$((line+15))
sed -n "${start},${end}p" "$f" | cat -n
fi
fi
doneRepository: wecode-ai/Wegent
Length of output: 10933
Add OpenTelemetry tracing to CodeXEventMapper.handle
executor/agents/codex/event_mapper.py — CodeXEventMapper.handle is the public async dispatch entrypoint but has no @trace_async/@trace_sync decorator and the file doesn’t import shared.telemetry.decorators (other executor code does). Add a @trace_async to handle (with an appropriate span_name/tracer_name) and set span attributes such as the incoming event.method (and usage/done outcomes when applicable).
🤖 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 `@executor/agents/codex/event_mapper.py` around lines 27 - 50, Add
OpenTelemetry tracing to the public async dispatch entrypoint by importing
trace_async from shared.telemetry.decorators and annotating
CodeXEventMapper.handle with `@trace_async`(span_name="codex.event_mapper.handle",
tracer_name="executor"); inside handle (after reading method/payload) set span
attributes for the incoming event method (e.g.
opentelemetry.trace.get_current_span().set_attribute("codex.event.method",
method)) and update the span with outcome attributes when branching to
_handle_usage (set "codex.event.usage" or usage details), _handle_completed_item
(set "codex.event.completed" and any result status), and when awaiting
_handle_turn_completed (set "codex.event.turn_completed" and the returned
outcome); ensure emitter.text_delta handling still records deltas but also
annotates the span (e.g. "codex.event.delta_seen" or delta length) so tracing
captures method and usage/done outcomes for CodeXEventMapper.handle.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
executor/agents/claude_code/claude_code_agent.py (1)
507-520: ⚡ Quick winRemove unused local variables.
Lines 507-509 read
project_id,workspace_source, andproject_pathinto local variables, but these are never meaningfully used after line 520. Theprepare_project_workspace_path()method reads directly fromtask_data, making these local variables redundant.♻️ Proposed refactor to remove dead code
def _prepare_project_workspace(self) -> None: """Resolve project workspace paths before Claude Code starts.""" - project_id = getattr(self.task_data, "project_id", None) - workspace_source = getattr(self.task_data, "workspace_source", None) - project_path = getattr(self.task_data, "project_workspace_path", None) - if not workspace_source: + if not getattr(self.task_data, "workspace_source", None): standalone_path = prepare_standalone_chat_workspace( self.task_data, self.prompt ) if standalone_path: self.task_data.workspace_source = "local_path" self.task_data.project_workspace_path = standalone_path - workspace_source = "local_path" - project_path = standalone_path project_path = self.prepare_project_workspace_path() if not project_path:🤖 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 `@executor/agents/claude_code/claude_code_agent.py` around lines 507 - 520, Remove the unused local variables project_id, workspace_source, and project_path and read/write directly from self.task_data; specifically, eliminate the initial getattr assignments, check self.task_data.workspace_source (or use getattr(self.task_data, "workspace_source", None)) before calling prepare_standalone_chat_workspace(self.task_data, self.prompt), update self.task_data.workspace_source and self.task_data.project_workspace_path when standalone_path is returned, and then call self.prepare_project_workspace_path() as before; ensure prepare_standalone_chat_workspace and prepare_project_workspace_path usage remains unchanged.
🤖 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.
Nitpick comments:
In `@executor/agents/claude_code/claude_code_agent.py`:
- Around line 507-520: Remove the unused local variables project_id,
workspace_source, and project_path and read/write directly from self.task_data;
specifically, eliminate the initial getattr assignments, check
self.task_data.workspace_source (or use getattr(self.task_data,
"workspace_source", None)) before calling
prepare_standalone_chat_workspace(self.task_data, self.prompt), update
self.task_data.workspace_source and self.task_data.project_workspace_path when
standalone_path is returned, and then call self.prepare_project_workspace_path()
as before; ensure prepare_standalone_chat_workspace and
prepare_project_workspace_path usage remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2f840fd-92fa-40e5-91dc-c5b9819a481d
📒 Files selected for processing (4)
executor/agents/base.pyexecutor/agents/claude_code/claude_code_agent.pyexecutor/agents/codex/codex_agent.pyexecutor/tests/agents/test_project_workspace_paths.py
🚧 Files skipped from review as they are similar to previous changes (1)
- executor/agents/codex/codex_agent.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/__tests__/features/tasks/hooks/useModelSelection.test.tsx (1)
206-246: ⚡ Quick winAdd one hook-level case for
region: 'domestic'without内网in the label.The new coverage only exercises the text-based priority path. A model ordered first solely because of
region: 'domestic'would catch the normalization bug above and keep this sorter regression from coming back.Example assertion to add
it('returns filtered models in selector priority order', async () => { mockModelLoad([ { name: 'public-deepseek-v4-pro', displayName: '公网:ali-deepseek-v4-pro', provider: 'openai', modelId: 'ali-deepseek-v4-pro', type: 'public', }, { name: 'internal-deepseek-r1', - displayName: '内网:DeepSeek-R1', + displayName: 'DeepSeek-R1', provider: 'openai', modelId: 'DeepSeek-R1', type: 'public', + region: 'domestic', }, { name: 'public-deepseek-v3.2', displayName: '公网:DeepSeek-V3.2', provider: 'openai', modelId: 'DeepSeek-V3.2-251201', type: 'public', }, ])🤖 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 `@frontend/src/__tests__/features/tasks/hooks/useModelSelection.test.tsx` around lines 206 - 246, Add a hook-level test case in useModelSelection's test that exercises the branch where model.region === 'domestic' but the displayName does not contain the "内网" label so the sorter must normalise region-based priority (i.e., ensure the model with region: 'domestic' is placed ahead of others even when its displayName lacks 内网); update the array passed to mockModelLoad and the expectation on hook.result.current.filteredModels.map(model => model.name) to include a domestic-region model without 内网 as the first element to catch the normalization/sorter regression in useModelSelection.
🤖 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 `@frontend/src/features/tasks/hooks/useModelSelection.ts`:
- Around line 147-151: The new prioritization in compareModelsForSelection that
checks model.region === 'domestic' won't work for API-loaded options because
unifiedToModel doesn't copy UnifiedModel.region into the produced Model; update
unifiedToModel to propagate UnifiedModel.region into Model.region (or otherwise
set Model.region when building options), so getModelNetworkPriority and
compareModelsForSelection can rely on model.region for API-loaded models; ensure
the symbol names to change are unifiedToModel (add mapping for region) and
verify getModelNetworkPriority/compareModelsForSelection behavior after the
change.
---
Nitpick comments:
In `@frontend/src/__tests__/features/tasks/hooks/useModelSelection.test.tsx`:
- Around line 206-246: Add a hook-level test case in useModelSelection's test
that exercises the branch where model.region === 'domestic' but the displayName
does not contain the "内网" label so the sorter must normalise region-based
priority (i.e., ensure the model with region: 'domestic' is placed ahead of
others even when its displayName lacks 内网); update the array passed to
mockModelLoad and the expectation on
hook.result.current.filteredModels.map(model => model.name) to include a
domestic-region model without 内网 as the first element to catch the
normalization/sorter regression in useModelSelection.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02507d2a-5cb1-4ce1-9a46-dd06d6250ba8
📒 Files selected for processing (2)
frontend/src/__tests__/features/tasks/hooks/useModelSelection.test.tsxfrontend/src/features/tasks/hooks/useModelSelection.ts
| function getModelNetworkPriority(model: Model): number { | ||
| if (model.region === 'domestic') { | ||
| return 0 | ||
| } | ||
| return getModelSearchText(model).includes('内网') ? 0 : 1 |
There was a problem hiding this comment.
Propagate region before relying on it here.
compareModelsForSelection() now prioritizes model.region === 'domestic', but every fetched option is still built through unifiedToModel(), which never copies UnifiedModel.region. That makes this new branch effectively dead for API-loaded models unless the label also contains 内网, so metadata-only domestic models will still be misordered.
Suggested fix
export function unifiedToModel(unified: UnifiedModel): Model {
return {
name: unified.name,
provider: unified.provider || 'claude',
modelId: unified.modelId || '',
displayName: unified.displayName,
+ region: unified.region,
type: unified.type,
isAdvanced: unified.isAdvanced ?? false,
config: unified.config,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getModelNetworkPriority(model: Model): number { | |
| if (model.region === 'domestic') { | |
| return 0 | |
| } | |
| return getModelSearchText(model).includes('内网') ? 0 : 1 | |
| export function unifiedToModel(unified: UnifiedModel): Model { | |
| return { | |
| name: unified.name, | |
| provider: unified.provider || 'claude', | |
| modelId: unified.modelId || '', | |
| displayName: unified.displayName, | |
| region: unified.region, | |
| type: unified.type, | |
| isAdvanced: unified.isAdvanced ?? false, | |
| config: unified.config, | |
| } | |
| } |
🤖 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 `@frontend/src/features/tasks/hooks/useModelSelection.ts` around lines 147 -
151, The new prioritization in compareModelsForSelection that checks
model.region === 'domestic' won't work for API-loaded options because
unifiedToModel doesn't copy UnifiedModel.region into the produced Model; update
unifiedToModel to propagate UnifiedModel.region into Model.region (or otherwise
set Model.region when building options), so getModelNetworkPriority and
compareModelsForSelection can rely on model.region for API-loaded models; ensure
the symbol names to change are unifiedToModel (add mapping for region) and
verify getModelNetworkPriority/compareModelsForSelection behavior after the
change.
6ffa32a to
0928899
Compare
Summary
Verification
No DingTalk review notification sent.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores