Repository navigation
Feat/workspace archive recovery - #848
Merged
Merged
Conversation
Implement Pod recovery feature that allows users to continue conversations after executor Pods are deleted: - Add ArchiveInfo schema to store archive metadata in Task.status.archive - Add workspace_archive service for MinIO presigned URL generation - Add archive/restore endpoints in executor and executor_manager - Add ExecutorRecoveryService for Pod recreation with workspace restoration - Add skip_git_clone support to prevent git clone conflicts during restore - Add Celery Beat scheduled task for expired archive cleanup (30-day retention) - Integrate archive workflow in executor_job before Pod deletion Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add minio package to backend dependencies for workspace archive storage. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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:
📝 WalkthroughWalkthroughAdds workspace archiving and recovery: archives are created/uploaded during executor cleanup and may be restored when redispatching deleted executors, with new services, storage integration (MinIO), executor-manager/executor endpoints, DB status fields, and orchestration across dispatcher/scheduler/cleanup paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Backend
participant DB
participant ExecManager as Executor Manager
participant Executor
participant MinIO as Storage
rect rgba(100,150,200,0.5)
Note over Backend,Executor: Archive Flow (on pod deletion or manual trigger)
Backend->>MinIO: request presigned upload URL
MinIO-->>Backend: upload URL
ExecManager->>Executor: POST /api/archive (upload URL)
Executor->>MinIO: PUT archive via presigned URL
MinIO-->>Executor: upload acknowledged
Executor-->>ExecManager: ArchiveResponse (size, flags)
ExecManager->>Backend: relay archive info
Backend->>DB: persist task.status.archive
end
rect rgba(200,100,150,0.5)
Note over Client,Backend: Recovery Flow (redispatch after executor_deleted_at)
Client->>Backend: dispatch execution request
Backend->>DB: load subtask (executor_deleted_at == True)
Backend->>MinIO: check archive availability/expiration
alt archive available
Backend->>MinIO: generate presigned download URL
MinIO-->>Backend: download URL
ExecManager->>Executor: POST /api/restore (download URL, skip_git_clone=True)
Executor->>MinIO: GET archive bytes
Executor->>Executor: extract archive into workspace
Executor-->>ExecManager: RestoreResponse(success)
Backend->>DB: update subtask executor info and set executor_deleted_at=False
else archive missing/expired
Backend-->>Client: raise/reject dispatch (or recreate sandbox without archive)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 7
🧹 Nitpick comments (20)
executor_manager/routers/routers.py (1)
1332-1400: Add route-level tracing to the new recovery handlers.These two async handlers do multi-hop recovery I/O, but they won't emit the span shape our Python guidelines expect. Wrapping them with
@trace_async(...)and attachingtask_id/ executor identifiers would make timeout and restore failures much easier to diagnose. As per coding guidelines,**/*.py: Use@trace_async(span_name, tracer_name, extract_attributes)decorator to trace entire async functions in OpenTelemetry; useset_span_attribute(key, value)to set attribute on current OpenTelemetry span.Also applies to: 1403-1471
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor_manager/routers/routers.py` around lines 1332 - 1400, Wrap the async route handler archive_executor_workspace with the `@trace_async` decorator (providing a descriptive span_name and tracer_name and an extract_attributes callable) so the whole function is traced, and inside the handler set span attributes for task_id and executor identifiers using set_span_attribute (e.g., set_span_attribute("task_id", request.task_id) and set_span_attribute("executor", f"{request.executor_namespace}/{request.executor_name}")); apply the same change to the other async recovery handler referenced at 1403-1471. Ensure imports for trace_async and set_span_attribute are added and keep existing logic using ExecutorDispatcher.get_executor and traced_async_client unchanged.backend/app/services/workspace_archive/storage.py (1)
187-193: Use timezone-aware datetime instead of deprecatedutcnow().
datetime.utcnow()is deprecated in Python 3.12+. Usedatetime.now(timezone.utc)for timezone-aware UTC timestamps.♻️ Proposed fix
+from datetime import datetime, timedelta, timezone -from datetime import datetime, timedelta ... def calculate_expiration_time(self) -> datetime: """Calculate archive expiration time based on retention days. Returns: Expiration datetime (UTC) """ - return datetime.utcnow() + timedelta(days=ARCHIVE_RETENTION_DAYS) + return datetime.now(timezone.utc) + timedelta(days=ARCHIVE_RETENTION_DAYS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/storage.py` around lines 187 - 193, The calculate_expiration_time method uses deprecated datetime.utcnow(); update it to return a timezone-aware UTC datetime by using datetime.now(timezone.utc) + timedelta(days=ARCHIVE_RETENTION_DAYS) (ensure timezone is imported from datetime) so calculate_expiration_time and any callers receive an aware UTC timestamp instead of a naive one.backend/app/services/execution/__init__.py (2)
99-106: Consider extractingSimpleSandboxas a module-level class or dataclass.Defining a class inside a method creates a new class object on each call. While this works, extracting it to module level (or using a
dataclass/NamedTuple) would be cleaner and more reusable.♻️ Example using dataclass at module level
from dataclasses import dataclass from typing import Optional `@dataclass` class SimpleSandbox: sandbox_id: Optional[str] container_name: Optional[str] base_url: Optional[str]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/__init__.py` around lines 99 - 106, Extract the inner class SimpleSandbox out of the function and define it at module level (preferably as a dataclass) so a single class object is reused; replace the nested class definition with an import/use of the module-level SimpleSandbox and keep the return as SimpleSandbox(data) or adapt construction to the dataclass signature (e.g., SimpleSandbox(sandbox_id=..., container_name=..., base_url=...)) to preserve behavior. Ensure the new module-level symbol name remains SimpleSandbox so existing code that references it continues to work.
57-66: Use explicitOptionalor| Nonefor optional parameters.PEP 484 prohibits implicit
Optionalwhen using= Nonedefault without theOptionaltype hint. This is flagged by static analysis and can cause issues with type checkers.♻️ Proposed fix
+ from typing import Optional + async def create_sandbox( self, shell_type: str, user_id: int, user_name: str, - timeout: int = None, - workspace_ref: str = None, - bot_config: dict = None, - metadata: dict = None, + timeout: Optional[int] = None, + workspace_ref: Optional[str] = None, + bot_config: Optional[dict] = None, + metadata: Optional[dict] = None, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/__init__.py` around lines 57 - 66, The create_sandbox function declares optional parameters with default None but lacks explicit Optional type hints; update the signature of create_sandbox to annotate timeout, workspace_ref, bot_config, and metadata using typing.Optional[...] (or use the PEP 604 union syntax e.g. int | None, str | None, dict | None if using Python 3.10+) so each parameter explicitly indicates it may be None (keep existing parameter names: timeout, workspace_ref, bot_config, metadata).backend/app/services/workspace_archive/__init__.py (1)
43-48: Consider sorting__all__for consistency.Static analysis suggests sorting
__all__alphabetically for consistency with isort conventions.♻️ Proposed fix
__all__ = [ "ArchiveService", + "ArchiveStorageService", "archive_service", - "ArchiveStorageService", "archive_storage_service", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/__init__.py` around lines 43 - 48, The __all__ export list in workspace_archive/__init__.py is unsorted; please reorder the names alphabetically (ArchiveService, ArchiveStorageService, archive_service, archive_storage_service) so the __all__ list follows isort-style consistency and deterministic ordering; update the __all__ assignment to contain the sorted sequence.backend/app/services/workspace_archive/cleanup_job.py (3)
46-55: Consider pagination for large task volumes.Querying all active tasks with
.all()loads everything into memory. For deployments with many tasks, this could cause memory issues. Consider using pagination or batch processing.♻️ Example with batching
BATCH_SIZE = 1000 offset = 0 while True: tasks = ( db.query(TaskResource) .filter(...) .offset(offset) .limit(BATCH_SIZE) .all() ) if not tasks: break # Process batch... offset += BATCH_SIZE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/cleanup_job.py` around lines 46 - 55, The current query in cleanup_job.py loading all active TaskResource rows via .all() can OOM for large datasets; change the logic in the cleanup_job task to process active TaskResource rows in batches (e.g., using limit/offset or SQLAlchemy yield_per) instead of .all(): perform repeated queries filtering TaskResource.kind == "Task" and TaskResource.is_active == TaskResource.STATE_ACTIVE with a BATCH_SIZE (e.g., 500–1000), loop until the returned batch is empty, and process/delete each batch before fetching the next so memory usage stays bounded while keeping the existing TaskResource handling code (the block that currently assigns to tasks) intact.
57-57: Use timezone-aware datetime instead of deprecatedutcnow().Same issue as in
storage.py-datetime.utcnow()is deprecated in Python 3.12+.♻️ Proposed fix
-from datetime import datetime +from datetime import datetime, timezone ... - now = datetime.utcnow() + now = datetime.now(timezone.utc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/cleanup_job.py` at line 57, The code uses the deprecated datetime.utcnow() to set the variable now in cleanup_job.py; replace it with a timezone-aware call (e.g., datetime.now(timezone.utc)) and ensure the datetime.timezone is imported or referenced (add import for timezone if missing) so the variable now becomes timezone-aware throughout the cleanup_job logic that relies on it.
86-109: Extract duplicate metadata clearing logic.The code to clear
task_json["status"]["archive"]is duplicated in both the success (lines 87-92) and failure (lines 99-104) branches. Consider extracting to reduce duplication.♻️ Proposed refactor
+ # Helper to clear archive metadata + def clear_archive_metadata(): + task_json = task.json + if "status" in task_json and "archive" in task_json["status"]: + del task_json["status"]["archive"] + task.json = task_json + db.add(task) + if deleted: - # Clear archive info from task - task_json = task.json - if "status" in task_json and "archive" in task_json["status"]: - del task_json["status"]["archive"] - task.json = task_json - db.add(task) - + clear_archive_metadata() cleaned_count += 1 logger.info( f"[ArchiveCleanup] Cleaned up archive for task {task.id}" ) else: - # Archive file may not exist, still clear metadata - task_json = task.json - if "status" in task_json and "archive" in task_json["status"]: - del task_json["status"]["archive"] - task.json = task_json - db.add(task) + clear_archive_metadata() cleaned_count += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/cleanup_job.py` around lines 86 - 109, Duplicate logic that removes task_json["status"]["archive"] and re-adds the task to the session should be extracted into a helper (e.g., clear_task_archive_metadata(task, db)) and called from both branches; implement clear_task_archive_metadata to load task.json, check for "status" and "archive", delete the archive key, assign back to task.json, and call db.add(task), then replace the duplicated blocks in cleanup_job.py with a call to this helper while keeping the existing cleaned_count increments and logger.info / logger.warning messages intact.backend/app/services/adapters/executor_job.py (3)
160-166: Duplicate task_type extraction and missing parentheses for clarity.
- The
task_typeis already extracted at lines 113-117 but not preserved inexecutor_task_map, requiring re-extraction here.- Static analysis correctly flags the
and/orchain for missing parentheses.♻️ Proposed fix for parentheses
task_type = ( - task_crd.metadata.labels - and task_crd.metadata.labels.get("taskType") + (task_crd.metadata.labels + and task_crd.metadata.labels.get("taskType")) or "chat" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/adapters/executor_job.py` around lines 160 - 166, The code re-extracts task_type from task JSON (via Task.model_validate and metadata.labels) even though it was previously computed and stored in executor_task_map, and the boolean-expression using "and/or" lacks parentheses; update the logic in executor_job.py to reuse the existing task_type from executor_task_map instead of calling Task.model_validate(task.json) again, and if you must compute it here wrap the conditional expression in parentheses or replace the and/or chain with an explicit conditional (e.g., labels and labels.get("taskType") or "chat") to make precedence clear; reference symbols: task_type, Task.model_validate, executor_task_map, and metadata.labels.
134-152: Redundant database query for task lookup.The task is already queried earlier in the loop (lines 83-91) during the
valid_candidatesfiltering phase. This second query at lines 142-150 is redundant since the sametaskobject is already available.Consider storing the task along with the subtask during the first filtering pass to avoid the duplicate query.
♻️ Proposed optimization to avoid duplicate query
Modify the
valid_candidatesfiltering to store(subtask, task)tuples instead of justsubtask:# Filter candidates by checking task status from JSON - valid_candidates = [] + valid_candidates: List[Tuple[Subtask, TaskResource]] = [] for subtask in candidates: # Get task from tasks table task = ( db.query(TaskResource) ... ) ... # Check if task status is in COMPLETED, FAILED, or CANCELLED if task_status in ["COMPLETED", "FAILED", "CANCELLED"]: - valid_candidates.append(subtask) + valid_candidates.append((subtask, task)) ... # Deduplicate by (namespace, name) and collect task info for archiving # Map: (namespace, name) -> (subtask, task) for archive operation executor_task_map: Dict[Tuple[str, str], Tuple[Subtask, TaskResource]] = {} - for subtask in valid_candidates: + for subtask, task in valid_candidates: if subtask.executor_name: key = (subtask.executor_namespace, subtask.executor_name) if key not in executor_task_map: - # Get task for this subtask - task = ( - db.query(TaskResource) - ... - ) - if task: - executor_task_map[key] = (subtask, task) + executor_task_map[key] = (subtask, task)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/adapters/executor_job.py` around lines 134 - 152, The loop builds executor_task_map by re-querying TaskResource for each Subtask in valid_candidates, which is redundant because the TaskResource was already fetched during the earlier valid_candidates filtering; change the initial filtering logic that builds valid_candidates to collect pairs (subtask, task) instead of just subtask (e.g., valid_candidates: List[Tuple[Subtask, TaskResource]]), update any type hints and the filtering code that fetches TaskResource (the earlier block around where valid_candidates is populated), and then modify the deduplication loop to iterate over (subtask, task) and use the existing task to populate executor_task_map (avoid the second db.query call and remove the TaskResource lookup inside the executor_task_map population).
236-261: Consider avoidingasyncio.set_event_loop()in worker thread.While creating a new event loop is correct for the Celery worker thread context, calling
asyncio.set_event_loop(loop)sets it as the thread's default loop. If the worker thread is reused by Celery's thread pool, this could interfere with other code expecting a different (or no) default loop.Since you immediately use
loop.run_until_complete(), setting the default loop is unnecessary.♻️ Proposed simplification
# Run async archive in new event loop loop = asyncio.new_event_loop() - asyncio.set_event_loop(loop) try: archive_info = loop.run_until_complete( archive_service.archive_workspace(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/adapters/executor_job.py` around lines 236 - 261, Remove the call to asyncio.set_event_loop(loop) and instead create a fresh event loop with asyncio.new_event_loop(), call loop.run_until_complete(...) to run archive_service.archive_workspace(...), then in the finally block call loop.run_until_complete(loop.shutdown_asyncgens()) and loop.close() as before; update the block around archive_service.archive_workspace, loop, run_until_complete, shutdown_asyncgens to no longer set the loop as the thread default.executor/envd/api/routes.py (2)
315-316: Remove extraneous f-string prefix.Static analysis correctly flags that this f-string has no placeholders.
♻️ Proposed fix
- logger.info(f"[archive] Uploading archive to MinIO") + logger.info("[archive] Uploading archive to MinIO")Also at line 373:
- logger.info(f"[restore] Downloading archive from MinIO") + logger.info("[restore] Downloading archive from MinIO")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/envd/api/routes.py` around lines 315 - 316, The log call uses an unnecessary f-string with no placeholders (logger.info(f"[archive] Uploading archive to MinIO")), which static analysis flags; change it to a plain string literal (logger.info("[archive] Uploading archive to MinIO"])) and do the same for the other occurrence referenced around line 373 so neither logger.info uses an extraneous f-prefix.
345-349: Preserve exception chain when re-raising HTTPException.Use
raise ... from eto preserve the original exception context for debugging.♻️ Proposed fix
except Exception as e: logger.exception(f"[archive] Error archiving workspace: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail=str(e)) from eSimilarly at line 431:
- raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail=str(e)) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/envd/api/routes.py` around lines 345 - 349, In the exception handler in api/routes.py update the generic catch block to preserve the original exception chain by re-raising the new HTTPException from the caught exception (use "raise HTTPException(... ) from e"); specifically replace the bare raise of HTTPException(status_code=500, detail=str(e)) in the except Exception as e block (the block that logs via logger.exception("[archive] Error archiving workspace: {e}")) with a raise from e, and apply the same change to the analogous handler referenced at line 431 so the original exception context is retained.backend/app/tasks/archive_tasks.py (1)
61-67: Preserve exception chain when retrying.Static analysis correctly flags that
raise self.retry(exc=e)should use exception chaining to preserve the original traceback for debugging purposes.♻️ Proposed fix
except Exception as e: logger.error( f"[Celery Archive Cleanup] Error: task_id={task_id}, " f"retry={retry_count}/{self.max_retries}, error={e}", exc_info=True, ) - raise self.retry(exc=e) + raise self.retry(exc=e) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tasks/archive_tasks.py` around lines 61 - 67, The exception chain is lost when calling raise self.retry(exc=e); update the retry raise to preserve the original traceback by using exception chaining: replace the current raise self.retry(exc=e) with raise self.retry(exc=e) from e so the original exception `e` (from the except block handling in archive_tasks.py, referencing variables task_id, retry_count, self.max_retries and method self.retry) is linked to the retry exception.backend/app/services/execution/recovery_service.py (4)
52-59: Consider adding OpenTelemetry tracing.Per coding guidelines, async functions should use the
@trace_asyncdecorator for OpenTelemetry tracing. Therecover(),_recover_with_archive(),_recover_without_archive(), and_create_sandbox()methods would benefit from tracing to aid debugging and observability of recovery operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/recovery_service.py` around lines 52 - 59, Add OpenTelemetry tracing by decorating the async methods recover, _recover_with_archive, _recover_without_archive, and _create_sandbox with the `@trace_async` decorator; import trace_async at the top of the module and apply it immediately above each async def (e.g., `@trace_async` on the recover(self,...), async def _recover_with_archive(...), async def _recover_without_archive(...), and async def _create_sandbox(...)) so the recovery flow is instrumented for observability.
261-268: Add return type hint.Per coding guidelines, type hints are required. The method's return type should be explicitly annotated.
🔧 Suggested fix
+ from typing import Tuple + # At method signature: async def _create_sandbox( self, task: TaskResource, subtask: Subtask, user_id: int, user_name: str, skip_git_clone: bool, - ): + ) -> Tuple[Any, Optional[str]]:Note: Replace
Anywith the actualSandboxtype once identified from the sandbox manager module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/recovery_service.py` around lines 261 - 268, The method _create_sandbox is missing a return type annotation; add an explicit return type to its signature (e.g., -> Sandbox or -> Any until the actual Sandbox type from the sandbox manager module is imported) so it conforms to typing guidelines; update the function definition for _create_sandbox in recovery_service.py and import the correct Sandbox symbol (or use Any temporarily) to annotate the return type for the method that accepts TaskResource, Subtask, user_id, user_name, skip_git_clone.
158-159: Hardcoded executor namespace.The namespace is hardcoded to
"default"rather than being derived from the sandbox or configuration. This could cause issues in multi-namespace deployments. Consider extracting this to a configuration constant or obtaining it from the sandbox manager.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/recovery_service.py` around lines 158 - 159, The code sets executor_namespace = "default" directly which breaks multi-namespace deployments; change it to derive the namespace from the sandbox or config: fetch the namespace from the sandbox object (e.g., sandbox.namespace or sandbox.get_namespace()) or from a configuration constant/service used by RecoveryService, and assign that value to executor_namespace instead of the hardcoded string; update any callers that construct executor_name using sandbox.container_name to use the same derived namespace and add a fallback (config default) only if the sandbox value is missing.
78-82: Unusedstorage_keyvariable.The
storage_keyreturned fromcheck_archive_available()is unpacked but never used. Prefix with underscore to indicate intentional discard.🔧 Suggested fix
- archive_available, storage_key = archive_service.check_archive_available( + archive_available, _storage_key = archive_service.check_archive_available( task )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/recovery_service.py` around lines 78 - 82, The unpacked return value storage_key from archive_service.check_archive_available(task) is unused; change the unpack to archive_available, _ = archive_service.check_archive_available(task) (or rename storage_key to _storage_key) in recovery_service.py to indicate intentional discard and avoid unused-variable warnings, leaving archive_available used as before.backend/app/services/workspace_archive/archive_service.py (2)
58-65: Consider adding OpenTelemetry tracing.Per coding guidelines, async functions should use the
@trace_asyncdecorator. Thearchive_workspace(),restore_workspace(),_call_executor_archive(), and_call_executor_restore()methods would benefit from tracing for observability of archive operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/archive_service.py` around lines 58 - 65, Add OpenTelemetry async tracing by decorating the async methods archive_workspace, restore_workspace, _call_executor_archive, and _call_executor_restore with the `@trace_async` decorator; ensure you import trace_async from the project's tracing/telemetry utilities and place the decorator directly above each async def so the coroutine is traced for observability during archive/restore operations.
109-110:datetime.utcnow()is deprecated.
datetime.utcnow()is deprecated since Python 3.12. Usedatetime.now(timezone.utc)for timezone-aware UTC datetimes.🔧 Suggested fix
+from datetime import datetime, timezone # At line 109: - archivedAt=datetime.utcnow(), + archivedAt=datetime.now(timezone.utc),Apply the same change at lines 172 and 236.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/archive_service.py` around lines 109 - 110, Replace deprecated datetime.utcnow() with timezone-aware datetime.now(timezone.utc): update the archivedAt assignments (the places setting archivedAt= datetime.utcnow()) to archivedAt= datetime.now(timezone.utc), add/import timezone from datetime at the top of the module, and make the same replacement for the other occurrences of datetime.utcnow() in this module (the other archive-related methods where expiresAt or archivedAt are set, e.g., the other archive creation/metadata assignments that call archive_storage_service.calculate_expiration_time()) so all UTC datetimes are timezone-aware.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/execution/schedule_helper.py`:
- Around line 253-258: The new type annotations on _recover_executor reference
Subtask, TaskResource, and User which are only imported inside
_dispatch_task_async and thus undefined at module scope; to fix, move or add
those imports into a typing-only import block at the top of the module (use
"from typing import TYPE_CHECKING" and under "if TYPE_CHECKING:" import Subtask,
TaskResource, User) so the annotations resolve for tools like Ruff F821 while
avoiding runtime imports, and leave _dispatch_task_async's internal imports
unchanged.
In `@backend/app/services/workspace_archive/archive_service.py`:
- Around line 219-243: check_archive_available currently only inspects
Task.status.archive metadata (expiresAt/storageKey) but doesn't verify the
archive file exists in object storage, causing restore_workspace to later fail;
update check_archive_available to, after validating archive_info.storageKey and
expiry, call the same storage existence check used in restore_workspace (the
storage client method that checks the object by storageKey / bucket) and if the
object is missing return False, None and log the missing object with context;
keep the existing exception handling and return semantics so callers like
recovery_service get a reliable availability result.
In `@executor_manager/routers/routers.py`:
- Line 1320: The hardcoded 120s upstream timeout will abort legitimate
archive/restore requests up to max_size_mb (500), so replace the fixed 120s with
a configurable timeout and/or a size-scaled calculation: introduce a config
variable (e.g., ARCHIVE_TIMEOUT_SEC) and compute effective_timeout =
max(ARCHIVE_TIMEOUT_SEC, scale_by_size(requested_size_mb, max_size_mb)) or
derive timeout from requested archive size (with a sane upper bound), then use
that effective_timeout in the proxy/upstream calls currently capped at 120s in
the handlers referenced (the blocks around the 1320/1377-1384/1447-1454 ranges)
instead of the hardcoded literal; validate and clamp the value to prevent
unbounded timeouts and update any tests or callers that assumed 120s.
- Around line 1313-1329: The OTEL body capture currently leaks presigned URLs
from ArchiveExecutorRequest and RestoreExecutorRequest; update log_requests() so
it either skips body capture for the /executor/archive and /executor/restore
routes or redacts the sensitive fields before calling log_json_body():
specifically remove or replace ArchiveExecutorRequest.upload_url and
RestoreExecutorRequest.download_url (and any matching keys in the raw request
JSON) prior to span/log capture. Locate the middleware/logging function
log_requests() and add a conditional that checks the request.path (or the
handler) for "/executor/archive" and "/executor/restore" and performs the
redaction or skip before calling log_json_body().
In `@executor/agents/base.py`:
- Around line 298-314: The skip_git_clone branch (skip_git_clone check in the
function handling task workspace setup) can return without setting
self.project_path when task_data.git_url/git_repo are empty (e.g., sandbox
tasks); update the branch to ensure self.project_path is always set for restored
archives by preferring an explicit restored path field (e.g.,
task_data.restored_path or task_data.archive_path) and falling back to a
deterministic workspace path derived from config.get_workspace_root() and
self.task_id (optionally using git_util.get_repo_name_from_url(git_url) when
available); ensure the code sets self.project_path unconditionally before
returning so later code paths (ClaudeCode cwd switching and repo-scoped
instruction loading) can rely on project_path being present.
In `@executor/envd/api/routes.py`:
- Around line 398-408: The code uses tarfile.extractall(tmp_path, ...) which is
vulnerable to path traversal; replace the extractall call in the archive
extraction block (where tmp_path, workspace_path, session_restored, git_restored
and tar are in scope) with a safe extraction loop that iterates
tar.getmembers(), skips members with absolute paths or those containing '..',
resolves the destination path (e.g., workspace_path / member.name) and ensures
dest.resolve().is_relative_to(workspace_path.resolve()) before extracting, and
properly handle/sanitize symlinks and directories (use tar.extractfile for files
and create parents as needed) so no archive entry can write outside
workspace_path.
---
Nitpick comments:
In `@backend/app/services/adapters/executor_job.py`:
- Around line 160-166: The code re-extracts task_type from task JSON (via
Task.model_validate and metadata.labels) even though it was previously computed
and stored in executor_task_map, and the boolean-expression using "and/or" lacks
parentheses; update the logic in executor_job.py to reuse the existing task_type
from executor_task_map instead of calling Task.model_validate(task.json) again,
and if you must compute it here wrap the conditional expression in parentheses
or replace the and/or chain with an explicit conditional (e.g., labels and
labels.get("taskType") or "chat") to make precedence clear; reference symbols:
task_type, Task.model_validate, executor_task_map, and metadata.labels.
- Around line 134-152: The loop builds executor_task_map by re-querying
TaskResource for each Subtask in valid_candidates, which is redundant because
the TaskResource was already fetched during the earlier valid_candidates
filtering; change the initial filtering logic that builds valid_candidates to
collect pairs (subtask, task) instead of just subtask (e.g., valid_candidates:
List[Tuple[Subtask, TaskResource]]), update any type hints and the filtering
code that fetches TaskResource (the earlier block around where valid_candidates
is populated), and then modify the deduplication loop to iterate over (subtask,
task) and use the existing task to populate executor_task_map (avoid the second
db.query call and remove the TaskResource lookup inside the executor_task_map
population).
- Around line 236-261: Remove the call to asyncio.set_event_loop(loop) and
instead create a fresh event loop with asyncio.new_event_loop(), call
loop.run_until_complete(...) to run archive_service.archive_workspace(...), then
in the finally block call loop.run_until_complete(loop.shutdown_asyncgens()) and
loop.close() as before; update the block around
archive_service.archive_workspace, loop, run_until_complete, shutdown_asyncgens
to no longer set the loop as the thread default.
In `@backend/app/services/execution/__init__.py`:
- Around line 99-106: Extract the inner class SimpleSandbox out of the function
and define it at module level (preferably as a dataclass) so a single class
object is reused; replace the nested class definition with an import/use of the
module-level SimpleSandbox and keep the return as SimpleSandbox(data) or adapt
construction to the dataclass signature (e.g., SimpleSandbox(sandbox_id=...,
container_name=..., base_url=...)) to preserve behavior. Ensure the new
module-level symbol name remains SimpleSandbox so existing code that references
it continues to work.
- Around line 57-66: The create_sandbox function declares optional parameters
with default None but lacks explicit Optional type hints; update the signature
of create_sandbox to annotate timeout, workspace_ref, bot_config, and metadata
using typing.Optional[...] (or use the PEP 604 union syntax e.g. int | None, str
| None, dict | None if using Python 3.10+) so each parameter explicitly
indicates it may be None (keep existing parameter names: timeout, workspace_ref,
bot_config, metadata).
In `@backend/app/services/execution/recovery_service.py`:
- Around line 52-59: Add OpenTelemetry tracing by decorating the async methods
recover, _recover_with_archive, _recover_without_archive, and _create_sandbox
with the `@trace_async` decorator; import trace_async at the top of the module and
apply it immediately above each async def (e.g., `@trace_async` on the
recover(self,...), async def _recover_with_archive(...), async def
_recover_without_archive(...), and async def _create_sandbox(...)) so the
recovery flow is instrumented for observability.
- Around line 261-268: The method _create_sandbox is missing a return type
annotation; add an explicit return type to its signature (e.g., -> Sandbox or ->
Any until the actual Sandbox type from the sandbox manager module is imported)
so it conforms to typing guidelines; update the function definition for
_create_sandbox in recovery_service.py and import the correct Sandbox symbol (or
use Any temporarily) to annotate the return type for the method that accepts
TaskResource, Subtask, user_id, user_name, skip_git_clone.
- Around line 158-159: The code sets executor_namespace = "default" directly
which breaks multi-namespace deployments; change it to derive the namespace from
the sandbox or config: fetch the namespace from the sandbox object (e.g.,
sandbox.namespace or sandbox.get_namespace()) or from a configuration
constant/service used by RecoveryService, and assign that value to
executor_namespace instead of the hardcoded string; update any callers that
construct executor_name using sandbox.container_name to use the same derived
namespace and add a fallback (config default) only if the sandbox value is
missing.
- Around line 78-82: The unpacked return value storage_key from
archive_service.check_archive_available(task) is unused; change the unpack to
archive_available, _ = archive_service.check_archive_available(task) (or rename
storage_key to _storage_key) in recovery_service.py to indicate intentional
discard and avoid unused-variable warnings, leaving archive_available used as
before.
In `@backend/app/services/workspace_archive/__init__.py`:
- Around line 43-48: The __all__ export list in workspace_archive/__init__.py is
unsorted; please reorder the names alphabetically (ArchiveService,
ArchiveStorageService, archive_service, archive_storage_service) so the __all__
list follows isort-style consistency and deterministic ordering; update the
__all__ assignment to contain the sorted sequence.
In `@backend/app/services/workspace_archive/archive_service.py`:
- Around line 58-65: Add OpenTelemetry async tracing by decorating the async
methods archive_workspace, restore_workspace, _call_executor_archive, and
_call_executor_restore with the `@trace_async` decorator; ensure you import
trace_async from the project's tracing/telemetry utilities and place the
decorator directly above each async def so the coroutine is traced for
observability during archive/restore operations.
- Around line 109-110: Replace deprecated datetime.utcnow() with timezone-aware
datetime.now(timezone.utc): update the archivedAt assignments (the places
setting archivedAt= datetime.utcnow()) to archivedAt=
datetime.now(timezone.utc), add/import timezone from datetime at the top of the
module, and make the same replacement for the other occurrences of
datetime.utcnow() in this module (the other archive-related methods where
expiresAt or archivedAt are set, e.g., the other archive creation/metadata
assignments that call archive_storage_service.calculate_expiration_time()) so
all UTC datetimes are timezone-aware.
In `@backend/app/services/workspace_archive/cleanup_job.py`:
- Around line 46-55: The current query in cleanup_job.py loading all active
TaskResource rows via .all() can OOM for large datasets; change the logic in the
cleanup_job task to process active TaskResource rows in batches (e.g., using
limit/offset or SQLAlchemy yield_per) instead of .all(): perform repeated
queries filtering TaskResource.kind == "Task" and TaskResource.is_active ==
TaskResource.STATE_ACTIVE with a BATCH_SIZE (e.g., 500–1000), loop until the
returned batch is empty, and process/delete each batch before fetching the next
so memory usage stays bounded while keeping the existing TaskResource handling
code (the block that currently assigns to tasks) intact.
- Line 57: The code uses the deprecated datetime.utcnow() to set the variable
now in cleanup_job.py; replace it with a timezone-aware call (e.g.,
datetime.now(timezone.utc)) and ensure the datetime.timezone is imported or
referenced (add import for timezone if missing) so the variable now becomes
timezone-aware throughout the cleanup_job logic that relies on it.
- Around line 86-109: Duplicate logic that removes
task_json["status"]["archive"] and re-adds the task to the session should be
extracted into a helper (e.g., clear_task_archive_metadata(task, db)) and called
from both branches; implement clear_task_archive_metadata to load task.json,
check for "status" and "archive", delete the archive key, assign back to
task.json, and call db.add(task), then replace the duplicated blocks in
cleanup_job.py with a call to this helper while keeping the existing
cleaned_count increments and logger.info / logger.warning messages intact.
In `@backend/app/services/workspace_archive/storage.py`:
- Around line 187-193: The calculate_expiration_time method uses deprecated
datetime.utcnow(); update it to return a timezone-aware UTC datetime by using
datetime.now(timezone.utc) + timedelta(days=ARCHIVE_RETENTION_DAYS) (ensure
timezone is imported from datetime) so calculate_expiration_time and any callers
receive an aware UTC timestamp instead of a naive one.
In `@backend/app/tasks/archive_tasks.py`:
- Around line 61-67: The exception chain is lost when calling raise
self.retry(exc=e); update the retry raise to preserve the original traceback by
using exception chaining: replace the current raise self.retry(exc=e) with raise
self.retry(exc=e) from e so the original exception `e` (from the except block
handling in archive_tasks.py, referencing variables task_id, retry_count,
self.max_retries and method self.retry) is linked to the retry exception.
In `@executor_manager/routers/routers.py`:
- Around line 1332-1400: Wrap the async route handler archive_executor_workspace
with the `@trace_async` decorator (providing a descriptive span_name and
tracer_name and an extract_attributes callable) so the whole function is traced,
and inside the handler set span attributes for task_id and executor identifiers
using set_span_attribute (e.g., set_span_attribute("task_id", request.task_id)
and set_span_attribute("executor",
f"{request.executor_namespace}/{request.executor_name}")); apply the same change
to the other async recovery handler referenced at 1403-1471. Ensure imports for
trace_async and set_span_attribute are added and keep existing logic using
ExecutorDispatcher.get_executor and traced_async_client unchanged.
In `@executor/envd/api/routes.py`:
- Around line 315-316: The log call uses an unnecessary f-string with no
placeholders (logger.info(f"[archive] Uploading archive to MinIO")), which
static analysis flags; change it to a plain string literal
(logger.info("[archive] Uploading archive to MinIO"])) and do the same for the
other occurrence referenced around line 373 so neither logger.info uses an
extraneous f-prefix.
- Around line 345-349: In the exception handler in api/routes.py update the
generic catch block to preserve the original exception chain by re-raising the
new HTTPException from the caught exception (use "raise HTTPException(... ) from
e"); specifically replace the bare raise of HTTPException(status_code=500,
detail=str(e)) in the except Exception as e block (the block that logs via
logger.exception("[archive] Error archiving workspace: {e}")) with a raise from
e, and apply the same change to the analogous handler referenced at line 431 so
the original exception context is retained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e545d22-fe16-4cda-82af-0bd490438a65
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
backend/app/core/celery_app.pybackend/app/schemas/kind.pybackend/app/services/adapters/executor_job.pybackend/app/services/execution/__init__.pybackend/app/services/execution/recovery_service.pybackend/app/services/execution/schedule_helper.pybackend/app/services/workspace_archive/__init__.pybackend/app/services/workspace_archive/archive_service.pybackend/app/services/workspace_archive/cleanup_job.pybackend/app/services/workspace_archive/storage.pybackend/app/tasks/archive_tasks.pybackend/pyproject.tomlexecutor/agents/base.pyexecutor/envd/api/models.pyexecutor/envd/api/routes.pyexecutor_manager/routers/routers.pyexecutor_manager/services/sandbox/manager.pyshared/models/execution.py
Comment on lines
+253
to
+258
| async def _recover_executor( | ||
| db: Session, | ||
| subtask: "Subtask", | ||
| task: "TaskResource", | ||
| user: "User", | ||
| ) -> bool: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if command -v ruff >/dev/null 2>&1; then
ruff check backend/app/services/execution/schedule_helper.py --select F821
else
python - <<'PY'
from pathlib import Path
text = Path("backend/app/services/execution/schedule_helper.py").read_text()
print("has_future_annotations =", "from __future__ import annotations" in text)
print("has_TYPE_CHECKING_block =", "TYPE_CHECKING" in text)
for symbol in ("Subtask", "TaskResource", "User"):
print(symbol, "annotation_present =", f'"{symbol}"' in text)
PY
fiRepository: wecode-ai/Wegent
Length of output: 1374
🏁 Script executed:
head -50 backend/app/services/execution/schedule_helper.pyRepository: wecode-ai/Wegent
Length of output: 1497
🏁 Script executed:
rg "from __future__" backend/app/services/execution/schedule_helper.pyRepository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg "TYPE_CHECKING" backend/app/services/execution/schedule_helper.pyRepository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -A 20 "def _dispatch_task_async" backend/app/services/execution/schedule_helper.py | head -40Repository: wecode-ai/Wegent
Length of output: 808
🏁 Script executed:
rg "from.*Subtask|from.*TaskResource|from.*User" backend/app/services/execution/schedule_helper.pyRepository: wecode-ai/Wegent
Length of output: 200
🏁 Script executed:
grep -n "Subtask\|TaskResource\|User" backend/app/services/execution/schedule_helper.py | grep -E "^\s*[0-9]+:.*:\s*\"(Subtask|TaskResource|User)\""Repository: wecode-ai/Wegent
Length of output: 139
🏁 Script executed:
sed -n '250,270p' backend/app/services/execution/schedule_helper.pyRepository: wecode-ai/Wegent
Length of output: 594
🏁 Script executed:
grep -n "Subtask\|TaskResource\|User" backend/app/services/execution/schedule_helper.pyRepository: wecode-ai/Wegent
Length of output: 985
🏁 Script executed:
head -20 backend/app/services/execution/schedule_helper.pyRepository: wecode-ai/Wegent
Length of output: 594
🏁 Script executed:
rg "import TYPE_CHECKING|from typing import" backend/app/services/execution/schedule_helper.py | head -5Repository: wecode-ai/Wegent
Length of output: 82
These annotations will trip Ruff F821 in CI.
Subtask, TaskResource, and User are only imported inside _dispatch_task_async() at lines 110–114, so the new type annotations at lines 255–257 reference undefined symbols at module scope. Add these imports under a TYPE_CHECKING block at the top of the file.
🛠️ Minimal fix
import asyncio
import logging
from abc import ABC, abstractmethod
from concurrent.futures import ThreadPoolExecutor
-from typing import Any
+from typing import TYPE_CHECKING, Any
from sqlalchemy.orm import Session
from app.utils.prompt_utils import extract_display_prompt
logger = logging.getLogger(__name__)
+
+if TYPE_CHECKING:
+ from app.models.subtask import Subtask
+ from app.models.task import TaskResource
+ from shared.models.db import User🧰 Tools
🪛 Ruff (0.15.6)
[error] 255-255: Undefined name Subtask
(F821)
[error] 256-256: Undefined name TaskResource
(F821)
[error] 257-257: Undefined name User
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/execution/schedule_helper.py` around lines 253 - 258,
The new type annotations on _recover_executor reference Subtask, TaskResource,
and User which are only imported inside _dispatch_task_async and thus undefined
at module scope; to fix, move or add those imports into a typing-only import
block at the top of the module (use "from typing import TYPE_CHECKING" and under
"if TYPE_CHECKING:" import Subtask, TaskResource, User) so the annotations
resolve for tools like Ruff F821 while avoiding runtime imports, and leave
_dispatch_task_async's internal imports unchanged.
Comment on lines
+219
to
+243
| def check_archive_available(self, task: TaskResource) -> Tuple[bool, Optional[str]]: | ||
| """Check if archive is available for restoration. | ||
|
|
||
| Args: | ||
| task: Task resource | ||
|
|
||
| Returns: | ||
| Tuple of (available, storage_key or None) | ||
| """ | ||
| try: | ||
| task_crd = Task.model_validate(task.json) | ||
| archive_info = task_crd.status.archive if task_crd.status else None | ||
|
|
||
| if not archive_info or not archive_info.storageKey: | ||
| return False, None | ||
|
|
||
| # Check expiration | ||
| if archive_info.expiresAt and archive_info.expiresAt < datetime.utcnow(): | ||
| return False, None | ||
|
|
||
| return True, archive_info.storageKey | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"[ArchiveService] Error checking archive: {e}") | ||
| return False, None |
Contributor
There was a problem hiding this comment.
check_archive_available() doesn't verify storage existence.
This method checks metadata (expiration) but doesn't verify the archive file actually exists in MinIO, unlike restore_workspace() which does (line 180). This inconsistency can cause issues:
recovery_servicecallscheck_archive_available()→ returnsTrue- Sandbox created with
skip_git_clone=True restore_workspace()fails because file doesn't exist in storage- Result: Pod with empty workspace (no git clone, no restored files)
Consider adding the storage existence check here for consistency:
🔧 Suggested fix
def check_archive_available(self, task: TaskResource) -> Tuple[bool, Optional[str]]:
...
# Check expiration
if archive_info.expiresAt and archive_info.expiresAt < datetime.utcnow():
return False, None
+ # Check if archive file exists in storage
+ if not archive_storage_service.archive_exists(archive_info.storageKey):
+ return False, None
+
return True, archive_info.storageKey🧰 Tools
🪛 Ruff (0.15.6)
[warning] 241-241: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/workspace_archive/archive_service.py` around lines 219 -
243, check_archive_available currently only inspects Task.status.archive
metadata (expiresAt/storageKey) but doesn't verify the archive file exists in
object storage, causing restore_workspace to later fail; update
check_archive_available to, after validating archive_info.storageKey and expiry,
call the same storage existence check used in restore_workspace (the storage
client method that checks the object by storageKey / bucket) and if the object
is missing return False, None and log the missing object with context; keep the
existing exception handling and return semantics so callers like
recovery_service get a reliable availability result.
Comment on lines
+1313
to
+1329
| class ArchiveExecutorRequest(BaseModel): | ||
| """Request model for /executor/archive endpoint.""" | ||
|
|
||
| task_id: int | ||
| upload_url: str # Presigned MinIO upload URL | ||
| executor_name: str | ||
| executor_namespace: str | ||
| max_size_mb: int = 500 | ||
|
|
||
|
|
||
| class RestoreExecutorRequest(BaseModel): | ||
| """Request model for /executor/restore endpoint.""" | ||
|
|
||
| task_id: int | ||
| download_url: str # Presigned MinIO download URL | ||
| executor_name: str | ||
| executor_namespace: str |
Contributor
There was a problem hiding this comment.
Redact presigned URLs from request-body telemetry.
upload_url and download_url are bearer-style secrets. log_requests() in this module currently captures POST bodies into OTEL spans, so these new payloads will leak active MinIO credentials into tracing/log storage. Please exclude /executor/archive and /executor/restore from body capture or redact those keys before log_json_body() runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor_manager/routers/routers.py` around lines 1313 - 1329, The OTEL body
capture currently leaks presigned URLs from ArchiveExecutorRequest and
RestoreExecutorRequest; update log_requests() so it either skips body capture
for the /executor/archive and /executor/restore routes or redacts the sensitive
fields before calling log_json_body(): specifically remove or replace
ArchiveExecutorRequest.upload_url and RestoreExecutorRequest.download_url (and
any matching keys in the raw request JSON) prior to span/log capture. Locate the
middleware/logging function log_requests() and add a conditional that checks the
request.path (or the handler) for "/executor/archive" and "/executor/restore"
and performs the redaction or skip before calling log_json_body().
| upload_url: str # Presigned MinIO upload URL | ||
| executor_name: str | ||
| executor_namespace: str | ||
| max_size_mb: int = 500 |
Contributor
There was a problem hiding this comment.
The proxy timeout is shorter than a valid large archive/restore.
Both handlers cap the upstream call at 120 seconds while the request contract allows 500 MB archives. Compression/extraction plus a transfer of that size can legitimately exceed two minutes, so successful archive/restore jobs will be aborted by the proxy. Please make this timeout configurable or scale it with the requested archive size.
Also applies to: 1377-1384, 1447-1454
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor_manager/routers/routers.py` at line 1320, The hardcoded 120s
upstream timeout will abort legitimate archive/restore requests up to
max_size_mb (500), so replace the fixed 120s with a configurable timeout and/or
a size-scaled calculation: introduce a config variable (e.g.,
ARCHIVE_TIMEOUT_SEC) and compute effective_timeout = max(ARCHIVE_TIMEOUT_SEC,
scale_by_size(requested_size_mb, max_size_mb)) or derive timeout from requested
archive size (with a sane upper bound), then use that effective_timeout in the
proxy/upstream calls currently capped at 120s in the handlers referenced (the
blocks around the 1320/1377-1384/1447-1454 ranges) instead of the hardcoded
literal; validate and clamp the value to prevent unbounded timeouts and update
any tests or callers that assumed 120s.
Comment on lines
+298
to
+314
| # Check if git clone should be skipped (e.g., for workspace recovery from archive) | ||
| skip_git_clone = getattr(self.task_data, "skip_git_clone", False) | ||
| if skip_git_clone: | ||
| logger.info( | ||
| f"Agent[{self.get_name()}][{self.task_id}] skip_git_clone=True, " | ||
| "workspace will be restored from archive" | ||
| ) | ||
| # Still set project_path for later use | ||
| git_url = self.task_data.git_url or "" | ||
| if git_url: | ||
| repo_name = git_util.get_repo_name_from_url(git_url) | ||
| project_path = os.path.join( | ||
| config.get_workspace_root(), str(self.task_id), repo_name | ||
| ) | ||
| if self.project_path is None: | ||
| self.project_path = project_path | ||
| return |
Contributor
There was a problem hiding this comment.
skip_git_clone can still leave recovery without a repo working directory.
Sandbox tasks coming from SandboxManager._build_sandbox_task() still carry empty git_url / git_repo, so this branch usually exits with self.project_path unset. The ClaudeCode path only switches cwd and loads repo-scoped instructions when project_path exists, which means a restored archive can come back in the wrong working directory. Please pass an explicit restored path (or enough repo metadata to derive it) instead of relying on git_url here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor/agents/base.py` around lines 298 - 314, The skip_git_clone branch
(skip_git_clone check in the function handling task workspace setup) can return
without setting self.project_path when task_data.git_url/git_repo are empty
(e.g., sandbox tasks); update the branch to ensure self.project_path is always
set for restored archives by preferring an explicit restored path field (e.g.,
task_data.restored_path or task_data.archive_path) and falling back to a
deterministic workspace path derived from config.get_workspace_root() and
self.task_id (optionally using git_util.get_repo_name_from_url(git_url) when
available); ensure the code sets self.project_path unconditionally before
returning so later code paths (ClaudeCode cwd switching and repo-scoped
instruction loading) can rely on project_path being present.
Comment on lines
+272
to
+296
| def should_exclude(name: str) -> bool: | ||
| """Check if file/dir should be excluded.""" | ||
| for pattern in ARCHIVE_EXCLUDE_PATTERNS: | ||
| if pattern.startswith("*"): | ||
| if name.endswith(pattern[1:]): | ||
| return True | ||
| elif pattern in name.split(os.sep): | ||
| return True | ||
| return False | ||
|
|
||
| with tarfile.open(tmp_path, "w:gz") as tar: | ||
| for item in workspace_path.iterdir(): | ||
| if should_exclude(item.name): | ||
| logger.debug(f"[archive] Excluding: {item.name}") | ||
| continue | ||
|
|
||
| # Track session and git files | ||
| if item.name.startswith(".claude_session_id"): | ||
| session_file_included = True | ||
| if item.name == ".git": | ||
| git_included = True | ||
|
|
||
| # Add to archive | ||
| tar.add(str(item), arcname=item.name) | ||
| logger.debug(f"[archive] Added: {item.name}") |
Contributor
There was a problem hiding this comment.
Exclusion filter doesn't catch nested directories.
The should_exclude() function only checks the immediate item name being added via tar.add(). Since tar.add() recursively includes directory contents, nested excluded directories (e.g., packages/foo/node_modules/) won't be filtered out.
Consider using tar.add() with a filter function to exclude nested paths during recursive traversal.
♻️ Proposed fix using tar filter
+ def tar_filter(tarinfo: tarfile.TarInfo) -> tarfile.TarInfo | None:
+ """Filter function to exclude unwanted paths."""
+ # Check each path component
+ parts = tarinfo.name.split(os.sep)
+ for part in parts:
+ for pattern in ARCHIVE_EXCLUDE_PATTERNS:
+ if pattern.startswith("*"):
+ if part.endswith(pattern[1:]):
+ logger.debug(f"[archive] Excluding nested: {tarinfo.name}")
+ return None
+ elif part == pattern:
+ logger.debug(f"[archive] Excluding nested: {tarinfo.name}")
+ return None
+ return tarinfo
+
with tarfile.open(tmp_path, "w:gz") as tar:
for item in workspace_path.iterdir():
- if should_exclude(item.name):
- logger.debug(f"[archive] Excluding: {item.name}")
- continue
-
# Track session and git files
if item.name.startswith(".claude_session_id"):
session_file_included = True
if item.name == ".git":
git_included = True
# Add to archive
- tar.add(str(item), arcname=item.name)
+ tar.add(str(item), arcname=item.name, filter=tar_filter)
Comment on lines
+398
to
+408
| # Extract archive | ||
| with tarfile.open(tmp_path, "r:gz") as tar: | ||
| # Get member names for tracking | ||
| for member in tar.getnames(): | ||
| if member.startswith(".claude_session_id"): | ||
| session_restored = True | ||
| if member == ".git" or member.startswith(".git/"): | ||
| git_restored = True | ||
|
|
||
| # Extract all | ||
| tar.extractall(path=str(workspace_path)) |
Contributor
There was a problem hiding this comment.
Security: tarfile.extractall() is vulnerable to path traversal attacks.
Using extractall() without filtering members can allow malicious archives to write files outside the target directory via paths like ../../../etc/passwd. This is flagged as a security issue (S202).
Since this archive is created by the same system, the immediate risk is lower, but defense-in-depth is recommended.
🔒 Proposed fix with path validation
# Extract archive
with tarfile.open(tmp_path, "r:gz") as tar:
+ # Validate all members to prevent path traversal
+ def is_within_directory(directory, target):
+ abs_directory = os.path.abspath(directory)
+ abs_target = os.path.abspath(target)
+ prefix = os.path.commonprefix([abs_directory, abs_target])
+ return prefix == abs_directory
+
+ def safe_extract(tar, path):
+ for member in tar.getmembers():
+ member_path = os.path.join(path, member.name)
+ if not is_within_directory(path, member_path):
+ raise HTTPException(
+ status_code=400,
+ detail=f"Attempted path traversal in archive: {member.name}"
+ )
+ tar.extractall(path)
+
# Get member names for tracking
for member in tar.getnames():
if member.startswith(".claude_session_id"):
session_restored = True
if member == ".git" or member.startswith(".git/"):
git_restored = True
# Extract all
- tar.extractall(path=str(workspace_path))
+ safe_extract(tar, str(workspace_path))🧰 Tools
🪛 Ruff (0.15.6)
[error] 408-408: Uses of tarfile.extractall()
(S202)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor/envd/api/routes.py` around lines 398 - 408, The code uses
tarfile.extractall(tmp_path, ...) which is vulnerable to path traversal; replace
the extractall call in the archive extraction block (where tmp_path,
workspace_path, session_restored, git_restored and tar are in scope) with a safe
extraction loop that iterates tar.getmembers(), skips members with absolute
paths or those containing '..', resolves the destination path (e.g.,
workspace_path / member.name) and ensures
dest.resolve().is_relative_to(workspace_path.resolve()) before extracting, and
properly handle/sanitize symlinks and directories (use tar.extractfile for files
and create parents as needed) so no archive entry can write outside
workspace_path.
- Add created_at field to AdminDeviceInfo schema in backend - Add created_at to AdminDeviceInfo TypeScript interface - Display creation time in device list UI - Add i18n translations for created_at column (zh-CN and en) Co-authored-by: yunpeng7 <yunpeng7@staff.weibo.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
…wecode-ai#852) * fix(chat_shell): prevent tool call group splitting in group chat history truncation _truncate_history blindly sliced the flat message list, which could split assistant+tool_calls and their tool response messages across the truncation boundary. This produced orphaned function_call_output items without matching function_call entries in the OpenAI Responses API format, causing 'No tool call found for function call output' errors. Add _group_messages() to treat assistant+tool_calls+tool responses as atomic units, and modify _truncate_history() to truncate by groups instead of individual messages. * fix(chat_shell): ensure tool call group integrity across all compression paths Extract group_tool_call_messages() into shared messages/utils.py and apply atomic group handling to: - HistoryTruncationStrategy.compress(): adjust first/last boundaries to avoid splitting tool call groups, add post-removal orphan cleanup - _force_compression_to_target(): Step 2 removes middle groups instead of individual messages, Step 5 keeps first/last groups - Refactor loader.py to import shared utility instead of inline helper All 95 related tests pass. Comprehensive audit confirms no other code paths in chat_shell break tool call group integrity. * test(chat_shell): add tool-call group truncation regressions Add regression coverage for preserving assistant tool calls with their matching tool results across group-chat history truncation, history compression boundaries, and forced compression fallback paths.
* docs: update contributors list * docs: update contributors list (zh) * docs: update contributors list * docs: update contributors list (zh) * fix: 在里面智能体的创建人和机器人的创建人不是一个人时会报错 * fix: 迁移其他的sql到kindReader --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: hongyu9 <hongyu9@staff.weibo.com>
* docs: update contributors list * docs: update contributors list (zh) * docs: update contributors list * docs: update contributors list (zh) * fix: telemetry的匹配规则调整 * fix: 订阅任务执行间隔调整 * fix: 调整逻辑 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: hongyu9 <hongyu9@staff.weibo.com>
…-ai#859) PyInstaller's bootloader prepends its extraction directory (/tmp/_MEIxxxxxx) to LD_LIBRARY_PATH, causing child processes (Claude Code, shell commands) to load wrong shared libraries (e.g., libtinfo.so.6). Extract `sanitize_ld_library_path()` into platform_compat and apply it in both local_mode_strategy (Claude SDK subprocess) and extension_handler (extension script subprocess) to restore the original library path. See: https://pyinstaller.org/en/stable/common-issues-and-pitfalls.html Co-authored-by: sifang <sifang@staff.weibo.com>
…#847) * refactor(rag): move internal retrieve routing into backend * refactor(rag): move retrieve persistence into backend * refactor(rag): move kb head persistence into backend * fix(rag): tighten backend retrieve routing * refactor(rag): simplify token estimation fallback * refactor(rag): batch route token estimation * fix(rag): token estimator decimal * fix(rag): harden kb read persistence
…de-ai#858) * fix(frontend): show 'indexing' status for documents pending indexing When a document is uploaded, is_active is false while indexing is in progress. Previously, this showed 'index unavailable' which was misleading. Now documents with status='enabled' but is_active=false will show 'indexing' instead of 'index unavailable', matching the existing translation key. Changes: - Updated compact mode status indicator (line 247) - Updated normal mode status badge (line 461) Fixes: knowledge base document shows 'index unavailable' during indexing * fix(i18n): update Developer role description in Chinese locale
* docs: update contributors list * docs: update contributors list (zh) * docs: update contributors list * docs: update contributors list (zh) * fix: 修复订阅器中脏数据导致的无法展示的问题 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: hongyu9 <hongyu9@staff.weibo.com>
…ecode-ai#844) * feat(backend): add knowledge base RAG support to /v1/responses API Implement knowledge base Q&A functionality for the OpenAI-compatible /v1/responses endpoint with the following changes: Schema Changes: - Add knowledge_base_names field to WegentTool schema - Support namespace#name format for KB specification New Services: - KnowledgeBaseNameResolver: resolves KB display names to IDs with permission checking via TaskKnowledgeBaseService - KnowledgeBaseContextCreator: creates SubtaskContext records for API-specified KBs to integrate with existing RAG pipeline Integration: - Extend parse_wegent_tools() to handle knowledge_base tool type - Add parse_knowledge_base_name() for namespace#name parsing - Modify build_execution_request() to accept knowledge_base_names - Add _create_kb_contexts_from_api_request() for KB context injection - Update openapi_responses.py endpoints to pass KB names parameter Testing: - Add comprehensive unit tests for KB name resolution - Add tests for KB context creation - Add tests for helper functions with KB support - All 32 new tests passing The implementation reuses the existing RAG flow through prepare_contexts_for_chat() and KnowledgeBaseTool, ensuring consistency with the web UI behavior. * refactor(kb_resolver): optimize database query complexity from O(n) to O(m) - Add batch_get_knowledge_base_by_names() to TaskKnowledgeBaseService for efficient batch queries - Extract _check_kb_access_permission() for unified permission checking logic - Update kb_resolver to use service methods instead of direct queries - Reduce DB queries from O(n) to O(m) where m is unique namespace count - Update tests to mock service methods instead of internal functions All 15 tests pass. * refactor(backend): optimize KB permission check in kb_resolver Simplify permission checking by using get_all_knowledge_bases_grouped() which already returns all KBs accessible to the user. Changes: - Replace _batch_query_kbs() and _check_kb_access_permission() with _get_accessible_kb_lookup() that builds a lookup dict from grouped results - Remove unused resolve_single() method and batch_get_knowledge_base_by_names() - Update tests to mock KnowledgeService.get_all_knowledge_bases_grouped() This reduces database queries and simplifies the permission check logic. * feat(backend): integrate knowledge base context creation into unified AI trigger - Add knowledge_base_names parameter to build_execution_request for OpenAPI v1/responses - Create _create_kb_contexts_from_api_request helper to process KB names from API requests - Integrate KB context processing into unified trigger flow - Add KnowledgeBaseContextCreator class for creating SubtaskContext records - Add comprehensive unit tests for KnowledgeBaseContextCreator This enables RAG functionality for OpenAPI v1/responses endpoint by creating SubtaskContext records that are processed by the existing context pipeline. * fix(chat): propagate HTTPException from KnowledgeBaseNameResolver in unified trigger - Add HTTPException import from fastapi - Catch and re-raise HTTPException explicitly in _create_kb_contexts_from_api_request - Keep generic exception handling for non-HTTP exceptions to preserve best-effort behavior - This ensures 403/404 errors from KB resolution are properly propagated to callers instead of being swallowed and logged as warnings
…el_id (wecode-ai#861) - Extract shared detect_provider() into models/providers.py with PROVIDER_ALIASES as single source of truth - model_type (env.model) is authoritative; raise ValueError on unknown - Remove _PROVIDER_ID_PATTERNS model_id prefix fallback (same model_id can serve multiple protocols) - Unify truncation notice logic for all providers (no system-role notice in middle of messages, preventing Anthropic non-consecutive system message errors) - Pass model_type from model_config through MessageCompressor to TokenCounter for accurate provider detection - Remove duplicate provider constants from factory.py and token_counter.py Co-authored-by: wenbo17 <wenbo17@staff.weibo.com>
* docs: update contributors list * docs: update contributors list (zh) * docs: update contributors list * docs: update contributors list (zh) * fix: claude的子代理模式不可用修复 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: hongyu9 <hongyu9@staff.weibo.com>
…de-ai#851) * feat(inbox): implement work queue and message forwarding system Add a comprehensive message forwarding and work queue system that enables users to forward conversation messages to other users. Recipients can manage received messages through work queues and optionally configure AI agents for automatic processing. Backend changes: - Add QueueMessage and RecentContact database models - Add WorkQueue CRD schema stored in kinds table - Implement work queue services for CRUD operations - Implement message forwarding service with queue selection - Add WebSocket events for real-time notifications - Add API endpoints for work queue management - Add ForbiddenException to core exceptions Frontend changes: - Add inbox page with queue sidebar and message list - Implement ForwardMessageDialog for recipient selection - Add queue edit/create dialog with auto-processing config - Add message detail dialog with content preview - Add i18n translations (zh-CN and en) - Register inbox route and navigation * fix: resolve lint errors in inbox feature - Remove unused imports (RefreshCw, Settings, Menu, Button, Input, Clock) - Format Python files with Black * fix: resolve TypeScript errors in inbox feature - Add 'flow' and 'inbox' to TopNavigation activePage type - Replace Badge variant 'outline' with 'info' (outline not supported) * fix: address CodeRabbit review comments for work queue feature - Fix status parameter shadowing fastapi status module in work_queue.py - Fix NamespaceMember.namespace to use group_name column reference - Replace func.json_set with Python-level update for PostgreSQL compatibility - Fix refreshQueues dependency loop using functional state update - Remove Chinese text from inbox page comment - Add NaN validation for schedule interval input - Replace clickable Badge with accessible button for recent contacts --------- Co-authored-by: qdaxb <4157870+qdaxb@users.noreply.github.com>
* feat(inbox): implement work queue and message forwarding system Add a comprehensive message forwarding and work queue system that enables users to forward conversation messages to other users. Recipients can manage received messages through work queues and optionally configure AI agents for automatic processing. Backend changes: - Add QueueMessage and RecentContact database models - Add WorkQueue CRD schema stored in kinds table - Implement work queue services for CRUD operations - Implement message forwarding service with queue selection - Add WebSocket events for real-time notifications - Add API endpoints for work queue management - Add ForbiddenException to core exceptions Frontend changes: - Add inbox page with queue sidebar and message list - Implement ForwardMessageDialog for recipient selection - Add queue edit/create dialog with auto-processing config - Add message detail dialog with content preview - Add i18n translations (zh-CN and en) - Register inbox route and navigation * fix: resolve lint errors in inbox feature - Remove unused imports (RefreshCw, Settings, Menu, Button, Input, Clock) - Format Python files with Black * fix: resolve TypeScript errors in inbox feature - Add 'flow' and 'inbox' to TopNavigation activePage type - Replace Badge variant 'outline' with 'info' (outline not supported) * fix: address CodeRabbit review comments for work queue feature - Fix status parameter shadowing fastapi status module in work_queue.py - Fix NamespaceMember.namespace to use group_name column reference - Replace func.json_set with Python-level update for PostgreSQL compatibility - Fix refreshQueues dependency loop using functional state update - Remove Chinese text from inbox page comment - Add NaN validation for schedule interval input - Replace clickable Badge with accessible button for recent contacts * feat: update work queue --------- Co-authored-by: qdaxb <4157870+qdaxb@users.noreply.github.com>
* docs: update contributors list * docs: update contributors list (zh) * docs: update contributors list * docs: update contributors list (zh) * fix: 修复脏数据导致的订阅任务无法执行的问题 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: hongyu9 <hongyu9@staff.weibo.com>
Co-authored-by: sifang <sifang@staff.weibo.com>
* docs: update contributors list * docs: update contributors list (zh) * docs: update contributors list * docs: update contributors list (zh) * fix: 修复无法使用自定义镜像 * fix: 修复无法使用自定义镜像以及自定义镜像无法校验的问题 * fix: 修复可能的bug --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: hongyu9 <hongyu9@staff.weibo.com>
* docs: update contributors list * docs: update contributors list (zh) * docs: update contributors list * docs: update contributors list (zh) * fix: 修复单测 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: hongyu9 <hongyu9@staff.weibo.com>
* fix(knowledge): harden document indexing state * fix(knowledge): harden indexing state transitions * fix(knowledge): tighten reindex review followups * fix(rag): KNOWLEDGE_INDEX_LOCK_MAX_RETRIES=1 * fix(knowledge): store index status without db enum * fix(alembic): correct migration revision metadata
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* docs: update contributors list * docs: update contributors list (zh) * docs: update contributors list * docs: update contributors list (zh) * docs: add skill http identity design and plan * feat(skill): add http identity token flow * feat: 修改样式 * feat: 修改样式 * feat: 移除计划 * feat: 调整为带有认证的接口 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: hongyu9 <hongyu9@staff.weibo.com> # Conflicts: # executor_manager/services/sandbox/manager.py
* docs(backend): add knowledge document read spec and plan * feat(backend): add orchestrated knowledge document reads * fix(backend): tighten knowledge document read handling
…ire special handling (wecode-ai#884) * refactor(web-scraper): extract site-specific config to scraper_config.py - Add scraper_config.py for site-specific crawling configurations * fix(web_scraper): load WEB_SCRAPER_SITE_CONFIG from Pydantic settings instead of os.environ - Add WEB_SCRAPER_SITE_CONFIG to Settings class in config.py - Refactor scraper_config.py to use settings.WEB_SCRAPER_SITE_CONFIG with proper logging - Replace module-level config loading with cached get_site_config() function - Update scraper_service.py to use new get_site_config() function - Add detailed logging for site config matching and application - Fix remove_overlay_elements duplicate key in CrawlerRunConfig This ensures the configuration is properly loaded from .env file via Pydantic settings." * refactor(backend): improve scraper_config security and type safety - Remove sensitive config value from INFO level logging - Add return type annotations to _load_site_config and get_site_config - Replace broad Exception catch with specific exception types (AttributeError, KeyError, TypeError, ValueError) - Import Dict and Any from typing module
FicoHu
force-pushed
the
feat/workspace-archive-recovery
branch
from
April 1, 2026 11:01
7bd3b52 to
cf83463
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/init_data/skills/wegent-knowledge/SKILL.md`:
- Line 59: The SKILL.md entry for update_document_content is incorrect: it
claims "TEXT type documents only" but the implementation of
update_document_content in knowledge_service (the logic that accepts
source_type="file" for plain-text extensions like "txt", "md", "markdown") also
allows file-backed plain-text updates; update the doc to reflect that
update_document_content supports TEXT documents and file source_type for
plain-text extensions (list allowed extensions), and remove the misleading "TEXT
type documents only" phrase so examples and description match the actual
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: 5b395817-091e-4b23-88c0-1be34a9ebfd2
📒 Files selected for processing (2)
backend/init_data/skills/wegent-knowledge/SKILL.mdbackend/tests/mcp_server/test_tools.py
💤 Files with no reviewable changes (1)
- backend/tests/mcp_server/test_tools.py
| - returns: content slice, total_length, returned_length, has_more, kb_id | ||
|
|
||
| - **update_document_content**: Update a document's content for text documents and editable plain-text files | ||
| - **update_document_content**: Update a document's content (TEXT type documents only) |
Contributor
There was a problem hiding this comment.
Documentation contract is narrower than actual tool behavior
Line 59 currently says “TEXT type documents only,” but the implementation also allows source_type="file" for plain-text extensions (txt, md, markdown) in backend/app/services/knowledge/knowledge_service.py (Line 1341 onward). This mismatch will mislead agents and block valid updates.
✏️ Suggested doc fix
-- **update_document_content**: Update a document's content (TEXT type documents only)
+- **update_document_content**: Update a document's content (TEXT type documents and plain-text files: txt/md/markdown)Based on learnings: "Keep documentation examples minimal but complete and remove outdated information immediately."
📝 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.
Suggested change
| - **update_document_content**: Update a document's content (TEXT type documents only) | |
| - **update_document_content**: Update a document's content (TEXT type documents and plain-text files: txt/md/markdown) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/init_data/skills/wegent-knowledge/SKILL.md` at line 59, The SKILL.md
entry for update_document_content is incorrect: it claims "TEXT type documents
only" but the implementation of update_document_content in knowledge_service
(the logic that accepts source_type="file" for plain-text extensions like "txt",
"md", "markdown") also allows file-backed plain-text updates; update the doc to
reflect that update_document_content supports TEXT documents and file
source_type for plain-text extensions (list allowed extensions), and remove the
misleading "TEXT type documents only" phrase so examples and description match
the actual behavior.
FicoHu
force-pushed
the
feat/workspace-archive-recovery
branch
from
April 1, 2026 12:27
aab96b5 to
25298c2
Compare
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (3)
executor/envd/api/routes.py (1)
321-345:⚠️ Potential issue | 🟡 MinorRecursive excludes are still bypassed.
should_exclude()only evaluates each top-levelitem.name, buttar.add()then walks that subtree recursively. Nestednode_modules,.next,dist, and similar directories inside monorepos will still end up in the archive unless the exclusion check runs on every archived member path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/envd/api/routes.py` around lines 321 - 345, should_exclude currently only checks top-level item.name, but tar.add recurses into subpaths so nested excludes are bypassed; modify the code to run exclusion logic for every archived member by using tar.add's filter (a function receiving TarInfo) or by walking the tree manually. Update should_exclude to accept a relative path (e.g., "pkg/node_modules/foo" or TarInfo.name) and apply ARCHIVE_EXCLUDE_PATTERNS against the full path components and suffixes (preserve the existing "*" suffix handling), then call tar.add(str(item), arcname=item.name, filter=<filter_func>) where <filter_func> returns None for excluded members and the TarInfo for included ones; reference should_exclude, ARCHIVE_EXCLUDE_PATTERNS, and tar.add to locate the change.executor/agents/base.py (1)
298-314:⚠️ Potential issue | 🟠 MajorAlways initialize
self.project_pathon the recovery fast path.When
skip_git_clone=Trueand the request has nogit_url, this returns withself.project_pathstill unset. The ClaudeCode flow later uses that path for cwd/custom-instruction setup andsetup_coordinate_mode(...), so a restored workspace can come back without a usable repo root.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/agents/base.py` around lines 298 - 314, When skip_git_clone is true the fast path returns without setting self.project_path if task_data.git_url is empty; ensure self.project_path is always initialized so later code (cwd/custom-instruction setup and setup_coordinate_mode) has a valid repo root. In the skip_git_clone branch (where skip_git_clone = getattr(self.task_data, "skip_git_clone", False)), if git_url is falsy compute a fallback project_path = os.path.join(config.get_workspace_root(), str(self.task_id)) and assign it to self.project_path when self.project_path is None; keep the existing behavior of using repo_name when git_url exists (via git_util.get_repo_name_from_url) and do not change the early return.backend/app/services/execution/schedule_helper.py (1)
253-257:⚠️ Potential issue | 🟠 MajorAdd typing-only imports for these annotations.
Ruff is still flagging
Subtask,TaskResource, andUseras undefined here because those symbols are only imported inside_dispatch_task_async(). Pull them into aTYPE_CHECKINGblock at module scope so the annotations resolve cleanly in CI.Minimal fix
import asyncio import logging from abc import ABC, abstractmethod from concurrent.futures import ThreadPoolExecutor -from typing import Any +from typing import TYPE_CHECKING, Any from sqlalchemy.orm import Session from app.utils.prompt_utils import extract_display_prompt logger = logging.getLogger(__name__) + +if TYPE_CHECKING: + from app.models.subtask import Subtask + from app.models.task import TaskResource + from shared.models.db import User🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/schedule_helper.py` around lines 253 - 257, Ruff is flagging Subtask, TaskResource, and User as undefined in the _recover_executor function signature; add typing-only imports at module scope by importing TYPE_CHECKING from typing and, inside an if TYPE_CHECKING: block, import Subtask, TaskResource, and User from their defining modules (same modules used in _dispatch_task_async()) so the forward annotations resolve for static checks without affecting runtime.
🧹 Nitpick comments (6)
backend/app/schemas/kind.py (1)
538-581: Please split the archive/task schema additions out ofkind.py.
backend/app/schemas/kind.pyis already past the repo’s 1000-line limit, and adding more task-recovery models here keeps growing a catch-all schema module. MovingArchiveInfoand the related task-status additions into a dedicated submodule will keep future changes much easier to localize.As per coding guidelines, "File size limit: If a file exceeds 1000 lines, split it into multiple sub-modules."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/schemas/kind.py` around lines 538 - 581, Move the ArchiveInfo model and the task-related additions out of kind.py into a new schema module (e.g., create a new module named archive_schema or task_status_schema) and update kind.py to import the moved symbols; specifically, extract the ArchiveInfo class and the TaskStatus.fields related to archive (the archive: Optional[ArchiveInfo] Field and any new task fields you added) into the new module, preserve all Field() metadata, BaseModel/Pydantic imports, and the TaskApp reference (or import TaskApp if it remains defined elsewhere), then replace the original definitions in kind.py with from <new_module> import ArchiveInfo, TaskStatus and remove the duplicated code; run static/type checks to ensure no unresolved imports remain.backend/tests/services/execution/test_dispatcher_recovery.py (1)
95-98: Assert the recovered executor on the callback invocation.These assertions only prove
recover()mutatedrequest; they don't lock in that_dispatch_http_callback(...)actually received the recovered executor name and namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/services/execution/test_dispatcher_recovery.py` around lines 95 - 98, The test currently only checks that recovery_service.recover mutated request but not that the recovered values were passed into the HTTP callback; update the assertions to inspect the awaited call to _dispatch_http_callback (using dispatch_mock.await_args / dispatch_mock.call_args or equivalent on the async mock) and assert that the dispatched call's payload (or the arguments passed) contains executor_name == "recovered-executor" and executor_namespace == "default" so the test confirms the recovered executor was actually sent to _dispatch_http_callback.backend/app/services/execution/recovery_service.py (1)
78-82: Prefix unused variable with underscore.The
storage_keyvariable is unpacked but never used. Prefix it with underscore to indicate intentional non-use and silence the linter.♻️ Proposed fix
# Check if archive is available - archive_available, storage_key = archive_service.check_archive_available( + archive_available, _storage_key = archive_service.check_archive_available( task )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/recovery_service.py` around lines 78 - 82, The unpacked second value from archive_service.check_archive_available(task) is unused (storage_key); update the unpacking to prefix that variable with an underscore (e.g., _storage_key) to signal intentional non-use and satisfy the linter, leaving the call and the first variable (archive_available) unchanged.executor_manager/routers/routers.py (2)
1495-1502: Use exception chaining for proper traceback preservation.Same issue as the archive endpoint - exceptions should be chained for traceback preservation.
♻️ Proposed fix
except HTTPException: raise except httpx.HTTPError as e: logger.error(f"[Restore] HTTP error for task {request.task_id}: {e}") - raise HTTPException(status_code=500, detail=f"HTTP error: {str(e)}") + raise HTTPException(status_code=500, detail=f"HTTP error: {e!s}") from e except Exception as e: logger.error(f"[Restore] Error restoring task {request.task_id}: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail=str(e)) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor_manager/routers/routers.py` around lines 1495 - 1502, The except handlers for httpx.HTTPError and generic Exception in the restore endpoint are raising new HTTPException instances without chaining, which loses the original traceback; update both handlers (the except httpx.HTTPError as e and except Exception as e blocks around the "[Restore]" logic that logs request.task_id) to re-raise the HTTPException using exception chaining (i.e., raise HTTPException(status_code=500, detail=...) from e) so the original exception is preserved in the traceback while keeping the existing logger.error calls.
1424-1431: Use exception chaining for proper traceback preservation.Static analysis correctly identifies that exceptions should be chained using
raise ... from errto preserve the original traceback context, making debugging easier.♻️ Proposed fix
except HTTPException: raise except httpx.HTTPError as e: logger.error(f"[Archive] HTTP error for task {request.task_id}: {e}") - raise HTTPException(status_code=500, detail=f"HTTP error: {str(e)}") + raise HTTPException(status_code=500, detail=f"HTTP error: {e!s}") from e except Exception as e: logger.error(f"[Archive] Error archiving task {request.task_id}: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail=str(e)) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor_manager/routers/routers.py` around lines 1424 - 1431, The except handlers in the archive flow are re-raising new HTTPException instances without chaining, losing the original traceback; update the two handlers that catch httpx.HTTPError (referenced via variable e) and the generic Exception (also e) to use exception chaining by raising the new HTTPException from e (e.g., raise HTTPException(... ) from e) while keeping the existing except HTTPException: raise intact; update the logger usage unchanged.scripts/smoke-test-workspace-recovery.sh (1)
247-249: Manual step prevents CI automation.The script requires manual UI interaction to send a chat message. Consider documenting this limitation clearly or providing an alternative API-based trigger for CI environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke-test-workspace-recovery.sh` around lines 247 - 249, The manual read prompt blocks CI because it requires UI interaction to send a chat message for task ${TASK_ID}; replace this with an automated option or document the limitation: add a CI-aware flag or env var (e.g., AUTO_TRIGGER or CI_MODE) that, when set, makes the script perform an API POST to send a chat message for the given TASK_ID using configured credentials (token/env vars) instead of prompting, and fall back to the existing echo/read behavior when the flag is absent; update the script near the TASK_ID prompt to detect the flag/CI env var and call the API trigger function (or print a clear documented message explaining that manual intervention is required and how to run the script in CI mode).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/endpoints/internal/workspace_archives.py`:
- Around line 32-36: The async endpoint archive_task_workspace is not
instrumented with the OTel async tracer; wrap it with the `@trace_async` decorator
so request-level traces are captured (e.g.,
`@trace_async`("archive_task_workspace", "internal", extract_attributes=...) above
the function), import trace_async from your tracing utilities, and provide an
extract_attributes callable that returns relevant attributes such as {"task_id":
task_id} (use kwargs.get("task_id") or inspect args) to ensure the task_id is
recorded on the span.
In `@backend/app/core/celery_app.py`:
- Around line 77-82: The scheduled task definition "cleanup-expired-archives" in
celery_app.py uses a relative interval (schedule: 86400.0) which drifts on Beat
restarts; replace that value with a fixed crontab schedule by importing crontab
from celery.schedules and setting the task's schedule to crontab(hour=3,
minute=0) for the "app.tasks.archive_tasks.cleanup_expired_archives" task,
leaving options untouched so it consistently runs at 03:00 UTC.
In `@backend/app/services/adapters/executor_job.py`:
- Around line 168-184: The current code calls _archive_workspace_sync(...) for
code tasks but proceeds to delete the executor even if archiving failed or
raised an exception; change _archive_workspace_sync to return a clear status
(e.g., True on archive produced, False otherwise) or expose whether an archive
was created, then update the call sites (the try/except around
_archive_workspace_sync in executor_job.py and the similar block at the other
location referenced) to: call _archive_workspace_sync, capture its boolean
result, and only proceed with deletion when it returns True; on False or on
exception, log the failure with context (task.id, ns, name) and skip deletion so
the executor stays in place for retry later. Ensure both the call at the shown
block and the analogous block (lines ~240-258) use the same pattern and
reference the _archive_workspace_sync return value.
- Around line 237-261: Before calling asyncio.set_event_loop(loop) capture the
previous event loop (e.g., prev_loop = asyncio.get_event_loop()), and in the
finally block restore it with asyncio.set_event_loop(prev_loop) before shutting
down and closing the temporary loop used to run
archive_service.archive_workspace; update the block around the temporary loop
creation in executor_job (the variables loop and
archive_service.archive_workspace call) so the thread's original event loop is
reinstated to avoid leaving a closed loop as the current loop.
In `@backend/app/services/execution/__init__.py`:
- Around line 89-106: The code treats any 2xx response as success by
constructing SimpleSandbox from the response body; instead, inspect the response
payload for error indicators (e.g., a top-level "error" key or missing required
fields) after response.json() and treat those as failures: in the try block
where you call httpx.AsyncClient.post and build SimpleSandbox(data), check for
data.get("error") or absence of sandbox_id/container_name/base_url and return
(None, error) or raise an exception so the caller doesn't proceed to clear
executor_deleted_at; update the SimpleSandbox construction logic in this
function to only return a SimpleSandbox when the contract is satisfied.
In `@backend/app/services/execution/dispatcher.py`:
- Around line 421-462: The code currently reads Subtask.executor_deleted_at and
then awaits recovery_service.recover, allowing races where multiple dispatchers
observe the flag and each spawn a replacement; fix by claiming recovery
atomically before the await: in the dispatcher code that queries Subtask (use
SessionLocal and the Subtask model), acquire a row lock or do an atomic
conditional UPDATE that flips a claim column (e.g. set
executor_recovery_claimed=True) or clears executor_deleted_at only if it was
set, check affected rows/locked row to ensure this caller won the claim, commit
the claim, and only then call recovery_service.recover(db=..., subtask=...,
task=..., user_id=..., user_name=...); use SQLAlchemy’s with_for_update() or a
conditional UPDATE (WHERE id==request.subtask_id AND executor_deleted_at IS NOT
NULL AND executor_recovery_claimed IS FALSE) to guarantee a single claimant.
In `@backend/app/services/execution/recovery_service.py`:
- Around line 285-296: The code currently hardcodes shell_type="ClaudeCode" and
leaves bot_config empty; instead, pull the bot ID from subtask.bot_ids[0] (when
present), query the bot Kind/resource for that ID, call the existing
_resolve_shell_info (or equivalent resolver used by request_builder) to get the
shell_type from the bot's Shell reference, and populate bot_config with
shell_type plus any other relevant bot fields (e.g., bot id, kind, metadata) so
recovery uses the correct shell environment; update the variables used later
(shell_type and bot_config) accordingly and guard for missing bot_ids by falling
back to a sensible default.
In `@backend/app/services/workspace_archive/cleanup_job.py`:
- Around line 126-129: The except block currently logs the error and returns 0,
which suppresses the exception so Celery can't retry; instead, after logging and
calling db.rollback() you should re-raise the exception (remove the `return 0`)
so the failure propagates to the caller; specifically update the except that
wraps the `db.commit()` path (references to `logger.error` and `db.rollback`) to
call `db.rollback()` then `raise` so the task in archive cleanup will be
retriable.
- Around line 81-109: The delete_archive() implementation should not swallow all
S3 errors; update delete_archive in the archive storage service to follow the
same pattern as archive_exists(): catch S3Error (or the minio/moto client
exception used), if e.code == "NoSuchKey" return False (archive not present) but
for any other error re-raise the exception so transient auth/network/permission
failures are propagated; this ensures cleanup_job.py only clears metadata when
the object truly doesn't exist and avoids orphaning archives.
- Line 13: The code mixes naive and offset-aware datetimes causing TypeError;
import timezone from datetime and replace uses of datetime.utcnow()/naive now
with datetime.now(timezone.utc) (or otherwise make the expiry timestamps
timezone-aware) so comparisons between the expiry timestamp and generated "now"
use the same UTC-aware timezone; specifically update the import line that
currently imports datetime and change the creation of "now" (the code calling
datetime.utcnow() and any comparisons around the expiry check in cleanup_job.py)
to use datetime.now(timezone.utc) so it matches the tests that use
datetime.now(UTC).
In `@backend/app/tasks/archive_tasks.py`:
- Around line 20-26: The Celery task cleanup_expired_archives_task is missing
the required sync tracing decorator; wrap the function with the repo's
`@trace_sync` decorator (e.g., `@trace_sync`("cleanup_expired_archives",
"default_tracer", extract_attributes)) to trace the entire synchronous task;
ensure you import trace_sync from the tracing module used in the repo (e.g.,
app.tracing) and pass the appropriate span_name, tracer_name and
extract_attributes function consistent with other tasks so task-level telemetry
is captured.
In `@executor/envd/api/routes.py`:
- Around line 85-101: The current upload_archive_to_url and
download_archive_from_url implementations buffer entire archives into memory;
change both to stream in fixed-size chunks to/from disk instead: for
upload_archive_to_url, open the tmp_path file and use httpx.AsyncClient.stream
or pass an async iterator to client.put so you read the file in chunks (e.g.,
read chunks from tmp_path asynchronously) and stream them to the presigned URL
without building a bytes object; for download_archive_from_url, use
client.stream("GET", ...) and iterate response.aiter_bytes() (or aiter_raw()) to
write chunks directly to tmp_path on disk; apply the same streaming approach to
the other similar helpers in the file that currently use response.content or
f.read() so no full tarball is held in memory.
In `@executor/tests/test_envd_workspace_archive.py`:
- Around line 86-113: Before calling the restore endpoint, remove any existing
git metadata and session file so the test actually verifies restoration: add
code to delete workspace_path / ".git" (e.g., shutil.rmtree(workspace_path /
".git", ignore_errors=True)) and ensure (workspace_path /
".claude_session_id").unlink() is executed; after the POST to
client.post("/api/restore", ...) assert not only the boolean flags but also that
the restored files exist and contain the expected contents by reading
(workspace_path / ".git" / "HEAD") and verifying its content, reading
(workspace_path / ".claude_session_id") and verifying the session id content, as
well as the existing checks for (workspace_path / ".claude" /
"workspace-memory.md"), (home_path / ".claude" / "home-memory.md"), and
json.loads((home_path / ".claude.json").read_text(...)) to confirm the restore
actually recreated the .git and session artifacts.
In `@scripts/smoke-test-workspace-recovery.sh`:
- Around line 5-9: The ROOT_DIR variable is hardcoded to a local machine path;
change it to be derived from the script location instead so the script is
portable. Update the assignment to ROOT_DIR in
scripts/smoke-test-workspace-recovery.sh (the symbol ROOT_DIR) to compute the
project root relative to the script (e.g., using the script's directory via
dirname/BASH_SOURCE and resolving to an absolute path), and ensure subsequent
variables (BACKEND_DIR, ENV_FILE, BACKEND_LOG, EXECUTOR_MANAGER_LOG) continue to
reference ROOT_DIR unchanged.
---
Duplicate comments:
In `@backend/app/services/execution/schedule_helper.py`:
- Around line 253-257: Ruff is flagging Subtask, TaskResource, and User as
undefined in the _recover_executor function signature; add typing-only imports
at module scope by importing TYPE_CHECKING from typing and, inside an if
TYPE_CHECKING: block, import Subtask, TaskResource, and User from their defining
modules (same modules used in _dispatch_task_async()) so the forward annotations
resolve for static checks without affecting runtime.
In `@executor/agents/base.py`:
- Around line 298-314: When skip_git_clone is true the fast path returns without
setting self.project_path if task_data.git_url is empty; ensure
self.project_path is always initialized so later code (cwd/custom-instruction
setup and setup_coordinate_mode) has a valid repo root. In the skip_git_clone
branch (where skip_git_clone = getattr(self.task_data, "skip_git_clone",
False)), if git_url is falsy compute a fallback project_path =
os.path.join(config.get_workspace_root(), str(self.task_id)) and assign it to
self.project_path when self.project_path is None; keep the existing behavior of
using repo_name when git_url exists (via git_util.get_repo_name_from_url) and do
not change the early return.
In `@executor/envd/api/routes.py`:
- Around line 321-345: should_exclude currently only checks top-level item.name,
but tar.add recurses into subpaths so nested excludes are bypassed; modify the
code to run exclusion logic for every archived member by using tar.add's filter
(a function receiving TarInfo) or by walking the tree manually. Update
should_exclude to accept a relative path (e.g., "pkg/node_modules/foo" or
TarInfo.name) and apply ARCHIVE_EXCLUDE_PATTERNS against the full path
components and suffixes (preserve the existing "*" suffix handling), then call
tar.add(str(item), arcname=item.name, filter=<filter_func>) where <filter_func>
returns None for excluded members and the TarInfo for included ones; reference
should_exclude, ARCHIVE_EXCLUDE_PATTERNS, and tar.add to locate the change.
---
Nitpick comments:
In `@backend/app/schemas/kind.py`:
- Around line 538-581: Move the ArchiveInfo model and the task-related additions
out of kind.py into a new schema module (e.g., create a new module named
archive_schema or task_status_schema) and update kind.py to import the moved
symbols; specifically, extract the ArchiveInfo class and the TaskStatus.fields
related to archive (the archive: Optional[ArchiveInfo] Field and any new task
fields you added) into the new module, preserve all Field() metadata,
BaseModel/Pydantic imports, and the TaskApp reference (or import TaskApp if it
remains defined elsewhere), then replace the original definitions in kind.py
with from <new_module> import ArchiveInfo, TaskStatus and remove the duplicated
code; run static/type checks to ensure no unresolved imports remain.
In `@backend/app/services/execution/recovery_service.py`:
- Around line 78-82: The unpacked second value from
archive_service.check_archive_available(task) is unused (storage_key); update
the unpacking to prefix that variable with an underscore (e.g., _storage_key) to
signal intentional non-use and satisfy the linter, leaving the call and the
first variable (archive_available) unchanged.
In `@backend/tests/services/execution/test_dispatcher_recovery.py`:
- Around line 95-98: The test currently only checks that
recovery_service.recover mutated request but not that the recovered values were
passed into the HTTP callback; update the assertions to inspect the awaited call
to _dispatch_http_callback (using dispatch_mock.await_args /
dispatch_mock.call_args or equivalent on the async mock) and assert that the
dispatched call's payload (or the arguments passed) contains executor_name ==
"recovered-executor" and executor_namespace == "default" so the test confirms
the recovered executor was actually sent to _dispatch_http_callback.
In `@executor_manager/routers/routers.py`:
- Around line 1495-1502: The except handlers for httpx.HTTPError and generic
Exception in the restore endpoint are raising new HTTPException instances
without chaining, which loses the original traceback; update both handlers (the
except httpx.HTTPError as e and except Exception as e blocks around the
"[Restore]" logic that logs request.task_id) to re-raise the HTTPException using
exception chaining (i.e., raise HTTPException(status_code=500, detail=...) from
e) so the original exception is preserved in the traceback while keeping the
existing logger.error calls.
- Around line 1424-1431: The except handlers in the archive flow are re-raising
new HTTPException instances without chaining, losing the original traceback;
update the two handlers that catch httpx.HTTPError (referenced via variable e)
and the generic Exception (also e) to use exception chaining by raising the new
HTTPException from e (e.g., raise HTTPException(... ) from e) while keeping the
existing except HTTPException: raise intact; update the logger usage unchanged.
In `@scripts/smoke-test-workspace-recovery.sh`:
- Around line 247-249: The manual read prompt blocks CI because it requires UI
interaction to send a chat message for task ${TASK_ID}; replace this with an
automated option or document the limitation: add a CI-aware flag or env var
(e.g., AUTO_TRIGGER or CI_MODE) that, when set, makes the script perform an API
POST to send a chat message for the given TASK_ID using configured credentials
(token/env vars) instead of prompting, and fall back to the existing echo/read
behavior when the flag is absent; update the script near the TASK_ID prompt to
detect the flag/CI env var and call the API trigger function (or print a clear
documented message explaining that manual intervention is required and how to
run the script in CI mode).
🪄 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: 7eeb4759-70cd-43a2-9970-c8c447740d78
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
backend/app/api/api.pybackend/app/api/endpoints/internal/__init__.pybackend/app/api/endpoints/internal/workspace_archives.pybackend/app/core/celery_app.pybackend/app/schemas/kind.pybackend/app/services/adapters/executor_job.pybackend/app/services/chat/storage/task_manager.pybackend/app/services/execution/__init__.pybackend/app/services/execution/dispatcher.pybackend/app/services/execution/recovery_service.pybackend/app/services/execution/schedule_helper.pybackend/app/services/workspace_archive/__init__.pybackend/app/services/workspace_archive/archive_service.pybackend/app/services/workspace_archive/cleanup_job.pybackend/app/services/workspace_archive/storage.pybackend/app/tasks/archive_tasks.pybackend/pyproject.tomlbackend/tests/api/endpoints/internal/test_workspace_archives_api.pybackend/tests/services/chat/storage/test_task_manager.pybackend/tests/services/execution/test_dispatcher_recovery.pybackend/tests/services/execution/test_sandbox_manager_client.pyexecutor/agents/base.pyexecutor/envd/api/models.pyexecutor/envd/api/routes.pyexecutor/tests/test_envd_workspace_archive.pyexecutor_manager/routers/routers.pyexecutor_manager/services/sandbox/manager.pyscripts/smoke-test-workspace-recovery.shshared/models/execution.py
✅ Files skipped from review due to trivial changes (1)
- backend/pyproject.toml
Comment on lines
+32
to
+36
| @router.post("/{task_id}/archive", response_model=ManualArchiveResponse) | ||
| async def archive_task_workspace( | ||
| task_id: int, | ||
| db: Session = Depends(get_db), | ||
| ): |
Contributor
There was a problem hiding this comment.
Trace this async internal endpoint.
This new archive control-plane path is missing the repo's async OTel wrapper, so request-level visibility depends on logs only.
As per coding guidelines, "Use @trace_async(span_name, tracer_name, extract_attributes) decorator to trace entire async functions in OpenTelemetry".
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 35-35: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/endpoints/internal/workspace_archives.py` around lines 32 -
36, The async endpoint archive_task_workspace is not instrumented with the OTel
async tracer; wrap it with the `@trace_async` decorator so request-level traces
are captured (e.g., `@trace_async`("archive_task_workspace", "internal",
extract_attributes=...) above the function), import trace_async from your
tracing utilities, and provide an extract_attributes callable that returns
relevant attributes such as {"task_id": task_id} (use kwargs.get("task_id") or
inspect args) to ensure the task_id is recorded on the span.
Comment on lines
+77
to
+82
| # Cleanup expired workspace archives daily at 3:00 AM UTC | ||
| "cleanup-expired-archives": { | ||
| "task": "app.tasks.archive_tasks.cleanup_expired_archives", | ||
| "schedule": 86400.0, # 24 hours in seconds | ||
| "options": {"queue": settings.CELERY_TASK_DEFAULT_QUEUE}, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the current state of the file and its imports
head -100 backend/app/core/celery_app.py | cat -nRepository: wecode-ai/Wegent
Length of output: 4481
🏁 Script executed:
# Check if crontab is already imported
rg "from celery" backend/app/core/celery_app.py -A 2Repository: wecode-ai/Wegent
Length of output: 194
🏁 Script executed:
# Check for other Celery Beat schedules in the file to understand patterns
rg "schedule.*:" backend/app/core/celery_app.py -B 1 -A 2Repository: wecode-ai/Wegent
Length of output: 654
🌐 Web query:
Celery Beat schedule relative interval 86400 vs crontab fixed time execution
💡 Result:
In Celery Beat, 'relative interval 86400' refers to using an interval schedule of 86400 seconds (24 hours), while 'crontab fixed time execution' uses a crontab schedule for calendar-based timing like daily at a specific hour. Key differences: - Interval schedule (e.g., 'schedule': 86400.0): Executes every 86400 seconds after the previous execution (or Beat start for first run). Timing drifts relative to wall-clock time if tasks overrun or Beat restarts mid-cycle. Default behavior is non-relative: next run = last_run_at + 86400. - Crontab schedule (e.g., 'schedule': crontab(hour=0, minute=0)): Executes at fixed wall-clock times matching the cron pattern, like midnight every day. Ignores last_run_at; checks if current time matches the pattern. No drift; always calendar-aligned. Relative option (relative=True in ScheduleEntry or schedule): For timedelta/interval schedules only, rounds next run time to the interval's resolution (e.g., nearest day for ~86400s), making it more clock-aligned like "every 24 hours from epoch" rather than strictly from last run. Not applicable to crontab. 86400 interval approximates daily but drifts (e.g., restarts shift cycle), while crontab ensures exact daily fixed time. Examples: app.conf.beat_schedule = { 'daily-interval': { 'task': 'tasks.daily', 'schedule': 86400.0, # Every 24h from last run/Beat start }, 'daily-crontab': { 'task': 'tasks.daily', 'schedule': crontab(hour=0, minute=0), # Midnight daily } }
Citations:
- 1: https://celery.readthedocs.io/en/stable/reference/celery.beat.html
- 2: https://docs.celeryq.dev/en/3.0/userguide/periodic-tasks.html
- 3: https://docs.celeryq.dev/en/v5.5.0/userguide/periodic-tasks.html
- 4: https://docs.celeryq.dev/en/latest/userguide/periodic-tasks.html?highlight=periodic
- 5: https://docs.celeryq.dev/en/latest/_modules/celery/schedules.html
- 6: https://docs.celeryproject.org/en/latest/userguide/periodic-tasks.html?highlight=periodic
Replace relative 24-hour interval with crontab to ensure fixed execution time.
The 86400.0 schedule is a relative interval that drifts on Beat restarts. With current setup, the task will not consistently execute at 3:00 AM UTC as the comment states—it will shift each time Beat restarts.
Use crontab(hour=3, minute=0) for fixed wall-clock execution. Also add the import: from celery.schedules import crontab
Proposed fix
from celery import Celery
+from celery.schedules import crontab
from celery.signals import after_setup_logger, after_setup_task_logger
# Cleanup expired workspace archives daily at 3:00 AM UTC
"cleanup-expired-archives": {
"task": "app.tasks.archive_tasks.cleanup_expired_archives",
- "schedule": 86400.0, # 24 hours in seconds
+ "schedule": crontab(hour=3, minute=0),
"options": {"queue": settings.CELERY_TASK_DEFAULT_QUEUE},
},📝 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.
Suggested change
| # Cleanup expired workspace archives daily at 3:00 AM UTC | |
| "cleanup-expired-archives": { | |
| "task": "app.tasks.archive_tasks.cleanup_expired_archives", | |
| "schedule": 86400.0, # 24 hours in seconds | |
| "options": {"queue": settings.CELERY_TASK_DEFAULT_QUEUE}, | |
| }, | |
| from celery import Celery | |
| from celery.schedules import crontab | |
| from celery.signals import after_setup_logger, after_setup_task_logger |
Suggested change
| # Cleanup expired workspace archives daily at 3:00 AM UTC | |
| "cleanup-expired-archives": { | |
| "task": "app.tasks.archive_tasks.cleanup_expired_archives", | |
| "schedule": 86400.0, # 24 hours in seconds | |
| "options": {"queue": settings.CELERY_TASK_DEFAULT_QUEUE}, | |
| }, | |
| # Cleanup expired workspace archives daily at 3:00 AM UTC | |
| "cleanup-expired-archives": { | |
| "task": "app.tasks.archive_tasks.cleanup_expired_archives", | |
| "schedule": crontab(hour=3, minute=0), | |
| "options": {"queue": settings.CELERY_TASK_DEFAULT_QUEUE}, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/core/celery_app.py` around lines 77 - 82, The scheduled task
definition "cleanup-expired-archives" in celery_app.py uses a relative interval
(schedule: 86400.0) which drifts on Beat restarts; replace that value with a
fixed crontab schedule by importing crontab from celery.schedules and setting
the task's schedule to crontab(hour=3, minute=0) for the
"app.tasks.archive_tasks.cleanup_expired_archives" task, leaving options
untouched so it consistently runs at 03:00 UTC.
Comment on lines
+168
to
+184
| if task_type == "code": | ||
| # Try to archive workspace before deletion | ||
| try: | ||
| self._archive_workspace_sync( | ||
| db=db, | ||
| subtask=subtask, | ||
| task=task, | ||
| executor_name=name, | ||
| executor_namespace=ns, | ||
| ) | ||
| except Exception as archive_error: | ||
| # Log but continue with deletion | ||
| # Recovery will fall back to git clone if archive failed | ||
| logger.warning( | ||
| f"[executor_job] Failed to archive workspace for task {task.id} " | ||
| f"ns={ns} name={name}: {archive_error}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Do not delete code-task executors unless archiving actually succeeded.
_archive_workspace_sync() only logs success/skip/fail, and this call site still deletes the executor after archive exceptions. For code tasks, that can discard the only copy of uncommitted workspace state and session metadata; cleanup should leave the executor in place and retry later whenever no archive was produced.
Also applies to: 240-258
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 178-178: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/adapters/executor_job.py` around lines 168 - 184, The
current code calls _archive_workspace_sync(...) for code tasks but proceeds to
delete the executor even if archiving failed or raised an exception; change
_archive_workspace_sync to return a clear status (e.g., True on archive
produced, False otherwise) or expose whether an archive was created, then update
the call sites (the try/except around _archive_workspace_sync in executor_job.py
and the similar block at the other location referenced) to: call
_archive_workspace_sync, capture its boolean result, and only proceed with
deletion when it returns True; on False or on exception, log the failure with
context (task.id, ns, name) and skip deletion so the executor stays in place for
retry later. Ensure both the call at the shown block and the analogous block
(lines ~240-258) use the same pattern and reference the _archive_workspace_sync
return value.
Comment on lines
+237
to
+261
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) | ||
| try: | ||
| archive_info = loop.run_until_complete( | ||
| archive_service.archive_workspace( | ||
| db=db, | ||
| subtask=subtask, | ||
| task=task, | ||
| executor_name=executor_name, | ||
| executor_namespace=executor_namespace, | ||
| ) | ||
| ) | ||
|
|
||
| if archive_info: | ||
| logger.info( | ||
| f"[executor_job] Workspace archived for task {task.id}, " | ||
| f"size={archive_info.sizeBytes} bytes" | ||
| ) | ||
| else: | ||
| logger.info( | ||
| f"[executor_job] Workspace archiving skipped or failed for task {task.id}" | ||
| ) | ||
| finally: | ||
| loop.run_until_complete(loop.shutdown_asyncgens()) | ||
| loop.close() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 300 backend/app/services/adapters/executor_job.py | tail -n 100Repository: wecode-ai/Wegent
Length of output: 3985
🏁 Script executed:
cd backend && find . -name "*.py" -type f | xargs rg "class JobService" -A 10Repository: wecode-ai/Wegent
Length of output: 939
🏁 Script executed:
cd backend && rg "job_service\." -B 2 -A 2 | head -40Repository: wecode-ai/Wegent
Length of output: 1972
🏁 Script executed:
cd backend && cat -n app/services/jobs.py | head -80Repository: wecode-ai/Wegent
Length of output: 3029
🏁 Script executed:
cd backend && cat -n app/services/jobs.py | sed -n '75,150p'Repository: wecode-ai/Wegent
Length of output: 3379
🏁 Script executed:
cd backend && cat -n app/services/jobs.py | sed -n '140,160p'Repository: wecode-ai/Wegent
Length of output: 1038
🏁 Script executed:
cd backend && cat -n app/services/jobs.py | sed -n '159,180p'Repository: wecode-ai/Wegent
Length of output: 900
🏁 Script executed:
cd backend && sed -n '171,280p' app/services/adapters/executor_job.pyRepository: wecode-ai/Wegent
Length of output: 3788
🌐 Web query:
asyncio event loop management background threads Python best practices
💡 Result:
Best practices for asyncio event loop management, especially with background threads in Python: 1. Use high-level entry points: Prefer asyncio.run(coro) or asyncio.Runner for managing the event loop lifecycle. These handle creation, running, cleanup (including shutdown_default_executor and close), and should be used as the main entry point. Avoid manual loop management unless necessary for low-level code. 2. Inside coroutines, use asyncio.get_running_loop instead of deprecated asyncio.get_event_loop. Do not use get_event_loop in async code. 3. For blocking synchronous code (CPU/IO-bound): Offload to thread pool using: - Python 3.9+: asyncio.to_thread(func, *args) for IO-bound (uses default ThreadPoolExecutor). - Or loop.run_in_executor(None, func, *args) or with custom concurrent.futures.ThreadPoolExecutor for more control. Use ProcessPoolExecutor for CPU-bound to bypass GIL. - asyncio.run handles default executor shutdown automatically. Example: import asyncio import concurrent.futures def blocking_io: ... async def main: loop = asyncio.get_running_loop result = await loop.run_in_executor(None, blocking_io) asyncio.run(main) 4. Background threads interacting with asyncio: - Event loop runs in one thread (usually main). asyncio objects are not thread-safe. - From other threads: Use loop.call_soon_threadsafe(callback) for callbacks, asyncio.run_coroutine_threadsafe(coro, loop) to schedule coroutines (returns concurrent.futures.Future). - To run asyncio in background thread: Create new loop with asyncio.new_event_loop, set with asyncio.set_event_loop(loop), then loop.run_forever. Submit tasks via thread-safe methods from main loop thread. Example for per-thread loops: import threading def thread_worker: loop = asyncio.new_event_loop asyncio.set_event_loop(loop) loop.run_forever # Start threads, submit from main via run_coroutine_threadsafe 5. Policies (deprecated in 3.14+, removal 3.16): Avoid set_event_loop_policy. Use loop_factory in asyncio.run(loop_factory=...) for custom loops (e.g., uvloop). 6. Multithreading best practices: - One event loop per thread. - Main thread for signals/subprocesses. - Enable debug mode (asyncio.run(debug=True)) for thread-safety checks, slow callbacks detection. - Close loops explicitly if not using asyncio.run: Call shutdown_asyncgens then close. - No direct coroutine scheduling from other processes (use multiprocessing separately). 7. General: - Avoid blocking the loop: Never run sync blocking code directly in coroutines. - Use create_task(coro) for concurrent async tasks, asyncio.gather for waiting. - For fire-and-forget: Track tasks in set to prevent GC. - Production: Consider uvloop for performance. These practices ensure responsiveness, thread-safety, and proper resource management per official Python 3.14+ docs.
Citations:
- 1: https://docs.python.org/3/library/asyncio-eventloop.html
- 2: https://docs.python.org/library/asyncio-runner.html
- 3: https://docs.python.org/3/library/asyncio-dev.html
- 4: https://docs.python.org/3/library/asyncio-task.html
- 5: https://runebook.dev/en/docs/python/library/asyncio-eventloop/asyncio.loop.set_default_executor
- 6: http://docs.python.org/library/asyncio-dev.html
- 7: https://stackoverflow.com/questions/59199218/when-you-need-to-use-asyncio-and-threadpoolexecutor-do-you-need-to-call-loop-cl
- 8: https://docs.python.org/3/library/asyncio-policy.html
- 9: https://oneuptime.com/blog/post/2026-01-30-how-to-build-asyncio-event-loops-in-python/view
- 10: https://docs.python.org/3.11/library/asyncio-eventloop.html
🏁 Script executed:
cd backend && python3 << 'EOF'
# Simulate the proposed fix behavior in a background thread context
import asyncio
def simulate_worker_thread_context():
"""Simulate what happens in cleanup_worker thread"""
print("1. Starting in background thread (no event loop set)")
# Try the proposed fix logic
try:
previous_loop = asyncio.get_event_loop_policy().get_event_loop()
print(f"2. Got previous loop: {previous_loop}")
except RuntimeError as e:
previous_loop = None
print(f"2. RuntimeError caught (expected in worker thread): {e}")
print(" Set previous_loop = None")
# Create new loop (as in _archive_workspace_sync)
loop = asyncio.new_event_loop()
asyncio.set_event_loop(loop)
print(f"3. Created and set new loop: {loop}")
# Use and close it
loop.close()
print("4. Closed new loop")
# Try to get event loop after closure
try:
current = asyncio.get_event_loop()
print(f"5. After closure, get_event_loop() returned: {current}")
if current.is_closed():
print(" Loop is closed!")
except RuntimeError as e:
print(f"5. After closure, RuntimeError: {e}")
# Restoration attempt (as in proposed fix)
asyncio.set_event_loop(previous_loop)
print(f"6. Restored previous_loop: {previous_loop}")
simulate_worker_thread_context()
EOFRepository: wecode-ai/Wegent
Length of output: 528
🏁 Script executed:
cd backend && sed -n '75,95p' app/services/jobs.pyRepository: wecode-ai/Wegent
Length of output: 768
Restore the prior event loop after closing it in finally.
asyncio.set_event_loop(loop) makes this temporary loop current for the worker thread, but the finally block closes it without restoring the prior loop. Although the current code path contains only synchronous code after, this leaves a closed loop as the thread's current event loop, creating a latent bug if any async code is added later or if this method is reused elsewhere.
Proposed fix
- loop = asyncio.new_event_loop()
- asyncio.set_event_loop(loop)
+ try:
+ previous_loop = asyncio.get_event_loop_policy().get_event_loop()
+ except RuntimeError:
+ previous_loop = None
+
+ loop = asyncio.new_event_loop()
+ asyncio.set_event_loop(loop)
try:
archive_info = loop.run_until_complete(
archive_service.archive_workspace(
db=db,
subtask=subtask,
@@
finally:
loop.run_until_complete(loop.shutdown_asyncgens())
loop.close()
+ asyncio.set_event_loop(previous_loop)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/adapters/executor_job.py` around lines 237 - 261, Before
calling asyncio.set_event_loop(loop) capture the previous event loop (e.g.,
prev_loop = asyncio.get_event_loop()), and in the finally block restore it with
asyncio.set_event_loop(prev_loop) before shutting down and closing the temporary
loop used to run archive_service.archive_workspace; update the block around the
temporary loop creation in executor_job (the variables loop and
archive_service.archive_workspace call) so the thread's original event loop is
reinstated to avoid leaving a closed loop as the current loop.
Comment on lines
+89
to
+106
| try: | ||
| async with httpx.AsyncClient(timeout=180.0) as client: | ||
| response = await client.post( | ||
| url, | ||
| json=payload, | ||
| headers={"Content-Type": "application/json"}, | ||
| ) | ||
| response.raise_for_status() | ||
| data = response.json() | ||
|
|
||
| # Create a simple sandbox object | ||
| class SimpleSandbox: | ||
| def __init__(self, data): | ||
| self.sandbox_id = data.get("sandbox_id") | ||
| self.container_name = data.get("container_name") | ||
| self.base_url = data.get("base_url") | ||
|
|
||
| return SimpleSandbox(data), None |
Contributor
There was a problem hiding this comment.
Treat executor-manager partial-failure responses as failures.
The executor-manager router can return a 200 response with an error when the sandbox record exists but container startup failed. This client currently wraps any 2xx body into SimpleSandbox, so recovery can proceed and clear executor_deleted_at even though the new executor never became ready.
Suggested guard for the response contract
response.raise_for_status()
data = response.json()
+
+ if data.get("error"):
+ return None, data["error"]
+ if not data.get("base_url"):
+ return None, "Sandbox creation did not reach running state"
# Create a simple sandbox object
class SimpleSandbox:📝 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.
Suggested change
| try: | |
| async with httpx.AsyncClient(timeout=180.0) as client: | |
| response = await client.post( | |
| url, | |
| json=payload, | |
| headers={"Content-Type": "application/json"}, | |
| ) | |
| response.raise_for_status() | |
| data = response.json() | |
| # Create a simple sandbox object | |
| class SimpleSandbox: | |
| def __init__(self, data): | |
| self.sandbox_id = data.get("sandbox_id") | |
| self.container_name = data.get("container_name") | |
| self.base_url = data.get("base_url") | |
| return SimpleSandbox(data), None | |
| try: | |
| async with httpx.AsyncClient(timeout=180.0) as client: | |
| response = await client.post( | |
| url, | |
| json=payload, | |
| headers={"Content-Type": "application/json"}, | |
| ) | |
| response.raise_for_status() | |
| data = response.json() | |
| if data.get("error"): | |
| return None, data["error"] | |
| if not data.get("base_url"): | |
| return None, "Sandbox creation did not reach running state" | |
| # Create a simple sandbox object | |
| class SimpleSandbox: | |
| def __init__(self, data): | |
| self.sandbox_id = data.get("sandbox_id") | |
| self.container_name = data.get("container_name") | |
| self.base_url = data.get("base_url") | |
| return SimpleSandbox(data), None |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/execution/__init__.py` around lines 89 - 106, The code
treats any 2xx response as success by constructing SimpleSandbox from the
response body; instead, inspect the response payload for error indicators (e.g.,
a top-level "error" key or missing required fields) after response.json() and
treat those as failures: in the try block where you call httpx.AsyncClient.post
and build SimpleSandbox(data), check for data.get("error") or absence of
sandbox_id/container_name/base_url and return (None, error) or raise an
exception so the caller doesn't proceed to clear executor_deleted_at; update the
SimpleSandbox construction logic in this function to only return a SimpleSandbox
when the contract is satisfied.
Comment on lines
+126
to
+129
| except Exception as e: | ||
| logger.error(f"[ArchiveCleanup] Error during cleanup: {e}", exc_info=True) | ||
| db.rollback() | ||
| return 0 |
Contributor
There was a problem hiding this comment.
Let the task fail here so Celery can retry.
backend/app/tasks/archive_tasks.py, Lines 61-67 only retries when this function raises. Returning 0 here turns a db.commit() failure at Line 119 into a successful run, so stale archive metadata can sit around until the next daily schedule.
🔁 Propagate the failure after rollback
except Exception as e:
logger.error(f"[ArchiveCleanup] Error during cleanup: {e}", exc_info=True)
db.rollback()
- return 0
+ raise📝 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.
Suggested change
| except Exception as e: | |
| logger.error(f"[ArchiveCleanup] Error during cleanup: {e}", exc_info=True) | |
| db.rollback() | |
| return 0 | |
| except Exception as e: | |
| logger.error(f"[ArchiveCleanup] Error during cleanup: {e}", exc_info=True) | |
| db.rollback() | |
| raise |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/workspace_archive/cleanup_job.py` around lines 126 -
129, The except block currently logs the error and returns 0, which suppresses
the exception so Celery can't retry; instead, after logging and calling
db.rollback() you should re-raise the exception (remove the `return 0`) so the
failure propagates to the caller; specifically update the except that wraps the
`db.commit()` path (references to `logger.error` and `db.rollback`) to call
`db.rollback()` then `raise` so the task in archive cleanup will be retriable.
Comment on lines
+20
to
+26
| @celery_app.task( | ||
| bind=True, | ||
| name="app.tasks.archive_tasks.cleanup_expired_archives", | ||
| max_retries=1, | ||
| default_retry_delay=300, | ||
| ) | ||
| def cleanup_expired_archives_task(self): |
Contributor
There was a problem hiding this comment.
Add the required sync tracing to this Celery task.
This is the daily archive-cleanup entry point, but it isn't wrapped with the repo's sync OTel decorator yet, so task-level visibility stops at the logs.
As per coding guidelines, "Use @trace_sync(span_name, tracer_name, extract_attributes) decorator to trace entire sync functions in OpenTelemetry".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/tasks/archive_tasks.py` around lines 20 - 26, The Celery task
cleanup_expired_archives_task is missing the required sync tracing decorator;
wrap the function with the repo's `@trace_sync` decorator (e.g.,
`@trace_sync`("cleanup_expired_archives", "default_tracer", extract_attributes))
to trace the entire synchronous task; ensure you import trace_sync from the
tracing module used in the repo (e.g., app.tracing) and pass the appropriate
span_name, tracer_name and extract_attributes function consistent with other
tasks so task-level telemetry is captured.
Comment on lines
+85
to
+101
| async def upload_archive_to_url(upload_url: str, content: bytes) -> None: | ||
| """Upload archive bytes to object storage using presigned URL.""" | ||
| async with httpx.AsyncClient(timeout=120.0) as client: | ||
| response = await client.put( | ||
| upload_url, | ||
| content=content, | ||
| headers={"Content-Type": "application/gzip"}, | ||
| ) | ||
| response.raise_for_status() | ||
|
|
||
|
|
||
| async def download_archive_from_url(download_url: str) -> bytes: | ||
| """Download archive bytes from object storage using presigned URL.""" | ||
| async with httpx.AsyncClient(timeout=120.0) as client: | ||
| response = await client.get(download_url) | ||
| response.raise_for_status() | ||
| return response.content |
Contributor
There was a problem hiding this comment.
Archive transfer paths buffer the whole tarball in RAM.
Both directions currently materialize the entire archive as bytes (f.read() on upload and response.content on restore) before forwarding it. For large workspaces, that can spike memory enough to OOM the executor pod. Stream upload/download directly from and to tmp_path in chunks instead.
Also applies to: 393-395, 451-454
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor/envd/api/routes.py` around lines 85 - 101, The current
upload_archive_to_url and download_archive_from_url implementations buffer
entire archives into memory; change both to stream in fixed-size chunks to/from
disk instead: for upload_archive_to_url, open the tmp_path file and use
httpx.AsyncClient.stream or pass an async iterator to client.put so you read the
file in chunks (e.g., read chunks from tmp_path asynchronously) and stream them
to the presigned URL without building a bytes object; for
download_archive_from_url, use client.stream("GET", ...) and iterate
response.aiter_bytes() (or aiter_raw()) to write chunks directly to tmp_path on
disk; apply the same streaming approach to the other similar helpers in the file
that currently use response.content or f.read() so no full tarball is held in
memory.
Comment on lines
+86
to
+113
| shutil.rmtree(workspace_path / ".claude") | ||
| (workspace_path / ".claude_session_id").unlink() | ||
| shutil.rmtree(home_path / ".claude") | ||
| (home_path / ".claude.json").unlink() | ||
|
|
||
| restore_response = await client.post( | ||
| "/api/restore", | ||
| json={ | ||
| "task_id": task_id, | ||
| "download_url": upload_url, | ||
| }, | ||
| ) | ||
|
|
||
| assert restore_response.status_code == 200 | ||
| restore_payload = restore_response.json() | ||
| assert restore_payload["success"] is True | ||
| assert restore_payload["session_restored"] is True | ||
| assert restore_payload["git_restored"] is True | ||
|
|
||
| assert (workspace_path / ".claude" / "workspace-memory.md").read_text( | ||
| encoding="utf-8" | ||
| ) == "workspace-context" | ||
| assert (home_path / ".claude" / "home-memory.md").read_text( | ||
| encoding="utf-8" | ||
| ) == "home-context" | ||
| assert json.loads((home_path / ".claude.json").read_text(encoding="utf-8")) == { | ||
| "theme": "dark" | ||
| } |
Contributor
There was a problem hiding this comment.
Remove .git before restore and assert the restored session/git files.
Right now the test never deletes .git, and it only checks a response flag for the session file. A restore regression for .git/HEAD or .claude_session_id would still leave this test green.
🧪 Tighten the restore assertions
shutil.rmtree(workspace_path / ".claude")
(workspace_path / ".claude_session_id").unlink()
+ shutil.rmtree(workspace_path / ".git")
shutil.rmtree(home_path / ".claude")
(home_path / ".claude.json").unlink() assert (workspace_path / ".claude" / "workspace-memory.md").read_text(
encoding="utf-8"
) == "workspace-context"
+ assert (workspace_path / ".claude_session_id").read_text(
+ encoding="utf-8"
+ ) == "session-id"
+ assert (workspace_path / ".git" / "HEAD").read_text(
+ encoding="utf-8"
+ ) == "ref: refs/heads/main"📝 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.
Suggested change
| shutil.rmtree(workspace_path / ".claude") | |
| (workspace_path / ".claude_session_id").unlink() | |
| shutil.rmtree(home_path / ".claude") | |
| (home_path / ".claude.json").unlink() | |
| restore_response = await client.post( | |
| "/api/restore", | |
| json={ | |
| "task_id": task_id, | |
| "download_url": upload_url, | |
| }, | |
| ) | |
| assert restore_response.status_code == 200 | |
| restore_payload = restore_response.json() | |
| assert restore_payload["success"] is True | |
| assert restore_payload["session_restored"] is True | |
| assert restore_payload["git_restored"] is True | |
| assert (workspace_path / ".claude" / "workspace-memory.md").read_text( | |
| encoding="utf-8" | |
| ) == "workspace-context" | |
| assert (home_path / ".claude" / "home-memory.md").read_text( | |
| encoding="utf-8" | |
| ) == "home-context" | |
| assert json.loads((home_path / ".claude.json").read_text(encoding="utf-8")) == { | |
| "theme": "dark" | |
| } | |
| shutil.rmtree(workspace_path / ".claude") | |
| (workspace_path / ".claude_session_id").unlink() | |
| shutil.rmtree(workspace_path / ".git") | |
| shutil.rmtree(home_path / ".claude") | |
| (home_path / ".claude.json").unlink() | |
| restore_response = await client.post( | |
| "/api/restore", | |
| json={ | |
| "task_id": task_id, | |
| "download_url": upload_url, | |
| }, | |
| ) | |
| assert restore_response.status_code == 200 | |
| restore_payload = restore_response.json() | |
| assert restore_payload["success"] is True | |
| assert restore_payload["session_restored"] is True | |
| assert restore_payload["git_restored"] is True | |
| assert (workspace_path / ".claude" / "workspace-memory.md").read_text( | |
| encoding="utf-8" | |
| ) == "workspace-context" | |
| assert (workspace_path / ".claude_session_id").read_text( | |
| encoding="utf-8" | |
| ) == "session-id" | |
| assert (workspace_path / ".git" / "HEAD").read_text( | |
| encoding="utf-8" | |
| ) == "ref: refs/heads/main" | |
| assert (home_path / ".claude" / "home-memory.md").read_text( | |
| encoding="utf-8" | |
| ) == "home-context" | |
| assert json.loads((home_path / ".claude.json").read_text(encoding="utf-8")) == { | |
| "theme": "dark" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor/tests/test_envd_workspace_archive.py` around lines 86 - 113, Before
calling the restore endpoint, remove any existing git metadata and session file
so the test actually verifies restoration: add code to delete workspace_path /
".git" (e.g., shutil.rmtree(workspace_path / ".git", ignore_errors=True)) and
ensure (workspace_path / ".claude_session_id").unlink() is executed; after the
POST to client.post("/api/restore", ...) assert not only the boolean flags but
also that the restored files exist and contain the expected contents by reading
(workspace_path / ".git" / "HEAD") and verifying its content, reading
(workspace_path / ".claude_session_id") and verifying the session id content, as
well as the existing checks for (workspace_path / ".claude" /
"workspace-memory.md"), (home_path / ".claude" / "home-memory.md"), and
json.loads((home_path / ".claude.json").read_text(...)) to confirm the restore
actually recreated the .git and session artifacts.
Remove automatic MinIO archive cleanup (Celery Beat task) and instead block task execution when workspace archive has expired (>30 days). Users will receive a clear error message and must create a new task. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
executor/agents/base.py (1)
298-314:⚠️ Potential issue | 🟠 MajorAlways set
project_pathin the recovery path.When
skip_git_clone=Trueandgit_urlis empty, this branch returns withself.project_pathstill unset. That's the normal sandbox-recovery payload fromSandboxManager, so Claude Code will not switch cwd or load repo-scoped instructions after the archive is restored. Set a deterministic fallback like<workspace_root>/<task_id>before returning.Based on learnings, "ClaudeCode agent MUST support Claude Code SDK, Git clone, Skills, MCP servers, and custom instructions (.cursorrules, .windsurfrules)".♻️ Suggested fix
if skip_git_clone: logger.info( f"Agent[{self.get_name()}][{self.task_id}] skip_git_clone=True, " "workspace will be restored from archive" ) - # Still set project_path for later use - git_url = self.task_data.git_url or "" - if git_url: - repo_name = git_util.get_repo_name_from_url(git_url) - project_path = os.path.join( - config.get_workspace_root(), str(self.task_id), repo_name - ) - if self.project_path is None: - self.project_path = project_path + if self.project_path is None: + workspace_path = os.path.join( + config.get_workspace_root(), str(self.task_id) + ) + git_url = self.task_data.git_url or "" + self.project_path = ( + os.path.join( + workspace_path, + git_util.get_repo_name_from_url(git_url), + ) + if git_url + else workspace_path + ) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/agents/base.py` around lines 298 - 314, When skip_git_clone is true but task_data.git_url is empty the method returns without setting self.project_path; update the skip_git_clone branch (check involving skip_git_clone, self.task_data, and git_url) to compute a deterministic fallback project_path = os.path.join(config.get_workspace_root(), str(self.task_id)) and assign it to self.project_path when self.project_path is None (instead of only setting project_path when git_url exists); keep using get_repo_name_from_url when git_url is present and still return after ensuring self.project_path is set.executor/envd/api/routes.py (3)
69-82:⚠️ Potential issue | 🟠 MajorFail closed when tar extraction filters are unavailable.
Lines 79-82 drop back to raw
tar.extractall()whenever the runtime does not supportfilter="data". That reopens path-traversal and symlink extraction on older executor images, so this helper is only safe on some Python versions. Please require a filter-capable runtime or replace the fallback with a manual safe extractor instead of calling unfilteredextractall().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/envd/api/routes.py` around lines 69 - 82, The helper extract_tar_members currently falls back to tar.extractall(**extract_kwargs) when tar.extractall(filter="data") raises TypeError, which is unsafe; replace that fallback by implementing a manual safe extraction inside extract_tar_members: iterate over members (use the provided members or tar.getmembers()), validate each TarInfo.name to forbid absolute paths and path traversal (no leading '/' and no '..' components), handle symlinks securely by either skipping them or resolving/creating only safe targets, create directories as needed, and write regular file contents via tar.extractfile into files with correct mode/uid/gid handling; do not call tar.extractall without a filter and ensure extract_tar_members always enforces these checks instead of requiring a newer Python runtime.
85-101:⚠️ Potential issue | 🟠 MajorDon't materialize the whole archive as
bytes.The helper contract forces both archive upload and restore download to hold the entire tarball in RAM. With the 500 MB limit used upstream, a single transfer can spike memory enough to OOM the executor pod. Stream from and to
tmp_pathin chunks instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/envd/api/routes.py` around lines 85 - 101, The current upload_archive_to_url and download_archive_from_url materialize the whole archive as bytes; change them to stream in chunks to/from disk instead: for download_archive_from_url, open a target tmp_path file and use httpx.AsyncClient.stream("GET", download_url) to iterate response.aiter_bytes(chunk_size) and write chunks to tmp_path without returning bytes; for upload_archive_to_url, read from the tmp_path file in chunks (or provide an async iterator over chunks) and use httpx.AsyncClient.put (or stream/async iterator as the request body) to upload without loading the entire file into memory. Update signatures to accept a Path/str tmp_path target/source and ensure response.raise_for_status() is still checked.
321-344:⚠️ Potential issue | 🟠 MajorThe exclude list still misses nested repo artifacts.
should_exclude()only sees the top-levelitem.name, buttar.add()then recurses into the cloned repo directory under/workspace/<task_id>/<repo_name>. That means innernode_modules,dist,.venv, etc. still get archived for normal code tasks, which will bloat archives and trip the size limit unexpectedly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/envd/api/routes.py` around lines 321 - 344, should_exclude currently checks only top-level item.name so nested artifacts are still archived; fix by applying exclusion logic to all files added (not just top-level) — either replace the loop+tar.add call with tar.add using a filter function that calls should_exclude on the tarinfo.name (or join path parts) or change the walk to recurse and call should_exclude on each relative path component; update references to should_exclude, tar.add and workspace_path.iterdir() so the filter inspects the full member path (tarinfo.name) against ARCHIVE_EXCLUDE_PATTERNS to exclude nested node_modules, dist, .venv, etc. before adding to the tar.executor/tests/test_envd_workspace_archive.py (1)
24-36:⚠️ Potential issue | 🟠 MajorMake the
.gitfixture match production and prove it was restored.This test keeps
.gitat the workspace root, never deletes it before restore, and never reads backHEADor.claude_session_id. That leaves both the real repo layout and the actual restore behavior unverified, so a regression can still pass green. Please move.gitunder a repo subdirectory, remove it beforePOST /api/restore, and assert the restored file contents afterward.Also applies to: 86-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/tests/test_envd_workspace_archive.py` around lines 24 - 36, The test currently leaves .git at the workspace root and doesn't verify restored contents; change the fixture to create the git repo under a subdirectory (e.g., create workspace_path / "repo" / ".git" and write HEAD there), then delete that repo/.git and .claude_session_id before invoking the restore endpoint (the code that calls client.post("/api/restore") in this test). After the POST, assert that the repo/.git/HEAD, .claude_session_id, and .claude/workspace-memory.md have been recreated with the expected contents (use the existing workspace_path, tmp_path, and task_id names to locate files).executor_manager/routers/routers.py (2)
1344-1360:⚠️ Potential issue | 🟠 MajorRedact presigned URLs from request-body telemetry.
upload_urlanddownload_urlare bearer-style secrets, andlog_requests()still captures POST bodies into OTEL spans. Adding these request models without a redaction/skip path will leak active MinIO credentials into tracing storage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor_manager/routers/routers.py` around lines 1344 - 1360, The ArchiveExecutorRequest and RestoreExecutorRequest models currently include bearer presigned URLs (upload_url, download_url) that will be captured by log_requests(); change those fields to a secret type (e.g., pydantic.SecretStr) so they are redacted by default and/or mark them with repr=False/Field(..., exclude=True) to prevent inclusion in model.dict()/repr(), and update the log_requests() telemetry serialization to explicitly exclude "upload_url" and "download_url" when serializing request bodies (or check for SecretStr and avoid calling get_secret_value()). Ensure you modify ArchiveExecutorRequest, RestoreExecutorRequest, and the log_requests() serializer logic so the presigned URLs are never recorded in OTEL spans.
1351-1351:⚠️ Potential issue | 🟠 MajorThe proxy timeout is shorter than the advertised archive budget.
max_size_mbdefaults to 500, but both proxy calls still hard-codetimeout=120.0. Compression/extraction plus transfer of a large workspace can exceed two minutes, so valid archive/restore requests will be aborted by the manager.Also applies to: 1408-1414, 1478-1484
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor_manager/routers/routers.py` at line 1351, The hard-coded proxy timeouts (timeout=120.0) will abort valid archive/restore operations when max_size_mb is large; update the proxy call sites that currently pass timeout=120.0 to compute a dynamic timeout based on the max_size_mb value (the variable max_size_mb) instead of a fixed 120s — e.g., derive timeout_seconds from max_size_mb (such as timeout_seconds = max(120.0, max_size_mb * X + Y) or a similar heuristic) and replace timeout=120.0 in the proxy.post/proxy.request calls so archive/restore operations use the computed timeout; make this change for all occurrences that pass timeout=120.0 near the archive/restore proxy calls.backend/app/services/workspace_archive/archive_service.py (1)
221-249:⚠️ Potential issue | 🟠 Major
check_archive_available()should verify storage existence.This method checks metadata (expiration) but doesn't verify the archive file actually exists in MinIO, unlike
restore_workspace()which does (line 182). This inconsistency can cause issues:
recovery_servicecallscheck_archive_available()→ returnsTrue- Sandbox created with
skip_git_clone=Truerestore_workspace()fails because file doesn't exist in storage- Result: Pod with empty workspace (no git clone, no restored files)
🔧 Suggested fix
def check_archive_available( self, task: TaskResource ) -> Tuple[bool, Optional[str], Optional[str]]: ... # Check expiration if archive_info.expiresAt and archive_info.expiresAt < datetime.utcnow(): return False, None, "expired" + # Check if archive file exists in storage + if not archive_storage_service.archive_exists(archive_info.storageKey): + logger.warning( + f"[ArchiveService] Archive metadata exists but file missing in storage: " + f"task={task.id}, key={archive_info.storageKey}" + ) + return False, None, None + return True, archive_info.storageKey, None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/archive_service.py` around lines 221 - 249, check_archive_available currently only validates metadata (expiresAt/storageKey) and must also verify the archive file exists in object storage like restore_workspace does; update check_archive_available to use the same storage client call used in restore_workspace to check object existence for archive_info.storageKey (e.g., call the MinIO/StorageClient method that restore_workspace uses), and return (False, None, "missing" or None) when the object is not present; keep the existing expiration check and logging behavior and reference archive_info.storageKey and the check_archive_available and restore_workspace functions to ensure consistent behavior.scripts/smoke-test-workspace-recovery.sh (1)
5-9:⚠️ Potential issue | 🟠 MajorHardcoded absolute path breaks portability.
ROOT_DIRis hardcoded to a developer's local machine path. This script will fail on any other machine or CI environment.🛠️ Proposed fix - derive ROOT_DIR from script location
-ROOT_DIR="/Users/yunpeng7/AIGCWorkSpace/Wegent" +# Derive ROOT_DIR from script location +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +ROOT_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" BACKEND_DIR="${ROOT_DIR}/backend" ENV_FILE="${ROOT_DIR}/.env" BACKEND_LOG="${ROOT_DIR}/.pids/backend.log" EXECUTOR_MANAGER_LOG="${ROOT_DIR}/.pids/executor_manager.log"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke-test-workspace-recovery.sh` around lines 5 - 9, The script currently hardcodes ROOT_DIR which breaks portability; change ROOT_DIR to be derived from the script location instead (e.g., compute the script directory via BASH_SOURCE/dirname and resolve the repo root with cd/realpath) and then recompute BACKEND_DIR, ENV_FILE, BACKEND_LOG and EXECUTOR_MANAGER_LOG from that ROOT_DIR; ensure the derived value works when the script is invoked via symlink or from another CWD by resolving to an absolute path and keep the existing variable names (ROOT_DIR, BACKEND_DIR, ENV_FILE, BACKEND_LOG, EXECUTOR_MANAGER_LOG) so the rest of the script is unchanged.backend/app/services/execution/recovery_service.py (1)
299-303:⚠️ Potential issue | 🟠 MajorExtract shell_type from bot configuration instead of hardcoding.
The
shell_typeis hardcoded to"ClaudeCode"andbot_configis left empty. For proper recovery, these should be derived from the subtask's bot configuration to ensure the recovered sandbox uses the correct shell environment.Consider querying the bot Kind resource using
subtask.bot_ids[0](if available) to resolve the correct shell_type, similar to howrequest_builderhandles this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/recovery_service.py` around lines 299 - 303, The code currently hardcodes shell_type = "ClaudeCode" and leaves bot_config = {} in recovery logic; change it to extract bot configuration from the subtask (use subtask.bot_ids[0] when present) and resolve the Kind/bot record the same way request_builder does to derive the correct shell_type and populate bot_config; update the recovery routine to query the bot Kind resource (or service/method used by request_builder) and fall back to a sensible default only if no bot_id or Kind is found.
🧹 Nitpick comments (10)
backend/app/services/execution/__init__.py (1)
57-66: Use explicitOptionalor union syntax for nullable parameters.PEP 484 prohibits implicit
Optional. The parameters with= Nonedefaults should explicitly declare nullability.Suggested fix
async def create_sandbox( self, shell_type: str, user_id: int, user_name: str, - timeout: int = None, - workspace_ref: str = None, - bot_config: dict = None, - metadata: dict = None, + timeout: int | None = None, + workspace_ref: str | None = None, + bot_config: dict | None = None, + metadata: dict | None = None, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/__init__.py` around lines 57 - 66, The create_sandbox signature uses default None for nullable parameters but lacks explicit Optional types; update the type hints for timeout, workspace_ref, bot_config, and metadata to use typing.Optional (or Union[..., None]) and add the necessary import (from typing import Optional) so the signature becomes e.g. timeout: Optional[int], workspace_ref: Optional[str], bot_config: Optional[dict], metadata: Optional[dict] while keeping defaults as = None and leaving other names (create_sandbox, shell_type, user_id, user_name) unchanged.backend/app/services/adapters/executor_job.py (1)
160-166: Clarify operator precedence fortaskTypeextraction.The expression
task_crd.metadata.labels and task_crd.metadata.labels.get("taskType") or "chat"has ambiguous precedence. IftaskTypeis set to an empty string"", this will incorrectly return"chat"instead of"".Suggested fix using explicit parentheses or ternary
- task_type = ( - task_crd.metadata.labels - and task_crd.metadata.labels.get("taskType") - or "chat" - ) + labels = task_crd.metadata.labels + task_type = labels.get("taskType", "chat") if labels else "chat"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/adapters/executor_job.py` around lines 160 - 166, The current taskType extraction uses a chained boolean expression that treats empty strings as falsy; update the logic in the Task.model_validate block (referencing task_crd and task_crd.metadata.labels) to use an explicit conditional so an empty string is preserved: e.g. if metadata.labels exists then fetch labels.get("taskType") and if that value is None use "chat" (or use a ternary expression) instead of relying on "and ... or ...".backend/app/services/execution/recovery_service.py (3)
275-328: Add return type annotation to_create_sandbox.The method is missing a return type annotation. Based on the implementation, it returns
Tuple[Optional[Sandbox], Optional[str]].async def _create_sandbox( self, task: TaskResource, subtask: Subtask, user_id: int, user_name: str, skip_git_clone: bool, - ): + ) -> tuple[Any, Optional[str]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/recovery_service.py` around lines 275 - 328, The _create_sandbox method is missing a return type annotation; update its signature on the _create_sandbox method to declare the return type as Tuple[Optional[Sandbox], Optional[str]] (or from typing import Tuple, Optional) so it reads something like def _create_sandbox(...) -> Tuple[Optional[Sandbox], Optional[str]]; ensure Sandbox is imported or referenced where needed and keep the existing parameters (task: TaskResource, subtask: Subtask, user_id: int, user_name: str, skip_git_clone: bool) unchanged.
78-82: Prefix unused variable with underscore.The
storage_keyvariable is unpacked but never used. Prefix it with an underscore to indicate it's intentionally ignored.- archive_available, storage_key, reason = ( + archive_available, _storage_key, reason = ( archive_service.check_archive_available(task) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/recovery_service.py` around lines 78 - 82, The unpacking of archive_service.check_archive_available(task) yields storage_key which is unused; change the unpacking in the try block where archive_available, storage_key, reason = archive_service.check_archive_available(task) to use an ignored-name by prefixing storage_key with an underscore (e.g., archive_available, _storage_key, reason or simply _,) so it signals the variable is intentionally unused; update the single occurrence in the recovery_service.py try block that calls archive_service.check_archive_available to avoid unused-variable warnings.
172-173: Consider makingexecutor_namespaceconfigurable instead of hardcoding "default".The
executor_namespaceis hardcoded to"default"in both recovery paths. If the system supports multiple namespaces, this could lead to incorrect namespace assignment for recovered executors.Consider retrieving the namespace from configuration or from the original subtask's executor namespace (if it was set before deletion).
- executor_namespace = "default" + executor_namespace = subtask.executor_namespace or "default"Also applies to: 251-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/execution/recovery_service.py` around lines 172 - 173, executor_namespace is hardcoded to "default" in recovery_service.py (near where executor_name = sandbox.container_name) which breaks multi-namespace setups; change both occurrences to derive the namespace instead: try to read the original executor namespace from the subtask (e.g., subtask.executor_namespace or subtask.metadata["executor_namespace"]) and fall back to a configurable default (e.g., settings.DEFAULT_EXECUTOR_NAMESPACE or a injected config value) if absent, and update the assignments that set executor_namespace to use that lookup so recovered executors get the correct namespace.backend/app/services/workspace_archive/archive_service.py (2)
111-112: Usedatetime.now(UTC)instead of deprecateddatetime.utcnow().
datetime.utcnow()is deprecated since Python 3.12. Use timezone-awaredatetime.now(UTC)for consistency and future compatibility.♻️ Proposed fix
+from datetime import UTC, datetime -from datetime import datetime ... # Line 111 - archivedAt=datetime.utcnow(), + archivedAt=datetime.now(UTC), ... # Line 174 - if archive_info.expiresAt and archive_info.expiresAt < datetime.utcnow(): + if archive_info.expiresAt and archive_info.expiresAt < datetime.now(UTC):Also applies to: 174-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/archive_service.py` around lines 111 - 112, Replace the deprecated datetime.utcnow() used when setting archivedAt with a timezone-aware call: use datetime.now(timezone.utc) (or datetime.now(UTC) if UTC is imported from zoneinfo) and add the corresponding import (e.g., from datetime import datetime, timezone) so archivedAt becomes timezone-aware; apply the same change to the second occurrence at the other assignment (the block that sets archivedAt and expiresAt near archive_storage_service.calculate_expiration_time()).
241-242: Samedatetime.utcnow()deprecation issue.- if archive_info.expiresAt and archive_info.expiresAt < datetime.utcnow(): + if archive_info.expiresAt and archive_info.expiresAt < datetime.now(UTC):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/archive_service.py` around lines 241 - 242, The comparison uses naive datetime.utcnow(), which is deprecated; update the check comparing archive_info.expiresAt to use a timezone-aware current time (e.g., datetime.now(timezone.utc)) and ensure you import timezone from datetime, so the condition becomes "if archive_info.expiresAt and archive_info.expiresAt < datetime.now(timezone.utc)"; also verify archive_info.expiresAt is timezone-aware or normalize it before comparison in ArchiveService methods that reference archive_info.expiresAt.scripts/smoke-test-workspace-recovery.sh (1)
42-117: Consider adding error handling for Python execution failures.The
load_task_statefunction usesevalto execute Python-generated shell commands. If the Python script fails (e.g., database connection error), the script will silently continue with unset variables.🛡️ Suggested improvement
load_task_state() { - eval "$( + local output + output="$( TASK_ID="${TASK_ID}" run_backend_python <<'PY' ... PY - )" + )" || { echo "Failed to load task state" >&2; exit 1; } + eval "${output}" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke-test-workspace-recovery.sh` around lines 42 - 117, The load_task_state function currently pipes Python-generated exports into eval via TASK_ID="${TASK_ID}" run_backend_python and will continue silently if the Python command fails; update load_task_state to detect Python execution failures by capturing the exit status of run_backend_python (or its stdout) before passing to eval and aborting the script on non-zero exit, ensuring TASK_USER_ID/LAST_ASSISTANT_ID/EXECUTOR_NAME/WORKSPACE_DIR/ARCHIVE_STORAGE_KEY are not left unset; specifically, wrap the run_backend_python call used to produce the export lines, check its return code and stderr for errors, and only run eval on the captured stdout when the Python invocation succeeded (or print a clear error and exit), referencing the existing load_task_state, TASK_ID, eval, and run_backend_python symbols.backend/app/services/workspace_archive/storage.py (1)
187-193: Usedatetime.now(UTC)instead of deprecateddatetime.utcnow().
datetime.utcnow()is deprecated since Python 3.12 in favor of timezone-awaredatetime.now(UTC). This also ensures consistency with other parts of the codebase that use UTC-aware datetimes.♻️ Proposed fix
+from datetime import UTC, datetime, timedelta -from datetime import datetime, timedelta ... def calculate_expiration_time(self) -> datetime: """Calculate archive expiration time based on retention days. Returns: Expiration datetime (UTC) """ - return datetime.utcnow() + timedelta(days=ARCHIVE_RETENTION_DAYS) + return datetime.now(UTC) + timedelta(days=ARCHIVE_RETENTION_DAYS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/workspace_archive/storage.py` around lines 187 - 193, The calculate_expiration_time method uses the deprecated datetime.utcnow(); update it to return a timezone-aware UTC datetime by replacing datetime.utcnow() with datetime.now(timezone.utc) (or datetime.now(UTC) if you import UTC) so the result is consistent with other UTC-aware timestamps; keep the addition of timedelta(days=ARCHIVE_RETENTION_DAYS) unchanged and ensure the timezone import (timezone or UTC) is present at the top of the module.backend/tests/api/endpoints/internal/test_workspace_archives_api.py (1)
69-69: Use UTC-aware datetime for consistency.The test uses
datetime.now()which creates a naive datetime, but the test later usesdatetime.now(UTC)forexpires_at(line 91). For consistency and to match production behavior, use UTC-aware datetimes throughout.- completed_at=datetime.now(), + completed_at=datetime.now(UTC),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/api/endpoints/internal/test_workspace_archives_api.py` at line 69, Replace the naive datetime used for the completed_at field in the test (currently set via datetime.now()) with a UTC-aware datetime to match the rest of the test (e.g., the expires_at value created with datetime.now(UTC)); update the completed_at assignment in test_workspace_archives_api.py to use the same UTC timezone helper/symbol used elsewhere in the file so both completed_at and expires_at are timezone-aware and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@executor_manager/routers/routers.py`:
- Around line 1408-1429: The handler currently collapses all httpx errors into a
500 in the except httpx.HTTPError block; update the exception handling around
the traced_async_client POST (the code using archive_url, payload and
response.raise_for_status()) to specifically catch httpx.HTTPStatusError and
re-raise an HTTPException that preserves the upstream status_code and detail
(e.g., use e.response.status_code and e.response.text or e.args) while still
logging, and keep a separate broader httpx.HTTPError catch for network/timeout
errors mapping to 500; apply the identical split to the restore handler so
executor 4xx/5xx responses are forwarded intact instead of flattened to 500.
In `@executor/envd/api/routes.py`:
- Around line 338-341: The checks that set git_included/git_restored only match
when the relative path equals ".git" or starts with ".git/", which misses .git
directories nested inside repo subfolders; update both places (the block that
sets git_included and the block that sets git_restored) to detect any path
component named ".git" (e.g., use pathlib.Path(item.name).parts and test if
".git" in parts or split on os.sep and check membership) instead of only
startswith/equals, and keep the existing logic for ".claude_session_id" using
item.name.startswith as-is.
---
Duplicate comments:
In `@backend/app/services/execution/recovery_service.py`:
- Around line 299-303: The code currently hardcodes shell_type = "ClaudeCode"
and leaves bot_config = {} in recovery logic; change it to extract bot
configuration from the subtask (use subtask.bot_ids[0] when present) and resolve
the Kind/bot record the same way request_builder does to derive the correct
shell_type and populate bot_config; update the recovery routine to query the bot
Kind resource (or service/method used by request_builder) and fall back to a
sensible default only if no bot_id or Kind is found.
In `@backend/app/services/workspace_archive/archive_service.py`:
- Around line 221-249: check_archive_available currently only validates metadata
(expiresAt/storageKey) and must also verify the archive file exists in object
storage like restore_workspace does; update check_archive_available to use the
same storage client call used in restore_workspace to check object existence for
archive_info.storageKey (e.g., call the MinIO/StorageClient method that
restore_workspace uses), and return (False, None, "missing" or None) when the
object is not present; keep the existing expiration check and logging behavior
and reference archive_info.storageKey and the check_archive_available and
restore_workspace functions to ensure consistent behavior.
In `@executor_manager/routers/routers.py`:
- Around line 1344-1360: The ArchiveExecutorRequest and RestoreExecutorRequest
models currently include bearer presigned URLs (upload_url, download_url) that
will be captured by log_requests(); change those fields to a secret type (e.g.,
pydantic.SecretStr) so they are redacted by default and/or mark them with
repr=False/Field(..., exclude=True) to prevent inclusion in model.dict()/repr(),
and update the log_requests() telemetry serialization to explicitly exclude
"upload_url" and "download_url" when serializing request bodies (or check for
SecretStr and avoid calling get_secret_value()). Ensure you modify
ArchiveExecutorRequest, RestoreExecutorRequest, and the log_requests()
serializer logic so the presigned URLs are never recorded in OTEL spans.
- Line 1351: The hard-coded proxy timeouts (timeout=120.0) will abort valid
archive/restore operations when max_size_mb is large; update the proxy call
sites that currently pass timeout=120.0 to compute a dynamic timeout based on
the max_size_mb value (the variable max_size_mb) instead of a fixed 120s — e.g.,
derive timeout_seconds from max_size_mb (such as timeout_seconds = max(120.0,
max_size_mb * X + Y) or a similar heuristic) and replace timeout=120.0 in the
proxy.post/proxy.request calls so archive/restore operations use the computed
timeout; make this change for all occurrences that pass timeout=120.0 near the
archive/restore proxy calls.
In `@executor/agents/base.py`:
- Around line 298-314: When skip_git_clone is true but task_data.git_url is
empty the method returns without setting self.project_path; update the
skip_git_clone branch (check involving skip_git_clone, self.task_data, and
git_url) to compute a deterministic fallback project_path =
os.path.join(config.get_workspace_root(), str(self.task_id)) and assign it to
self.project_path when self.project_path is None (instead of only setting
project_path when git_url exists); keep using get_repo_name_from_url when
git_url is present and still return after ensuring self.project_path is set.
In `@executor/envd/api/routes.py`:
- Around line 69-82: The helper extract_tar_members currently falls back to
tar.extractall(**extract_kwargs) when tar.extractall(filter="data") raises
TypeError, which is unsafe; replace that fallback by implementing a manual safe
extraction inside extract_tar_members: iterate over members (use the provided
members or tar.getmembers()), validate each TarInfo.name to forbid absolute
paths and path traversal (no leading '/' and no '..' components), handle
symlinks securely by either skipping them or resolving/creating only safe
targets, create directories as needed, and write regular file contents via
tar.extractfile into files with correct mode/uid/gid handling; do not call
tar.extractall without a filter and ensure extract_tar_members always enforces
these checks instead of requiring a newer Python runtime.
- Around line 85-101: The current upload_archive_to_url and
download_archive_from_url materialize the whole archive as bytes; change them to
stream in chunks to/from disk instead: for download_archive_from_url, open a
target tmp_path file and use httpx.AsyncClient.stream("GET", download_url) to
iterate response.aiter_bytes(chunk_size) and write chunks to tmp_path without
returning bytes; for upload_archive_to_url, read from the tmp_path file in
chunks (or provide an async iterator over chunks) and use httpx.AsyncClient.put
(or stream/async iterator as the request body) to upload without loading the
entire file into memory. Update signatures to accept a Path/str tmp_path
target/source and ensure response.raise_for_status() is still checked.
- Around line 321-344: should_exclude currently checks only top-level item.name
so nested artifacts are still archived; fix by applying exclusion logic to all
files added (not just top-level) — either replace the loop+tar.add call with
tar.add using a filter function that calls should_exclude on the tarinfo.name
(or join path parts) or change the walk to recurse and call should_exclude on
each relative path component; update references to should_exclude, tar.add and
workspace_path.iterdir() so the filter inspects the full member path
(tarinfo.name) against ARCHIVE_EXCLUDE_PATTERNS to exclude nested node_modules,
dist, .venv, etc. before adding to the tar.
In `@executor/tests/test_envd_workspace_archive.py`:
- Around line 24-36: The test currently leaves .git at the workspace root and
doesn't verify restored contents; change the fixture to create the git repo
under a subdirectory (e.g., create workspace_path / "repo" / ".git" and write
HEAD there), then delete that repo/.git and .claude_session_id before invoking
the restore endpoint (the code that calls client.post("/api/restore") in this
test). After the POST, assert that the repo/.git/HEAD, .claude_session_id, and
.claude/workspace-memory.md have been recreated with the expected contents (use
the existing workspace_path, tmp_path, and task_id names to locate files).
In `@scripts/smoke-test-workspace-recovery.sh`:
- Around line 5-9: The script currently hardcodes ROOT_DIR which breaks
portability; change ROOT_DIR to be derived from the script location instead
(e.g., compute the script directory via BASH_SOURCE/dirname and resolve the repo
root with cd/realpath) and then recompute BACKEND_DIR, ENV_FILE, BACKEND_LOG and
EXECUTOR_MANAGER_LOG from that ROOT_DIR; ensure the derived value works when the
script is invoked via symlink or from another CWD by resolving to an absolute
path and keep the existing variable names (ROOT_DIR, BACKEND_DIR, ENV_FILE,
BACKEND_LOG, EXECUTOR_MANAGER_LOG) so the rest of the script is unchanged.
---
Nitpick comments:
In `@backend/app/services/adapters/executor_job.py`:
- Around line 160-166: The current taskType extraction uses a chained boolean
expression that treats empty strings as falsy; update the logic in the
Task.model_validate block (referencing task_crd and task_crd.metadata.labels) to
use an explicit conditional so an empty string is preserved: e.g. if
metadata.labels exists then fetch labels.get("taskType") and if that value is
None use "chat" (or use a ternary expression) instead of relying on "and ... or
...".
In `@backend/app/services/execution/__init__.py`:
- Around line 57-66: The create_sandbox signature uses default None for nullable
parameters but lacks explicit Optional types; update the type hints for timeout,
workspace_ref, bot_config, and metadata to use typing.Optional (or Union[...,
None]) and add the necessary import (from typing import Optional) so the
signature becomes e.g. timeout: Optional[int], workspace_ref: Optional[str],
bot_config: Optional[dict], metadata: Optional[dict] while keeping defaults as =
None and leaving other names (create_sandbox, shell_type, user_id, user_name)
unchanged.
In `@backend/app/services/execution/recovery_service.py`:
- Around line 275-328: The _create_sandbox method is missing a return type
annotation; update its signature on the _create_sandbox method to declare the
return type as Tuple[Optional[Sandbox], Optional[str]] (or from typing import
Tuple, Optional) so it reads something like def _create_sandbox(...) ->
Tuple[Optional[Sandbox], Optional[str]]; ensure Sandbox is imported or
referenced where needed and keep the existing parameters (task: TaskResource,
subtask: Subtask, user_id: int, user_name: str, skip_git_clone: bool) unchanged.
- Around line 78-82: The unpacking of
archive_service.check_archive_available(task) yields storage_key which is
unused; change the unpacking in the try block where archive_available,
storage_key, reason = archive_service.check_archive_available(task) to use an
ignored-name by prefixing storage_key with an underscore (e.g.,
archive_available, _storage_key, reason or simply _,) so it signals the variable
is intentionally unused; update the single occurrence in the recovery_service.py
try block that calls archive_service.check_archive_available to avoid
unused-variable warnings.
- Around line 172-173: executor_namespace is hardcoded to "default" in
recovery_service.py (near where executor_name = sandbox.container_name) which
breaks multi-namespace setups; change both occurrences to derive the namespace
instead: try to read the original executor namespace from the subtask (e.g.,
subtask.executor_namespace or subtask.metadata["executor_namespace"]) and fall
back to a configurable default (e.g., settings.DEFAULT_EXECUTOR_NAMESPACE or a
injected config value) if absent, and update the assignments that set
executor_namespace to use that lookup so recovered executors get the correct
namespace.
In `@backend/app/services/workspace_archive/archive_service.py`:
- Around line 111-112: Replace the deprecated datetime.utcnow() used when
setting archivedAt with a timezone-aware call: use datetime.now(timezone.utc)
(or datetime.now(UTC) if UTC is imported from zoneinfo) and add the
corresponding import (e.g., from datetime import datetime, timezone) so
archivedAt becomes timezone-aware; apply the same change to the second
occurrence at the other assignment (the block that sets archivedAt and expiresAt
near archive_storage_service.calculate_expiration_time()).
- Around line 241-242: The comparison uses naive datetime.utcnow(), which is
deprecated; update the check comparing archive_info.expiresAt to use a
timezone-aware current time (e.g., datetime.now(timezone.utc)) and ensure you
import timezone from datetime, so the condition becomes "if
archive_info.expiresAt and archive_info.expiresAt < datetime.now(timezone.utc)";
also verify archive_info.expiresAt is timezone-aware or normalize it before
comparison in ArchiveService methods that reference archive_info.expiresAt.
In `@backend/app/services/workspace_archive/storage.py`:
- Around line 187-193: The calculate_expiration_time method uses the deprecated
datetime.utcnow(); update it to return a timezone-aware UTC datetime by
replacing datetime.utcnow() with datetime.now(timezone.utc) (or
datetime.now(UTC) if you import UTC) so the result is consistent with other
UTC-aware timestamps; keep the addition of
timedelta(days=ARCHIVE_RETENTION_DAYS) unchanged and ensure the timezone import
(timezone or UTC) is present at the top of the module.
In `@backend/tests/api/endpoints/internal/test_workspace_archives_api.py`:
- Line 69: Replace the naive datetime used for the completed_at field in the
test (currently set via datetime.now()) with a UTC-aware datetime to match the
rest of the test (e.g., the expires_at value created with datetime.now(UTC));
update the completed_at assignment in test_workspace_archives_api.py to use the
same UTC timezone helper/symbol used elsewhere in the file so both completed_at
and expires_at are timezone-aware and consistent.
In `@scripts/smoke-test-workspace-recovery.sh`:
- Around line 42-117: The load_task_state function currently pipes
Python-generated exports into eval via TASK_ID="${TASK_ID}" run_backend_python
and will continue silently if the Python command fails; update load_task_state
to detect Python execution failures by capturing the exit status of
run_backend_python (or its stdout) before passing to eval and aborting the
script on non-zero exit, ensuring
TASK_USER_ID/LAST_ASSISTANT_ID/EXECUTOR_NAME/WORKSPACE_DIR/ARCHIVE_STORAGE_KEY
are not left unset; specifically, wrap the run_backend_python call used to
produce the export lines, check its return code and stderr for errors, and only
run eval on the captured stdout when the Python invocation succeeded (or print a
clear error and exit), referencing the existing load_task_state, TASK_ID, eval,
and run_backend_python symbols.
🪄 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: f7ff1890-dfa1-465b-a2f2-f62cae4baba7
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
backend/app/api/api.pybackend/app/api/endpoints/internal/__init__.pybackend/app/api/endpoints/internal/workspace_archives.pybackend/app/schemas/kind.pybackend/app/services/adapters/executor_job.pybackend/app/services/chat/storage/task_manager.pybackend/app/services/execution/__init__.pybackend/app/services/execution/dispatcher.pybackend/app/services/execution/recovery_service.pybackend/app/services/execution/schedule_helper.pybackend/app/services/workspace_archive/__init__.pybackend/app/services/workspace_archive/archive_service.pybackend/app/services/workspace_archive/storage.pybackend/pyproject.tomlbackend/tests/api/endpoints/internal/test_workspace_archives_api.pybackend/tests/services/chat/storage/test_task_manager.pybackend/tests/services/execution/test_dispatcher_recovery.pybackend/tests/services/execution/test_sandbox_manager_client.pyexecutor/agents/base.pyexecutor/envd/api/models.pyexecutor/envd/api/routes.pyexecutor/tests/test_envd_workspace_archive.pyexecutor_manager/routers/routers.pyexecutor_manager/services/sandbox/manager.pyscripts/smoke-test-workspace-recovery.shshared/models/execution.py
✅ Files skipped from review due to trivial changes (3)
- backend/pyproject.toml
- backend/app/api/endpoints/internal/init.py
- executor/envd/api/models.py
Comment on lines
+1408
to
+1429
| async with traced_async_client(timeout=120.0) as client: | ||
| response = await client.post( | ||
| archive_url, | ||
| json=payload, | ||
| headers={"Content-Type": "application/json"}, | ||
| ) | ||
| response.raise_for_status() | ||
| archive_result = response.json() | ||
|
|
||
| logger.info( | ||
| f"[Archive] Successfully archived task {request.task_id}, " | ||
| f"size={archive_result.get('size_bytes')} bytes" | ||
| ) | ||
|
|
||
| return archive_result | ||
|
|
||
| except HTTPException: | ||
| raise | ||
| except httpx.HTTPError as e: | ||
| logger.error(f"[Archive] HTTP error for task {request.task_id}: {e}") | ||
| raise HTTPException(status_code=500, detail=f"HTTP error: {str(e)}") | ||
| except Exception as e: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd executor_manager/routers && wc -l routers.pyRepository: wecode-ai/Wegent
Length of output: 75
🏁 Script executed:
cd executor_manager/routers && sed -n '1400,1440p' routers.pyRepository: wecode-ai/Wegent
Length of output: 1414
🏁 Script executed:
cd executor_manager/routers && sed -n '1470,1510p' routers.pyRepository: wecode-ai/Wegent
Length of output: 1351
🏁 Script executed:
head -50 executor_manager/routers/routers.py | grep -E "^import|^from"Repository: wecode-ai/Wegent
Length of output: 1128
🏁 Script executed:
rg "from httpx|import httpx" executor_manager/routers/routers.pyRepository: wecode-ai/Wegent
Length of output: 72
🏁 Script executed:
python3 <<'EOF'
import httpx
# Verify HTTPStatusError exists and is subclass of HTTPError
print(f"HTTPStatusError exists: {hasattr(httpx, 'HTTPStatusError')}")
print(f"Is HTTPStatusError subclass of HTTPError: {issubclass(httpx.HTTPStatusError, httpx.HTTPError)}")
print(f"HTTPStatusError MRO: {httpx.HTTPStatusError.__mro__}")
EOFRepository: wecode-ai/Wegent
Length of output: 281
Preserve executor's failure status instead of rewriting to 500.
response.raise_for_status() raises httpx.HTTPStatusError for executor 4xx/5xx responses, but the catch block treats all httpx.HTTPError uniformly and flattens them to 500. This hides actionable failures like 404 Workspace not found or 413 Archive too large from the backend.
♻️ Suggested pattern
- except httpx.HTTPError as e:
- logger.error(f"[Archive] HTTP error for task {request.task_id}: {e}")
- raise HTTPException(status_code=500, detail=f"HTTP error: {str(e)}")
+ except httpx.HTTPStatusError as e:
+ logger.warning(
+ f"[Archive] Executor returned {e.response.status_code} for task {request.task_id}: {e.response.text}"
+ )
+ raise HTTPException(
+ status_code=e.response.status_code,
+ detail=e.response.text,
+ ) from e
+ except httpx.HTTPError as e:
+ logger.error(f"[Archive] HTTP error for task {request.task_id}: {e}")
+ raise HTTPException(status_code=502, detail=f"HTTP error: {e!s}") from eApply the same split to the restore handler.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 1428-1428: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 1428-1428: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[warning] 1429-1429: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor_manager/routers/routers.py` around lines 1408 - 1429, The handler
currently collapses all httpx errors into a 500 in the except httpx.HTTPError
block; update the exception handling around the traced_async_client POST (the
code using archive_url, payload and response.raise_for_status()) to specifically
catch httpx.HTTPStatusError and re-raise an HTTPException that preserves the
upstream status_code and detail (e.g., use e.response.status_code and
e.response.text or e.args) while still logging, and keep a separate broader
httpx.HTTPError catch for network/timeout errors mapping to 500; apply the
identical split to the restore handler so executor 4xx/5xx responses are
forwarded intact instead of flattened to 500.
Comment on lines
+338
to
+341
| if item.name.startswith(".claude_session_id"): | ||
| session_file_included = True | ||
| if item.name == ".git": | ||
| git_included = True |
Contributor
There was a problem hiding this comment.
Detect .git by path component, not only at the workspace root.
The current checks only set git_included / git_restored when the relative path is exactly .git or starts with .git/. Real cloned repos usually place .git under the repo directory, so these flags will be false even when git history was actually archived and restored.
Also applies to: 474-477
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor/envd/api/routes.py` around lines 338 - 341, The checks that set
git_included/git_restored only match when the relative path equals ".git" or
starts with ".git/", which misses .git directories nested inside repo
subfolders; update both places (the block that sets git_included and the block
that sets git_restored) to detect any path component named ".git" (e.g., use
pathlib.Path(item.name).parts and test if ".git" in parts or split on os.sep and
check membership) instead of only startswith/equals, and keep the existing logic
for ".claude_session_id" using item.name.startswith as-is.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
executor_manager/tests/routers/test_workspace_archive_routes.py (1)
16-16: Extract repeatedtask_idliteral into a named constant.
1385is repeated across tests. Promoting it to a module-level constant improves maintainability and follows the repo’s Python rule.♻️ Suggested patch
from executor_manager.routers import routers +TASK_ID = 1385 + `@pytest.mark.asyncio` async def test_archive_executor_workspace_returns_404_when_runtime_unavailable( @@ request = routers.ArchiveExecutorRequest( - task_id=1385, + task_id=TASK_ID, @@ request = routers.ArchiveExecutorRequest( - task_id=1385, + task_id=TASK_ID, @@ request = routers.RestoreExecutorRequest( - task_id=1385, + task_id=TASK_ID,As per coding guidelines,
**/*.py: "Python: Extract magic numbers to constants".Also applies to: 40-40, 82-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor_manager/tests/routers/test_workspace_archive_routes.py` at line 16, Extract the repeated literal 1385 into a module-level constant (e.g., TASK_ID = 1385) at the top of executor_manager/tests/routers/test_workspace_archive_routes.py and replace all inline uses such as the keyword argument task_id=1385 (seen in test functions and fixtures) with task_id=TASK_ID; update all occurrences referenced in the file (the three places where task_id=1385 is used) so tests import/use the constant and no magic number remains inline.scripts/smoke-test-workspace-recovery.sh (2)
248-250: Manual step breaks CI automation potential.Requiring user interaction (
read -r) makes this script unsuitable for automated CI/CD pipelines. Consider adding an option to skip the manual prompt or auto-trigger the recovery via API.♻️ Proposed enhancement for CI compatibility
+AUTO_TRIGGER="${AUTO_TRIGGER:-false}" + echo "==> Manual step required" -echo "Send a new chat message for task ${TASK_ID} in the UI, then press Enter to continue." -read -r +if [[ "${AUTO_TRIGGER}" == "true" ]]; then + echo "Auto-triggering recovery dispatch via API..." + # Add API call to trigger task dispatch/recovery + curl -sS -X POST "http://localhost:8000/api/internal/tasks/${TASK_ID}/dispatch" || true +else + echo "Send a new chat message for task ${TASK_ID} in the UI, then press Enter to continue." + read -r +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke-test-workspace-recovery.sh` around lines 248 - 250, The script currently halts with a blocking interactive prompt (the read -r after echo lines referencing TASK_ID) which prevents CI automation; modify the logic in scripts/smoke-test-workspace-recovery.sh to support non-interactive runs by adding a flag or env var (e.g., --non-interactive or SKIP_PROMPT/CI=true) that bypasses the read -r, and when non-interactive is requested either skip the manual pause or invoke the recovery endpoint programmatically (e.g., a curl POST to the recovery API using TASK_ID) so the script can run unattended in CI while preserving the interactive behavior by default.
32-34: Add ShellCheck directive to silence the non-constant source warning.ShellCheck cannot statically analyze the sourced file since its path is determined at runtime. Add a directive to acknowledge this is intentional.
♻️ Proposed fix
set -a +# shellcheck source=/dev/null source "${ENV_FILE}" set +a🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke-test-workspace-recovery.sh` around lines 32 - 34, Add a ShellCheck directive to silence the non-constant source warning by placing a comment like "shellcheck disable=SC1091" immediately above the dynamic source call; update the block containing set -a / source "${ENV_FILE}" / set +a so the line with source "${ENV_FILE}" is preceded by the ShellCheck disable comment, ensuring the intent is clear and ShellCheck no longer flags the runtime-determined path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/services/execution/test_schedule_helper_recovery.py`:
- Around line 35-52: Replace the tautological assertions with an actual call to
the production helper: mock recovery_service.recover to return False, call the
_recover_executor function with the MagicMock subtask and mocked
recovery_service (and db), then assert _recover_executor returns False, that
subtask.status is set to SubtaskStatus.FAILED and subtask.error_message matches
"Failed to recover executor after Pod deletion", and that db.commit() was called
once; reference the _recover_executor function, recovery_service.recover,
SubtaskStatus, and db.commit in your changes.
In `@backend/tests/services/test_preserve_executor.py`:
- Around line 270-315: The test needs to assert that the archive path was
invoked even though it failed: inside test_cleanup_continues_when_archive_fails,
after calling job_service.cleanup_stale_executors(mock_db) add an assertion that
the patched job_service._archive_workspace_sync was called (e.g.,
assert_called_once_with or assert_called) with the expected workspace/task
identifiers (referencing _archive_workspace_sync and the mock_subtask/mock_task
used in the test) so the test verifies the archive attempt occurred before the
deletion assertion against executor_kinds_service.delete_executor_task_sync.
- Around line 226-268: Change the final assertions in
test_code_task_archives_before_cleanup to assert call order (archive before
delete) instead of only existence: after invoking
job_service.cleanup_stale_executors(mock_db), verify that
_archive_workspace_sync (archive_mock) was called and
executor_kinds_service.delete_executor_task_sync
(mock_executor_service.delete_executor_task_sync) was called, then assert
archive_mock's call appears earlier than the delete call by checking the
combined mock_calls ordering (or comparing indices of archive_mock.call_args and
mock_executor_service.delete_executor_task_sync.call_args within the parent
mocks); reference the test function name test_code_task_archives_before_cleanup
and the symbols _archive_workspace_sync and delete_executor_task_sync to locate
where to change the assertions.
- Around line 226-320: Add explicit return type hints (at minimum "-> None") to
the new test methods test_code_task_archives_before_cleanup and
test_cleanup_continues_when_archive_fails (and any other newly added tests in
this file), e.g. change their def signatures to include "-> None"; optionally
add parameter type annotations for job_service and mock_db if desired to satisfy
linters. Locate the method definitions by their names
(test_code_task_archives_before_cleanup,
test_cleanup_continues_when_archive_fails) and update the def lines accordingly,
keeping the rest of the test bodies unchanged.
In `@executor_manager/tests/routers/test_workspace_archive_routes.py`:
- Line 14: The async test function
test_archive_executor_workspace_returns_404_when_runtime_unavailable (and the
other async tests flagged at lines 38 and 80) are missing type hints; update
their signatures to annotate the mocker parameter (e.g., mocker: MockerFixture
from pytest_mock) and the return type as -> None, and add the necessary import
for MockerFixture at the top of the test module so the annotations are valid.
In `@scripts/smoke-test-workspace-recovery.sh`:
- Around line 265-271: The ripgrep (rg) commands in the error reporting block
(tail ... "${BACKEND_LOG}" | rg ... "${TASK_ID}" and tail ...
"${EXECUTOR_MANAGER_LOG}" | rg ... "${TASK_ID}") can return non-zero when no
matches are found and, combined with set -e, will abort the script before the
explicit exit 1; update those pipelines to tolerate no matches by appending "||
true" to each rg invocation so the debug output is always printed and the script
reaches the intended exit path.
- Around line 199-206: The inner docker exec shell command uses single quotes
around '${file_path}', which breaks when file_path contains single quotes;
update the docker exec invocation in the while loop to run sh -lc with
double-quoted command and escape the embedded quotes around the variable (e.g.,
sh -lc "test -f \"${file_path}\"") so file_path is safely quoted; apply this
change where docker exec "${container_name}" sh -lc "..." is used to check for
the file.
---
Nitpick comments:
In `@executor_manager/tests/routers/test_workspace_archive_routes.py`:
- Line 16: Extract the repeated literal 1385 into a module-level constant (e.g.,
TASK_ID = 1385) at the top of
executor_manager/tests/routers/test_workspace_archive_routes.py and replace all
inline uses such as the keyword argument task_id=1385 (seen in test functions
and fixtures) with task_id=TASK_ID; update all occurrences referenced in the
file (the three places where task_id=1385 is used) so tests import/use the
constant and no magic number remains inline.
In `@scripts/smoke-test-workspace-recovery.sh`:
- Around line 248-250: The script currently halts with a blocking interactive
prompt (the read -r after echo lines referencing TASK_ID) which prevents CI
automation; modify the logic in scripts/smoke-test-workspace-recovery.sh to
support non-interactive runs by adding a flag or env var (e.g.,
--non-interactive or SKIP_PROMPT/CI=true) that bypasses the read -r, and when
non-interactive is requested either skip the manual pause or invoke the recovery
endpoint programmatically (e.g., a curl POST to the recovery API using TASK_ID)
so the script can run unattended in CI while preserving the interactive behavior
by default.
- Around line 32-34: Add a ShellCheck directive to silence the non-constant
source warning by placing a comment like "shellcheck disable=SC1091" immediately
above the dynamic source call; update the block containing set -a / source
"${ENV_FILE}" / set +a so the line with source "${ENV_FILE}" is preceded by the
ShellCheck disable comment, ensuring the intent is clear and ShellCheck no
longer flags the runtime-determined path.
🪄 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: 0ca414c4-d170-4840-b124-63849f2d62f0
📒 Files selected for processing (9)
backend/tests/api/endpoints/internal/test_workspace_archives_api.pybackend/tests/services/execution/test_dispatcher_recovery.pybackend/tests/services/execution/test_recovery_service.pybackend/tests/services/execution/test_schedule_helper_recovery.pybackend/tests/services/test_preserve_executor.pyexecutor/tests/test_envd_workspace_archive.pyexecutor_manager/tests/routers/test_workspace_archive_routes.pyexecutor_manager/tests/services/test_sandbox_manager.pyscripts/smoke-test-workspace-recovery.sh
✅ Files skipped from review due to trivial changes (4)
- executor_manager/tests/services/test_sandbox_manager.py
- backend/tests/api/endpoints/internal/test_workspace_archives_api.py
- backend/tests/services/execution/test_dispatcher_recovery.py
- executor/tests/test_envd_workspace_archive.py
Comment on lines
+35
to
+52
| @pytest.mark.asyncio | ||
| async def test_schedule_marks_subtask_failed_when_recovery_returns_false(): | ||
| """Recovery failures should update subtask status in schedule dispatch logic.""" | ||
| subtask = MagicMock() | ||
| subtask.id = 123 | ||
| subtask.status = SubtaskStatus.PENDING | ||
| subtask.error_message = "" | ||
| db = MagicMock() | ||
|
|
||
| recovery_success = False | ||
| if not recovery_success: | ||
| subtask.status = SubtaskStatus.FAILED | ||
| subtask.error_message = "Failed to recover executor after Pod deletion" | ||
| db.commit() | ||
|
|
||
| assert subtask.status == SubtaskStatus.FAILED | ||
| assert subtask.error_message == "Failed to recover executor after Pod deletion" | ||
| db.commit.assert_called_once() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what _recover_executor does when recovery returns False
ast-grep --pattern $'async def _recover_executor($$$):
$$$
'
# Also check schedule_helper for recovery failure handling
rg -n -A 20 "def _recover_executor" --type=pyRepository: wecode-ai/Wegent
Length of output: 3597
🏁 Script executed:
# Read the test file to see the actual test implementation
cat -n backend/tests/services/execution/test_schedule_helper_recovery.py | head -80Repository: wecode-ai/Wegent
Length of output: 2130
🏁 Script executed:
# Get the full implementation of _recover_executor to see what happens on recovery failure
sed -n '253,330p' backend/app/services/execution/schedule_helper.pyRepository: wecode-ai/Wegent
Length of output: 2210
🏁 Script executed:
# Check what happens in dispatcher.py when _recover_executor returns False
sed -n '415,480p' backend/app/services/execution/dispatcher.pyRepository: wecode-ai/Wegent
Length of output: 2470
Test does not exercise any production code—it only asserts values it directly set.
Lines 45-48 manually set subtask.status and subtask.error_message on a mock, then the assertions verify those same values. The test never calls _recover_executor, making this a tautology that will always pass regardless of actual implementation behavior.
The first test in this file (test_recover_executor_propagates_expired_archive_error) demonstrates the correct pattern: mock the recovery_service, call the actual _recover_executor function, and verify its behavior. The test at lines 35-52 should follow the same approach—call _recover_executor with recovery_service.recover() mocked to return False, and verify the function returns False.
🛠️ Proposed fix structure
`@pytest.mark.asyncio`
async def test_schedule_marks_subtask_failed_when_recovery_returns_false():
"""Recovery failures should update subtask status in schedule dispatch logic."""
subtask = MagicMock()
subtask.id = 123
subtask.status = SubtaskStatus.PENDING
subtask.error_message = ""
+ subtask.executor_deleted_at = True
db = MagicMock()
+ task = MagicMock()
+ user = MagicMock()
+ user.id = 7
+ user.name = "yunpeng7"
- recovery_success = False
- if not recovery_success:
- subtask.status = SubtaskStatus.FAILED
- subtask.error_message = "Failed to recover executor after Pod deletion"
- db.commit()
+ with patch(
+ "app.services.execution.recovery_service.recovery_service",
+ new=MagicMock(recover=AsyncMock(return_value=False)),
+ ):
+ result = await _recover_executor(db=db, subtask=subtask, task=task, user=user)
- assert subtask.status == SubtaskStatus.FAILED
- assert subtask.error_message == "Failed to recover executor after Pod deletion"
- db.commit.assert_called_once()
+ assert result is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/services/execution/test_schedule_helper_recovery.py` around
lines 35 - 52, Replace the tautological assertions with an actual call to the
production helper: mock recovery_service.recover to return False, call the
_recover_executor function with the MagicMock subtask and mocked
recovery_service (and db), then assert _recover_executor returns False, that
subtask.status is set to SubtaskStatus.FAILED and subtask.error_message matches
"Failed to recover executor after Pod deletion", and that db.commit() was called
once; reference the _recover_executor function, recovery_service.recover,
SubtaskStatus, and db.commit in your changes.
Comment on lines
+226
to
+268
| def test_code_task_archives_before_cleanup(self, job_service, mock_db): | ||
| """Test code tasks trigger archive before deleting the executor.""" | ||
| mock_subtask = self._create_mock_subtask(1, 100) | ||
| mock_task = self._create_mock_task_resource(100, 1, preserve_executor=False) | ||
| mock_task.json["metadata"]["labels"]["taskType"] = "code" | ||
| mock_subtask.updated_at = datetime.now() - timedelta(hours=72) | ||
|
|
||
| mock_subtask_query = MagicMock() | ||
| mock_subtask_query.join.return_value = mock_subtask_query | ||
| mock_subtask_query.filter.return_value = mock_subtask_query | ||
| mock_subtask_query.all.return_value = [mock_subtask] | ||
| mock_subtask_query.update.return_value = 1 | ||
|
|
||
| mock_task_query = MagicMock() | ||
| mock_task_query.filter.return_value = mock_task_query | ||
| mock_task_query.first.return_value = mock_task | ||
|
|
||
| def query_side_effect(model): | ||
| if model == Subtask: | ||
| return mock_subtask_query | ||
| if model == TaskResource: | ||
| return mock_task_query | ||
| return MagicMock() | ||
|
|
||
| mock_db.query.side_effect = query_side_effect | ||
|
|
||
| with ( | ||
| patch.object(job_service, "_archive_workspace_sync") as archive_mock, | ||
| patch( | ||
| "app.services.adapters.executor_job.executor_kinds_service" | ||
| ) as mock_executor_service, | ||
| patch("app.services.adapters.executor_job.settings") as mock_settings, | ||
| ): | ||
| mock_settings.CHAT_TASK_EXECUTOR_DELETE_AFTER_HOURS = 24 | ||
| mock_settings.CODE_TASK_EXECUTOR_DELETE_AFTER_HOURS = 48 | ||
| mock_executor_service.delete_executor_task_sync.return_value = True | ||
|
|
||
| job_service.cleanup_stale_executors(mock_db) | ||
|
|
||
| archive_mock.assert_called_once() | ||
| mock_executor_service.delete_executor_task_sync.assert_called_once_with( | ||
| "executor-1", "default" | ||
| ) |
Contributor
There was a problem hiding this comment.
Assert archive/delete call order explicitly.
This test name says “before cleanup”, but current assertions only check both calls happened, not ordering.
🧪 Proposed fix
@@
with (
patch.object(job_service, "_archive_workspace_sync") as archive_mock,
@@
):
mock_settings.CHAT_TASK_EXECUTOR_DELETE_AFTER_HOURS = 24
mock_settings.CODE_TASK_EXECUTOR_DELETE_AFTER_HOURS = 48
- mock_executor_service.delete_executor_task_sync.return_value = True
+ events = []
+ archive_mock.side_effect = lambda *args, **kwargs: events.append("archive")
+ mock_executor_service.delete_executor_task_sync.side_effect = (
+ lambda *args, **kwargs: (events.append("delete") or True)
+ )
job_service.cleanup_stale_executors(mock_db)
archive_mock.assert_called_once()
mock_executor_service.delete_executor_task_sync.assert_called_once_with(
"executor-1", "default"
)
+ assert events == ["archive", "delete"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/services/test_preserve_executor.py` around lines 226 - 268,
Change the final assertions in test_code_task_archives_before_cleanup to assert
call order (archive before delete) instead of only existence: after invoking
job_service.cleanup_stale_executors(mock_db), verify that
_archive_workspace_sync (archive_mock) was called and
executor_kinds_service.delete_executor_task_sync
(mock_executor_service.delete_executor_task_sync) was called, then assert
archive_mock's call appears earlier than the delete call by checking the
combined mock_calls ordering (or comparing indices of archive_mock.call_args and
mock_executor_service.delete_executor_task_sync.call_args within the parent
mocks); reference the test function name test_code_task_archives_before_cleanup
and the symbols _archive_workspace_sync and delete_executor_task_sync to locate
where to change the assertions.
Comment on lines
+226
to
+320
| def test_code_task_archives_before_cleanup(self, job_service, mock_db): | ||
| """Test code tasks trigger archive before deleting the executor.""" | ||
| mock_subtask = self._create_mock_subtask(1, 100) | ||
| mock_task = self._create_mock_task_resource(100, 1, preserve_executor=False) | ||
| mock_task.json["metadata"]["labels"]["taskType"] = "code" | ||
| mock_subtask.updated_at = datetime.now() - timedelta(hours=72) | ||
|
|
||
| mock_subtask_query = MagicMock() | ||
| mock_subtask_query.join.return_value = mock_subtask_query | ||
| mock_subtask_query.filter.return_value = mock_subtask_query | ||
| mock_subtask_query.all.return_value = [mock_subtask] | ||
| mock_subtask_query.update.return_value = 1 | ||
|
|
||
| mock_task_query = MagicMock() | ||
| mock_task_query.filter.return_value = mock_task_query | ||
| mock_task_query.first.return_value = mock_task | ||
|
|
||
| def query_side_effect(model): | ||
| if model == Subtask: | ||
| return mock_subtask_query | ||
| if model == TaskResource: | ||
| return mock_task_query | ||
| return MagicMock() | ||
|
|
||
| mock_db.query.side_effect = query_side_effect | ||
|
|
||
| with ( | ||
| patch.object(job_service, "_archive_workspace_sync") as archive_mock, | ||
| patch( | ||
| "app.services.adapters.executor_job.executor_kinds_service" | ||
| ) as mock_executor_service, | ||
| patch("app.services.adapters.executor_job.settings") as mock_settings, | ||
| ): | ||
| mock_settings.CHAT_TASK_EXECUTOR_DELETE_AFTER_HOURS = 24 | ||
| mock_settings.CODE_TASK_EXECUTOR_DELETE_AFTER_HOURS = 48 | ||
| mock_executor_service.delete_executor_task_sync.return_value = True | ||
|
|
||
| job_service.cleanup_stale_executors(mock_db) | ||
|
|
||
| archive_mock.assert_called_once() | ||
| mock_executor_service.delete_executor_task_sync.assert_called_once_with( | ||
| "executor-1", "default" | ||
| ) | ||
|
|
||
| def test_cleanup_continues_when_archive_fails(self, job_service, mock_db): | ||
| """Test executor cleanup continues even when workspace archive fails.""" | ||
| mock_subtask = self._create_mock_subtask(1, 100) | ||
| mock_task = self._create_mock_task_resource(100, 1, preserve_executor=False) | ||
| mock_task.json["metadata"]["labels"]["taskType"] = "code" | ||
| mock_subtask.updated_at = datetime.now() - timedelta(hours=72) | ||
|
|
||
| mock_subtask_query = MagicMock() | ||
| mock_subtask_query.join.return_value = mock_subtask_query | ||
| mock_subtask_query.filter.return_value = mock_subtask_query | ||
| mock_subtask_query.all.return_value = [mock_subtask] | ||
| mock_subtask_query.update.return_value = 1 | ||
|
|
||
| mock_task_query = MagicMock() | ||
| mock_task_query.filter.return_value = mock_task_query | ||
| mock_task_query.first.return_value = mock_task | ||
|
|
||
| def query_side_effect(model): | ||
| if model == Subtask: | ||
| return mock_subtask_query | ||
| if model == TaskResource: | ||
| return mock_task_query | ||
| return MagicMock() | ||
|
|
||
| mock_db.query.side_effect = query_side_effect | ||
|
|
||
| with ( | ||
| patch.object( | ||
| job_service, | ||
| "_archive_workspace_sync", | ||
| side_effect=RuntimeError("archive failed"), | ||
| ), | ||
| patch( | ||
| "app.services.adapters.executor_job.executor_kinds_service" | ||
| ) as mock_executor_service, | ||
| patch("app.services.adapters.executor_job.settings") as mock_settings, | ||
| ): | ||
| mock_settings.CHAT_TASK_EXECUTOR_DELETE_AFTER_HOURS = 24 | ||
| mock_settings.CODE_TASK_EXECUTOR_DELETE_AFTER_HOURS = 48 | ||
| mock_executor_service.delete_executor_task_sync.return_value = True | ||
|
|
||
| job_service.cleanup_stale_executors(mock_db) | ||
|
|
||
| mock_executor_service.delete_executor_task_sync.assert_called_once_with( | ||
| "executor-1", "default" | ||
| ) | ||
|
|
||
| def test_code_task_is_not_cleaned_up_before_code_threshold( | ||
| self, job_service, mock_db | ||
| ): | ||
| """Test code tasks younger than the code threshold are skipped.""" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify these new test methods have explicit return annotations.
rg -nP '^\s*def\s+test_(code_task_archives_before_cleanup|cleanup_continues_when_archive_fails|code_task_is_not_cleaned_up_before_code_threshold)\s*\(.*\)\s*->\s*None\s*:' backend/tests/services/test_preserve_executor.pyRepository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
# Check the actual method definitions in the file
sed -n '220,330p' backend/tests/services/test_preserve_executor.pyRepository: wecode-ai/Wegent
Length of output: 4919
🏁 Script executed:
# Check for fixture definitions and imports at the top of the file
head -50 backend/tests/services/test_preserve_executor.pyRepository: wecode-ai/Wegent
Length of output: 1552
Add explicit type hints to the newly added test methods.
The new methods should include explicit parameter/return annotations (at minimum -> None) to match the Python guideline that requires type hints.
♻️ Proposed refactor
- def test_code_task_archives_before_cleanup(self, job_service, mock_db):
+ def test_code_task_archives_before_cleanup(
+ self, job_service: JobService, mock_db: Session
+ ) -> None:
@@
- def test_cleanup_continues_when_archive_fails(self, job_service, mock_db):
+ def test_cleanup_continues_when_archive_fails(
+ self, job_service: JobService, mock_db: Session
+ ) -> None:
@@
def test_code_task_is_not_cleaned_up_before_code_threshold(
- self, job_service, mock_db
- ):
+ self, job_service: JobService, mock_db: Session
+ ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/services/test_preserve_executor.py` around lines 226 - 320, Add
explicit return type hints (at minimum "-> None") to the new test methods
test_code_task_archives_before_cleanup and
test_cleanup_continues_when_archive_fails (and any other newly added tests in
this file), e.g. change their def signatures to include "-> None"; optionally
add parameter type annotations for job_service and mock_db if desired to satisfy
linters. Locate the method definitions by their names
(test_code_task_archives_before_cleanup,
test_cleanup_continues_when_archive_fails) and update the def lines accordingly,
keeping the rest of the test bodies unchanged.
Comment on lines
+270
to
+315
| def test_cleanup_continues_when_archive_fails(self, job_service, mock_db): | ||
| """Test executor cleanup continues even when workspace archive fails.""" | ||
| mock_subtask = self._create_mock_subtask(1, 100) | ||
| mock_task = self._create_mock_task_resource(100, 1, preserve_executor=False) | ||
| mock_task.json["metadata"]["labels"]["taskType"] = "code" | ||
| mock_subtask.updated_at = datetime.now() - timedelta(hours=72) | ||
|
|
||
| mock_subtask_query = MagicMock() | ||
| mock_subtask_query.join.return_value = mock_subtask_query | ||
| mock_subtask_query.filter.return_value = mock_subtask_query | ||
| mock_subtask_query.all.return_value = [mock_subtask] | ||
| mock_subtask_query.update.return_value = 1 | ||
|
|
||
| mock_task_query = MagicMock() | ||
| mock_task_query.filter.return_value = mock_task_query | ||
| mock_task_query.first.return_value = mock_task | ||
|
|
||
| def query_side_effect(model): | ||
| if model == Subtask: | ||
| return mock_subtask_query | ||
| if model == TaskResource: | ||
| return mock_task_query | ||
| return MagicMock() | ||
|
|
||
| mock_db.query.side_effect = query_side_effect | ||
|
|
||
| with ( | ||
| patch.object( | ||
| job_service, | ||
| "_archive_workspace_sync", | ||
| side_effect=RuntimeError("archive failed"), | ||
| ), | ||
| patch( | ||
| "app.services.adapters.executor_job.executor_kinds_service" | ||
| ) as mock_executor_service, | ||
| patch("app.services.adapters.executor_job.settings") as mock_settings, | ||
| ): | ||
| mock_settings.CHAT_TASK_EXECUTOR_DELETE_AFTER_HOURS = 24 | ||
| mock_settings.CODE_TASK_EXECUTOR_DELETE_AFTER_HOURS = 48 | ||
| mock_executor_service.delete_executor_task_sync.return_value = True | ||
|
|
||
| job_service.cleanup_stale_executors(mock_db) | ||
|
|
||
| mock_executor_service.delete_executor_task_sync.assert_called_once_with( | ||
| "executor-1", "default" | ||
| ) |
Contributor
There was a problem hiding this comment.
Also verify archive was attempted in the failure-path test.
Right now the test only asserts deletion happens. Add an assertion that _archive_workspace_sync was actually invoked and failed.
🧪 Proposed fix
@@
with (
patch.object(
job_service,
"_archive_workspace_sync",
side_effect=RuntimeError("archive failed"),
- ),
+ ) as archive_mock,
@@
):
@@
job_service.cleanup_stale_executors(mock_db)
+ archive_mock.assert_called_once()
mock_executor_service.delete_executor_task_sync.assert_called_once_with(
"executor-1", "default"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/services/test_preserve_executor.py` around lines 270 - 315, The
test needs to assert that the archive path was invoked even though it failed:
inside test_cleanup_continues_when_archive_fails, after calling
job_service.cleanup_stale_executors(mock_db) add an assertion that the patched
job_service._archive_workspace_sync was called (e.g., assert_called_once_with or
assert_called) with the expected workspace/task identifiers (referencing
_archive_workspace_sync and the mock_subtask/mock_task used in the test) so the
test verifies the archive attempt occurred before the deletion assertion against
executor_kinds_service.delete_executor_task_sync.
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_archive_executor_workspace_returns_404_when_runtime_unavailable(mocker): |
Contributor
There was a problem hiding this comment.
Add type hints to async test signatures.
These test functions currently omit type hints (mocker parameter and return type), which violates the Python guideline for typed code.
♻️ Suggested patch
from types import SimpleNamespace
import httpx
import pytest
+from pytest_mock import MockerFixture
from executor_manager.routers import routers
`@pytest.mark.asyncio`
-async def test_archive_executor_workspace_returns_404_when_runtime_unavailable(mocker):
+async def test_archive_executor_workspace_returns_404_when_runtime_unavailable(
+ mocker: MockerFixture,
+) -> None:
@@
`@pytest.mark.asyncio`
-async def test_archive_executor_workspace_forwards_payload(mocker):
+async def test_archive_executor_workspace_forwards_payload(
+ mocker: MockerFixture,
+) -> None:
@@
`@pytest.mark.asyncio`
-async def test_restore_executor_workspace_wraps_http_errors(mocker):
+async def test_restore_executor_workspace_wraps_http_errors(
+ mocker: MockerFixture,
+) -> None:As per coding guidelines, **/*.py: "Python: Follow PEP 8, Black formatter (line length: 88), isort, type hints required".
Also applies to: 38-38, 80-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor_manager/tests/routers/test_workspace_archive_routes.py` at line 14,
The async test function
test_archive_executor_workspace_returns_404_when_runtime_unavailable (and the
other async tests flagged at lines 38 and 80) are missing type hints; update
their signatures to annotate the mocker parameter (e.g., mocker: MockerFixture
from pytest_mock) and the return type as -> None, and add the necessary import
for MockerFixture at the top of the test module so the annotations are valid.
Comment on lines
+199
to
+206
| while (( SECONDS < deadline )); do | ||
| if docker ps --format '{{.Names}}' | grep -Fxq "${container_name}"; then | ||
| if docker exec "${container_name}" sh -lc "test -f '${file_path}'"; then | ||
| return 0 | ||
| fi | ||
| fi | ||
| sleep 2 | ||
| done |
Contributor
There was a problem hiding this comment.
File path with special characters may break the test command.
If file_path contains single quotes, the shell quoting will break. Use double quotes consistently for the inner command.
🛠️ Proposed fix
while (( SECONDS < deadline )); do
if docker ps --format '{{.Names}}' | grep -Fxq "${container_name}"; then
- if docker exec "${container_name}" sh -lc "test -f '${file_path}'"; then
+ if docker exec "${container_name}" test -f "${file_path}"; then
return 0
fi
fi📝 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.
Suggested change
| while (( SECONDS < deadline )); do | |
| if docker ps --format '{{.Names}}' | grep -Fxq "${container_name}"; then | |
| if docker exec "${container_name}" sh -lc "test -f '${file_path}'"; then | |
| return 0 | |
| fi | |
| fi | |
| sleep 2 | |
| done | |
| while (( SECONDS < deadline )); do | |
| if docker ps --format '{{.Names}}' | grep -Fxq "${container_name}"; then | |
| if docker exec "${container_name}" test -f "${file_path}"; then | |
| return 0 | |
| fi | |
| fi | |
| sleep 2 | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/smoke-test-workspace-recovery.sh` around lines 199 - 206, The inner
docker exec shell command uses single quotes around '${file_path}', which breaks
when file_path contains single quotes; update the docker exec invocation in the
while loop to run sh -lc with double-quoted command and escape the embedded
quotes around the variable (e.g., sh -lc "test -f \"${file_path}\"") so
file_path is safely quoted; apply this change where docker exec
"${container_name}" sh -lc "..." is used to check for the file.
Comment on lines
+265
to
+271
| echo "Sentinel file did not appear in ${NEW_EXECUTOR_NAME}" >&2 | ||
| echo "--- backend log ---" | ||
| tail -n 120 "${BACKEND_LOG}" | rg 'RecoveryService|restore|archive|sandbox|ExecutionDispatcher|'"${TASK_ID}" | ||
| echo "--- executor-manager log ---" | ||
| tail -n 120 "${EXECUTOR_MANAGER_LOG}" | rg 'restore|archive|sandbox|heartbeat|'"${TASK_ID}" | ||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
rg exit code may truncate debug output due to set -e.
If rg finds no matches, it exits with code 1, which with set -e would terminate the script before exit 1. Add || true to ensure all debug output is printed.
🛠️ Proposed fix
if ! wait_for_file_in_container "${NEW_EXECUTOR_NAME}" "${TARGET_FILE}"; then
echo "Sentinel file did not appear in ${NEW_EXECUTOR_NAME}" >&2
echo "--- backend log ---"
- tail -n 120 "${BACKEND_LOG}" | rg 'RecoveryService|restore|archive|sandbox|ExecutionDispatcher|'"${TASK_ID}"
+ tail -n 120 "${BACKEND_LOG}" | rg 'RecoveryService|restore|archive|sandbox|ExecutionDispatcher|'"${TASK_ID}" || true
echo "--- executor-manager log ---"
- tail -n 120 "${EXECUTOR_MANAGER_LOG}" | rg 'restore|archive|sandbox|heartbeat|'"${TASK_ID}"
+ tail -n 120 "${EXECUTOR_MANAGER_LOG}" | rg 'restore|archive|sandbox|heartbeat|'"${TASK_ID}" || true
exit 1
fi📝 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.
Suggested change
| echo "Sentinel file did not appear in ${NEW_EXECUTOR_NAME}" >&2 | |
| echo "--- backend log ---" | |
| tail -n 120 "${BACKEND_LOG}" | rg 'RecoveryService|restore|archive|sandbox|ExecutionDispatcher|'"${TASK_ID}" | |
| echo "--- executor-manager log ---" | |
| tail -n 120 "${EXECUTOR_MANAGER_LOG}" | rg 'restore|archive|sandbox|heartbeat|'"${TASK_ID}" | |
| exit 1 | |
| fi | |
| echo "Sentinel file did not appear in ${NEW_EXECUTOR_NAME}" >&2 | |
| echo "--- backend log ---" | |
| tail -n 120 "${BACKEND_LOG}" | rg 'RecoveryService|restore|archive|sandbox|ExecutionDispatcher|'"${TASK_ID}" || true | |
| echo "--- executor-manager log ---" | |
| tail -n 120 "${EXECUTOR_MANAGER_LOG}" | rg 'restore|archive|sandbox|heartbeat|'"${TASK_ID}" || true | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/smoke-test-workspace-recovery.sh` around lines 265 - 271, The ripgrep
(rg) commands in the error reporting block (tail ... "${BACKEND_LOG}" | rg ...
"${TASK_ID}" and tail ... "${EXECUTOR_MANAGER_LOG}" | rg ... "${TASK_ID}") can
return non-zero when no matches are found and, combined with set -e, will abort
the script before the explicit exit 1; update those pipelines to tolerate no
matches by appending "|| true" to each rg invocation so the debug output is
always printed and the script reaches the intended exit path.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/services/execution/test_recovery_service.py (1)
79-79: Strengthen async mock assertions to verify full call contract.Checking only
skip_git_cloneis a bit narrow; preferassert_awaited_once_with(...)for_create_sandboxandrestore_workspaceso argument regressions are caught immediately.Also applies to: 125-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/services/execution/test_recovery_service.py` at line 79, Replace the narrow assertion that inspects only skip_git_clone with full async-mock contract checks by using assert_awaited_once_with on the _create_sandbox mock (e.g., create_sandbox_mock.assert_awaited_once_with(...)) and similarly for the restore_workspace mock, supplying the complete expected positional and keyword arguments used in the test (including skip_git_clone and any workspace/id/context args) so the tests verify the full call signature rather than a single kwarg.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/services/execution/test_recovery_service.py`:
- Line 19: Add explicit return type hints "-> None" to the async test functions
in this file (e.g., async def test_recover_raises_when_archive_is_expired) so
they comply with the repo typing rule; update each async test declaration to
include "-> None" (for all async tests in
backend/tests/services/execution/test_recovery_service.py) without changing
function bodies.
---
Nitpick comments:
In `@backend/tests/services/execution/test_recovery_service.py`:
- Line 79: Replace the narrow assertion that inspects only skip_git_clone with
full async-mock contract checks by using assert_awaited_once_with on the
_create_sandbox mock (e.g., create_sandbox_mock.assert_awaited_once_with(...))
and similarly for the restore_workspace mock, supplying the complete expected
positional and keyword arguments used in the test (including skip_git_clone and
any workspace/id/context args) so the tests verify the full call signature
rather than a single kwarg.
🪄 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: 301acad4-e629-43c1-8fca-fdc26cd619a9
📒 Files selected for processing (3)
backend/tests/api/endpoints/internal/test_workspace_archives_api.pybackend/tests/services/execution/test_recovery_service.pybackend/tests/services/execution/test_schedule_helper_recovery.py
✅ Files skipped from review due to trivial changes (2)
- backend/tests/services/execution/test_schedule_helper_recovery.py
- backend/tests/api/endpoints/internal/test_workspace_archives_api.py
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_recover_raises_when_archive_is_expired(): |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add return type hints to async test functions.
All three async test functions should declare -> None to comply with the repo’s Python typing rule.
Proposed diff
`@pytest.mark.asyncio`
-async def test_recover_raises_when_archive_is_expired():
+async def test_recover_raises_when_archive_is_expired() -> None:
@@
`@pytest.mark.asyncio`
-async def test_recover_without_archive_uses_normal_clone():
+async def test_recover_without_archive_uses_normal_clone() -> None:
@@
`@pytest.mark.asyncio`
-async def test_recover_with_archive_continues_when_restore_fails():
+async def test_recover_with_archive_continues_when_restore_fails() -> None:As per coding guidelines, "Python: Follow PEP 8, Black formatter (line length: 88), isort, type hints required".
Also applies to: 42-42, 83-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/services/execution/test_recovery_service.py` at line 19, Add
explicit return type hints "-> None" to the async test functions in this file
(e.g., async def test_recover_raises_when_archive_is_expired) so they comply
with the repo typing rule; update each async test declaration to include "->
None" (for all async tests in
backend/tests/services/execution/test_recovery_service.py) without changing
function bodies.
cocowh
pushed a commit
to cocowh/Wegent
that referenced
this pull request
May 11, 2026
fix: thinking display (wecode-ai#848) See merge request weibo_rd/common/wecode/wegent!1034
cocowh
pushed a commit
to cocowh/Wegent
that referenced
this pull request
May 11, 2026
Feat/workspace archive recovery (wecode-ai#848) See merge request weibo_rd/common/wecode/wegent!1889
This was referenced Jun 11, 2026
2 tasks done
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
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 by CodeRabbit
New Features
Tests