Fix/sandbox skill activation gate - #2577
Closed
FicoHu wants to merge 4 commits into
Closed
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change adds sandbox attachment synchronization, propagates loaded Skills into sandbox metadata, deploys required Skills through the executor, validates attachment downloads, and reports sandbox readiness failures. ChangesSandbox synchronization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ChatService
participant AttachmentSync
participant SandboxManager
participant Backend
participant Executor
ChatService->>AttachmentSync: synchronize current-turn attachments
AttachmentSync->>SandboxManager: create or reuse sandbox
SandboxManager->>Backend: resolve required Skills
SandboxManager->>Executor: deploy required Skills
AttachmentSync->>Backend: download authenticated attachments
AttachmentSync->>SandboxManager: write files and update attachment metadata
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c314acaba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+281
to
+284
| try: | ||
| await self._skill_synchronizer.sync(base_url, task_data, resolved_skills) | ||
| except SandboxSkillSyncError as exc: | ||
| return str(exc) |
There was a problem hiding this comment.
If /v1/skills/sync fails after the container is already ready—for example, because a required skill download returns an error—this returns without deleting the live container, while the caller marks the sandbox record FAILED. A retry therefore skips reuse and submits a new sandbox with the same deterministic executor name, which collides with the orphaned running container and can leave the task unable to create a sandbox even after the original download problem is fixed. Terminate the created container before returning this error, or retain the running record in a retryable state.
Useful? React with 👍 / 👎.
![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: 11
🧹 Nitpick comments (4)
executor_manager/tests/services/test_sandbox_manager.py (1)
594-643: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the reuse failure path.
This test covers only the successful warm reuse. The failure branch in
_create_sandbox_locked(Lines 180-183) callsset_failedon a healthy container and persists it. That branch is the one I flagged as a critical issue inexecutor_manager/services/sandbox/manager.py. Add a test that makessyncraiseSandboxSkillSyncErrorand asserts the resulting sandbox status and the returned error, so the corrected behavior is locked in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor_manager/tests/services/test_sandbox_manager.py` around lines 594 - 643, The existing test only verifies successful warm-sandbox skill synchronization; add a failure-path test for _create_sandbox_locked. Reuse the same healthy-container setup, make _skill_synchronizer.sync raise SandboxSkillSyncError, then assert the returned sandbox is marked failed and persisted and that create_sandbox returns the synchronization error.executor/src/agents/runtime_capabilities.rs (1)
371-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared deployment and validation flow.
deploy_request_skillsandsync_skills_for_requestrepeat the same five steps: collect required names, resolve the primary bot, build the plan, deploy, and validate missing required Skills. Only the skills directory and the success value differ. Extract one helper that returns theSkillDeploymentReportand the plan, then let each caller shape its own result.The coding guidelines require reuse of shared logic instead of duplication.
♻️ Suggested shape
async fn deploy_and_validate( request: &ExecutionRequest, skills_dir: PathBuf, clear_cache: bool, ) -> Result<Option<(SkillDeploymentPlan, SkillDeploymentReport)>, String> { // shared: required_skill_names, primary_bot, build plan, deploy_skills, // missing_required_skills; returns Ok(None) when there is nothing to deploy. }As per coding guidelines: "Before adding code, search for and reuse existing components, services, utilities, and patterns; extract shared logic instead of duplicating it."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/agents/runtime_capabilities.rs` around lines 371 - 461, Extract the duplicated deployment and required-Skill validation flow from deploy_request_skills and sync_skills_for_request into a shared async helper, such as deploy_and_validate, accepting the request, skills directory, and clear_cache flag. Have it resolve required skills and the primary bot, build the plan, deploy skills, validate missing requirements, and return the plan/report pair, using Ok(None) when no deployment is needed; preserve existing errors for missing bots, plans, and failed required skills. Update both callers to use the helper and retain their distinct success/result shaping.Source: Coding guidelines
backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py (2)
87-96: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the command consumes the environment variables.
The test passes if a future command omits the Authorization header but still receives the same
envsdictionary. Assert thatcmdcontains$WEGENT_ATTACHMENT_TOKEN,$WEGENT_ATTACHMENT_SAVE_PATH, and$WEGENT_ATTACHMENT_DOWNLOAD_URL.Proposed test addition
assert "task-token" not in calls[0]["cmd"] assert "/home/user/report.csv" not in calls[0]["cmd"] + assert "$WEGENT_ATTACHMENT_TOKEN" in calls[0]["cmd"] + assert "$WEGENT_ATTACHMENT_SAVE_PATH" in calls[0]["cmd"] + assert "$WEGENT_ATTACHMENT_DOWNLOAD_URL" in calls[0]["cmd"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py` around lines 87 - 96, Update the assertions in the test covering the attachment download command to verify that calls[0]["cmd"] references $WEGENT_ATTACHMENT_TOKEN, $WEGENT_ATTACHMENT_SAVE_PATH, and $WEGENT_ATTACHMENT_DOWNLOAD_URL, while preserving the existing checks that raw secret and path values are absent.
41-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type hints to the new test hooks and fake methods.
backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py#L41-L64: Annotatemonkeypatch,**kwargs, and the fake manager return type.chat_shell/tests/test_sandbox_skill_identity.py#L54-L60: Annotatemonkeypatch.chat_shell/tests/test_sandbox_attachment_sync.py#L46-L63: Annotate fake async method parameters and return types.As per coding guidelines, “Python code must follow PEP 8, Black with 88-column formatting, isort, and type hints.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py` around lines 41 - 64, Update the test hooks and fake methods with explicit type hints: in backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py lines 41-64, annotate monkeypatch, FakeCommands.run kwargs, FakeManager.get_or_create_sandbox kwargs, and its return type; in chat_shell/tests/test_sandbox_skill_identity.py lines 54-60, annotate monkeypatch; and in chat_shell/tests/test_sandbox_attachment_sync.py lines 46-63, annotate all fake async method parameters and return types. Preserve behavior and format the annotations with the project’s typing conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/services/sandbox_file_syncer.py`:
- Line 43: Define a single canonical fallback filename and reuse it in both the
sandbox path builder and the attachment path builder, including the wrapper
around sanitize_attachment_filename in SandboxFileSyncer. Ensure empty, dot,
dot-dot, and control-only names produce matching paths across both layers, and
add a cross-layer regression test covering these inputs.
- Around line 280-281: Update the docstring sentence describing the asynchronous
task so “and handles all exceptions internally” remains grammatically connected:
remove the period after “task” or split the text into two complete sentences.
In `@backend/tests/init_data/skills/sandbox/test_provider.py`:
- Around line 30-45: Add the explicit None return annotation to
test_prepare_base_params_includes_load_skill_tool while leaving its existing
test logic unchanged.
In `@chat_shell/chat_shell/services/sandbox_attachment_sync.py`:
- Around line 262-279: Update _create_task_sandbox to resolve the persistent
task sandbox through the shared lifecycle path used by sandbox tools instead of
directly creating a new sandbox via get_or_create_sandbox. Preserve the existing
task configuration and return the shared sandbox identity so attachment
synchronization and subsequent tools use the same sandbox. Add a regression test
covering attachment synchronization, asserting the tool receives the identical
sandbox ID and can access the synchronized file.
- Around line 198-204: Update the expected_size validation in the attachment
synchronization check to reject only missing or negative sizes, while allowing
expected_size == 0. Preserve the existing sandbox file lookup and size
comparison behavior for valid zero-byte and positive-size attachments.
In `@executor_manager/services/sandbox/manager.py`:
- Around line 174-183: Update the reused-sandbox path around
_prepare_sandbox_skills so a skill sync error returns existing_sandbox and
skill_error without calling set_failed or persisting a FAILED status. If the
sandbox must not remain reusable, explicitly terminate its container before
returning, while preserving healthy sandbox reuse for transient sync failures.
- Around line 292-303: Update _prepare_sandbox_skills to read
sandbox.metadata["synced_required_skills"] and return None before resolving or
syncing when the current required Skill set matches the stored set. Only call
_skill_synchronizer.resolve and sync when the required set has changed, then
update the metadata after a successful sync while preserving
SandboxSkillSyncError handling.
In `@executor_manager/services/sandbox/skill_sync.py`:
- Around line 109-114: Update the error handling in the resolve branches around
the response status check and decoded result handling to keep upstream bodies
out of SandboxSkillSyncError messages. Log response.text or result at error
level for diagnostics, then raise errors containing only a clear failure
description and HTTP status code; apply this consistently to both the Backend
response path and executor result path.
In `@executor/src/agents/runtime_capabilities.rs`:
- Around line 463-493: Update required_skill_names and missing_required_skills
so advisory preload_skills are excluded from hard-requirement validation; use
only required_skills when determining execution-blocking failures. Preserve
preload deployment behavior while preventing missing references, download
errors, or absent SKILL.md files for preloads from failing Claude execution.
- Around line 411-421: Update sync_skills_for_request to resolve skills_dir
through the runtime skills-directory resolver used by Claude’s SKILLS_DIR
configuration, rather than building it from claude_config_dir(&request, None).
Ensure standalone project-zero requests write to <task_dir>/.claude/skills while
preserving existing request and bot handling.
In `@shared/utils/attachment_block.py`:
- Around line 138-140: Update build_attachment_header to encode the sanitized
filename with a dedicated display-name encoder before inserting it into the
bracket- and pipe-delimited prompt block; do not rely on
sanitize_attachment_filename alone. Ensure the encoder safely escapes ], [, and
|, and add tests covering each delimiter and prompt-injection-style filenames.
---
Nitpick comments:
In `@backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py`:
- Around line 87-96: Update the assertions in the test covering the attachment
download command to verify that calls[0]["cmd"] references
$WEGENT_ATTACHMENT_TOKEN, $WEGENT_ATTACHMENT_SAVE_PATH, and
$WEGENT_ATTACHMENT_DOWNLOAD_URL, while preserving the existing checks that raw
secret and path values are absent.
- Around line 41-64: Update the test hooks and fake methods with explicit type
hints: in
backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py lines
41-64, annotate monkeypatch, FakeCommands.run kwargs,
FakeManager.get_or_create_sandbox kwargs, and its return type; in
chat_shell/tests/test_sandbox_skill_identity.py lines 54-60, annotate
monkeypatch; and in chat_shell/tests/test_sandbox_attachment_sync.py lines
46-63, annotate all fake async method parameters and return types. Preserve
behavior and format the annotations with the project’s typing conventions.
In `@executor_manager/tests/services/test_sandbox_manager.py`:
- Around line 594-643: The existing test only verifies successful warm-sandbox
skill synchronization; add a failure-path test for _create_sandbox_locked. Reuse
the same healthy-container setup, make _skill_synchronizer.sync raise
SandboxSkillSyncError, then assert the returned sandbox is marked failed and
persisted and that create_sandbox returns the synchronization error.
In `@executor/src/agents/runtime_capabilities.rs`:
- Around line 371-461: Extract the duplicated deployment and required-Skill
validation flow from deploy_request_skills and sync_skills_for_request into a
shared async helper, such as deploy_and_validate, accepting the request, skills
directory, and clear_cache flag. Have it resolve required skills and the primary
bot, build the plan, deploy skills, validate missing requirements, and return
the plan/report pair, using Ok(None) when no deployment is needed; preserve
existing errors for missing bots, plans, and failed required skills. Update both
callers to use the helper and retain their distinct success/result shaping.
🪄 Autofix
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 Plus
Run ID: 8f70bf2b-3f74-4abe-84fb-e0f4a29c5139
📒 Files selected for processing (26)
backend/app/services/sandbox_file_syncer.pybackend/init_data/skills/sandbox/download_attachment_tool.pybackend/init_data/skills/sandbox/provider.pybackend/tests/init_data/skills/sandbox/test_download_attachment_tool.pybackend/tests/init_data/skills/sandbox/test_provider.pybackend/tests/services/test_sandbox_file_syncer.pychat_shell/chat_shell/services/chat_service.pychat_shell/chat_shell/services/sandbox_attachment_sync.pychat_shell/chat_shell/skills/context.pychat_shell/chat_shell/tools/sandbox/_base.pychat_shell/chat_shell/tools/skill_factory.pychat_shell/tests/test_sandbox_attachment_sync.pychat_shell/tests/test_sandbox_skill_identity.pyexecutor/src/agents/mod.rsexecutor/src/agents/runtime_capabilities.rsexecutor/src/server/mod.rsexecutor/tests/agent_runtime_capabilities_contract.rsexecutor/tests/http_contract.rsexecutor_manager/routers/e2b.pyexecutor_manager/services/sandbox/manager.pyexecutor_manager/services/sandbox/skill_sync.pyexecutor_manager/tests/routers/test_e2b_sandbox_creation.pyexecutor_manager/tests/services/test_sandbox_manager.pyexecutor_manager/tests/services/test_sandbox_skill_sync.pyshared/tests/utils/test_attachment_block.pyshared/utils/attachment_block.py
Comment on lines
+280
to
281
| This function is designed to be scheduled as an asynchronous task. | ||
| and handles all exceptions internally. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the updated docstring sentence.
Line 280 ends the sentence before and handles all exceptions internally on Line 281. Remove the period after task, or split the text into two complete sentences.
Proposed fix
- This function is designed to be scheduled as an asynchronous task.
- and handles all exceptions internally.
+ This function is designed to be scheduled as an asynchronous task
+ and handles all exceptions internally.📝 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
| This function is designed to be scheduled as an asynchronous task. | |
| and handles all exceptions internally. | |
| This function is designed to be scheduled as an asynchronous task | |
| and handles all exceptions internally. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/sandbox_file_syncer.py` around lines 280 - 281, Update
the docstring sentence describing the asynchronous task so “and handles all
exceptions internally” remains grammatically connected: remove the period after
“task” or split the text into two complete sentences.
Comment on lines
+30
to
+45
| def test_prepare_base_params_includes_load_skill_tool(): | ||
| """Sandbox provider should expose active Skill state to sandbox creation.""" | ||
| provider = SandboxToolProvider() | ||
| load_skill_tool = object() | ||
| context = SkillToolContext( | ||
| task_id=1, | ||
| subtask_id=2, | ||
| user_id=3, | ||
| db_session=None, | ||
| ws_emitter=None, | ||
| load_skill_tool=load_skill_tool, | ||
| ) | ||
|
|
||
| params = provider._prepare_base_params(context, {}) | ||
|
|
||
| assert params["load_skill_tool"] is load_skill_tool |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a return annotation to the new test.
test_prepare_base_params_includes_load_skill_tool is new and omits -> None.
Proposed fix
-def test_prepare_base_params_includes_load_skill_tool():
+def test_prepare_base_params_includes_load_skill_tool() -> None:As per coding guidelines, Python code must follow PEP 8, Black with 88-column formatting, isort, and type hints.
📝 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
| def test_prepare_base_params_includes_load_skill_tool(): | |
| """Sandbox provider should expose active Skill state to sandbox creation.""" | |
| provider = SandboxToolProvider() | |
| load_skill_tool = object() | |
| context = SkillToolContext( | |
| task_id=1, | |
| subtask_id=2, | |
| user_id=3, | |
| db_session=None, | |
| ws_emitter=None, | |
| load_skill_tool=load_skill_tool, | |
| ) | |
| params = provider._prepare_base_params(context, {}) | |
| assert params["load_skill_tool"] is load_skill_tool | |
| def test_prepare_base_params_includes_load_skill_tool() -> None: | |
| """Sandbox provider should expose active Skill state to sandbox creation.""" | |
| provider = SandboxToolProvider() | |
| load_skill_tool = object() | |
| context = SkillToolContext( | |
| task_id=1, | |
| subtask_id=2, | |
| user_id=3, | |
| db_session=None, | |
| ws_emitter=None, | |
| load_skill_tool=load_skill_tool, | |
| ) | |
| params = provider._prepare_base_params(context, {}) | |
| assert params["load_skill_tool"] is load_skill_tool |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/init_data/skills/sandbox/test_provider.py` around lines 30 -
45, Add the explicit None return annotation to
test_prepare_base_params_includes_load_skill_tool while leaving its existing
test logic unchanged.
Source: Coding guidelines
Comment on lines
+198
to
+204
| if not expected_size or expected_size < 0: | ||
| return False | ||
| try: | ||
| file_info = await sandbox.files.get_info(path) | ||
| except Exception: | ||
| return False | ||
| return int(file_info.size) == expected_size |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reuse valid zero-byte attachments.
expected_size == 0 is a valid attachment size. The current truthiness check always returns False for that case and downloads the file again.
Proposed fix
- if not expected_size or expected_size < 0:
+ if expected_size is None or expected_size < 0:
return False📝 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
| if not expected_size or expected_size < 0: | |
| return False | |
| try: | |
| file_info = await sandbox.files.get_info(path) | |
| except Exception: | |
| return False | |
| return int(file_info.size) == expected_size | |
| if expected_size is None or expected_size < 0: | |
| return False | |
| try: | |
| file_info = await sandbox.files.get_info(path) | |
| except Exception: | |
| return False | |
| return int(file_info.size) == expected_size |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 202-202: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@chat_shell/chat_shell/services/sandbox_attachment_sync.py` around lines 198 -
204, Update the expected_size validation in the attachment synchronization check
to reject only missing or negative sizes, while allowing expected_size == 0.
Preserve the existing sandbox file lookup and size comparison behavior for valid
zero-byte and positive-size attachments.
Comment on lines
+262
to
+279
| async def _create_task_sandbox(request: ExecutionRequest) -> tuple[Any, str | None]: | ||
| """Create or reconnect to the sandbox using the active skill's config.""" | ||
| from chat_shell.tools.sandbox._base import SandboxManager | ||
|
|
||
| sandbox_config = _sandbox_skill_config(request) | ||
| manager = SandboxManager.get_instance( | ||
| task_id=request.task_id, | ||
| user_id=request.user_id, | ||
| user_name=request.user_name, | ||
| bot_config=sandbox_config.get("bot_config", []), | ||
| auth_token=request.auth_token, | ||
| skill_identity_token=request.skill_identity_token, | ||
| ) | ||
| return await manager.get_or_create_sandbox( | ||
| shell_type=sandbox_config.get("default_shell_type", "ClaudeCode"), | ||
| workspace_ref=None, | ||
| task_type="sandbox", | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use the sandbox that tools will execute in.
SandboxManager.get_or_create_sandbox() creates a fresh sandbox for each call. This function writes attachments into one sandbox before tool creation. Later sandbox tools call the same method and receive another sandbox. The prompt then advertises paths that do not exist in the tool sandbox.
Create or resolve the persistent task sandbox through the shared lifecycle path. Add a regression test that synchronizes an attachment and then verifies a sandbox tool receives the same sandbox ID and file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@chat_shell/chat_shell/services/sandbox_attachment_sync.py` around lines 262 -
279, Update _create_task_sandbox to resolve the persistent task sandbox through
the shared lifecycle path used by sandbox tools instead of directly creating a
new sandbox via get_or_create_sandbox. Preserve the existing task configuration
and return the shared sandbox identity so attachment synchronization and
subsequent tools use the same sandbox. Add a regression test covering attachment
synchronization, asserting the tool receives the identical sandbox ID and can
access the synchronized file.
Comment on lines
+292
to
+303
| async def _prepare_sandbox_skills( | ||
| self, sandbox: Sandbox, base_url: str | ||
| ) -> Optional[str]: | ||
| """Synchronize newly active Skills before reusing a running sandbox.""" | ||
| try: | ||
| resolved = await self._skill_synchronizer.resolve(sandbox) | ||
| task = self._build_sandbox_task(sandbox, resolved) | ||
| await self._skill_synchronizer.sync(base_url, task, resolved) | ||
| except SandboxSkillSyncError as exc: | ||
| return str(exc) | ||
| sandbox.metadata["synced_required_skills"] = resolved.required_skills | ||
| return None |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Skip the sync round trip when the required Skills are already synchronized.
_prepare_sandbox_skills runs on every reuse of a warm sandbox. It always calls resolve against Backend and, because ResolvedTaskSkills.needs_sync is true whenever any Skill exists, it always calls sync against the executor. SKILL_SYNC_TIMEOUT defaults to 180 seconds, so this adds two blocking calls to every sandbox activation.
sandbox.metadata["synced_required_skills"] is written on Line 302 but never read. Use it to short-circuit when the required set has not changed.
⚡ Proposed fix
async def _prepare_sandbox_skills(
self, sandbox: Sandbox, base_url: str
) -> Optional[str]:
"""Synchronize newly active Skills before reusing a running sandbox."""
+ try:
+ required = required_skill_names(sandbox.metadata)
+ except SandboxSkillSyncError as exc:
+ return str(exc)
+ if required and required == sandbox.metadata.get("synced_required_skills"):
+ return None
try:
resolved = await self._skill_synchronizer.resolve(sandbox)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor_manager/services/sandbox/manager.py` around lines 292 - 303, Update
_prepare_sandbox_skills to read sandbox.metadata["synced_required_skills"] and
return None before resolving or syncing when the current required Skill set
matches the stored set. Only call _skill_synchronizer.resolve and sync when the
required set has changed, then update the metadata after a successful sync while
preserving SandboxSkillSyncError handling.
Comment on lines
+109
to
+114
| if response.status_code != 200: | ||
| body = response.text[:300] | ||
| raise SandboxSkillSyncError( | ||
| "Failed to resolve task Skills: " | ||
| f"HTTP {response.status_code}; body={body}" | ||
| ) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not put raw upstream response bodies into the error that reaches the API client.
SandboxSkillSyncError messages embed response.text from Backend and from the executor. SandboxManager converts the exception to a string, and executor_manager/routers/e2b.py returns that string in the HTTP 503 detail field. Any header echo, stack trace, or token fragment in an upstream error body is then forwarded to the SDK caller.
Log the body at error level and raise a message that names the failure and the status code only.
🛡️ Proposed fix
if response.status_code != 200:
- body = response.text[:500]
- raise SandboxSkillSyncError(
- "Sandbox Skill deployment failed: "
- f"HTTP {response.status_code}; body={body}"
- )
+ logger.error(
+ "[SandboxSkillSync] Deployment failed task_id=%s status=%s body=%s",
+ task.get("task_id"),
+ response.status_code,
+ response.text[:500],
+ )
+ raise SandboxSkillSyncError(
+ f"Sandbox Skill deployment failed: HTTP {response.status_code}"
+ )Apply the same change to the resolve branch at Line 109 and to Line 163, where the whole decoded result is interpolated.
Also applies to: 146-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor_manager/services/sandbox/skill_sync.py` around lines 109 - 114,
Update the error handling in the resolve branches around the response status
check and decoded result handling to keep upstream bodies out of
SandboxSkillSyncError messages. Log response.text or result at error level for
diagnostics, then raise errors containing only a clear failure description and
HTTP status code; apply this consistently to both the Backend response path and
executor result path.
Comment on lines
+411
to
+421
| pub async fn sync_skills_for_request(request: ExecutionRequest) -> Result<Value, String> { | ||
| let required_skills = required_skill_names(&request); | ||
| let Some(primary_bot) = primary_bot(&request) else { | ||
| return required_skills | ||
| .is_empty() | ||
| .then(|| json!({"success": true, "skill_count": 0, "failed_skills": []})) | ||
| .ok_or_else(|| "required Skills cannot be deployed without a bot".to_owned()); | ||
| }; | ||
| let skills_dir = claude_config_dir(&request, None) | ||
| .ok_or_else(|| "Claude config directory is unavailable".to_owned())? | ||
| .join("skills"); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether SKILLS_DIR is set for sandbox/executor containers.
set -euo pipefail
rg -n 'SKILLS_DIR' --hidden -g '!target' | head -60Repository: wecode-ai/Wegent
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- runtime_capabilities.rs structure ---'
ast-grep outline executor/src/agents/runtime_capabilities.rs
printf '%s\n' '--- relevant implementation ---'
sed -n '250,330p;390,450p' executor/src/agents/runtime_capabilities.rs
printf '%s\n' '--- all SKILLS_DIR references, including ignored files ---'
rg -n --hidden -uuu 'SKILLS_DIR' -g '!target' . || true
printf '%s\n' '--- skills directory helpers and call sites ---'
rg -n --hidden -uuu 'claude_config_dir|prepare_claude_runtime|sync_skills_for_request|skills_dir' executor -g '!target' | head -160Repository: wecode-ai/Wegent
Length of output: 32177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Claude command preparation and SKILLS_DIR injection ---'
sed -n '550,675p' executor/src/agents/claude_code.rs
printf '%s\n' '--- runtime preparation call sites ---'
sed -n '280,335p;380,425p' executor/src/agents/mod.rs
printf '%s\n' '--- sync endpoint and command contract assertions ---'
sed -n '140,175p' executor/src/server/mod.rs
sed -n '120,165p;640,710p;770,835p' executor/tests/agent_command_contract.rs
printf '%s\n' '--- directory resolution implementation ---'
sed -n '600,635p' executor/src/agents/claude_code.rs
rg -n 'sync_skills|capabilit|/skills|local.*sync' executor/src/server executor/tests -g '*.rs' | head -120Repository: wecode-ai/Wegent
Length of output: 33263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- skill sync HTTP contract ---'
sed -n '70,175p' executor/tests/http_contract.rs
printf '%s\n' '--- runtime capability contract around sync ---'
sed -n '250,335p' executor/tests/agent_runtime_capabilities_contract.rs
printf '%s\n' '--- all callers or clients of the sync route ---'
rg -n --hidden -uuu 'v1/skills/sync|skills/sync|sync_skills_for_request|syncSkills' . -g '!target' | head -160
printf '%s\n' '--- request shape helpers and standalone skill selection ---'
rg -n 'required_skills|skill_names|standalone_chat_workspace|project_id' executor/src executor/tests -g '*.rs' | head -180Repository: wecode-ai/Wegent
Length of output: 26302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sandbox skill sync client ---'
sed -n '1,220p' executor_manager/services/sandbox/skill_sync.py
printf '%s\n' '--- standalone runtime request construction ---'
sed -n '110,175p' executor/src/runtime_work/handler.rs
sed -n '1400,1445p' executor/src/local/capabilities.rs
printf '%s\n' '--- command planner environment application ---'
rg -n 'apply_claude_workspace_environment|build_claude_command|CommandSpec' executor/src/agents/claude_code.rs executor/src/agents -g '*.rs' | head -100
sed -n '430,555p' executor/src/agents/claude_code.rsRepository: wecode-ai/Wegent
Length of output: 19587
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete Claude command construction order ---'
sed -n '280,350p' executor/src/agents/claude_code.rs
printf '%s\n' '--- sandbox synchronizer call sites and task lifecycle ---'
rg -n --hidden -uuu 'SandboxSkillSynchronizer|apply_to_task|\.sync\(' executor_manager -g '*.py' | head -120
printf '%s\n' '--- task fields passed into sandbox synchronization ---'
rg -n --hidden -uuu 'standalone_chat_workspace|skill_names|required_skills|preload_skills' executor_manager -g '*.py' | head -180
printf '%s\n' '--- standalone skill deployment contract ---'
sed -n '360,475p' executor/tests/agent_process_engine_contract.rsRepository: wecode-ai/Wegent
Length of output: 12846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sandbox manager synchronization flow ---'
sed -n '245,315p;450,490p' executor_manager/services/sandbox/manager.py
printf '%s\n' '--- process environment key filtering ---'
rg -n 'fn is_process_env_key|is_process_env_key' executor/src/agents/claude_code.rs executor/src -g '*.rs'
sed -n '870,910p' executor/src/agents/claude_code.rs
printf '%s\n' '--- read-only path-contract verifier ---'
python3 - <<'PY'
from pathlib import Path
runtime = Path("executor/src/agents/runtime_capabilities.rs").read_text()
claude = Path("executor/src/agents/claude_code.rs").read_text()
sync = Path("executor_manager/services/sandbox/skill_sync.py").read_text()
checks = {
"runtime deployment reads explicit command SKILLS_DIR":
'.envs()' in runtime and '.get("SKILLS_DIR")' in runtime,
"runtime deployment falls back to config skills":
'unwrap_or_else(|| config_dir.join("skills"))' in runtime,
"sync endpoint uses config skills":
'claude_config_dir(&request, None)' in runtime
and '.join("skills")' in runtime,
"command builder applies workspace environment":
'apply_claude_workspace_environment(spec, request, task_dir.as_ref())' in claude,
"workspace environment sets SKILLS_DIR when absent":
'if !spec.envs().contains_key("SKILLS_DIR")' in claude,
"standalone task skills use task directory":
'return task_dir.join(".claude/skills")' in claude,
"sandbox sync client only adds skill fields":
'"skill_names": list(self.skills)' in sync
and '"required_skills": list(self.required_skills)' in sync,
}
for name, result in checks.items():
print(f"{'PASS' if result else 'FAIL'}: {name}")
PYRepository: wecode-ai/Wegent
Length of output: 6365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sandbox task payload construction ---'
sed -n '400,480p' executor_manager/services/sandbox/manager.py
printf '%s\n' '--- allowed model environment keys ---'
sed -n '940,980p' executor/src/agents/claude_code.rs
printf '%s\n' '--- sandbox task construction tests for runtime selectors ---'
rg -n -C 5 'standalone_chat_workspace|project_id|SKILLS_DIR|skill_names' executor_manager/tests/services/test_sandbox_manager.py executor/tests/agent_process_engine_contract.rsRepository: wecode-ai/Wegent
Length of output: 8929
Align /v1/skills/sync with the runtime skills directory. For standalone project-zero requests with task Skills, sync writes to ~/.claude/skills, while Claude reads <task_dir>/.claude/skills through SKILLS_DIR. Reuse the runtime directory resolver for both paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/agents/runtime_capabilities.rs` around lines 411 - 421, Update
sync_skills_for_request to resolve skills_dir through the runtime
skills-directory resolver used by Claude’s SKILLS_DIR configuration, rather than
building it from claude_config_dir(&request, None). Ensure standalone
project-zero requests write to <task_dir>/.claude/skills while preserving
existing request and bot handling.
Comment on lines
+463
to
+493
| fn required_skill_names(request: &ExecutionRequest) -> Vec<String> { | ||
| let mut names = BTreeSet::new(); | ||
| for key in ["required_skills", "preload_skills"] { | ||
| if let Some(values) = request.extra.get(key).and_then(Value::as_array) { | ||
| names.extend( | ||
| values | ||
| .iter() | ||
| .filter_map(Value::as_str) | ||
| .map(str::trim) | ||
| .filter(|value| !value.is_empty()) | ||
| .map(ToOwned::to_owned), | ||
| ); | ||
| } | ||
| } | ||
| names.into_iter().collect() | ||
| } | ||
|
|
||
| fn missing_required_skills( | ||
| required_skills: &[String], | ||
| plan: &SkillDeploymentPlan, | ||
| report: &SkillDeploymentReport, | ||
| ) -> Vec<String> { | ||
| required_skills | ||
| .iter() | ||
| .filter(|skill| { | ||
| !plan.skills.contains(skill) | ||
| || report.failed_skills.contains(skill) | ||
| || !plan.skills_dir.join(skill).join("SKILL.md").is_file() | ||
| }) | ||
| .cloned() | ||
| .collect() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace how preload_skills is populated and whether it always lands in the deployment plan.
set -euo pipefail
rg -n --type=py -C4 '\bpreload_skills\b' | head -120
rg -n --type=rust -C6 'preload_skills' executor/src | head -120Repository: wecode-ai/Wegent
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tracked files ---'
git ls-files | rg '(^|/)(executor/src/agents/runtime_capabilities\.rs|executor/src/agents/mod\.rs|.*skill.*|.*runtime.*)$' | head -200
printf '%s\n' '--- preload_skills references ---'
rg -n -i -C5 'preload[_-]?skills|required[_-]?skills' . --glob '!target/**' --glob '!node_modules/**' | head -300
printf '%s\n' '--- runtime_capabilities outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline executor/src/agents/runtime_capabilities.rs
fi
printf '%s\n' '--- relevant source ---'
sed -n '400,530p' executor/src/agents/runtime_capabilities.rs
rg -n -C8 'prepare_claude_runtime|missing_required_skills|required_skill_names|SkillDeploymentPlan|SkillDeploymentReport' executor/src/agents/runtime_capabilities.rs executor/src/agents/mod.rsRepository: wecode-ai/Wegent
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all production references ---'
rg -n 'preload_skills|required_skills|preload_skill_refs' \
backend/app chat_shell executor executor_manager shared \
--glob '!**/tests/**' --glob '!**/test_*.py' --glob '!**/*_test.rs' \
| head -400
printf '%s\n' '--- executor runtime preparation ---'
sed -n '250,430p' executor/src/agents/runtime_capabilities.rs
sed -n '760,930p' executor/src/agents/runtime_capabilities.rs
sed -n '1120,1230p' executor/src/agents/runtime_capabilities.rs
printf '%s\n' '--- request model and conversion ---'
sed -n '80,135p' shared/models/execution.py
sed -n '170,210p' shared/models/openai_converter.py
sed -n '340,380p' shared/models/openai_converter.py
printf '%s\n' '--- backend population contexts ---'
rg -l 'preload_skills' backend/app chat_shell executor_manager shared \
--glob '!**/tests/**' | while read -r f; do
echo "### $f"
rg -n -C10 'preload_skills|required_skills|preload_skill_refs' "$f"
doneRepository: wecode-ai/Wegent
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- executor skill plan construction ---'
sed -n '1,230p' executor/src/services/skill_deployer.rs
rg -n -C12 'struct SkillDeploymentPlan|fn build_skill_deployment_plan|build_skill_deployment_plan\(' executor/src
printf '%s\n' '--- request-builder resolution ---'
sed -n '220,315p' backend/app/services/execution/request_builder.py
sed -n '485,590p' backend/app/services/execution/request_builder.py
sed -n '1190,1475p' backend/app/services/execution/request_builder.py
printf '%s\n' '--- task skill resolver ---'
sed -n '1,350p' backend/app/services/adapters/task_kinds/task_skills_resolver.py
printf '%s\n' '--- direct task/request schemas and executor input ---'
sed -n '340,380p' backend/app/schemas/task.py
sed -n '130,185p' executor/src/server/mod.rs
rg -n -C8 'ExecutionRequest|request.extra|skill_names|skill_refs' executor/src/server executor/src/services/api_client.rsRepository: wecode-ai/Wegent
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- request construction and additional skill sources ---'
sed -n '300,435p' backend/app/services/execution/request_builder.py
sed -n '3450,3500p' backend/app/services/runtime_work_service.py
sed -n '350,390p' backend/app/services/chat/trigger/unified.py
sed -n '520,575p' backend/app/services/chat/trigger/unified.py
sed -n '820,855p' backend/app/services/chat/trigger/unified.py
printf '%s\n' '--- task resolver completion and unresolved branches ---'
sed -n '175,330p' backend/app/services/adapters/task_kinds/task_skills_resolver.py
sed -n '330,370p' backend/app/services/adapters/task_kinds/task_skills_resolver.py
printf '%s\n' '--- preload semantics in docs and tests ---'
rg -n -C4 'preload_skills|preload skill|preload.*Skill|best.?effort|advisory' \
docs backend/tests chat_shell/tests executor/tests executor_manager/tests \
--glob '!**/target/**' | head -300
printf '%s\n' '--- static write-site inventory ---'
python3 - <<'PY'
from pathlib import Path
for root in ("backend/app", "chat_shell/chat_shell", "executor_manager", "shared", "executor/src"):
for path in Path(root).rglob("*"):
if path.is_file() and path.suffix in {".py", ".rs"}:
text = path.read_text(errors="replace")
hits = [i + 1 for i, line in enumerate(text.splitlines()) if "preload_skills" in line]
if hits:
print(f"{path}: {','.join(map(str, hits))}")
PYRepository: wecode-ai/Wegent
Length of output: 48450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- consumers of task skill resolution ---'
rg -n -C8 'resolve_task_skills|get_task_skills|tasks/.*/skills|task.*skills' \
backend/app executor/src executor_manager --glob '!**/tests/**' | head -300
printf '%s\n' '--- executor task-skill fetch path ---'
rg -n -C12 'task_skills|skill_refs|preload_skill_refs' executor/src \
--glob '!**/target/**' | head -400
printf '%s\n' '--- deployment contract tests ---'
sed -n '1,180p' executor/tests/skill_deployer_contract.rs
sed -n '300,390p' executor/tests/agent_runtime_capabilities_contract.rs
rg -n -C8 'missing_required_skills|required Skill deployment|preload.*failed|failed.*preload|sync_skills' executor/tests
printf '%s\n' '--- runtime-flow contract ---'
sed -n '1,115p' docs/en/wegent/developer-guide/skill-runtime-flow.md
printf '%s\n' '--- read-only behavioral model ---'
python3 - <<'PY'
from collections import OrderedDict
def collect(bot_skills, request_skill_names, request_preload):
names = []
for group in (bot_skills, request_skill_names, request_preload):
for name in group:
if name not in names:
names.append(name)
return names
cases = [
([], [], ["preload-only"]),
(["bot-skill"], [], ["preload-only"]),
(["bot-skill"], ["resolved-skill"], ["preload-only"]),
]
for bot, request, preload in cases:
plan = collect(bot, request, preload)
print({"bot_skills": bot, "request_skill_names": request,
"preload_skills": preload, "plan_skills": plan,
"preload_in_plan": all(x in plan for x in preload)})
PYRepository: wecode-ai/Wegent
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- production ExecutionRequest construction ---'
rg -n -C4 'ExecutionRequest\(' backend/app chat_shell/chat_shell executor_manager shared \
--glob '!**/tests/**' --glob '!**/test_*.py' | head -300
printf '%s\n' '--- download fallback for unresolved names ---'
sed -n '1000,1068p' executor/src/agents/runtime_capabilities.rs
printf '%s\n' '--- focused static evidence ---'
python3 - <<'PY'
from pathlib import Path
runtime = Path("executor/src/agents/runtime_capabilities.rs").read_text()
deployer = Path("executor/src/services/skill_deployer.rs").read_text()
resolver = Path("backend/app/services/adapters/task_kinds/task_skills_resolver.py").read_text()
assert 'for key in ["required_skills", "preload_skills"]' in runtime
assert 'report.failed_skills.contains(skill)' in runtime
assert 'add_skill_names(&mut names, request.extra.get("preload_skills"));' in deployer
assert 'all_preload_skills.update(ghost_crd.spec.preload_skills)' in resolver
print("required-set merge: present")
print("preload names added to deployment plan: present")
print("task resolver preserves Ghost preload names independently of skill resolution: present")
PY
printf '%s\n' '--- exact behavior model of the changed predicates ---'
python3 - <<'PY'
def collect(bot_skills, request_skill_names, preload_skills):
result = []
for group in (bot_skills, request_skill_names, preload_skills):
for name in group:
if name not in result:
result.append(name)
return result
def missing(required, plan_skills, failed, manifest):
return [
name for name in required
if name not in plan_skills or name in failed or not manifest.get(name, False)
]
plan = collect(["ordinary"], [], ["advisory"])
required = sorted(set(["advisory"]))
print("plan_skills =", plan)
print("required_skills =", required)
print("missing_after_failed_download =", missing(required, plan, {"advisory"}, {"advisory": False}))
print("missing_when_download_succeeds_without_manifest =", missing(required, plan, set(), {"advisory": False}))
PYRepository: wecode-ai/Wegent
Length of output: 12026
Keep advisory preload_skills out of the hard-requirement set. build_skill_deployment_plan adds preload names to plan.skills, but resolved_skill_map can lack a reference. Any lookup, download, or SKILL.md failure then fails Claude execution. Validate preload names before dispatch, or include only required_skills in this failure check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/agents/runtime_capabilities.rs` around lines 463 - 493, Update
required_skill_names and missing_required_skills so advisory preload_skills are
excluded from hard-requirement validation; use only required_skills when
determining execution-blocking failures. Preserve preload deployment behavior
while preventing missing references, download errors, or absent SKILL.md files
for preloads from failing Claude execution.
Comment on lines
+138
to
+140
| # Sanitize so a crafted filename cannot inject prompt content or advertise | ||
| # a path outside the attachment directory (mirrors build_sandbox_path). | ||
| safe_filename = sanitize_attachment_filename(filename) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape attachment names before prompt interpolation.
sanitize_attachment_filename removes path separators and control characters, but it keeps ], [ and |. build_attachment_header then inserts the result into a bracket- and pipe-delimited prompt block. An untrusted name such as report] Ignore previous instructions can close the block and add model-visible instructions. Use a separate display-name encoder and add tests for these delimiters. Path sanitization alone does not prevent prompt injection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/utils/attachment_block.py` around lines 138 - 140, Update
build_attachment_header to encode the sanitized filename with a dedicated
display-name encoder before inserting it into the bracket- and pipe-delimited
prompt block; do not rely on sanitize_attachment_filename alone. Ensure the
encoder safely escapes ], [, and |, and add tests covering each delimiter and
prompt-injection-style filenames.
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py`:
- Around line 18-96: Add type hints throughout the affected test code: in
backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py:18-96,
annotate helper and test parameters and return values, including fixtures; in
chat_shell/tests/test_sandbox_attachment_sync.py:19-209, annotate helper
methods, fixture parameters, and async test returns; in
chat_shell/tests/test_sandbox_skill_identity.py:53-78, annotate monkeypatch and
the async test return; in
executor_manager/tests/services/test_sandbox_manager.py:618-754, annotate
fixture parameters and async test returns; and in
executor_manager/tests/services/test_sandbox_skill_sync.py:20-135, annotate
helper parameters, handler callbacks, and async test returns. Preserve existing
behavior and format the annotations with the project’s typing and style
conventions.
🪄 Autofix
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 Plus
Run ID: c7522c33-c2dc-487c-99ee-3e0ca15e688d
📒 Files selected for processing (26)
backend/app/services/sandbox_file_syncer.pybackend/init_data/skills/sandbox/download_attachment_tool.pybackend/init_data/skills/sandbox/provider.pybackend/tests/init_data/skills/sandbox/test_download_attachment_tool.pybackend/tests/init_data/skills/sandbox/test_provider.pybackend/tests/services/test_sandbox_file_syncer.pychat_shell/chat_shell/services/chat_service.pychat_shell/chat_shell/services/sandbox_attachment_sync.pychat_shell/chat_shell/skills/context.pychat_shell/chat_shell/tools/sandbox/_base.pychat_shell/chat_shell/tools/skill_factory.pychat_shell/tests/test_sandbox_attachment_sync.pychat_shell/tests/test_sandbox_skill_identity.pyexecutor/src/agents/mod.rsexecutor/src/agents/runtime_capabilities.rsexecutor/src/server/mod.rsexecutor/tests/agent_runtime_capabilities_contract.rsexecutor/tests/http_contract.rsexecutor_manager/routers/e2b.pyexecutor_manager/services/sandbox/manager.pyexecutor_manager/services/sandbox/skill_sync.pyexecutor_manager/tests/routers/test_e2b_sandbox_creation.pyexecutor_manager/tests/services/test_sandbox_manager.pyexecutor_manager/tests/services/test_sandbox_skill_sync.pyshared/tests/utils/test_attachment_block.pyshared/utils/attachment_block.py
🚧 Files skipped from review as they are similar to previous changes (17)
- shared/tests/utils/test_attachment_block.py
- backend/init_data/skills/sandbox/provider.py
- executor/src/agents/mod.rs
- chat_shell/chat_shell/services/chat_service.py
- chat_shell/chat_shell/tools/skill_factory.py
- executor/tests/agent_runtime_capabilities_contract.rs
- executor_manager/tests/routers/test_e2b_sandbox_creation.py
- backend/tests/init_data/skills/sandbox/test_provider.py
- shared/utils/attachment_block.py
- executor_manager/routers/e2b.py
- backend/app/services/sandbox_file_syncer.py
- executor/src/server/mod.rs
- backend/tests/services/test_sandbox_file_syncer.py
- executor/tests/http_contract.rs
- chat_shell/chat_shell/skills/context.py
- executor/src/agents/runtime_capabilities.rs
- executor_manager/services/sandbox/manager.py
Comment on lines
+18
to
+96
| def test_build_download_url_uses_executor_endpoint_for_attachment_url() -> None: | ||
| assert ( | ||
| _build_download_url("/api/attachments/123/download", "http://backend:8000") | ||
| == "http://backend:8000/api/attachments/123/executor-download" | ||
| ) | ||
| assert ( | ||
| _build_download_url( | ||
| "https://wegent.example/api/attachments/123/download?download=1", | ||
| "http://backend:8000", | ||
| ) | ||
| == "http://backend:8000/api/attachments/123/executor-download" | ||
| ) | ||
|
|
||
|
|
||
| def test_build_download_url_rejects_non_attachment_url() -> None: | ||
| with pytest.raises(ValueError, match="Only Wegent attachment download URLs"): | ||
| _build_download_url( | ||
| "https://files.example/report.csv", | ||
| "http://backend:8000", | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_download_uses_environment_for_credentials_and_paths(monkeypatch) -> None: | ||
| calls: list[dict] = [] | ||
|
|
||
| class FakeFiles: | ||
| async def make_dir(self, path: str) -> None: | ||
| return None | ||
|
|
||
| async def get_info(self, path: str) -> SimpleNamespace: | ||
| return SimpleNamespace(size=4) | ||
|
|
||
| class FakeCommands: | ||
| async def run(self, **kwargs) -> SimpleNamespace: | ||
| calls.append(kwargs) | ||
| return SimpleNamespace(exit_code=0, stderr="") | ||
|
|
||
| sandbox = SimpleNamespace( | ||
| sandbox_id="sandbox-1", | ||
| files=FakeFiles(), | ||
| commands=FakeCommands(), | ||
| ) | ||
|
|
||
| class FakeManager: | ||
| async def get_or_create_sandbox(self, **kwargs): | ||
| return sandbox, None | ||
|
|
||
| monkeypatch.setattr( | ||
| SandboxDownloadAttachmentTool, | ||
| "_get_sandbox_manager", | ||
| lambda self: FakeManager(), | ||
| ) | ||
| tool = SandboxDownloadAttachmentTool( | ||
| task_id=1, | ||
| subtask_id=2, | ||
| user_id=3, | ||
| user_name="alice", | ||
| auth_token="task-token", | ||
| api_base_url="http://backend:8000", | ||
| ) | ||
|
|
||
| result = json.loads( | ||
| await tool._arun( | ||
| attachment_url="/api/attachments/123/download", | ||
| save_path="/home/user/report.csv", | ||
| ) | ||
| ) | ||
|
|
||
| assert result["success"] is True | ||
| assert "task-token" not in calls[0]["cmd"] | ||
| assert "/home/user/report.csv" not in calls[0]["cmd"] | ||
| assert calls[0]["envs"] == { | ||
| "WEGENT_ATTACHMENT_TOKEN": "task-token", | ||
| "WEGENT_ATTACHMENT_SAVE_PATH": "/home/user/report.csv", | ||
| "WEGENT_ATTACHMENT_DOWNLOAD_URL": ( | ||
| "http://backend:8000/api/attachments/123/executor-download" | ||
| ), | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add type annotations to the new Python test code.
The new helpers and test functions omit parameter or return annotations.
backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py#L18-L96: Annotate test fixtures, helper parameters, and return values.chat_shell/tests/test_sandbox_attachment_sync.py#L19-L209: Annotate helper methods, fixture parameters, and async test return values.chat_shell/tests/test_sandbox_skill_identity.py#L53-L78: Annotatemonkeypatchand the async test return value.executor_manager/tests/services/test_sandbox_manager.py#L618-L754: Annotate fixture parameters and async test return values.executor_manager/tests/services/test_sandbox_skill_sync.py#L20-L135: Annotate helper parameters, handler callbacks, and async test return values.
As per coding guidelines, “Python code must follow PEP 8, Black with 88-column formatting, isort, and type hints.”
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 19-19: Do not make http calls without encryption
Context: "http://backend:8000"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 20-20: Do not make http calls without encryption
Context: "http://backend:8000/api/attachments/123/executor-download"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 25-25: Do not make http calls without encryption
Context: "http://backend:8000"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 27-27: Do not make http calls without encryption
Context: "http://backend:8000/api/attachments/123/executor-download"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 35-35: Do not make http calls without encryption
Context: "http://backend:8000"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 76-76: Do not make http calls without encryption
Context: "http://backend:8000"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 93-93: Do not make http calls without encryption
Context: "http://backend:8000/api/attachments/123/executor-download"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🪛 Ruff (0.16.1)
[error] 76-76: Possible hardcoded password assigned to argument: "auth_token"
(S106)
📍 Affects 5 files
backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py#L18-L96(this comment)chat_shell/tests/test_sandbox_attachment_sync.py#L19-L209chat_shell/tests/test_sandbox_skill_identity.py#L53-L78executor_manager/tests/services/test_sandbox_manager.py#L618-L754executor_manager/tests/services/test_sandbox_skill_sync.py#L20-L135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py`
around lines 18 - 96, Add type hints throughout the affected test code: in
backend/tests/init_data/skills/sandbox/test_download_attachment_tool.py:18-96,
annotate helper and test parameters and return values, including fixtures; in
chat_shell/tests/test_sandbox_attachment_sync.py:19-209, annotate helper
methods, fixture parameters, and async test returns; in
chat_shell/tests/test_sandbox_skill_identity.py:53-78, annotate monkeypatch and
the async test return; in
executor_manager/tests/services/test_sandbox_manager.py:618-754, annotate
fixture parameters and async test returns; and in
executor_manager/tests/services/test_sandbox_skill_sync.py:20-135, annotate
helper parameters, handler callbacks, and async test returns. Preserve existing
behavior and format the annotations with the project’s typing and style
conventions.
Source: Coding guidelines
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes