Conversation
Add per-tool configuration for the Python sandbox and file operations: new config params (allowed/blocked read/write paths, extra/blocked imports, max file size, encodings, allowed extensions) and enforcement when reading/writing/editing files. Pass combined allowed/blocked import lists and path lists into PythonSandbox and apply user-configured blocked paths to the agent security context. Replace the tool details side panel with a modal dialog in the tools UI, add a Configure action when a tool exposes configParams, and remove the old panel styles. Also bump prompture dependency to >=1.0.48.
Add an agent_mock to security tests and pass it to _harden_security_context so the tests match the method signature (agent_mock with tool_configs = None). Also tidy up string descriptions in python_sandbox (join wrapped lines) and remove an unnecessary inline type-ignore comment in bot_creation_service.
There was a problem hiding this comment.
Pull request overview
Adds per-tool configuration metadata end-to-end (backend skill descriptors → bot toolConfigs → frontend dynamic config UI) and applies the new settings to sandbox hardening / file tools.
Changes:
- Introduces
config_paramsfor tools (notablypython_executeand file ops) and wires them through tool metadata. - Updates agent sandbox setup/hardening to apply per-bot
python_executeconfig (paths/imports/timeout). - Reworks the Tools UI to include a tool detail modal and a tool configuration dialog.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
cachibot/agent.py |
Applies per-tool sandbox config (paths/imports/timeout) and extends hardening to include blocked paths. |
cachibot/plugins/python_sandbox.py |
Adds python_execute config param definitions for timeout/output/path/import controls. |
cachibot/plugins/file_ops.py |
Adds per-tool config params (max file size, encoding, allowed extensions) and enforces them in read/write/edit. |
frontend/src/components/views/ToolsView.tsx |
Adds tool detail modal and integrates ToolConfigDialog entrypoints. |
frontend/src/styles/components/tools-view.less |
Removes now-unused tool detail panel styles after UI refactor. |
tests/test_security.py |
Fixes _harden_security_context tests for method signature change. |
cachibot/services/bot_creation_service.py |
Removes stale type: ignore on extract_with_model. |
pyproject.toml |
Bumps prompture minimum version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+526
to
+555
| <div | ||
| className="fixed inset-0 z-50 flex items-center justify-center bg-black/50 p-4" | ||
| onClick={(e) => { if (e.target === e.currentTarget) onClose() }} | ||
| > | ||
| <div className="w-full max-w-lg rounded-xl bg-[var(--color-bg-dialog)] shadow-xl border border-[var(--color-border-primary)]"> | ||
| {/* Header */} | ||
| <div className="flex items-center justify-between border-b border-[var(--color-border-primary)] p-4"> | ||
| <div className="flex items-center gap-3"> | ||
| <div className="flex h-10 w-10 items-center justify-center rounded-lg bg-[var(--color-bg-secondary)]"> | ||
| <ToolIconRenderer toolId={tool.id} icon={tool.icon} className="h-5 w-5 text-[var(--color-text-primary)]" /> | ||
| </div> | ||
| <div> | ||
| <div className="flex items-center gap-2"> | ||
| <h2 className="text-lg font-semibold text-[var(--color-text-primary)]">{tool.name}</h2> | ||
| <span className={cn('inline-flex items-center gap-1 rounded-full px-2 py-0.5 text-xs font-medium', config.bg, config.color, 'border', config.border)}> | ||
| <RiskIcon className="h-3 w-3" /> | ||
| {tool.riskLevel} | ||
| </span> | ||
| </div> | ||
| <p className="text-sm text-[var(--color-text-secondary)]"> | ||
| {categoryConfig[tool.category]?.label || tool.category} | ||
| </p> | ||
| </div> | ||
| </div> | ||
| <button | ||
| onClick={onClose} | ||
| className="rounded-lg p-2 text-[var(--color-text-secondary)] hover:bg-[var(--color-hover-bg)] hover:text-[var(--color-text-primary)]" | ||
| > | ||
| <X className="h-5 w-5" /> | ||
| </button> |
There was a problem hiding this comment.
The new Tool Detail modal is missing basic dialog accessibility hooks: the close button is icon-only (no aria-label), and the dialog wrapper/content lacks role="dialog" + aria-modal="true" (and ideally focus management / Escape-to-close). Adding these will make the modal usable with screen readers and keyboard navigation.
Comment on lines
+68
to
+96
| ConfigParam( | ||
| name="allowedReadPaths", | ||
| display_name="Extra Read Paths", | ||
| description="Additional directories the sandbox can read from " | ||
| "(workspace is always included).", | ||
| type="string[]", | ||
| default=[], | ||
| placeholder="/data/shared", | ||
| max_items=20, | ||
| ), | ||
| ConfigParam( | ||
| name="allowedWritePaths", | ||
| display_name="Extra Write Paths", | ||
| description="Additional directories the sandbox can write to " | ||
| "(workspace is always included).", | ||
| type="string[]", | ||
| default=[], | ||
| placeholder="/data/output", | ||
| max_items=20, | ||
| ), | ||
| ConfigParam( | ||
| name="blockedPaths", | ||
| display_name="Blocked Paths", | ||
| description="Directories forbidden from access, even if within allowed paths.", | ||
| type="string[]", | ||
| default=[], | ||
| placeholder="/etc/secrets", | ||
| max_items=20, | ||
| ), |
There was a problem hiding this comment.
These array-valued ConfigParams set placeholder=..., but the frontend array editor uses itemPlaceholder (and ignores placeholder) for string[] / number[] inputs. As a result, the intended placeholder text won't render for these params. Consider using the array-specific field (item_placeholder / itemPlaceholder) instead of placeholder for string[] config params.
Comment on lines
+147
to
+152
| allowed_ext: list[str] = cfg.get("allowedExtensions", []) | ||
|
|
||
| if allowed_ext and full_path.suffix not in allowed_ext: | ||
| return ( | ||
| f"Error: Extension '{full_path.suffix}' is not allowed. " | ||
| f"Allowed: {', '.join(allowed_ext)}" |
There was a problem hiding this comment.
allowed_ext is assumed to be list[str], but tool configs are user-provided/unvalidated and could be a string or other type; in that case full_path.suffix not in allowed_ext becomes a substring check and can silently allow/deny unexpected extensions. Consider validating/coercing allowedExtensions to a list of strings (and normalizing to lowercase / trimming whitespace) before applying the restriction.
Suggested change
| allowed_ext: list[str] = cfg.get("allowedExtensions", []) | |
| if allowed_ext and full_path.suffix not in allowed_ext: | |
| return ( | |
| f"Error: Extension '{full_path.suffix}' is not allowed. " | |
| f"Allowed: {', '.join(allowed_ext)}" | |
| # Normalize allowedExtensions config to a list of lowercase, dot-prefixed extensions. | |
| raw_allowed_ext = cfg.get("allowedExtensions", []) | |
| normalized_allowed_ext: list[str] = [] | |
| if isinstance(raw_allowed_ext, str): | |
| raw_iter = [raw_allowed_ext] | |
| elif isinstance(raw_allowed_ext, (list, tuple, set)): | |
| raw_iter = raw_allowed_ext | |
| else: | |
| raw_iter = [] | |
| for ext in raw_iter: | |
| if not isinstance(ext, str): | |
| continue | |
| cleaned = ext.strip().lower() | |
| if not cleaned: | |
| continue | |
| if not cleaned.startswith("."): | |
| cleaned = "." + cleaned | |
| normalized_allowed_ext.append(cleaned) | |
| suffix = full_path.suffix.lower() | |
| if normalized_allowed_ext and suffix not in normalized_allowed_ext: | |
| return ( | |
| f"Error: Extension '{full_path.suffix}' is not allowed. " | |
| f"Allowed: {', '.join(normalized_allowed_ext)}" |
Comment on lines
+115
to
144
| # Get per-tool config or fall back to config defaults | ||
| timeout = self.config.sandbox.timeout_seconds | ||
| py_cfg: dict[str, Any] = {} | ||
| if self.tool_configs and "python_execute" in self.tool_configs: | ||
| py_cfg = self.tool_configs["python_execute"] | ||
| timeout = py_cfg.get("timeoutSeconds", timeout) | ||
|
|
||
| # Build path lists: workspace is always included, configs can only add | ||
| blocked_paths = set(py_cfg.get("blockedPaths", [])) | ||
| read_paths = [workspace] + [ | ||
| p for p in py_cfg.get("allowedReadPaths", []) if p not in blocked_paths | ||
| ] | ||
| write_paths = [workspace] + [ | ||
| p for p in py_cfg.get("allowedWritePaths", []) if p not in blocked_paths | ||
| ] | ||
|
|
||
| # Build import lists: base allowed + extra, with separate blocked set | ||
| allowed_imports = list(self.config.sandbox.allowed_imports) | ||
| for mod in py_cfg.get("extraImports", []): | ||
| if mod not in allowed_imports: | ||
| allowed_imports.append(mod) | ||
| blocked_imports = py_cfg.get("blockedImports", []) or None | ||
|
|
||
| self.sandbox = PythonSandbox( | ||
| allowed_imports=self.config.sandbox.allowed_imports, | ||
| allowed_imports=allowed_imports, | ||
| blocked_imports=blocked_imports, | ||
| timeout_seconds=timeout, | ||
| allowed_read_paths=[workspace], | ||
| allowed_write_paths=[workspace], | ||
| allowed_read_paths=read_paths, | ||
| allowed_write_paths=write_paths, | ||
| ) |
There was a problem hiding this comment.
The per-tool sandbox configuration logic (blocked/allowed paths, extra/blocked imports) is new behavior that directly affects runtime safety and can break agent initialization if mis-specified. There are existing security tests in tests/test_security.py, but nothing currently exercises the new _setup_sandbox config handling; adding focused tests for these config knobs (e.g., blockedPaths filtering, extraImports inclusion, blockedImports passthrough) would help prevent regressions.
Comment on lines
119
to
+136
| py_cfg = self.tool_configs["python_execute"] | ||
| timeout = py_cfg.get("timeoutSeconds", timeout) | ||
|
|
||
| # Build path lists: workspace is always included, configs can only add | ||
| blocked_paths = set(py_cfg.get("blockedPaths", [])) | ||
| read_paths = [workspace] + [ | ||
| p for p in py_cfg.get("allowedReadPaths", []) if p not in blocked_paths | ||
| ] | ||
| write_paths = [workspace] + [ | ||
| p for p in py_cfg.get("allowedWritePaths", []) if p not in blocked_paths | ||
| ] | ||
|
|
||
| # Build import lists: base allowed + extra, with separate blocked set | ||
| allowed_imports = list(self.config.sandbox.allowed_imports) | ||
| for mod in py_cfg.get("extraImports", []): | ||
| if mod not in allowed_imports: | ||
| allowed_imports.append(mod) | ||
| blocked_imports = py_cfg.get("blockedImports", []) or None |
There was a problem hiding this comment.
self.tool_configs ultimately comes from unvalidated JSON (e.g., websocket payload). Here py_cfg.get("blockedPaths"), allowedReadPaths, etc. are assumed to be list-like; if a client sends null or a non-list (string/object), set(...) / list comprehensions will raise and prevent the agent from starting. Consider normalizing each config field (treat None as [], enforce list[str], drop non-strings) before building blocked_paths, read_paths, and write_paths (and similarly for import lists).
Suggested change
| py_cfg = self.tool_configs["python_execute"] | |
| timeout = py_cfg.get("timeoutSeconds", timeout) | |
| # Build path lists: workspace is always included, configs can only add | |
| blocked_paths = set(py_cfg.get("blockedPaths", [])) | |
| read_paths = [workspace] + [ | |
| p for p in py_cfg.get("allowedReadPaths", []) if p not in blocked_paths | |
| ] | |
| write_paths = [workspace] + [ | |
| p for p in py_cfg.get("allowedWritePaths", []) if p not in blocked_paths | |
| ] | |
| # Build import lists: base allowed + extra, with separate blocked set | |
| allowed_imports = list(self.config.sandbox.allowed_imports) | |
| for mod in py_cfg.get("extraImports", []): | |
| if mod not in allowed_imports: | |
| allowed_imports.append(mod) | |
| blocked_imports = py_cfg.get("blockedImports", []) or None | |
| raw_py_cfg = self.tool_configs["python_execute"] | |
| if isinstance(raw_py_cfg, dict): | |
| py_cfg = raw_py_cfg | |
| timeout = py_cfg.get("timeoutSeconds", timeout) | |
| def _normalize_str_list(value: Any) -> list[str]: | |
| """Normalize a potentially untrusted list-like config field to list[str].""" | |
| if not isinstance(value, list): | |
| return [] | |
| return [item for item in value if isinstance(item, str)] | |
| # Build path lists: workspace is always included, configs can only add | |
| blocked_paths = set(_normalize_str_list(py_cfg.get("blockedPaths"))) | |
| read_paths = [workspace] + [ | |
| p for p in _normalize_str_list(py_cfg.get("allowedReadPaths")) if p not in blocked_paths | |
| ] | |
| write_paths = [workspace] + [ | |
| p for p in _normalize_str_list(py_cfg.get("allowedWritePaths")) if p not in blocked_paths | |
| ] | |
| # Build import lists: base allowed + extra, with separate blocked set | |
| allowed_imports = list(self.config.sandbox.allowed_imports) | |
| for mod in _normalize_str_list(py_cfg.get("extraImports")): | |
| if mod not in allowed_imports: | |
| allowed_imports.append(mod) | |
| blocked_imports_list = _normalize_str_list(py_cfg.get("blockedImports")) | |
| blocked_imports = blocked_imports_list or None |
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.
Tools used to be take-it-or-leave-it black boxes — same timeout, same output limit, same sandbox rules for everyone. Now each tool can declare its own knobs, and the frontend gives users a proper dialog to turn them.
Highlights
Technical changes
cachibot/plugins/python_sandbox.py: Addedconfig_paramsto thepython_execute@skilldecorator —timeoutSeconds,maxOutputLength,allowedReadPaths,allowedWritePaths,blockedPaths,extraImports,blockedImports— each with typedConfigParammetadata (min/max, units, placeholders, item limits)cachibot/api/routes/plugins.py: Skill metadata endpoint now serializesconfigParamsviadesc.config_paramsso the frontend can discover available knobsfrontend/src/types/index.ts: AddedConfigParaminterface andToolConfigstype; extendedToolwith optionalconfigParamsarrayfrontend/src/components/dialogs/ToolConfigDialog.tsx: New dialog component that dynamically renders inputs for 12 param types (number, boolean, select, string, secret, text, path, string[], number[], map, multiselect, url, code) with save/reset/cancel actionsfrontend/src/components/views/ToolsView.tsx: IntegratedToolConfigDialog— tools withconfigParamsshow a "Configure" button that opens the dialog, passing current per-bottoolConfigscachibot/agent.py:_harden_security_contextnow readsself.tool_configs["python_execute"]["blockedPaths"]to apply user-configured blocked paths to the sandbox security contexttests/test_security.py: Fixed_harden_security_contexttest calls to pass a mockself(method usesself.tool_configs), resolvingTypeErrorfrom missing positional argumentcachibot/services/bot_creation_service.py: Removed stale# type: ignore[no-any-return]onextract_with_modelcall (return type now properly inferred)