-
Notifications
You must be signed in to change notification settings - Fork 1
feat(dify-plugin): port external PRs #28
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
base: main
Are you sure you want to change the base?
Changes from all commits
a649b99
efdb9af
cc427b3
284a831
3064f14
229b97f
055cd7a
a5b8ec5
260bafe
0159592
85ed15a
55c580e
4c3aabb
4bf53f6
211b610
147e5c1
dd93e82
efb0550
29224ee
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 |
|---|---|---|
| @@ -1,6 +1,22 @@ | ||
| import hashlib | ||
| from typing import Any | ||
|
|
||
| from daytona import Daytona, DaytonaConfig, DaytonaNotFoundError, Sandbox | ||
| from daytona import Daytona, DaytonaConfig, DaytonaError, DaytonaNotFoundError, Sandbox | ||
|
|
||
| MAX_FILE_SIZE = 100 * 1024 * 1024 # 100 MB | ||
|
|
||
| # The plugin daemon is long-lived, so cache one Daytona client per unique | ||
| # credential set to avoid re-creating an HTTP session on every tool call. | ||
| _client_cache: dict[str, Daytona] = {} | ||
|
|
||
|
|
||
| def _cache_key(credentials: dict[str, Any]) -> str: | ||
| # Length-prefix each field so distinct api_key/api_url pairs can't collide | ||
| # into the same entry (e.g. "a:" + "b" vs "a" + ":b"). | ||
| api_key = str(credentials.get("api_key", "")) | ||
| api_url = str(credentials.get("api_url", "")) | ||
| raw = f"{len(api_key)}:{api_key}{len(api_url)}:{api_url}" | ||
| return hashlib.sha256(raw.encode()).hexdigest() | ||
|
|
||
|
|
||
| def to_int(value: Any, name: str) -> int: | ||
|
|
@@ -14,10 +30,13 @@ def to_int(value: Any, name: str) -> int: | |
|
|
||
|
|
||
| def build_client(credentials: dict[str, Any]) -> Daytona: | ||
| config = DaytonaConfig(api_key=credentials["api_key"]) | ||
| if api_url := credentials.get("api_url"): | ||
| config.api_url = api_url | ||
| return Daytona(config) | ||
| key = _cache_key(credentials) | ||
| if key not in _client_cache: | ||
|
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. P2: Concurrent first calls for the same credentials can create and use multiple Daytona clients, defeating the one-client-per-credential invariant and potentially abandoning the overwritten session. Protect the check/create/store sequence with a lock. Prompt for AI agents
Collaborator
Author
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. Won't fix. A brief first-call race would at worst create a second client that replaces the cached one; both are valid authenticated clients and the Daytona client holds no exclusive session that would leak. A global lock around every 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. The parent comment is wrong here: a brief first-call race only produces another valid authenticated client, and there’s no exclusive Daytona session to leak. A global lock would just serialize all |
||
| config = DaytonaConfig(api_key=credentials["api_key"]) | ||
| if api_url := credentials.get("api_url"): | ||
| config.api_url = api_url | ||
| _client_cache[key] = Daytona(config) | ||
| return _client_cache[key] | ||
|
|
||
|
|
||
| def get_sandbox(client: Daytona, sandbox_id: str) -> Sandbox: | ||
|
|
@@ -27,6 +46,8 @@ def get_sandbox(client: Daytona, sandbox_id: str) -> Sandbox: | |
| sandbox = client.get(sandbox_id) | ||
| except DaytonaNotFoundError as e: | ||
| raise ValueError(f"Sandbox '{sandbox_id}' not found") from e | ||
| except DaytonaError as e: | ||
| raise ValueError(f"Failed to retrieve sandbox '{sandbox_id}': {e}") from e | ||
| if sandbox is None: | ||
| raise ValueError(f"Sandbox '{sandbox_id}' not found") | ||
| return sandbox | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
| from dify_plugin import Tool | ||
| from dify_plugin.entities.tool import ToolInvokeMessage | ||
|
|
||
| from _client import build_client, get_sandbox | ||
| from _client import MAX_FILE_SIZE, build_client, get_sandbox | ||
|
|
||
|
|
||
| class DownloadFileTool(Tool): | ||
|
|
@@ -21,7 +21,20 @@ def _invoke(self, tool_parameters: dict[str, Any]) -> Generator[ToolInvokeMessag | |
|
|
||
| daytona = build_client(self.runtime.credentials) | ||
| sandbox = get_sandbox(daytona, sandbox_id) | ||
|
|
||
| info = sandbox.fs.get_file_info(remote_path) | ||
| if info.size and info.size > MAX_FILE_SIZE: | ||
| raise ValueError( | ||
| f"File size ({info.size} bytes) exceeds maximum allowed size " | ||
| f"({MAX_FILE_SIZE} bytes)." | ||
| ) | ||
|
|
||
| content = sandbox.fs.download_file(remote_path) | ||
| if len(content) > MAX_FILE_SIZE: | ||
|
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. P1: An oversized or concurrently growing sandbox file can still exhaust the plugin worker's memory because the size limit is checked only after Prompt for AI agents
Collaborator
Author
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. Won't fix for now. 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. The parent comment is wrong for this PR: |
||
| raise ValueError( | ||
| f"Downloaded file size ({len(content)} bytes) exceeds maximum allowed size " | ||
| f"({MAX_FILE_SIZE} bytes)." | ||
| ) | ||
|
|
||
| filename = os.path.basename(remote_path) or "downloaded_file" | ||
| mime_type, _ = mimetypes.guess_type(filename) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| from collections.abc import Generator | ||
| from typing import Any | ||
|
|
||
| from dify_plugin import Tool | ||
| from dify_plugin.entities.tool import ToolInvokeMessage | ||
|
|
||
| from _client import build_client, get_sandbox | ||
|
|
||
|
|
||
| class FindInFilesTool(Tool): | ||
| def _invoke(self, tool_parameters: dict[str, Any]) -> Generator[ToolInvokeMessage]: | ||
| sandbox_id = tool_parameters.get("sandbox_id") | ||
| if not sandbox_id: | ||
| raise ValueError("sandbox_id is required") | ||
|
|
||
| path = tool_parameters.get("path") | ||
| if not path: | ||
| raise ValueError("path is required") | ||
|
|
||
| pattern = tool_parameters.get("pattern") | ||
| if not pattern: | ||
| raise ValueError("pattern is required") | ||
|
|
||
| daytona = build_client(self.runtime.credentials) | ||
| sandbox = get_sandbox(daytona, sandbox_id) | ||
| # find_files returns list[Match]; each Match has flat .file / .line / .content | ||
| results = sandbox.fs.find_files(path, pattern) | ||
|
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. P2: A broad recursive content search can exhaust the plugin's 256 MiB process or produce an unusably large Dify tool result because every match is copied and returned without a result limit. The Daytona endpoint has no pagination/limit argument, so the wrapper should expose and enforce a bounded Prompt for AI agents
Collaborator
Author
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. Won't fix for now. Daytona's 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. The parent comment is too broad for this PR: |
||
|
|
||
| matches = [ | ||
| { | ||
| "file": m.file, | ||
| "line_number": m.line, | ||
| "line_content": m.content, | ||
| } | ||
| for m in results | ||
| ] | ||
|
|
||
| yield self.create_json_message({ | ||
| "sandbox_id": sandbox_id, | ||
| "path": path, | ||
| "pattern": pattern, | ||
| "matches": matches, | ||
| "count": len(matches), | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| identity: | ||
| name: find_in_files | ||
| author: daytonaio | ||
| label: | ||
| en_US: Find in Files | ||
| zh_Hans: Find in Files | ||
| ja_JP: Find in Files | ||
| pt_BR: Find in Files | ||
| description: | ||
| human: | ||
| en_US: Search for a text pattern inside file contents in a Daytona sandbox (like grep). | ||
| zh_Hans: Search for a text pattern inside file contents in a Daytona sandbox (like grep). | ||
| ja_JP: Search for a text pattern inside file contents in a Daytona sandbox (like grep). | ||
| pt_BR: Search for a text pattern inside file contents in a Daytona sandbox (like grep). | ||
| llm: Search for a text pattern inside the contents of files within an existing Daytona sandbox. Returns matching results with file path, line number, and matching line content. Similar to running grep recursively. Requires a sandbox_id. | ||
| parameters: | ||
| - name: sandbox_id | ||
| type: string | ||
| required: true | ||
| label: | ||
| en_US: Sandbox ID | ||
| zh_Hans: Sandbox ID | ||
| ja_JP: Sandbox ID | ||
| pt_BR: Sandbox ID | ||
| human_description: | ||
| en_US: The ID of the sandbox to search within. | ||
| zh_Hans: The ID of the sandbox to search within. | ||
| ja_JP: The ID of the sandbox to search within. | ||
| pt_BR: The ID of the sandbox to search within. | ||
| llm_description: The ID of the Daytona sandbox to search within. | ||
| form: llm | ||
| - name: path | ||
| type: string | ||
| required: true | ||
| label: | ||
| en_US: Path | ||
| zh_Hans: Path | ||
| ja_JP: Path | ||
| pt_BR: Path | ||
| human_description: | ||
| en_US: 'Absolute directory path inside the sandbox to search within (e.g. /home/daytona).' | ||
| zh_Hans: 'Absolute directory path inside the sandbox to search within (e.g. /home/daytona).' | ||
| ja_JP: 'Absolute directory path inside the sandbox to search within (e.g. /home/daytona).' | ||
| pt_BR: 'Absolute directory path inside the sandbox to search within (e.g. /home/daytona).' | ||
| llm_description: 'Absolute directory path inside the sandbox to search within (e.g. "/home/daytona", "/tmp", "/").' | ||
| form: llm | ||
| - name: pattern | ||
| type: string | ||
| required: true | ||
| label: | ||
| en_US: Pattern | ||
| zh_Hans: Pattern | ||
| ja_JP: Pattern | ||
| pt_BR: Pattern | ||
| human_description: | ||
| en_US: 'Text pattern to search for inside file contents.' | ||
| zh_Hans: 'Text pattern to search for inside file contents.' | ||
| ja_JP: 'Text pattern to search for inside file contents.' | ||
| pt_BR: 'Text pattern to search for inside file contents.' | ||
| llm_description: 'Text pattern to search for inside file contents (e.g. "def main", "ERROR", "TODO").' | ||
| form: llm | ||
| extra: | ||
| python: | ||
| source: tools/find_in_files.py |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,7 @@ parameters: | |
| zh_Hans: The port number inside the sandbox where the service is listening (e.g. 3000, 5000, 8080). | ||
| ja_JP: The port number inside the sandbox where the service is listening (e.g. 3000, 5000, 8080). | ||
| pt_BR: The port number inside the sandbox where the service is listening (e.g. 3000, 5000, 8080). | ||
| llm_description: 'The port number inside the sandbox where the service is listening (e.g. 3000 for Next.js, 5000 for Flask, 8080 for generic HTTP, 8501 for Streamlit). Range 3000-9999.' | ||
| llm_description: 'The port number inside the sandbox where the service is listening (e.g. 3000 for Next.js, 5000 for Flask, 8080 for generic HTTP, 8501 for Streamlit). Range 1-65535.' | ||
|
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. P3: The README Tool Reference still advertises the old 3000–9999 restriction, so users will not know that ports 1–65535 are now accepted. The documented Prompt for AI agents
Collaborator
Author
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. Fixed in efb0550 — the README Tool Reference now lists the port range as 1-65535 to match the widened schema. 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. The parent comment was addressed in efb0550, so this no longer applies for this PR. Thanks! |
||
| form: llm | ||
| extra: | ||
| python: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| from collections.abc import Generator | ||
| from typing import Any | ||
|
|
||
| from dify_plugin import Tool | ||
| from dify_plugin.entities.tool import ToolInvokeMessage | ||
|
|
||
| from _client import build_client, get_sandbox | ||
|
|
||
|
|
||
| class GetServiceLogsTool(Tool): | ||
| def _invoke(self, tool_parameters: dict[str, Any]) -> Generator[ToolInvokeMessage]: | ||
| sandbox_id = tool_parameters.get("sandbox_id") | ||
| if not sandbox_id: | ||
| raise ValueError("sandbox_id is required") | ||
|
|
||
| session_id = tool_parameters.get("session_id") | ||
| if not session_id: | ||
| raise ValueError("session_id is required") | ||
|
|
||
| cmd_id = tool_parameters.get("cmd_id") or None | ||
|
|
||
| daytona = build_client(self.runtime.credentials) | ||
| sandbox = get_sandbox(daytona, sandbox_id) | ||
|
|
||
| if not cmd_id: | ||
| session = sandbox.process.get_session(session_id) | ||
| commands = getattr(session, "commands", None) or [] | ||
| if commands: | ||
| cmd_id = getattr(commands[-1], "id", None) | ||
| if not cmd_id: | ||
| raise ValueError( | ||
| f"Could not determine cmd_id for session '{session_id}'. " | ||
| "Provide cmd_id explicitly (from start_service output)." | ||
| ) | ||
|
|
||
| logs = sandbox.process.get_session_command_logs(session_id, cmd_id) | ||
|
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. P2: Log retrieval is unbounded for the long-running processes this tool targets: it loads the command's complete history and then sends it once as text and again as Prompt for AI agents
Collaborator
Author
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. Won't fix for now. 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. The parent comment is too broad for this PR: Thanks for the feedback! I've saved this as a new learning to improve future reviews. |
||
|
|
||
| stdout = getattr(logs, "stdout", None) or "" | ||
| stderr = getattr(logs, "stderr", None) or "" | ||
| output = getattr(logs, "output", None) or "" | ||
| if output: | ||
| combined = output | ||
| elif stdout and stderr: | ||
| # Join the two streams with a single newline, without doubling one | ||
| # that stdout already ends with. | ||
| separator = "" if stdout.endswith("\n") else "\n" | ||
| combined = stdout + separator + stderr | ||
| else: | ||
| combined = stdout or stderr | ||
|
|
||
| yield self.create_text_message(combined or "(no output)") | ||
| yield self.create_json_message({ | ||
| "session_id": session_id, | ||
| "cmd_id": cmd_id, | ||
| "sandbox_id": sandbox_id, | ||
| "stdout": stdout, | ||
| "stderr": stderr, | ||
| }) | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: A long-lived, multi-user plugin daemon will retain every Daytona client and its HTTP connection resources indefinitely because
_client_cachehas no eviction or cleanup. This grows with credential churn and tenant count; a bounded cache with an eviction callback that closes theDaytonaclient would preserve session reuse without turning credentials/clients into process-lifetime state.Prompt for AI agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't fix for now. In practice a Dify install authorizes this plugin with one (or a few) credential sets, so the cache holds a handful of clients for the process lifetime rather than growing unbounded. The Daytona client is a thin HTTP wrapper with no persistent session to leak. A bounded LRU with a close-on-evict callback is a reasonable future enhancement, but not needed for correctness now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent comment is too broad for this PR: the cache is effectively bounded by a small number of credential sets, and the Daytona client here doesn’t hold a persistent session to leak. An LRU with close-on-evict could be a nice enhancement later, but it isn’t needed for correctness now.