-
Notifications
You must be signed in to change notification settings - Fork 125
Fix/sandbox skill activation gate #2577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8b8313f
2c314ac
a03fc1c
98e0778
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,13 +12,14 @@ | |||||||||
| block the attachment upload flow. | ||||||||||
| """ | ||||||||||
|
|
||||||||||
| import asyncio | ||||||||||
| import logging | ||||||||||
| import os | ||||||||||
| from typing import Optional | ||||||||||
|
|
||||||||||
| import httpx | ||||||||||
|
|
||||||||||
| from shared.utils.attachment_block import sanitize_attachment_filename | ||||||||||
|
|
||||||||||
| logger = logging.getLogger(__name__) | ||||||||||
|
|
||||||||||
| # Configuration | ||||||||||
|
|
@@ -39,13 +40,7 @@ def _sanitize_filename(filename: str) -> str: | |||||||||
| Returns: | ||||||||||
| Sanitized filename safe for use in file paths | ||||||||||
| """ | ||||||||||
| # Get basename to remove any directory components | ||||||||||
| safe_name = os.path.basename(filename or "attachment") | ||||||||||
| # Replace path separators that might have been encoded | ||||||||||
| safe_name = safe_name.replace("/", "_").replace("\\", "_") | ||||||||||
| # Remove control characters | ||||||||||
| safe_name = safe_name.replace("\n", "").replace("\r", "") | ||||||||||
| return safe_name if safe_name else "attachment" | ||||||||||
| return sanitize_attachment_filename(filename, fallback="attachment") | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def build_sandbox_attachment_path(task_id: int, subtask_id: int, filename: str) -> str: | ||||||||||
|
|
@@ -282,7 +277,7 @@ async def sync_attachment_to_sandbox_background( | |||||||||
| ) -> None: | ||||||||||
| """Sync attachment to sandbox in background. | ||||||||||
|
|
||||||||||
| This function is designed to be called from asyncio.create_task() | ||||||||||
| This function is designed to be scheduled as an asynchronous task. | ||||||||||
| and handles all exceptions internally. | ||||||||||
|
Comment on lines
+280
to
281
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Fix the updated docstring sentence. Line 280 ends the sentence before 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||
|
|
||||||||||
| Args: | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| # SPDX-FileCopyrightText: 2026 Weibo, Inc. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Tests for task-token attachment downloads in the sandbox skill.""" | ||
|
|
||
| import json | ||
| from types import SimpleNamespace | ||
|
|
||
| import pytest | ||
|
|
||
| from init_data.skills.sandbox.download_attachment_tool import ( | ||
| SandboxDownloadAttachmentTool, | ||
| _build_download_url, | ||
| ) | ||
|
|
||
|
|
||
| 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" | ||
| ), | ||
| } | ||
|
Comment on lines
+18
to
+96
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 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.
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 (requests-http) [warning] 20-20: Do not make http calls without encryption (requests-http) [warning] 25-25: Do not make http calls without encryption (requests-http) [warning] 27-27: Do not make http calls without encryption (requests-http) [warning] 35-35: Do not make http calls without encryption (requests-http) [warning] 76-76: Do not make http calls without encryption (requests-http) [warning] 93-93: Do not make http calls without encryption (requests-http) 🪛 Ruff (0.16.1)[error] 76-76: Possible hardcoded password assigned to argument: "auth_token" (S106) 📍 Affects 5 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -25,3 +25,21 @@ def test_prepare_base_params_includes_skill_identity_token(): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| params = provider._prepare_base_params(context, {}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert params["skill_identity_token"] == "skill-jwt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add a return annotation to the new test.
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
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.