Repository navigation
feat(codex): persist runtime session in task status - #1267
Closed
icycrystal4 wants to merge 1 commit into
Closed
icycrystal4 wants to merge 1 commit into
icycrystal4 wants to merge 1 commit into
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR implements API-backed Codex runtime session persistence by storing session metadata in task status, adding a backend endpoint to persist sessions, integrating session resolution into request building, and updating the executor agent to use API persistence instead of local file storage. ChangesTask Runtime Session Persistence
Sequence DiagramsequenceDiagram
participant RequestBuilder
participant TaskService as task_runtime_session
participant Agent as CodeXAgent
participant ApiClient
participant Backend as Backend API
RequestBuilder->>TaskService: get_task_runtime_session_id()
TaskService-->>RequestBuilder: session_id or None
RequestBuilder->>Agent: ExecutionRequest (runtime_session_id, provider)
Agent->>Agent: _open_thread()
alt new_session or no session_id
Agent->>Agent: thread_start()
Agent->>ApiClient: put(/runtime-session, provider, new_id)
ApiClient->>Backend: PUT /api/tasks/{task_id}/runtime-session
Backend-->>ApiClient: TaskRuntimeSessionResponse
ApiClient-->>Agent: success or retry
else existing session_id
Agent->>Agent: thread_resume(session_id)
Agent->>Agent: validate resumed_id matches
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
backend/tests/services/execution/test_request_builder_codex_runtime_session.py (1)
68-73: ⚡ Quick winHarden fixture shape to include
providerkey.To lock the runtime-routing contract, include
provider: "openai"in the mockedmodel_config(or replacemodel) so this suite catches future key-shape regressions.Suggested test update
monkeypatch.setattr( builder, "_get_model_config", lambda **kwargs: { - "model": "openai", + "provider": "openai", "model_id": "gpt-5.5", "api_format": "responses", }, )🤖 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/execution/test_request_builder_codex_runtime_session.py` around lines 68 - 73, Update the mocked model_config in the test (the lambda returning {"model","model_id","api_format"}) to include provider: "openai" so the fixture shape enforces the runtime-routing contract; locate the mock in backend/tests/services/execution/test_request_builder_codex_runtime_session.py and add the provider key to the returned dict (or replace the existing "model" key with a full model_config that includes provider) so future shape regressions are caught.backend/app/services/execution/request_builder.py (1)
53-66: ⚡ Quick winAdd tracing on the new Codex runtime decision path.
The new runtime branching (compatibility detection + prior-turn check) is high-signal execution logic but currently has no tracing decorators/span attributes, which makes runtime-session mismatch diagnosis harder.
As per coding guidelines:
Use OpenTelemetry tracing decorators:@trace_asyncor@trace_syncfor entire functions, and add_span_event() or set_span_attribute() for adding details to spans.Also applies to: 560-586
🤖 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/execution/request_builder.py` around lines 53 - 66, Add OpenTelemetry tracing to the Codex compatibility decision and the higher-level runtime branching: decorate _is_codex_compatible_model_config with `@trace_sync` (or `@trace_async` if it's async) and inside the function call set_span_attribute for the evaluated inputs (provider, api_format, protocol, wire_api) and the boolean result; likewise instrument the surrounding runtime branching that combines compatibility detection with the prior-turn check (the branch around lines 560-586) with a trace decorator and use add_span_event()/set_span_attribute to record the compatibility boolean, prior_turn flag, and final routing decision so these values are visible in traces for debugging session mismatches.executor/services/api_client.py (2)
128-151: ⚡ Quick winEnhance docstring to match
get()pattern.The
put()method docstring is minimal compared toget(). For consistency and maintainability, add Args and Returns sections matching the style ofget().📝 Proposed docstring enhancement
def put( self, path: str, timeout: int = DEFAULT_TIMEOUT, **kwargs, ) -> Optional[requests.Response]: - """Make PUT request to Backend API.""" + """Make PUT request to Backend API. + + Args: + path: API path (e.g., "/api/v1/tasks/123/runtime-session") + timeout: Request timeout in seconds + **kwargs: Additional arguments passed to requests.put() + + Returns: + Response object if successful (status 200), None otherwise + """🤖 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/api_client.py` around lines 128 - 151, Update the put() docstring to match the get() docstring style: add a one-line description ("Make PUT request to Backend API.") followed by Args specifying path (str): endpoint path, timeout (int): request timeout in seconds, and **kwargs: forwarded to requests.put; then add a Returns section documenting Optional[requests.Response] (response on HTTP 200, otherwise None). Keep wording and formatting consistent with the get() docstring and place it directly above the put() function definition.
140-145: 💤 Low valueConsider accepting 2xx status codes for PUT requests.
The method currently returns
Nonefor any non-200 response, but HTTP PUT requests commonly return201 Createdor204 No Content. While the current backend endpoint returns200, making the client accept any2xxstatus would improve robustness for future use cases.♻️ Proposed enhancement
- if response.status_code == 200: + if 200 <= response.status_code < 300: return response logger.warning(🤖 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/api_client.py` around lines 140 - 145, The PUT handler in ApiClient currently only treats status 200 as success and returns None otherwise; change the success check to accept any 2xx response (e.g., use response.ok or response.status_code // 100 == 2) so responses like 201 or 204 are treated as success, and keep the existing warning/logging and return None path for non-2xx codes; update the conditional where the code checks response.status_code == 200 (inside the ApiClient PUT method) to use the 2xx check instead.backend/app/services/task_runtime_session.py (1)
99-99: 💤 Low valueConsider using timezone-aware datetime for distributed systems.
datetime.now()returns a naive datetime without timezone information. For a distributed system with backend services potentially running in different timezones, consider usingdatetime.now(timezone.utc)ordatetime.utcnow()for consistency.🌍 Proposed fix to use UTC timezone
+from datetime import datetime, timezone + - now = datetime.now() + now = datetime.now(timezone.utc)🤖 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/task_runtime_session.py` at line 99, The assignment now = datetime.now() in task_runtime_session.py creates a naive datetime; change it to a timezone-aware UTC timestamp (e.g., use datetime.now(timezone.utc) or datetime.utcnow() replaced with timezone-aware equivalent) and update imports to include timezone (from datetime import datetime, timezone) so all downstream uses of the now variable carry UTC tzinfo for consistent cross-service 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/execution/request_builder.py`:
- Around line 53-66: The function _is_codex_compatible_model_config incorrectly
reads the provider from model_config["model"]; update it to prefer
model_config.get("provider") and fall back to model_config.get("model") (both
lowercased) so provider identity follows the ExecutionRequest contract, then use
that provider variable in the existing condition that compares to "openai"
alongside RESPONSES_WIRE_API and OPENAI_RESPONSES_PROTOCOL checks.
---
Nitpick comments:
In `@backend/app/services/execution/request_builder.py`:
- Around line 53-66: Add OpenTelemetry tracing to the Codex compatibility
decision and the higher-level runtime branching: decorate
_is_codex_compatible_model_config with `@trace_sync` (or `@trace_async` if it's
async) and inside the function call set_span_attribute for the evaluated inputs
(provider, api_format, protocol, wire_api) and the boolean result; likewise
instrument the surrounding runtime branching that combines compatibility
detection with the prior-turn check (the branch around lines 560-586) with a
trace decorator and use add_span_event()/set_span_attribute to record the
compatibility boolean, prior_turn flag, and final routing decision so these
values are visible in traces for debugging session mismatches.
In `@backend/app/services/task_runtime_session.py`:
- Line 99: The assignment now = datetime.now() in task_runtime_session.py
creates a naive datetime; change it to a timezone-aware UTC timestamp (e.g., use
datetime.now(timezone.utc) or datetime.utcnow() replaced with timezone-aware
equivalent) and update imports to include timezone (from datetime import
datetime, timezone) so all downstream uses of the now variable carry UTC tzinfo
for consistent cross-service behavior.
In
`@backend/tests/services/execution/test_request_builder_codex_runtime_session.py`:
- Around line 68-73: Update the mocked model_config in the test (the lambda
returning {"model","model_id","api_format"}) to include provider: "openai" so
the fixture shape enforces the runtime-routing contract; locate the mock in
backend/tests/services/execution/test_request_builder_codex_runtime_session.py
and add the provider key to the returned dict (or replace the existing "model"
key with a full model_config that includes provider) so future shape regressions
are caught.
In `@executor/services/api_client.py`:
- Around line 128-151: Update the put() docstring to match the get() docstring
style: add a one-line description ("Make PUT request to Backend API.") followed
by Args specifying path (str): endpoint path, timeout (int): request timeout in
seconds, and **kwargs: forwarded to requests.put; then add a Returns section
documenting Optional[requests.Response] (response on HTTP 200, otherwise None).
Keep wording and formatting consistent with the get() docstring and place it
directly above the put() function definition.
- Around line 140-145: The PUT handler in ApiClient currently only treats status
200 as success and returns None otherwise; change the success check to accept
any 2xx response (e.g., use response.ok or response.status_code // 100 == 2) so
responses like 201 or 204 are treated as success, and keep the existing
warning/logging and return None path for non-2xx codes; update the conditional
where the code checks response.status_code == 200 (inside the ApiClient PUT
method) to use the 2xx check instead.
🪄 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: 17002084-51d0-4bf5-93aa-675f7bee2fd6
📒 Files selected for processing (14)
backend/app/api/endpoints/adapter/tasks.pybackend/app/schemas/kind.pybackend/app/schemas/task.pybackend/app/services/execution/request_builder.pybackend/app/services/task_runtime_session.pybackend/tests/services/execution/test_request_builder_codex_runtime_session.pybackend/tests/services/test_task_runtime_session.pyexecutor/agents/codex/codex_agent.pyexecutor/agents/codex/session_store.pyexecutor/services/api_client.pyexecutor/tests/agents/test_codex_runtime_session.pyshared/models/db/subtask.pyshared/models/execution.pyshared/models/openai_converter.py
💤 Files with no reviewable changes (1)
- executor/agents/codex/session_store.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/tests/api/ws/test_chat_namespace_team_resolution.py (1)
43-86: ⚡ Quick winAdd a conflict-case test for payload/team binding precedence.
The new resolver behavior explicitly ignores a mismatched
payload.team_idwhen the task is already bound. Please add a test wherepayload.team_idis a non-zero conflicting value and assert the bound team still wins.🤖 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/api/ws/test_chat_namespace_team_resolution.py` around lines 43 - 86, Add a new test (e.g., test_chat_send_ignores_conflicting_payload_team_when_task_bound) that mirrors the existing cases but sets payload.team_id to a non-zero conflicting id; create the team and task binding (via Subtask.team_id or task team_ref using _make_team and _make_task/_make_task(task_id=...) as in test_chat_send_resolves_existing_task_team_from_subtask and test_chat_send_resolves_existing_task_team_from_team_ref), commit, then call _load_team_for_chat_send with ChatSendPayload(task_id=<bound task>, team_id=<conflicting non-zero>, message="...") and assert the returned team is the bound team (team.id == 4252) to verify payload.team_id is ignored when the task is already bound.frontend/src/features/tasks/components/chat/useChatStreamHandlers.tsx (1)
345-345:effectiveTeamIdfallback is mostly safe for new-chat sends, but guard against transient state
team_idis derived fromeffectiveTeamId(which falls back toselectedTaskDetail?.team?.id) even whentask_idisundefinedfor new sends. The usual “new chat” flows clear the previous task viaselectTask(null), which triggersresetSelectedTaskState→clearSelectedTaskArtifacts→setSelectedTaskDetail(null), anduseChatAreaStatealso setsselectedTeamwhen!selectedTaskDetail. Stale team fallback shouldn’t persist into the first send in those flows; the only remaining edge is an early send during a transient render window beforeselectedTaskDetail/selectedTeamupdate—consider gatingteam_idtoselectedTeam?.idfor new tasks (or only usingselectedTaskDetail?.team?.idwhencurrentTaskIdexists).🤖 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/components/chat/useChatStreamHandlers.tsx` at line 345, The effectiveTeamId fallback can pick up a stale selectedTaskDetail.team.id during new-chat sends; change the assignment so that when currentTaskId is falsy (new/send without a task) you only use selectedTeam?.id (no fallback to selectedTaskDetail), and when currentTaskId exists allow the fallback to selectedTaskDetail?.team?.id; update the expression that sets effectiveTeamId in useChatStreamHandlers (and any derived team_id usage) to branch on currentTaskId (or currentTaskId !== undefined) so transient renders cannot reuse a stale task's team id.
🤖 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/api/ws/chat_namespace.py`:
- Around line 99-106: The current lookup uses
db.query(...).filter(Subtask.task_id ==
task_id).order_by(Subtask.id.asc()).first() which can return a row with a NULL
team_id and miss later valid bindings; change the query to only consider
non-null team bindings by adding a filter for Subtask.team_id != None (e.g.
.filter(Subtask.task_id == task_id, Subtask.team_id !=
None).order_by(Subtask.id.asc()).first()), then return int(row[0]) if a row is
found; this ensures the earliest subtask with a real team_id is returned instead
of a NULL one.
- Around line 123-131: The query currently conditionally adds Kind.user_id only
if team_ref["user_id"] exists, which can match the wrong Team in shared
namespaces; always include the user_id filter when resolving a Team. Update the
code that builds the query (the query variable using Kind, team_name and
team_ref) to unconditionally filter by Kind.user_id == team_ref.get("user_id")
(so the filter is always present even if the value is None), removing the
conditional branch that skips adding the user_id filter.
- Line 91: Replace the boolean equality comparisons on the SQLAlchemy column
Kind.is_active (e.g. occurrences of "Kind.is_active == True") with
SQLAlchemy-safe predicates such as "Kind.is_active.is_(True)" or simply
"Kind.is_active" in the filter expressions; update each occurrence referenced in
chat_namespace.py (the filter expressions using Kind.is_active at the three
spots) so they use Kind.is_active.is_(True) (or Kind.is_active) instead of "==
True".
---
Nitpick comments:
In `@backend/tests/api/ws/test_chat_namespace_team_resolution.py`:
- Around line 43-86: Add a new test (e.g.,
test_chat_send_ignores_conflicting_payload_team_when_task_bound) that mirrors
the existing cases but sets payload.team_id to a non-zero conflicting id; create
the team and task binding (via Subtask.team_id or task team_ref using _make_team
and _make_task/_make_task(task_id=...) as in
test_chat_send_resolves_existing_task_team_from_subtask and
test_chat_send_resolves_existing_task_team_from_team_ref), commit, then call
_load_team_for_chat_send with ChatSendPayload(task_id=<bound task>,
team_id=<conflicting non-zero>, message="...") and assert the returned team is
the bound team (team.id == 4252) to verify payload.team_id is ignored when the
task is already bound.
In `@frontend/src/features/tasks/components/chat/useChatStreamHandlers.tsx`:
- Line 345: The effectiveTeamId fallback can pick up a stale
selectedTaskDetail.team.id during new-chat sends; change the assignment so that
when currentTaskId is falsy (new/send without a task) you only use
selectedTeam?.id (no fallback to selectedTaskDetail), and when currentTaskId
exists allow the fallback to selectedTaskDetail?.team?.id; update the expression
that sets effectiveTeamId in useChatStreamHandlers (and any derived team_id
usage) to branch on currentTaskId (or currentTaskId !== undefined) so transient
renders cannot reuse a stale task's team id.
🪄 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: 5dfa726c-f884-492d-ae5d-2ec62b09f221
📒 Files selected for processing (3)
backend/app/api/ws/chat_namespace.pybackend/tests/api/ws/test_chat_namespace_team_resolution.pyfrontend/src/features/tasks/components/chat/useChatStreamHandlers.tsx
| .filter( | ||
| Kind.id == team_id, | ||
| Kind.kind == "Team", | ||
| Kind.is_active == True, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "is_active\s*==\s*True" backend/app/api/ws/chat_namespace.pyRepository: wecode-ai/Wegent
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba backend/app/api/ws/chat_namespace.py | sed -n '70,110p'
nl -ba backend/app/api/ws/chat_namespace.py | sed -n '110,150p'
nl -ba backend/app/api/ws/chat_namespace.py | sed -n '240,280p'Repository: wecode-ai/Wegent
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'NR>=80 && NR<=105 {printf "%d:%s\n", NR, $0}' backend/app/api/ws/chat_namespace.py
echo "----"
awk 'NR>=115 && NR<=145 {printf "%d:%s\n", NR, $0}' backend/app/api/ws/chat_namespace.py
echo "----"
awk 'NR>=245 && NR<=275 {printf "%d:%s\n", NR, $0}' backend/app/api/ws/chat_namespace.pyRepository: wecode-ai/Wegent
Length of output: 3303
Fix Ruff E712 SQLAlchemy boolean predicates (Kind.is_active == True)
Kind.is_active == True is flagged by Ruff in SQLAlchemy filters; use Kind.is_active.is_(True) (or Kind.is_active) instead. Occurs at lines 91, 127, and 259.
🧰 Tools
🪛 Ruff (0.15.15)
[error] 91-91: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
🤖 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/ws/chat_namespace.py` at line 91, Replace the boolean
equality comparisons on the SQLAlchemy column Kind.is_active (e.g. occurrences
of "Kind.is_active == True") with SQLAlchemy-safe predicates such as
"Kind.is_active.is_(True)" or simply "Kind.is_active" in the filter expressions;
update each occurrence referenced in chat_namespace.py (the filter expressions
using Kind.is_active at the three spots) so they use Kind.is_active.is_(True)
(or Kind.is_active) instead of "== True".
Comment on lines
+99
to
+106
| row = ( | ||
| db.query(Subtask.team_id) | ||
| .filter(Subtask.task_id == task_id) | ||
| .order_by(Subtask.id.asc()) | ||
| .first() | ||
| ) | ||
| if row and row[0]: | ||
| return int(row[0]) |
Contributor
There was a problem hiding this comment.
Subtask team lookup can miss an existing binding when the first row has NULL team_id.
first() over all task subtasks can return a null team_id row even if later subtasks have a valid team binding.
Suggested fix
row = (
db.query(Subtask.team_id)
- .filter(Subtask.task_id == task_id)
+ .filter(
+ Subtask.task_id == task_id,
+ Subtask.team_id.isnot(None),
+ )
.order_by(Subtask.id.asc())
.first()
)🤖 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/ws/chat_namespace.py` around lines 99 - 106, The current
lookup uses db.query(...).filter(Subtask.task_id ==
task_id).order_by(Subtask.id.asc()).first() which can return a row with a NULL
team_id and miss later valid bindings; change the query to only consider
non-null team bindings by adding a filter for Subtask.team_id != None (e.g.
.filter(Subtask.task_id == task_id, Subtask.team_id !=
None).order_by(Subtask.id.asc()).first()), then return int(row[0]) if a row is
found; this ensures the earliest subtask with a real team_id is returned instead
of a NULL one.
Comment on lines
+123
to
+131
| query = db.query(Kind.id).filter( | ||
| Kind.kind == "Team", | ||
| Kind.name == team_name, | ||
| Kind.namespace == team_ref.get("namespace", "default"), | ||
| Kind.is_active == True, | ||
| ) | ||
| if team_ref.get("user_id") is not None: | ||
| query = query.filter(Kind.user_id == team_ref["user_id"]) | ||
|
|
Contributor
There was a problem hiding this comment.
Always include user_id when resolving Team from teamRef.
When teamRef.user_id is absent, this query matches by name + namespace only and can resolve the wrong Team in shared namespaces.
Suggested fix
- query = db.query(Kind.id).filter(
+ team_user_id = team_ref.get("user_id")
+ if team_user_id is None and task:
+ team_user_id = task.user_id
+ if team_user_id is None:
+ return None
+
+ query = db.query(Kind.id).filter(
Kind.kind == "Team",
Kind.name == team_name,
Kind.namespace == team_ref.get("namespace", "default"),
- Kind.is_active == True,
+ Kind.user_id == team_user_id,
+ Kind.is_active.is_(True),
)
- if team_ref.get("user_id") is not None:
- query = query.filter(Kind.user_id == team_ref["user_id"])As per coding guidelines, "A Kind resource is uniquely identified by THREE fields: namespace, name, and user_id - always use all three fields to locate a Kind".
🧰 Tools
🪛 Ruff (0.15.15)
[error] 127-127: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
🤖 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/ws/chat_namespace.py` around lines 123 - 131, The query
currently conditionally adds Kind.user_id only if team_ref["user_id"] exists,
which can match the wrong Team in shared namespaces; always include the user_id
filter when resolving a Team. Update the code that builds the query (the query
variable using Kind, team_name and team_ref) to unconditionally filter by
Kind.user_id == team_ref.get("user_id") (so the filter is always present even if
the value is None), removing the conditional branch that skips adding the
user_id filter.
icycrystal4
force-pushed
the
codex/persist-codex-runtime-session
branch
2 times, most recently
from
June 4, 2026 10:59
5c7f9ce to
fef7d7c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/api/endpoints/adapter/tasks.py (1)
389-445: ⚡ Quick winAdd tracing to this new endpoint.
update_task_runtime_sessionis a new backend entry point in the executor/session handshake, but it is not wrapped in@trace_syncand it does not attach task/provider details to the span. That makes slow or failed registrations much harder to correlate in production.As per coding guidelines,
**/*.py:Use OpenTelemetry tracing decorators:@trace_asyncor@trace_syncfor entire functions, and add_span_event() or set_span_attribute() for adding details to spans.🤖 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/tasks.py` around lines 389 - 445, Wrap the update_task_runtime_session endpoint with the OpenTelemetry decorator `@trace_sync` and add span attributes/events inside the function to record key identifiers (e.g., task_id, provider and session id) for easier correlation; specifically, import trace_sync and decorate update_task_runtime_session with `@trace_sync`, then call set_span_attribute("task.id", task_id) and set_span_attribute("runtime.provider", provider) (and add_span_event("runtime.session.created", {"session_id": request.id}) or similar) right after validation or right before persistence, ensuring attributes/events are added both on success and inside exception handlers so traces include contextual details.
🤖 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/api/endpoints/adapter/tasks.py`:
- Around line 408-427: This code performs a read-modify-write on TaskResource
without row-level locking, allowing concurrent PUTs to overwrite runtime
sessions; either add a SELECT ... FOR UPDATE (row-level lock) when fetching the
TaskResource before calling set_task_runtime_session (i.e., lock the
TaskResource row returned by the TaskResource query in this block) or move an
atomic compare-and-set into set_task_runtime_session (have
set_task_runtime_session perform the read-with-lock or an UPDATE ... WHERE
runtime_sessions_codex IS NULL RETURNING ... and return a 409 on failure) so
that concurrent callers cannot both register the same session and the
create-only contract is preserved (refer to TaskResource,
TaskResource.is_active_query(), task_member_service.is_member(),
set_task_runtime_session(), db.commit(), db.refresh()).
---
Nitpick comments:
In `@backend/app/api/endpoints/adapter/tasks.py`:
- Around line 389-445: Wrap the update_task_runtime_session endpoint with the
OpenTelemetry decorator `@trace_sync` and add span attributes/events inside the
function to record key identifiers (e.g., task_id, provider and session id) for
easier correlation; specifically, import trace_sync and decorate
update_task_runtime_session with `@trace_sync`, then call
set_span_attribute("task.id", task_id) and
set_span_attribute("runtime.provider", provider) (and
add_span_event("runtime.session.created", {"session_id": request.id}) or
similar) right after validation or right before persistence, ensuring
attributes/events are added both on success and inside exception handlers so
traces include contextual details.
🪄 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: 65208c5c-e700-4130-a7f4-5919a3b264f2
📒 Files selected for processing (13)
backend/app/api/endpoints/adapter/tasks.pybackend/app/schemas/kind.pybackend/app/schemas/task.pybackend/app/services/execution/request_builder.pybackend/app/services/task_runtime_session.pybackend/tests/services/execution/test_request_builder_codex_runtime_session.pybackend/tests/services/test_task_runtime_session.pyexecutor/agents/codex/codex_agent.pyexecutor/agents/codex/session_store.pyexecutor/services/api_client.pyexecutor/tests/agents/test_codex_runtime_session.pyshared/models/execution.pyshared/models/openai_converter.py
💤 Files with no reviewable changes (1)
- executor/agents/codex/session_store.py
🚧 Files skipped from review as they are similar to previous changes (10)
- shared/models/execution.py
- backend/app/schemas/task.py
- shared/models/openai_converter.py
- executor/services/api_client.py
- backend/app/schemas/kind.py
- backend/tests/services/test_task_runtime_session.py
- executor/agents/codex/codex_agent.py
- backend/tests/services/execution/test_request_builder_codex_runtime_session.py
- backend/app/services/task_runtime_session.py
- backend/app/services/execution/request_builder.py
icycrystal4
force-pushed
the
codex/persist-codex-runtime-session
branch
from
June 4, 2026 11:12
fef7d7c to
76823a1
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
shared/models/codex_routing.py (1)
33-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
provideras the primary provider key here.Line 35 keys off
model_config["model"], but the sharedExecutionRequestcontract usesproviderfor provider identity. When onlyprovider="openai"is present, this helper returnsFalse, so backend/session routing silently skips the Codex resume path.Suggested fix
def is_codex_compatible_model_config(model_config: dict[str, Any]) -> bool: """Return whether executor routing will choose CodeXAgent for a model.""" - provider = str(model_config.get("model") or "").lower() + provider = str( + model_config.get("provider") or model_config.get("model") or "" + ).lower() return provider == "openai" and uses_responses_wire_api(model_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 `@shared/models/codex_routing.py` around lines 33 - 36, The helper is_codex_compatible_model_config currently reads provider identity from model_config["model"], but the canonical key is "provider"; update the function to first read provider = str(model_config.get("provider") or model_config.get("model") or "").lower() (or equivalent) and then return provider == "openai" and uses_responses_wire_api(model_config) so Codex routing triggers when provider="openai" even if "model" is absent; keep uses_responses_wire_api(model_config) unchanged.backend/app/api/endpoints/adapter/tasks.py (1)
408-427:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize runtime-session registration at the row boundary.
This is still an unlocked read-modify-write on
TaskResource. Two concurrentPUTs can both read an empty runtime session, both passset_task_runtime_session(...), and the later commit overwrites the earlier one instead of returning409.🤖 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/tasks.py` around lines 408 - 427, The current read-modify-write on TaskResource is racy: two concurrent PUTs can read the same unlocked row and both set runtime session; change the initial lookup to acquire a row lock using SQLAlchemy's FOR UPDATE (use db.query(TaskResource).filter(...).with_for_update() or the equivalent in your DB session) before calling set_task_runtime_session, then re-check the runtime-session state on the locked TaskResource (the same TaskResource instance/variable used above) and if a session is already present return HTTP 409 instead of overwriting; keep the membership check (task_member_service.is_member) and the call site names (TaskResource lookup, set_task_runtime_session, db.commit(), db.refresh(task)) intact so reviewers can find the change.
🧹 Nitpick comments (1)
backend/app/api/endpoints/adapter/tasks.py (1)
389-445: ⚡ Quick winAdd tracing to this new persistence endpoint.
This write path is now the backend source of truth for Codex session persistence, but it is missing the repo-standard
@trace_syncdecoration and span attributes/events for the persisted provider/task outcome. That will make production resume failures much harder to diagnose. As per coding guidelines "Use OpenTelemetry tracing decorators:@trace_asyncor@trace_syncfor entire functions, andadd_span_event()orset_span_attribute()for adding details to spans".🤖 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/tasks.py` around lines 389 - 445, The endpoint update_task_runtime_session is missing OpenTelemetry tracing: add the repo-standard `@trace_sync` decorator to the function and, inside the handler, call set_span_attribute() or add_span_event() to record the provider (provider variable), the task id/outcome (task_id and whether session persisted), and any errors; also add span attributes when catching RuntimeSessionAlreadyExistsError and ValueError (include error.type/message) and before returning TaskRuntimeSessionResponse(**session) mark success so traces show the persisted provider/task outcome; ensure imports for trace_sync, set_span_attribute, and add_span_event are added and used around calls to set_task_runtime_session and db.commit().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@backend/app/api/endpoints/adapter/tasks.py`:
- Around line 408-427: The current read-modify-write on TaskResource is racy:
two concurrent PUTs can read the same unlocked row and both set runtime session;
change the initial lookup to acquire a row lock using SQLAlchemy's FOR UPDATE
(use db.query(TaskResource).filter(...).with_for_update() or the equivalent in
your DB session) before calling set_task_runtime_session, then re-check the
runtime-session state on the locked TaskResource (the same TaskResource
instance/variable used above) and if a session is already present return HTTP
409 instead of overwriting; keep the membership check
(task_member_service.is_member) and the call site names (TaskResource lookup,
set_task_runtime_session, db.commit(), db.refresh(task)) intact so reviewers can
find the change.
In `@shared/models/codex_routing.py`:
- Around line 33-36: The helper is_codex_compatible_model_config currently reads
provider identity from model_config["model"], but the canonical key is
"provider"; update the function to first read provider =
str(model_config.get("provider") or model_config.get("model") or "").lower() (or
equivalent) and then return provider == "openai" and
uses_responses_wire_api(model_config) so Codex routing triggers when
provider="openai" even if "model" is absent; keep
uses_responses_wire_api(model_config) unchanged.
---
Nitpick comments:
In `@backend/app/api/endpoints/adapter/tasks.py`:
- Around line 389-445: The endpoint update_task_runtime_session is missing
OpenTelemetry tracing: add the repo-standard `@trace_sync` decorator to the
function and, inside the handler, call set_span_attribute() or add_span_event()
to record the provider (provider variable), the task id/outcome (task_id and
whether session persisted), and any errors; also add span attributes when
catching RuntimeSessionAlreadyExistsError and ValueError (include
error.type/message) and before returning TaskRuntimeSessionResponse(**session)
mark success so traces show the persisted provider/task outcome; ensure imports
for trace_sync, set_span_attribute, and add_span_event are added and used around
calls to set_task_runtime_session and db.commit().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9aabe76e-843a-43dd-b01f-0c48b1d64083
📒 Files selected for processing (17)
backend/app/api/endpoints/adapter/tasks.pybackend/app/schemas/kind.pybackend/app/schemas/task.pybackend/app/services/execution/request_builder.pybackend/app/services/model_aggregation_service.pybackend/app/services/task_runtime_session.pybackend/tests/services/execution/test_request_builder_codex_runtime_session.pybackend/tests/services/test_task_runtime_session.pyexecutor/agents/codex/codex_agent.pyexecutor/agents/codex/config_builder.pyexecutor/agents/codex/session_store.pyexecutor/services/api_client.pyexecutor/tests/agents/test_codex_runtime_session.pyshared/models/codex_routing.pyshared/models/execution.pyshared/models/openai_converter.pyshared/tests/test_codex_routing.py
💤 Files with no reviewable changes (1)
- executor/agents/codex/session_store.py
✅ Files skipped from review due to trivial changes (1)
- shared/tests/test_codex_routing.py
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/app/schemas/task.py
- executor/services/api_client.py
- shared/models/openai_converter.py
- backend/tests/services/test_task_runtime_session.py
- backend/tests/services/execution/test_request_builder_codex_runtime_session.py
- backend/app/services/task_runtime_session.py
- executor/agents/codex/codex_agent.py
icycrystal4
force-pushed
the
codex/persist-codex-runtime-session
branch
from
June 4, 2026 11:22
76823a1 to
10d3244
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tasks.json.status.runtime.sessions.codexthrough a dedicated task-token API.Testing
cd backend && uv run pytest tests/services/test_task_runtime_session.py tests/services/execution/test_request_builder_codex_runtime_session.py(9 passed)cd executor && uv run pytest tests/agents/test_codex_runtime_session.py(5 passed)git diff --check origin/main..HEADIntegration Environment
ghcr.io/wecode-ai/weibo-wegent-executor:dev(sha256:d6e1eee052ac3bb06796671baecb71c0e642e3fede87725530d83afbb27d65b7).status.runtime: nullby rerunning backend focused tests remotely and restarting backend; health check was OK.runtime-session,Failed to persist runtime session,AttributeError, andERROR; no new matching failures observed after restart.Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
Tests