From e3137b13cfc22acb5778e30adb2ed5c7964a4b53 Mon Sep 17 00:00:00 2001 From: Vanzeren <53075619+Vanzeren@users.noreply.github.com> Date: Wed, 8 Jul 2026 14:41:16 +0800 Subject: [PATCH 001/116] fix(provisioner): gate legacy skills mount by user visibility (#3985) * fix(provisioner): gate legacy skills mount by user visibility * fix(test): aio sandbox provider * fix: use shared legacy skill visibility helper for sandbox mounts --- backend/AGENTS.md | 4 +- .../aio_sandbox/aio_sandbox_provider.py | 85 ++++- .../community/aio_sandbox/remote_backend.py | 3 + .../sandbox/local/local_sandbox_provider.py | 4 +- .../deerflow/skills/storage/__init__.py | 18 + backend/tests/test_aio_sandbox_provider.py | 4 + backend/tests/test_provisioner_pvc_volumes.py | 266 ++++++++++---- .../test_provisioner_request_threading.py | 45 +++ backend/tests/test_remote_sandbox_backend.py | 27 ++ .../tests/test_three_way_skills_mount_e2e.py | 336 ++++++++++++++++++ docker/docker-compose-dev.yaml | 2 + docker/docker-compose.yaml | 1 + docker/provisioner/app.py | 172 +++++++-- 13 files changed, 848 insertions(+), 119 deletions(-) create mode 100644 backend/tests/test_three_way_skills_mount_e2e.py diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 20f63e0a46a..95b9a1cda3e 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -347,8 +347,8 @@ Proxied through nginx: `/api/langgraph/*` → Gateway LangGraph-compatible runti **Provider Pattern**: `SandboxProvider` with `acquire`, `acquire_async`, `get`, `release` lifecycle. Async agent/tool paths call async sandbox lifecycle hooks so Docker sandbox creation, discovery, cross-process locking, readiness polling, and release stay off the event loop. **Environment policy** (`sandbox/env_policy.py`): `execute_command` no longer inherits the full `os.environ`. `build_sandbox_env()` scrubs secret-looking names (`*KEY*`/`*SECRET*`/`*TOKEN*`/`*PASSWORD*`/`*CREDENTIAL*`) from the inherited environment before layering injected request secrets on top, so platform credentials (e.g. `OPENAI_API_KEY`) never leak into skill subprocesses. Benign vars (`PATH`, `HOME`, `LANG`, `VIRTUAL_ENV`, ...) are preserved. **Implementations**: -- `LocalSandboxProvider` - Local filesystem execution. `acquire(thread_id)` returns a per-thread `LocalSandbox` (id `local:{thread_id}`) whose `path_mappings` resolve `/mnt/user-data/{workspace,uploads,outputs}` and `/mnt/acp-workspace` to that thread's host directories, so the public `Sandbox` API honours the `/mnt/user-data` contract uniformly with AIO. `acquire()` / `acquire(None)` keeps the legacy generic singleton (id `local`) for callers without a thread context. Per-thread sandboxes are held in an LRU cache (default 256 entries) guarded by a `threading.Lock`. -- `AioSandboxProvider` (`packages/harness/deerflow/community/`) - Docker-based isolation. Active-cache and warm-pool entries are checked with the backend during acquire/reuse; definitively dead containers are dropped from all in-process maps so the thread can discover or create a fresh sandbox instead of reusing a stale client. Backend health-check failures are treated as unknown, not dead; local discovery likewise treats an unverifiable container as not adoptable and falls through to create rather than failing acquire. `get()` remains an in-memory lookup for event-loop-safe tool paths. +- `LocalSandboxProvider` - Local filesystem execution. `acquire(thread_id)` returns a per-thread `LocalSandbox` (id `local:{thread_id}`) whose `path_mappings` resolve `/mnt/user-data/{workspace,uploads,outputs}` and `/mnt/acp-workspace` to that thread's host directories, so the public `Sandbox` API honours the `/mnt/user-data` contract uniformly with AIO. `acquire()` / `acquire(None)` keeps the legacy generic singleton (id `local`) for callers without a thread context. Per-thread sandboxes are held in an LRU cache (default 256 entries) guarded by a `threading.Lock`. Legacy global-custom mounts are gated by the same user-scoped skill discovery rule used for prompt/list visibility; providers must not infer visibility from raw directory presence alone. +- `AioSandboxProvider` (`packages/harness/deerflow/community/`) - Docker-based isolation. Active-cache and warm-pool entries are checked with the backend during acquire/reuse; definitively dead containers are dropped from all in-process maps so the thread can discover or create a fresh sandbox instead of reusing a stale client. Backend health-check failures are treated as unknown, not dead; local discovery likewise treats an unverifiable container as not adoptable and falls through to create rather than failing acquire. `get()` remains an in-memory lookup for event-loop-safe tool paths. Legacy global-custom mounts follow the same shared visibility helper as local and remote providers. - `BoxliteProvider` (`packages/harness/deerflow/community/boxlite/`) - BoxLite micro-VM isolation. The `boxlite` runtime is optional (`deerflow-harness[boxlite]`) and lazy-imported only when this provider is selected. The provider owns one private asyncio event loop on a daemon thread because BoxLite handles are loop-affine; sync `Sandbox` calls marshal onto that loop with `run_coroutine_threadsafe`. Boxes are named deterministically from `user_id:thread_id`, released into an in-process warm pool after each agent turn, and reclaimed only by the same user/thread. Warm-pool health checks use a short explicit timeout and forward that timeout through both BoxLite `exec(timeout=...)` and the private-loop `.result(timeout)` bridge so a hung VM cannot pin the per-thread acquire lock indefinitely. `sandbox.replicas` caps active + warm VMs per gateway process; if capacity is exhausted, only warm-pool VMs are evicted. `sandbox.idle_timeout` stops idle warm VMs after the configured seconds. `reset()` is intentionally a lightweight registry clear for `reset_sandbox_provider()` and does not close boxes, stop the idle reaper, or close the private loop; full teardown remains `shutdown()`. diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py b/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py index 8874cd47e31..663d1cbd375 100644 --- a/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py +++ b/backend/packages/harness/deerflow/community/aio_sandbox/aio_sandbox_provider.py @@ -36,10 +36,11 @@ IDLE_CHECK_INTERVAL as _SHARED_IDLE_CHECK_INTERVAL, ) from deerflow.config import get_app_config -from deerflow.config.paths import VIRTUAL_PATH_PREFIX, get_paths +from deerflow.config.paths import VIRTUAL_PATH_PREFIX, get_paths, join_host_path from deerflow.runtime.user_context import get_effective_user_id from deerflow.sandbox.sandbox import Sandbox from deerflow.sandbox.sandbox_provider import SandboxProvider +from deerflow.skills.storage import user_should_see_legacy_skills from .aio_sandbox import AioSandbox from .backend import SandboxBackend, wait_for_sandbox_ready, wait_for_sandbox_ready_async @@ -308,10 +309,10 @@ def _get_extra_mounts(self, thread_id: str | None, *, user_id: str | None = None mounts.extend(self._get_thread_mounts(thread_id, user_id=user_id)) logger.info(f"Adding thread mounts for thread {thread_id}: {mounts}") - skills_mount = self._get_skills_mount() - if skills_mount: - mounts.append(skills_mount) - logger.info(f"Adding skills mount: {skills_mount}") + skills_mounts = self._get_skills_mounts(user_id=user_id) + if skills_mounts: + mounts.extend(skills_mounts) + logger.info(f"Adding skills mounts: {skills_mounts}") return mounts @@ -337,24 +338,76 @@ def _get_thread_mounts(thread_id: str, *, user_id: str | None = None) -> list[tu ] @staticmethod - def _get_skills_mount() -> tuple[str, str, bool] | None: - """Get the skills directory mount configuration. - - Mount source uses DEER_FLOW_HOST_SKILLS_PATH when running inside Docker (DooD) - so the host Docker daemon can resolve the path. + def _get_skills_mounts(*, user_id: str | None = None) -> list[tuple[str, str, bool]]: + """Get skills directory mount configurations for three-way skills layout. + + Mirrors ``LocalSandboxProvider._build_thread_path_mappings`` for AIO + sandboxes: public, per-user custom, and legacy (pre-migration + global-custom) skills are mounted to separate container subdirectories so + that ``Skill.get_container_path()`` category-aware paths resolve + correctly inside the sandbox. + + Mount sources use ``DEER_FLOW_HOST_SKILLS_PATH`` and + ``DEER_FLOW_HOST_BASE_DIR`` when running inside Docker (DooD) so the + host Docker daemon can resolve the paths. """ + mounts: list[tuple[str, str, bool]] = [] try: config = get_app_config() skills_path = config.skills.get_skills_path() container_path = config.skills.container_path - if skills_path.exists(): - # When running inside Docker with DooD, use host-side skills path. - host_skills = os.environ.get("DEER_FLOW_HOST_SKILLS_PATH") or str(skills_path) - return (host_skills, container_path, True) # Read-only for security + # When running inside Docker with DooD, use host-side skills path. + host_skills_root = os.environ.get("DEER_FLOW_HOST_SKILLS_PATH") or str(skills_path) + + # 1. Public skills: global, read-only — static, shared by all threads + public_skills_path = skills_path / "public" + if public_skills_path.exists(): + mounts.append( + ( + join_host_path(host_skills_root, "public"), + f"{container_path}/public", + True, + ) + ) + + # 2. Per-user custom skills: read-only, per-thread/per-user + effective_user_id = AioSandboxProvider._effective_acquire_user_id(user_id) + paths = get_paths() + user_custom_path = paths.user_custom_skills_dir(effective_user_id) + user_custom_path.mkdir(parents=True, exist_ok=True) + + host_user_custom = join_host_path( + str(paths.host_base_dir), + "users", + effective_user_id, + "skills", + "custom", + ) + mounts.append( + ( + host_user_custom, + f"{container_path}/custom", + True, + ) + ) + + # 3. Legacy (pre-migration global-custom) skills: only mount for + # users who have no per-user custom skills yet, mirroring + # ``UserScopedSkillStorage._iter_skill_files`` visibility rule. + legacy_skills_path = skills_path / "custom" + if user_should_see_legacy_skills(effective_user_id, host_path=str(skills_path)) and legacy_skills_path.exists(): + mounts.append( + ( + join_host_path(host_skills_root, "custom"), + f"{container_path}/legacy", + True, + ) + ) except Exception as e: - logger.warning(f"Could not setup skills mount: {e}") - return None + logger.warning("Could not setup skills mounts: %s", e) + + return mounts # ── Idle timeout management ────────────────────────────────────────── diff --git a/backend/packages/harness/deerflow/community/aio_sandbox/remote_backend.py b/backend/packages/harness/deerflow/community/aio_sandbox/remote_backend.py index ee9848d48a5..9c448c4a539 100644 --- a/backend/packages/harness/deerflow/community/aio_sandbox/remote_backend.py +++ b/backend/packages/harness/deerflow/community/aio_sandbox/remote_backend.py @@ -22,6 +22,7 @@ import requests from deerflow.runtime.user_context import get_effective_user_id +from deerflow.skills.storage import user_should_see_legacy_skills from .backend import SandboxBackend from .sandbox_info import SandboxInfo @@ -145,6 +146,7 @@ def _provisioner_create( """POST /api/sandboxes → create Pod + Service.""" del extra_mounts effective_user_id = user_id or get_effective_user_id() + include_legacy_skills = user_should_see_legacy_skills(effective_user_id) try: resp = requests.post( f"{self._provisioner_url}/api/sandboxes", @@ -152,6 +154,7 @@ def _provisioner_create( "sandbox_id": sandbox_id, "thread_id": thread_id, "user_id": effective_user_id, + "include_legacy_skills": include_legacy_skills, }, timeout=30, ) diff --git a/backend/packages/harness/deerflow/sandbox/local/local_sandbox_provider.py b/backend/packages/harness/deerflow/sandbox/local/local_sandbox_provider.py index 192fc3271d3..c02f2cb32e4 100644 --- a/backend/packages/harness/deerflow/sandbox/local/local_sandbox_provider.py +++ b/backend/packages/harness/deerflow/sandbox/local/local_sandbox_provider.py @@ -6,6 +6,7 @@ from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping from deerflow.sandbox.sandbox import Sandbox from deerflow.sandbox.sandbox_provider import SandboxProvider +from deerflow.skills.storage import user_should_see_legacy_skills logger = logging.getLogger(__name__) @@ -310,8 +311,7 @@ def _build_thread_path_mappings(thread_id: str, *, user_id: str | None = None) - skills_container_path = config.skills.container_path user_custom_path = paths.user_custom_skills_dir(effective_user_id) legacy_skills_path = config.skills.get_skills_path() / "custom" - user_has_no_custom_skills = not any(p.is_dir() and not p.name.startswith(".") for p in user_custom_path.iterdir()) if user_custom_path.exists() else True - if user_has_no_custom_skills and legacy_skills_path.exists() and any((legacy_skills_path / d / "SKILL.md").exists() for d in legacy_skills_path.iterdir() if d.is_dir() and not d.name.startswith(".")): + if user_should_see_legacy_skills(effective_user_id, host_path=str(config.skills.get_skills_path())) and legacy_skills_path.exists(): mappings.append( PathMapping( container_path=f"{skills_container_path}/legacy", diff --git a/backend/packages/harness/deerflow/skills/storage/__init__.py b/backend/packages/harness/deerflow/skills/storage/__init__.py index f91a1ace093..1d8940140af 100644 --- a/backend/packages/harness/deerflow/skills/storage/__init__.py +++ b/backend/packages/harness/deerflow/skills/storage/__init__.py @@ -12,6 +12,7 @@ from deerflow.skills.storage.local_skill_storage import LocalSkillStorage from deerflow.skills.storage.skill_storage import SkillStorage from deerflow.skills.storage.user_scoped_skill_storage import UserScopedSkillStorage +from deerflow.skills.types import SkillCategory logger = logging.getLogger(__name__) @@ -142,6 +143,22 @@ def get_or_new_user_skill_storage(user_id: str, **kwargs) -> SkillStorage: return cached +def user_should_see_legacy_skills(user_id: str, **kwargs) -> bool: + """Return whether discovery exposes any LEGACY skills for this user. + + Sandbox mounts must not be more permissive than skill discovery. This + helper centralizes that contract so local, AIO, and remote providers all + follow the same visibility rule. + """ + if kwargs: + from deerflow.config.paths import make_safe_user_id + + storage = UserScopedSkillStorage(make_safe_user_id(user_id), **kwargs) + else: + storage = get_or_new_user_skill_storage(user_id) + return any((skill.category.value if hasattr(skill.category, "value") else skill.category) == SkillCategory.LEGACY.value for skill in storage.load_skills(enabled_only=False)) + + def reset_skill_storage() -> None: """Clear all cached storage instances (used in tests and hot-reload scenarios).""" global _default_skill_storage, _default_skill_storage_config @@ -180,6 +197,7 @@ def reset_user_skill_storage(user_id: str | None = None) -> None: "UserScopedSkillStorage", "get_or_new_skill_storage", "get_or_new_user_skill_storage", + "user_should_see_legacy_skills", "reset_skill_storage", "reset_user_skill_storage", ] diff --git a/backend/tests/test_aio_sandbox_provider.py b/backend/tests/test_aio_sandbox_provider.py index e0349f0a098..df1745873f3 100644 --- a/backend/tests/test_aio_sandbox_provider.py +++ b/backend/tests/test_aio_sandbox_provider.py @@ -373,6 +373,7 @@ def _post(url, json, timeout): # noqa: A002 - mirrors requests.post kwarg return _Response() monkeypatch.setattr(remote_mod.requests, "post", _post) + monkeypatch.setattr(remote_mod, "user_should_see_legacy_skills", lambda user_id: True) try: backend.create("thread-42", "sandbox-42") @@ -384,6 +385,7 @@ def _post(url, json, timeout): # noqa: A002 - mirrors requests.post kwarg "sandbox_id": "sandbox-42", "thread_id": "thread-42", "user_id": "user-7", + "include_legacy_skills": True, } @@ -406,10 +408,12 @@ def _post(url, json, timeout): # noqa: A002 - mirrors requests.post kwarg monkeypatch.setattr(remote_mod.requests, "post", _post) monkeypatch.setattr(remote_mod, "get_effective_user_id", lambda: "default") + monkeypatch.setattr(remote_mod, "user_should_see_legacy_skills", lambda user_id: False) backend.create("thread-42", "sandbox-42", user_id="ou-user") assert posted["json"]["user_id"] == "ou-user" + assert posted["json"]["include_legacy_skills"] is False # ── Sandbox client teardown (#2872) ────────────────────────────────────────── diff --git a/backend/tests/test_provisioner_pvc_volumes.py b/backend/tests/test_provisioner_pvc_volumes.py index d5b66a2c763..2d0668de3bb 100644 --- a/backend/tests/test_provisioner_pvc_volumes.py +++ b/backend/tests/test_provisioner_pvc_volumes.py @@ -1,40 +1,84 @@ -"""Regression tests for provisioner PVC volume support.""" +"""Regression tests for provisioner three-way skills + PVC volume support.""" # ── _build_volumes ───────────────────────────────────────────────────── class TestBuildVolumes: - """Tests for _build_volumes: PVC vs hostPath selection.""" + """Tests for _build_volumes: hostPath three-way vs PVC fallback.""" - def test_default_uses_hostpath_for_skills(self, provisioner_module): - """When SKILLS_PVC_NAME is empty, skills volume should use hostPath.""" + # ── hostPath mode (default) ──────────────────────────────────────── + + def test_hostpath_without_legacy_returns_three_volumes(self, provisioner_module): + """hostPath mode omits legacy volume unless the backend requests it.""" provisioner_module.SKILLS_PVC_NAME = "" + provisioner_module.USERDATA_PVC_NAME = "" volumes = provisioner_module._build_volumes("thread-1") - skills_vol = volumes[0] - assert skills_vol.host_path is not None - assert skills_vol.host_path.path == provisioner_module.SKILLS_HOST_PATH - assert skills_vol.host_path.type == "Directory" - assert skills_vol.persistent_volume_claim is None + assert len(volumes) == 3 - def test_default_uses_hostpath_for_userdata(self, provisioner_module): - """When USERDATA_PVC_NAME is empty, user-data volume should use hostPath.""" - provisioner_module.USERDATA_PVC_NAME = "" + def test_hostpath_skills_public_volume(self, provisioner_module): + """First skills volume mounts public/ subdirectory.""" + provisioner_module.SKILLS_PVC_NAME = "" volumes = provisioner_module._build_volumes("thread-1") - userdata_vol = volumes[1] - assert userdata_vol.host_path is not None - assert userdata_vol.persistent_volume_claim is None + pub = volumes[0] + assert pub.name == "skills-public" + assert pub.host_path is not None + assert pub.host_path.path.endswith("/public") + assert pub.host_path.type == "Directory" + assert pub.persistent_volume_claim is None + + def test_hostpath_skills_custom_volume(self, provisioner_module): + """Second skills volume mounts per-user custom directory.""" + provisioner_module.SKILLS_PVC_NAME = "" + volumes = provisioner_module._build_volumes("thread-1", user_id="user-7") + custom = volumes[1] + assert custom.name == "skills-custom" + assert custom.host_path is not None + assert "users/user-7/skills/custom" in custom.host_path.path + assert custom.host_path.type == "DirectoryOrCreate" + + def test_hostpath_skills_legacy_volume(self, provisioner_module): + """Legacy global-custom directory is mounted only when requested.""" + provisioner_module.SKILLS_PVC_NAME = "" + volumes = provisioner_module._build_volumes( + "thread-1", + include_legacy_skills=True, + ) + legacy = volumes[2] + assert legacy.name == "skills-legacy" + assert legacy.host_path is not None + assert legacy.host_path.path.endswith("/custom") + assert legacy.host_path.type == "Directory" + + def test_hostpath_without_legacy_has_no_legacy_volume(self, provisioner_module): + """Fresh installs should not require a missing global legacy directory.""" + provisioner_module.SKILLS_PVC_NAME = "" + volumes = provisioner_module._build_volumes("thread-1") + assert [volume.name for volume in volumes] == [ + "skills-public", + "skills-custom", + "user-data", + ] def test_hostpath_userdata_includes_thread_id(self, provisioner_module): """hostPath user-data path should include thread_id.""" provisioner_module.USERDATA_PVC_NAME = "" volumes = provisioner_module._build_volumes("my-thread-42") - userdata_vol = volumes[1] + userdata_vol = volumes[-1] path = userdata_vol.host_path.path assert "my-thread-42" in path assert path.endswith("user-data") assert userdata_vol.host_path.type == "DirectoryOrCreate" + # ── PVC mode (single-volume fallback) ────────────────────────────── + + def test_pvc_returns_two_volumes(self, provisioner_module): + """PVC mode falls back to 1 skills volume + 1 user-data volume.""" + provisioner_module.SKILLS_PVC_NAME = "my-skills-pvc" + provisioner_module.USERDATA_PVC_NAME = "" + volumes = provisioner_module._build_volumes("thread-1") + assert len(volumes) == 2 + def test_skills_pvc_overrides_hostpath(self, provisioner_module): """When SKILLS_PVC_NAME is set, skills volume should use PVC.""" provisioner_module.SKILLS_PVC_NAME = "my-skills-pvc" @@ -49,7 +93,7 @@ def test_userdata_pvc_overrides_hostpath(self, provisioner_module): """When USERDATA_PVC_NAME is set, user-data volume should use PVC.""" provisioner_module.USERDATA_PVC_NAME = "my-userdata-pvc" volumes = provisioner_module._build_volumes("thread-1") - userdata_vol = volumes[1] + userdata_vol = volumes[-1] assert userdata_vol.persistent_volume_claim is not None assert userdata_vol.persistent_volume_claim.claim_name == "my-userdata-pvc" assert userdata_vol.host_path is None @@ -60,78 +104,112 @@ def test_both_pvc_set(self, provisioner_module): provisioner_module.USERDATA_PVC_NAME = "userdata-pvc" volumes = provisioner_module._build_volumes("thread-1") assert volumes[0].persistent_volume_claim is not None - assert volumes[1].persistent_volume_claim is not None + assert volumes[-1].persistent_volume_claim is not None - def test_returns_two_volumes(self, provisioner_module): - """Should always return exactly two volumes.""" - provisioner_module.SKILLS_PVC_NAME = "" - provisioner_module.USERDATA_PVC_NAME = "" - assert len(provisioner_module._build_volumes("t")) == 2 - - provisioner_module.SKILLS_PVC_NAME = "a" - provisioner_module.USERDATA_PVC_NAME = "b" - assert len(provisioner_module._build_volumes("t")) == 2 - - def test_volume_names_are_stable(self, provisioner_module): - """Volume names must stay 'skills' and 'user-data'.""" + def test_pvc_volume_names_are_stable(self, provisioner_module): + """PVC mode volume names must stay 'skills' and 'user-data'.""" + provisioner_module.SKILLS_PVC_NAME = "x" volumes = provisioner_module._build_volumes("thread-1") assert volumes[0].name == "skills" - assert volumes[1].name == "user-data" + assert volumes[-1].name == "user-data" # ── _build_volume_mounts ─────────────────────────────────────────────── class TestBuildVolumeMounts: - """Tests for _build_volume_mounts: mount paths and subPath behavior.""" + """Tests for _build_volume_mounts: three-way mount paths and subPath.""" + + # ── hostPath mode ────────────────────────────────────────────────── - def test_default_no_subpath(self, provisioner_module): + def test_hostpath_without_legacy_returns_three_mounts(self, provisioner_module): + """hostPath mode omits legacy mount unless the backend requests it.""" + provisioner_module.SKILLS_PVC_NAME = "" + provisioner_module.USERDATA_PVC_NAME = "" + mounts = provisioner_module._build_volume_mounts("thread-1") + assert len(mounts) == 3 + + def test_hostpath_skills_public_mount(self, provisioner_module): + """Public skills mount at /mnt/skills/public, read-only.""" + provisioner_module.SKILLS_PVC_NAME = "" + mounts = provisioner_module._build_volume_mounts("thread-1") + assert mounts[0].name == "skills-public" + assert mounts[0].mount_path == "/mnt/skills/public" + assert mounts[0].read_only is True + + def test_hostpath_skills_custom_mount(self, provisioner_module): + """Per-user custom skills mount at /mnt/skills/custom, read-only.""" + provisioner_module.SKILLS_PVC_NAME = "" + mounts = provisioner_module._build_volume_mounts("thread-1") + assert mounts[1].name == "skills-custom" + assert mounts[1].mount_path == "/mnt/skills/custom" + assert mounts[1].read_only is True + + def test_hostpath_skills_legacy_mount(self, provisioner_module): + """Legacy skills mount at /mnt/skills/legacy, read-only.""" + provisioner_module.SKILLS_PVC_NAME = "" + mounts = provisioner_module._build_volume_mounts( + "thread-1", + include_legacy_skills=True, + ) + assert mounts[2].name == "skills-legacy" + assert mounts[2].mount_path == "/mnt/skills/legacy" + assert mounts[2].read_only is True + + def test_hostpath_without_legacy_has_no_legacy_mount(self, provisioner_module): + """Users with custom skills should not see hidden legacy content in the sandbox.""" + provisioner_module.SKILLS_PVC_NAME = "" + mounts = provisioner_module._build_volume_mounts("thread-1") + assert [mount.name for mount in mounts] == [ + "skills-public", + "skills-custom", + "user-data", + ] + + def test_hostpath_userdata_read_write(self, provisioner_module): + """User-data mount should always be read-write.""" + provisioner_module.SKILLS_PVC_NAME = "" + mounts = provisioner_module._build_volume_mounts("thread-1") + userdata = mounts[-1] + assert userdata.name == "user-data" + assert userdata.mount_path == "/mnt/user-data" + assert userdata.read_only is False + + # ── PVC mode ─────────────────────────────────────────────────────── + + def test_pvc_returns_two_mounts(self, provisioner_module): + """PVC mode falls back to 1 skills mount + 1 user-data mount.""" + provisioner_module.SKILLS_PVC_NAME = "x" + mounts = provisioner_module._build_volume_mounts("thread-1") + assert len(mounts) == 2 + + def test_pvc_skills_mount_is_single_root(self, provisioner_module): + """PVC mode skills mount is at /mnt/skills.""" + provisioner_module.SKILLS_PVC_NAME = "x" + mounts = provisioner_module._build_volume_mounts("thread-1") + assert mounts[0].mount_path == "/mnt/skills" + + def test_pvc_no_subpath_on_userdata(self, provisioner_module): """hostPath mode should not set sub_path on user-data mount.""" provisioner_module.USERDATA_PVC_NAME = "" mounts = provisioner_module._build_volume_mounts("thread-1") - userdata_mount = mounts[1] + userdata_mount = mounts[-1] assert userdata_mount.sub_path is None def test_pvc_sets_user_scoped_subpath(self, provisioner_module): """PVC mode should include user_id in the user-data subPath.""" provisioner_module.USERDATA_PVC_NAME = "my-pvc" mounts = provisioner_module._build_volume_mounts("thread-42", user_id="user-7") - userdata_mount = mounts[1] + userdata_mount = mounts[-1] assert userdata_mount.sub_path == "deer-flow/users/user-7/threads/thread-42/user-data" def test_pvc_defaults_to_default_user_subpath(self, provisioner_module): """Older callers should still land under a stable default user namespace.""" provisioner_module.USERDATA_PVC_NAME = "my-pvc" mounts = provisioner_module._build_volume_mounts("thread-42") - userdata_mount = mounts[1] + userdata_mount = mounts[-1] assert userdata_mount.sub_path == "deer-flow/users/default/threads/thread-42/user-data" - def test_skills_mount_read_only(self, provisioner_module): - """Skills mount should always be read-only.""" - mounts = provisioner_module._build_volume_mounts("thread-1") - assert mounts[0].read_only is True - - def test_userdata_mount_read_write(self, provisioner_module): - """User-data mount should always be read-write.""" - mounts = provisioner_module._build_volume_mounts("thread-1") - assert mounts[1].read_only is False - - def test_mount_paths_are_stable(self, provisioner_module): - """Mount paths must stay /mnt/skills and /mnt/user-data.""" - mounts = provisioner_module._build_volume_mounts("thread-1") - assert mounts[0].mount_path == "/mnt/skills" - assert mounts[1].mount_path == "/mnt/user-data" - - def test_mount_names_match_volumes(self, provisioner_module): - """Mount names should match the volume names.""" - mounts = provisioner_module._build_volume_mounts("thread-1") - assert mounts[0].name == "skills" - assert mounts[1].name == "user-data" - - def test_returns_two_mounts(self, provisioner_module): - """Should always return exactly two mounts.""" - assert len(provisioner_module._build_volume_mounts("t")) == 2 - # ── _build_pod integration ───────────────────────────────────────────── @@ -139,18 +217,54 @@ def test_returns_two_mounts(self, provisioner_module): class TestBuildPodVolumes: """Integration: _build_pod should wire volumes and mounts correctly.""" - def test_pod_spec_has_volumes(self, provisioner_module): - """Pod spec should contain exactly 2 volumes.""" + def test_pod_hostpath_without_legacy_has_three_volumes(self, provisioner_module): + """hostPath Pod spec should omit legacy volume by default.""" provisioner_module.SKILLS_PVC_NAME = "" provisioner_module.USERDATA_PVC_NAME = "" pod = provisioner_module._build_pod("sandbox-1", "thread-1") - assert len(pod.spec.volumes) == 2 + assert len(pod.spec.volumes) == 3 - def test_pod_spec_has_volume_mounts(self, provisioner_module): - """Container should have exactly 2 volume mounts.""" + def test_pod_hostpath_without_legacy_has_three_mounts(self, provisioner_module): + """hostPath container should omit legacy mount by default.""" provisioner_module.SKILLS_PVC_NAME = "" provisioner_module.USERDATA_PVC_NAME = "" pod = provisioner_module._build_pod("sandbox-1", "thread-1") + assert len(pod.spec.containers[0].volume_mounts) == 3 + + def test_pod_hostpath_with_legacy_has_four_volumes(self, provisioner_module): + """Legacy volume should be present when the backend requests it.""" + provisioner_module.SKILLS_PVC_NAME = "" + provisioner_module.USERDATA_PVC_NAME = "" + pod = provisioner_module._build_pod( + "sandbox-1", + "thread-1", + include_legacy_skills=True, + ) + assert len(pod.spec.volumes) == 4 + + def test_pod_hostpath_with_legacy_has_four_mounts(self, provisioner_module): + """Legacy mount should be present when the backend requests it.""" + provisioner_module.SKILLS_PVC_NAME = "" + provisioner_module.USERDATA_PVC_NAME = "" + pod = provisioner_module._build_pod( + "sandbox-1", + "thread-1", + include_legacy_skills=True, + ) + assert len(pod.spec.containers[0].volume_mounts) == 4 + + def test_pod_pvc_has_two_volumes(self, provisioner_module): + """PVC Pod spec should contain exactly 2 volumes.""" + provisioner_module.SKILLS_PVC_NAME = "skills-pvc" + provisioner_module.USERDATA_PVC_NAME = "" + pod = provisioner_module._build_pod("sandbox-1", "thread-1") + assert len(pod.spec.volumes) == 2 + + def test_pod_pvc_has_two_mounts(self, provisioner_module): + """PVC container should have exactly 2 volume mounts.""" + provisioner_module.SKILLS_PVC_NAME = "skills-pvc" + provisioner_module.USERDATA_PVC_NAME = "" + pod = provisioner_module._build_pod("sandbox-1", "thread-1") assert len(pod.spec.containers[0].volume_mounts) == 2 def test_pod_pvc_mode_uses_user_scoped_subpath(self, provisioner_module): @@ -159,6 +273,20 @@ def test_pod_pvc_mode_uses_user_scoped_subpath(self, provisioner_module): provisioner_module.USERDATA_PVC_NAME = "userdata-pvc" pod = provisioner_module._build_pod("sandbox-1", "thread-1", user_id="user-7") assert pod.spec.volumes[0].persistent_volume_claim is not None - assert pod.spec.volumes[1].persistent_volume_claim is not None - userdata_mount = pod.spec.containers[0].volume_mounts[1] + assert pod.spec.volumes[-1].persistent_volume_claim is not None + userdata_mount = pod.spec.containers[0].volume_mounts[-1] assert userdata_mount.sub_path == "deer-flow/users/user-7/threads/thread-1/user-data" + + def test_pod_three_way_skills_mount_paths(self, provisioner_module): + """Ensure public/custom/legacy mount paths are correct.""" + provisioner_module.SKILLS_PVC_NAME = "" + provisioner_module.USERDATA_PVC_NAME = "" + pod = provisioner_module._build_pod( + "sandbox-1", + "thread-1", + include_legacy_skills=True, + ) + mount_paths = {m.name: m.mount_path for m in pod.spec.containers[0].volume_mounts} + assert mount_paths["skills-public"] == "/mnt/skills/public" + assert mount_paths["skills-custom"] == "/mnt/skills/custom" + assert mount_paths["skills-legacy"] == "/mnt/skills/legacy" diff --git a/backend/tests/test_provisioner_request_threading.py b/backend/tests/test_provisioner_request_threading.py index 24ea81b3afc..a834d085df9 100644 --- a/backend/tests/test_provisioner_request_threading.py +++ b/backend/tests/test_provisioner_request_threading.py @@ -27,6 +27,7 @@ def __init__( self.ready_after_service_reads = ready_after_service_reads or {} self.service_read_counts: dict[str, int] = {} self.created_pods: list[str] = [] + self.created_pod_specs: dict[str, object] = {} self.created_services: list[str] = [] def _record_k8s_call(self) -> None: @@ -58,6 +59,7 @@ def create_namespaced_pod(self, _namespace: str, pod) -> None: self._record_k8s_call() sandbox_id = pod.metadata.labels["sandbox-id"] self.created_pods.append(sandbox_id) + self.created_pod_specs[sandbox_id] = pod def create_namespaced_service(self, _namespace: str, service) -> None: self._record_k8s_call() @@ -157,3 +159,46 @@ async def test_sandbox_business_routes_run_k8s_client_off_event_loop_thread( if expected_created_sandbox is not None: assert fake_core_v1.created_pods == [expected_created_sandbox] assert fake_core_v1.created_services == [expected_created_sandbox] + + +@pytest.mark.parametrize( + ("include_legacy_skills", "expected_mount_names"), + [ + ( + False, + ["skills-public", "skills-custom", "user-data"], + ), + ( + True, + ["skills-public", "skills-custom", "skills-legacy", "user-data"], + ), + ], + ids=["without-legacy", "with-legacy"], +) +def test_create_sandbox_route_builds_expected_skills_mount_layout( + include_legacy_skills: bool, + expected_mount_names: list[str], + monkeypatch: pytest.MonkeyPatch, + provisioner_module, +) -> None: + fake_core_v1 = _RecordingCoreV1( + event_loop_thread_id=-1, + ready_after_service_reads={"sandbox-layout": 1}, + ) + monkeypatch.setattr(provisioner_module, "core_v1", fake_core_v1) + + response = provisioner_module.create_sandbox( + provisioner_module.CreateSandboxRequest( + sandbox_id="sandbox-layout", + thread_id="thread-1", + user_id="user-1", + include_legacy_skills=include_legacy_skills, + ) + ) + + assert response.status == "Running" + pod = fake_core_v1.created_pod_specs["sandbox-layout"] + volume_names = [volume.name for volume in pod.spec.volumes] + mount_names = [mount.name for mount in pod.spec.containers[0].volume_mounts] + assert volume_names == expected_mount_names + assert mount_names == expected_mount_names diff --git a/backend/tests/test_remote_sandbox_backend.py b/backend/tests/test_remote_sandbox_backend.py index 6550cae10bd..f6abe4e08ce 100644 --- a/backend/tests/test_remote_sandbox_backend.py +++ b/backend/tests/test_remote_sandbox_backend.py @@ -3,8 +3,11 @@ import pytest import requests +import deerflow.skills.storage as storage_mod +from deerflow.community.aio_sandbox import remote_backend as remote_backend_mod from deerflow.community.aio_sandbox.remote_backend import RemoteSandboxBackend from deerflow.community.aio_sandbox.sandbox_info import SandboxInfo +from deerflow.skills.types import SkillCategory class _StubResponse: @@ -123,6 +126,25 @@ def mock_get(url: str, timeout: int): assert infos[0].sandbox_url == "http://k3s:31001" +@pytest.mark.parametrize( + ("categories", "expected"), + [ + ([SkillCategory.LEGACY], True), + (["legacy"], True), + ([SkillCategory.CUSTOM], False), + ], +) +def test_user_should_see_legacy_skills_follows_storage_visibility_rule(monkeypatch, categories, expected): + class _Storage: + def load_skills(self, *, enabled_only: bool = False): + assert enabled_only is False + return [type("SkillStub", (), {"category": category})() for category in categories] + + monkeypatch.setattr(storage_mod, "get_or_new_user_skill_storage", lambda user_id: _Storage()) + + assert storage_mod.user_should_see_legacy_skills("user-1") is expected + + @pytest.mark.parametrize("expected_user_id", [None, "owner-1"]) def test_create_delegates_to_provisioner_create(monkeypatch, expected_user_id): backend = RemoteSandboxBackend("http://provisioner:8002") @@ -148,6 +170,7 @@ def mock_create(thread_id: str, sandbox_id: str, extra_mounts=None, *, user_id=N def test_provisioner_create_returns_sandbox_info(monkeypatch): backend = RemoteSandboxBackend("http://provisioner:8002") + monkeypatch.setattr(remote_backend_mod, "user_should_see_legacy_skills", lambda user_id: True) def mock_post(url: str, json: dict, timeout: int): assert url == "http://provisioner:8002/api/sandboxes" @@ -155,6 +178,7 @@ def mock_post(url: str, json: dict, timeout: int): "sandbox_id": "abc123", "thread_id": "thread-1", "user_id": "test-user-autouse", + "include_legacy_skills": True, } assert timeout == 30 return _StubResponse(payload={"sandbox_id": "abc123", "sandbox_url": "http://k3s:31001"}) @@ -168,6 +192,7 @@ def mock_post(url: str, json: dict, timeout: int): def test_provisioner_create_accepts_anonymous_thread_id(monkeypatch): backend = RemoteSandboxBackend("http://provisioner:8002") + monkeypatch.setattr(remote_backend_mod, "user_should_see_legacy_skills", lambda user_id: False) def mock_post(url: str, json: dict, timeout: int): assert url == "http://provisioner:8002/api/sandboxes" @@ -175,6 +200,7 @@ def mock_post(url: str, json: dict, timeout: int): "sandbox_id": "anon123", "thread_id": None, "user_id": "test-user-autouse", + "include_legacy_skills": False, } assert timeout == 30 return _StubResponse(payload={"sandbox_id": "anon123", "sandbox_url": "http://k3s:31002"}) @@ -188,6 +214,7 @@ def mock_post(url: str, json: dict, timeout: int): def test_provisioner_create_raises_runtime_error_on_request_exception(monkeypatch): backend = RemoteSandboxBackend("http://provisioner:8002") + monkeypatch.setattr(remote_backend_mod, "user_should_see_legacy_skills", lambda user_id: False) def mock_post(url: str, json: dict, timeout: int): raise requests.RequestException("boom") diff --git a/backend/tests/test_three_way_skills_mount_e2e.py b/backend/tests/test_three_way_skills_mount_e2e.py new file mode 100644 index 00000000000..bb166740a79 --- /dev/null +++ b/backend/tests/test_three_way_skills_mount_e2e.py @@ -0,0 +1,336 @@ +"""End-to-end tests for three-way skills mount across sandbox providers. + +Verifies that (a) public, (b) per-user custom, and (c) legacy global-custom +skills all resolve to correct container paths that the sandbox providers +actually mount — covering ``LocalSandboxProvider`` and +``AioSandboxProvider`` (DooD / local-backend path). + +Includes a full-pipeline test that exercises the actual path the model +takes: ``UserScopedSkillStorage`` category assignment → ``Skill.get_container_file_path()`` → ``sandbox.read_file()``. +""" + +import importlib +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + +from deerflow.config.paths import Paths +from deerflow.sandbox.local.local_sandbox import PathMapping +from deerflow.sandbox.local.local_sandbox_provider import LocalSandboxProvider +from deerflow.skills.types import SKILL_MD_FILE, Skill, SkillCategory + +_AIO_MODULE = "deerflow.community.aio_sandbox.aio_sandbox_provider" +_AIO_GET_CONFIG = f"{_AIO_MODULE}.get_app_config" + + +def _write_skill(base: Path, name: str, description: str = "test skill") -> Path: + skill_dir = base / name + skill_dir.mkdir(parents=True, exist_ok=True) + skill_md = skill_dir / SKILL_MD_FILE + skill_md.write_text( + f"---\nname: {name}\ndescription: {description}\n---\n\n# {name}\n", + encoding="utf-8", + ) + return skill_md + + +def _build_config(skills_root: Path): + from deerflow.config.sandbox_config import SandboxConfig + + return SimpleNamespace( + skills=SimpleNamespace( + container_path="/mnt/skills", + get_skills_path=lambda sk=skills_root: sk, + use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage", + ), + sandbox=SandboxConfig( + use="deerflow.sandbox.local:LocalSandboxProvider", + mounts=[], + ), + ) + + +def _local_mounts(provider: LocalSandboxProvider, thread_id: str, user_id: str) -> dict[str, PathMapping]: + mappings = list(provider._path_mappings) + provider._build_thread_path_mappings(thread_id, user_id=user_id) + return {m.container_path: m for m in mappings} + + +@pytest.fixture +def skills_fs(tmp_path: Path) -> dict: + root = tmp_path / "skills" + pub = root / "public" + legacy = root / "custom" + users_dir = tmp_path / "users" + user_custom = users_dir / "user-1" / "skills" / "custom" + + return { + "root": root, + "public": pub, + "legacy_global": legacy, + "user_custom": user_custom, + "users_dir": users_dir, + "pub_skill": _write_skill(pub, "pub-skill", "public skill"), + "legacy_skill": _write_skill(legacy, "leg-skill", "legacy skill"), + "user_skill": _write_skill(user_custom, "usr-skill", "user custom skill"), + } + + +@pytest.fixture +def aio_mod(): + return importlib.import_module(_AIO_MODULE) + + +class TestThreeWayMountEndToEnd: + # ── LocalSandboxProvider: mount structure ────────────────────────── + + def test_local_public_skill_mounted(self, skills_fs): + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + with patch("deerflow.config.get_app_config", return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + provider = LocalSandboxProvider() + idx = _local_mounts(provider, "thread-1", user_id="user-1") + assert "/mnt/skills/public" in idx + assert idx["/mnt/skills/public"].read_only is True + + def test_local_per_user_custom_skill_mounted(self, skills_fs): + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + with patch("deerflow.config.get_app_config", return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + provider = LocalSandboxProvider() + idx = _local_mounts(provider, "thread-1", user_id="user-1") + assert "/mnt/skills/custom" in idx + assert str(skills_fs["user_custom"]) in idx["/mnt/skills/custom"].local_path + + def test_local_legacy_mounted_for_user_without_custom(self, skills_fs): + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + with patch("deerflow.config.get_app_config", return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + provider = LocalSandboxProvider() + idx = _local_mounts(provider, "thread-1", user_id="noob") + assert "/mnt/skills/legacy" in idx + assert str(skills_fs["legacy_global"]) in idx["/mnt/skills/legacy"].local_path + + def test_local_legacy_not_mounted_when_user_has_custom(self, skills_fs): + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + with patch("deerflow.config.get_app_config", return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + provider = LocalSandboxProvider() + idx = _local_mounts(provider, "thread-1", user_id="user-1") + assert "/mnt/skills/legacy" not in idx + + def test_local_legacy_still_mounted_when_user_has_only_non_skill_subdir(self, skills_fs): + (skills_fs["users_dir"] / "ghost" / "skills" / "custom" / "dangling-dir").mkdir(parents=True, exist_ok=True) + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + with patch("deerflow.config.get_app_config", return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + provider = LocalSandboxProvider() + idx = _local_mounts(provider, "thread-1", user_id="ghost") + assert "/mnt/skills/legacy" in idx + + # ── LocalSandboxProvider: read_file on container paths ───────────── + + def test_local_read_file_resolves_public_and_custom(self, skills_fs): + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + with patch("deerflow.config.get_app_config", return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + provider = LocalSandboxProvider() + sid = provider.acquire("thread-1", user_id="user-1") + sandbox = provider.get(sid) + assert "pub-skill" in sandbox.read_file("/mnt/skills/public/pub-skill/SKILL.md") + assert "usr-skill" in sandbox.read_file("/mnt/skills/custom/usr-skill/SKILL.md") + + def test_local_read_file_resolves_legacy_skill(self, skills_fs): + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + with patch("deerflow.config.get_app_config", return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + provider = LocalSandboxProvider() + sid = provider.acquire("thread-1", user_id="noob") + sandbox = provider.get(sid) + assert "leg-skill" in sandbox.read_file("/mnt/skills/legacy/leg-skill/SKILL.md") + + # ── Full pipeline: registry → container path → sandbox read ──────── + + def test_registry_to_sandbox_full_pipeline(self, skills_fs): + """Model's exact path: storage category → get_container_file_path → sandbox.read_file.""" + from deerflow.skills.storage.user_scoped_skill_storage import UserScopedSkillStorage + + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + + with patch("deerflow.config.get_app_config", return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + provider = LocalSandboxProvider() + sid_user = provider.acquire("t1", user_id="user-1") + sid_noob = provider.acquire("t2", user_id="noob") + sandbox_user = provider.get(sid_user) + sandbox_noob = provider.get(sid_noob) + + # user-1 storage: sees public + custom, no legacy + with patch("deerflow.config.paths.get_paths", return_value=paths): + storage = UserScopedSkillStorage(user_id="user-1", host_path=str(skills_fs["root"])) + skills = list(storage._iter_skill_files()) + by_name = {sf.parent.name: (cat, sf) for cat, _root, sf in skills} + + # public + assert "pub-skill" in by_name + cat, _ = by_name["pub-skill"] + assert cat == SkillCategory.PUBLIC + s = Skill(name="pub-skill", description="p", license=None, skill_dir=skills_fs["public"] / "pub-skill", skill_file=skills_fs["pub_skill"], relative_path=Path("pub-skill"), category=cat) + cp = s.get_container_file_path("/mnt/skills") + assert cp == "/mnt/skills/public/pub-skill/SKILL.md" + assert "pub-skill" in sandbox_user.read_file(cp) + + # custom + assert "usr-skill" in by_name + cat, _ = by_name["usr-skill"] + assert cat == SkillCategory.CUSTOM + s = Skill(name="usr-skill", description="u", license=None, skill_dir=skills_fs["user_custom"] / "usr-skill", skill_file=skills_fs["user_skill"], relative_path=Path("usr-skill"), category=cat) + cp = s.get_container_file_path("/mnt/skills") + assert cp == "/mnt/skills/custom/usr-skill/SKILL.md" + assert "usr-skill" in sandbox_user.read_file(cp) + + # noob storage: sees public + legacy (no per-user custom) + with patch("deerflow.config.paths.get_paths", return_value=paths): + storage = UserScopedSkillStorage(user_id="noob", host_path=str(skills_fs["root"])) + skills = list(storage._iter_skill_files()) + by_name = {sf.parent.name: (cat, sf) for cat, _root, sf in skills} + + assert "leg-skill" in by_name + cat, _ = by_name["leg-skill"] + assert cat == SkillCategory.LEGACY + s = Skill(name="leg-skill", description="l", license=None, skill_dir=skills_fs["legacy_global"] / "leg-skill", skill_file=skills_fs["legacy_skill"], relative_path=Path("leg-skill"), category=cat) + cp = s.get_container_file_path("/mnt/skills") + assert cp == "/mnt/skills/legacy/leg-skill/SKILL.md" + assert "leg-skill" in sandbox_noob.read_file(cp) + + # ── AioSandboxProvider ────────────────────────────────────────────── + + def test_aio_public_skill_mount(self, skills_fs, aio_mod): + cfg = _build_config(skills_fs["root"]) + with patch(_AIO_GET_CONFIG, return_value=cfg): + mounts = aio_mod.AioSandboxProvider._get_skills_mounts(user_id="user-1") + idx = {m[1]: m for m in mounts} + assert "/mnt/skills/public" in idx + + def test_aio_per_user_custom_skill_mount(self, skills_fs, aio_mod, monkeypatch): + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + monkeypatch.setattr(aio_mod, "get_paths", lambda: paths) + with patch(_AIO_GET_CONFIG, return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + mounts = aio_mod.AioSandboxProvider._get_skills_mounts(user_id="user-1") + idx = {m[1]: m for m in mounts} + assert "/mnt/skills/custom" in idx + host, _, _ = idx["/mnt/skills/custom"] + assert "users/user-1/skills/custom" in host.replace("\\", "/") + + def test_aio_legacy_mounted_for_user_without_custom(self, skills_fs, aio_mod, monkeypatch): + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + monkeypatch.setattr(aio_mod, "get_paths", lambda: paths) + with patch(_AIO_GET_CONFIG, return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + mounts = aio_mod.AioSandboxProvider._get_skills_mounts(user_id="noob") + idx = {m[1]: m for m in mounts} + assert "/mnt/skills/legacy" in idx + + def test_aio_legacy_not_mounted_when_user_has_custom(self, skills_fs, aio_mod, monkeypatch): + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + monkeypatch.setattr(aio_mod, "get_paths", lambda: paths) + with patch(_AIO_GET_CONFIG, return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + mounts = aio_mod.AioSandboxProvider._get_skills_mounts(user_id="user-1") + idx = {m[1]: m for m in mounts} + assert "/mnt/skills/legacy" not in idx + + def test_aio_legacy_still_mounted_when_user_has_only_non_skill_subdir(self, skills_fs, aio_mod, monkeypatch): + (skills_fs["users_dir"] / "ghost" / "skills" / "custom" / "dangling-dir").mkdir(parents=True, exist_ok=True) + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + monkeypatch.setattr(aio_mod, "get_paths", lambda: paths) + with patch(_AIO_GET_CONFIG, return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + mounts = aio_mod.AioSandboxProvider._get_skills_mounts(user_id="ghost") + idx = {m[1]: m for m in mounts} + assert "/mnt/skills/legacy" in idx + + # ── AIO → Docker --mount translation ─────────────────────────────── + + def test_aio_extra_mounts_translate_to_docker_bind_mounts(self, skills_fs, aio_mod, monkeypatch): + """extra_mounts → _format_container_mount → correct Docker --mount args.""" + from deerflow.community.aio_sandbox.local_backend import _format_container_mount + + cfg = _build_config(skills_fs["root"]) + paths = Paths(base_dir=skills_fs["users_dir"].parent) + monkeypatch.setattr(aio_mod, "get_paths", lambda: paths) + + with patch(_AIO_GET_CONFIG, return_value=cfg), patch("deerflow.config.paths.get_paths", return_value=paths): + extra = aio_mod.AioSandboxProvider._get_extra_mounts( + aio_mod.AioSandboxProvider.__new__(aio_mod.AioSandboxProvider), + "thread-1", + user_id="noob", + ) + + # extra includes thread mounts + skills mounts + docker_args: list[str] = [] + mount_entries: dict[str, str] = {} + for host, container, ro in extra: + args = _format_container_mount("docker", host, container, ro) + docker_args.extend(args) + if args[0] == "--mount": + mount_entries[container] = args[1] + + assert "--mount" in docker_args + # Skills mounts must be present + assert "/mnt/skills/public" in mount_entries + assert "dst=/mnt/skills/public" in mount_entries["/mnt/skills/public"] + assert "readonly" in mount_entries["/mnt/skills/public"] + + assert "/mnt/skills/custom" in mount_entries + assert "dst=/mnt/skills/custom" in mount_entries["/mnt/skills/custom"] + assert "users/noob/skills/custom" in mount_entries["/mnt/skills/custom"] + + # noob has no per-user custom → legacy is mounted + assert "/mnt/skills/legacy" in mount_entries + assert "dst=/mnt/skills/legacy" in mount_entries["/mnt/skills/legacy"] + + # ── Path alignment ────────────────────────────────────────────────── + + def test_skill_container_paths_match_expected_mounts(self, skills_fs): + cr = "/mnt/skills" + assert ( + Skill( + name="p", + description="", + license=None, + skill_dir=skills_fs["public"] / "pub-skill", + skill_file=skills_fs["pub_skill"], + relative_path=Path("pub-skill"), + category=SkillCategory.PUBLIC, + ).get_container_path(cr) + == "/mnt/skills/public/pub-skill" + ) + + assert ( + Skill( + name="u", + description="", + license=None, + skill_dir=skills_fs["user_custom"] / "usr-skill", + skill_file=skills_fs["user_skill"], + relative_path=Path("usr-skill"), + category=SkillCategory.CUSTOM, + ).get_container_path(cr) + == "/mnt/skills/custom/usr-skill" + ) + + assert ( + Skill( + name="l", + description="", + license=None, + skill_dir=skills_fs["legacy_global"] / "leg-skill", + skill_file=skills_fs["legacy_skill"], + relative_path=Path("leg-skill"), + category=SkillCategory.LEGACY, + ).get_container_path(cr) + == "/mnt/skills/legacy/leg-skill" + ) diff --git a/docker/docker-compose-dev.yaml b/docker/docker-compose-dev.yaml index 19ffc2e4c06..69c185baa9a 100644 --- a/docker/docker-compose-dev.yaml +++ b/docker/docker-compose-dev.yaml @@ -52,6 +52,8 @@ services: # export DEER_FLOW_ROOT=/absolute/path/to/deer-flow - SKILLS_HOST_PATH=${DEER_FLOW_ROOT}/skills - THREADS_HOST_PATH=${DEER_FLOW_ROOT}/backend/.deer-flow/threads + # Per-user data base directory for user-scoped skill mounts + - DEER_FLOW_HOST_BASE_DIR=${DEER_FLOW_ROOT}/backend/.deer-flow # Production: use PVC instead of hostPath to avoid data loss on node failure. # When set, hostPath vars above are ignored for the corresponding volume. # USERDATA_PVC_NAME uses subPath (deer-flow/users/{user_id}/threads/{thread_id}/user-data) automatically. diff --git a/docker/docker-compose.yaml b/docker/docker-compose.yaml index 30e7880b573..32b07f7a73d 100644 --- a/docker/docker-compose.yaml +++ b/docker/docker-compose.yaml @@ -158,6 +158,7 @@ services: - SANDBOX_IMAGE=enterprise-public-cn-beijing.cr.volces.com/vefaas-public/all-in-one-sandbox:latest - SKILLS_HOST_PATH=${DEER_FLOW_REPO_ROOT}/skills - THREADS_HOST_PATH=${DEER_FLOW_HOME}/threads + - DEER_FLOW_HOST_BASE_DIR=${DEER_FLOW_HOME} - KUBECONFIG_PATH=/root/.kube/config - NODE_HOST=host.docker.internal - K8S_API_SERVER=https://host.docker.internal:26443 diff --git a/docker/provisioner/app.py b/docker/provisioner/app.py index 8015ba5ead0..77f559c5fea 100644 --- a/docker/provisioner/app.py +++ b/docker/provisioner/app.py @@ -60,6 +60,7 @@ ) SKILLS_HOST_PATH = os.environ.get("SKILLS_HOST_PATH", "/skills") THREADS_HOST_PATH = os.environ.get("THREADS_HOST_PATH", "/.deer-flow/threads") +DEER_FLOW_HOST_BASE_DIR = os.environ.get("DEER_FLOW_HOST_BASE_DIR", "/.deer-flow") SKILLS_PVC_NAME = os.environ.get("SKILLS_PVC_NAME", "") USERDATA_PVC_NAME = os.environ.get("USERDATA_PVC_NAME", "") SANDBOX_CONTAINER_PORT_RAW = os.environ.get("SANDBOX_CONTAINER_PORT", "8080") @@ -227,6 +228,7 @@ class CreateSandboxRequest(BaseModel): sandbox_id: str thread_id: str = Field(pattern=SAFE_THREAD_ID_PATTERN) user_id: str = Field(default=DEFAULT_USER_ID, pattern=SAFE_USER_ID_PATTERN) + include_legacy_skills: bool = False class SandboxResponse(BaseModel): @@ -250,26 +252,79 @@ def _sandbox_url(node_port: int) -> str: """Build the sandbox URL using the configured NODE_HOST.""" return f"http://{NODE_HOST}:{node_port}" +def _build_volumes( + thread_id: str, + user_id: str = DEFAULT_USER_ID, + *, + include_legacy_skills: bool = False, +) -> list[k8s_client.V1Volume]: + """Build volume list: PVC when configured, otherwise hostPath. + + Skills are split into public, per-user custom, and legacy (global-custom) + volumes so that ``/mnt/skills/{public,custom,legacy}/`` paths resolve + correctly inside the sandbox — matching the hostPath layout produced by + ``LocalSandboxProvider`` and ``AioSandboxProvider``. + """ + volumes: list[k8s_client.V1Volume] = [] + + # ── Skills volumes ──────────────────────────────────────────────── -def _build_volumes(thread_id: str) -> list[k8s_client.V1Volume]: - """Build volume list: PVC when configured, otherwise hostPath.""" if SKILLS_PVC_NAME: - skills_vol = k8s_client.V1Volume( - name="skills", - persistent_volume_claim=k8s_client.V1PersistentVolumeClaimVolumeSource( - claim_name=SKILLS_PVC_NAME, - read_only=True, - ), + # PVC mode: three-way subPath not yet supported; fall back to + # single-volume mount for backward compatibility. + logger.warning( + "SKILLS_PVC_NAME is set — three-way skills layout is not " + "supported in PVC mode yet; falling back to single /mnt/skills mount" + ) + volumes.append( + k8s_client.V1Volume( + name="skills", + persistent_volume_claim=k8s_client.V1PersistentVolumeClaimVolumeSource( + claim_name=SKILLS_PVC_NAME, + read_only=True, + ), + ) ) else: - skills_vol = k8s_client.V1Volume( - name="skills", - host_path=k8s_client.V1HostPathVolumeSource( - path=SKILLS_HOST_PATH, - type="Directory", - ), + # hostPath mode: three-way layout + public_path = join_host_path(SKILLS_HOST_PATH, "public") + volumes.append( + k8s_client.V1Volume( + name="skills-public", + host_path=k8s_client.V1HostPathVolumeSource( + path=public_path, + type="Directory", + ), + ) + ) + + user_custom_path = join_host_path( + DEER_FLOW_HOST_BASE_DIR, "users", user_id, "skills", "custom", + ) + volumes.append( + k8s_client.V1Volume( + name="skills-custom", + host_path=k8s_client.V1HostPathVolumeSource( + path=user_custom_path, + type="DirectoryOrCreate", + ), + ) ) + if include_legacy_skills: + legacy_path = join_host_path(SKILLS_HOST_PATH, "custom") + volumes.append( + k8s_client.V1Volume( + name="skills-legacy", + host_path=k8s_client.V1HostPathVolumeSource( + path=legacy_path, + type="Directory", + ), + ) + ) + + # ── User-data volume ────────────────────────────────────────────── + if USERDATA_PVC_NAME: userdata_vol = k8s_client.V1Volume( name="user-data", @@ -286,13 +341,56 @@ def _build_volumes(thread_id: str) -> list[k8s_client.V1Volume]: ), ) - return [skills_vol, userdata_vol] + volumes.append(userdata_vol) + return volumes def _build_volume_mounts( - thread_id: str, user_id: str = DEFAULT_USER_ID + thread_id: str, + user_id: str = DEFAULT_USER_ID, + *, + include_legacy_skills: bool = False, ) -> list[k8s_client.V1VolumeMount]: - """Build volume mount list, using subPath for PVC user-data.""" + """Build volume mount list, mirroring three-way skills layout. + + Skills are mounted to ``/mnt/skills/{public,custom,legacy}/`` so that + category-aware ``Skill.get_container_path()`` paths resolve correctly. + PVC mode falls back to a single ``/mnt/skills`` mount. + """ + mounts: list[k8s_client.V1VolumeMount] = [] + + if SKILLS_PVC_NAME: + mounts.append( + k8s_client.V1VolumeMount( + name="skills", + mount_path="/mnt/skills", + read_only=True, + ) + ) + else: + mounts.extend( + [ + k8s_client.V1VolumeMount( + name="skills-public", + mount_path="/mnt/skills/public", + read_only=True, + ), + k8s_client.V1VolumeMount( + name="skills-custom", + mount_path="/mnt/skills/custom", + read_only=True, + ), + ] + ) + if include_legacy_skills: + mounts.append( + k8s_client.V1VolumeMount( + name="skills-legacy", + mount_path="/mnt/skills/legacy", + read_only=True, + ) + ) + userdata_mount = k8s_client.V1VolumeMount( name="user-data", mount_path="/mnt/user-data", @@ -302,19 +400,17 @@ def _build_volume_mounts( userdata_mount.sub_path = ( f"deer-flow/users/{user_id}/threads/{thread_id}/user-data" ) + mounts.append(userdata_mount) - return [ - k8s_client.V1VolumeMount( - name="skills", - mount_path="/mnt/skills", - read_only=True, - ), - userdata_mount, - ] + return mounts def _build_pod( - sandbox_id: str, thread_id: str, user_id: str = DEFAULT_USER_ID + sandbox_id: str, + thread_id: str, + user_id: str = DEFAULT_USER_ID, + *, + include_legacy_skills: bool = False, ) -> k8s_client.V1Pod: """Construct a Pod manifest for a single sandbox.""" return k8s_client.V1Pod( @@ -373,14 +469,22 @@ def _build_pod( "ephemeral-storage": "500Mi", }, ), - volume_mounts=_build_volume_mounts(thread_id, user_id=user_id), + volume_mounts=_build_volume_mounts( + thread_id, + user_id=user_id, + include_legacy_skills=include_legacy_skills, + ), security_context=k8s_client.V1SecurityContext( privileged=False, allow_privilege_escalation=True, ), ) ], - volumes=_build_volumes(thread_id), + volumes=_build_volumes( + thread_id, + user_id=user_id, + include_legacy_skills=include_legacy_skills, + ), restart_policy="Always", ), ) @@ -457,12 +561,14 @@ def create_sandbox(req: CreateSandboxRequest): sandbox_id = req.sandbox_id thread_id = req.thread_id user_id = req.user_id + include_legacy_skills = req.include_legacy_skills logger.info( - "Received request to create sandbox '%s' for thread '%s' user '%s'", + "Received request to create sandbox '%s' for thread '%s' user '%s' include_legacy_skills=%s", sandbox_id, thread_id, user_id, + include_legacy_skills, ) # ── Fast path: sandbox already exists ──────────────────────────── @@ -477,7 +583,13 @@ def create_sandbox(req: CreateSandboxRequest): # ── Create Pod ─────────────────────────────────────────────────── try: core_v1.create_namespaced_pod( - K8S_NAMESPACE, _build_pod(sandbox_id, thread_id, user_id=user_id) + K8S_NAMESPACE, + _build_pod( + sandbox_id, + thread_id, + user_id=user_id, + include_legacy_skills=include_legacy_skills, + ), ) logger.info(f"Created Pod {_pod_name(sandbox_id)}") except ApiException as exc: From 01dc06799719df12e424c9790c92f890a3891be5 Mon Sep 17 00:00:00 2001 From: Ryker_Feng <90562015+18062706139fcz@users.noreply.github.com> Date: Wed, 8 Jul 2026 17:10:27 +0800 Subject: [PATCH 002/116] feat: add composer input polishing (#3986) * feat: add composer input polishing * Revert "Merge branch 'main' into feat/input-polish" This reverts commit 5b6ceccf0db3092bc62fde3b05e7816829601756, reversing changes made to 45fbc57fef5fa5fd878cf0176c37f3e3bc7ebef6. * Merge main into feat/input-polish * style(frontend): format input helper polish guard * fix(input-polish): address composer polish review findings Frontend - Add a cancel affordance to the in-flight polish status pill that calls abortInputPolishRequest(), so a slow/hung provider no longer hard-locks the composer for up to stream_chunk_timeout with a page reload (and draft loss) as the only escape. - Reset promptHistoryIndexRef/promptHistoryDraftRef when a rewrite is applied (and on undo), so a stale history-browse index can no longer let the next ArrowDown silently overwrite the polished draft. - Disable polishing while an open human-input card is present, matching the frontend/AGENTS.md rule that composer entry points defer to the card so card-reply metadata is preserved. - canPolishInput now reuses parseGoalCommand/parseCompactCommand instead of a third hardcoded reserved-command regex, and drops the phantom /help entry (no /help parser exists in the composer), so future builtins only need to be taught to the existing parsers. Backend - Extract the non-graph one-shot LLM path (build model + inject Langfuse metadata + system/user invoke + text extract) into deerflow.utils.oneshot_llm.run_oneshot_llm, shared by the input-polish and suggestions routers so tracing-metadata and invocation shape cannot drift between the two copies. - strip_think_blocks gains truncate_unclosed (default True, preserving the suggestions/goal JSON-prep behavior); input polish passes False so a draft that legitimately contains a literal substring is no longer truncated into a partial rewrite or a spurious 503. - Validate the empty-check and max_chars boundary against the same stripped view of the draft that is sent to the model, so the user-facing length boundary and the model input can no longer disagree. Tests / docs - Backend: literal- preservation, whitespace-only rejection, and normalized-length/model-input agreement cases; suggestions tests repoint the create_chat_model patch to the shared helper module. - Frontend: helper unit tests updated for the /help/reserved-command change; a new Playwright case covers cancelling an in-flight polish request. - backend/AGENTS.md documents the shared one-shot helper and the polish normalization/think-tag behavior. --------- Co-authored-by: Willem Jiang --- README.md | 2 + backend/AGENTS.md | 1 + backend/app/gateway/app.py | 8 + backend/app/gateway/routers/__init__.py | 2 + backend/app/gateway/routers/input_polish.py | 107 ++++++++ backend/app/gateway/routers/suggestions.py | 23 +- .../harness/deerflow/config/app_config.py | 2 + .../deerflow/config/input_polish_config.py | 9 + .../harness/deerflow/utils/llm_text.py | 21 +- .../harness/deerflow/utils/oneshot_llm.py | 72 +++++ backend/tests/test_input_polish_router.py | 195 ++++++++++++++ backend/tests/test_suggestions_router.py | 15 +- config.example.yaml | 16 +- frontend/AGENTS.md | 4 +- .../components/workspace/input-box-helpers.ts | 11 + .../src/components/workspace/input-box.tsx | 250 +++++++++++++++++- frontend/src/core/i18n/locales/en-US.ts | 6 + frontend/src/core/i18n/locales/types.ts | 6 + frontend/src/core/i18n/locales/zh-CN.ts | 6 + frontend/src/core/input-polish/api.ts | 32 +++ frontend/tests/e2e/chat.spec.ts | 153 +++++++++++ .../workspace/input-box-helpers.test.ts | 27 ++ 22 files changed, 925 insertions(+), 43 deletions(-) create mode 100644 backend/app/gateway/routers/input_polish.py create mode 100644 backend/packages/harness/deerflow/config/input_polish_config.py create mode 100644 backend/packages/harness/deerflow/utils/oneshot_llm.py create mode 100644 backend/tests/test_input_polish_router.py create mode 100644 frontend/src/core/input-polish/api.ts diff --git a/README.md b/README.md index dae57c915b2..0fb6176c882 100644 --- a/README.md +++ b/README.md @@ -631,6 +631,8 @@ Tools follow the same philosophy. DeerFlow comes with a core toolset — web sea Gateway-generated follow-up suggestions now normalize both plain-string model output and block/list-style rich content before parsing the JSON array response, so provider-specific content wrappers do not silently drop suggestions. +The Web UI composer can polish draft input before sending. The rewrite runs as a short Gateway LLM request using the `input_polish` model configuration, keeps slash skill prefixes such as `/data-analysis`, and only replaces the local draft after the user clicks the polish button; it does not create a thread run or persist a message. + Interrupted first-turn runs still persist a fallback conversation title, so stopping a streaming response does not leave the thread as "Untitled" after refresh. In the Web UI, completed assistant turns can be branched into a new main conversation. The new thread starts from that turn's checkpoint. Because workspace files are not checkpointed, the branch only receives a best-effort copy of the current workspace when you branch from the latest turn; branching from an older turn keeps just the restored message history so the branch never inherits files that were created in a later part of the conversation. diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 95b9a1cda3e..c43e6f85346 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -315,6 +315,7 @@ CORS is same-origin by default when requests enter through nginx on port 2026. S | **Threads** (`/api/threads/{id}`) | `DELETE /` - remove DeerFlow-managed local thread data after LangGraph thread deletion; `POST /branches` - create a new main-thread branch from a completed assistant turn checkpoint. Workspace files are not checkpointed, so the branch only best-effort copies the current workspace when branching from the **latest** turn (`workspace_clone_mode="current_thread_best_effort"`); branching from an older/historical turn skips the copy (`workspace_clone_mode="skipped_historical_turn"`) so the branch never inherits files that only exist in a later timeline; `GET /goal`, `PUT /goal`, `DELETE /goal` - read, set, and clear the active thread goal; `POST /compact` - manually summarize older active context into `summary_text` and retain the recent message window, blocked while a run is in flight; unexpected failures are logged server-side and return a generic 500 detail | | **Artifacts** (`/api/threads/{id}/artifacts`) | `GET /{path}` - serve artifacts; active content types (`text/html`, `application/xhtml+xml`, `image/svg+xml`) are always forced as download attachments to reduce XSS risk; `?download=true` still forces download for other file types | | **Suggestions** (`/api/suggestions`) | `GET /config` - returns global suggestions config boolean; `POST /threads/{id}/suggestions` - generate follow-up questions; rich list/block model content is normalized and inline reasoning (`...`, including unclosed/truncated blocks from reasoning models like MiniMax-M3) is stripped before JSON parsing | +| **Input Polish** (`/api/input-polish`) | `POST /` - rewrite a composer draft before it is sent. This is a short authenticated `runs:create` LLM request using `input_polish` config; it does not create a LangGraph run, persist a message, or modify thread state. Shares the non-graph one-shot LLM path (`deerflow.utils.oneshot_llm.run_oneshot_llm`) with the suggestions route so model build + Langfuse metadata + invoke stay in one place; validates the same stripped view of the draft it sends to the model, and preserves literal `` substrings in the rewrite (`strip_think_blocks(truncate_unclosed=False)`) | | **Thread Runs** (`/api/threads/{id}/runs`) | `POST /` - create background run; `POST /stream` - create + SSE stream; `POST /wait` - create + block; `POST /regenerate/prepare` - prepare clean input + checkpoint metadata for regenerating the latest assistant answer; `GET /` - list runs; `GET /{rid}` - run details; `POST /{rid}/cancel` - cancel; `GET /{rid}/join` - join SSE; `GET /{rid}/messages` - paginated messages `{data, has_more}`; `GET /{rid}/events` - full event stream; `GET /{rid}/workspace-changes` - workspace/output file change summary and optional diffs; `GET /../messages` - thread messages with feedback; `GET /../token-usage` - aggregate tokens | | **Feedback** (`/api/threads/{id}/runs/{rid}/feedback`) | `PUT /` - upsert feedback; `DELETE /` - delete user feedback; `POST /` - create feedback; `GET /` - list feedback; `GET /stats` - aggregate stats; `DELETE /{fid}` - delete specific | | **Runs** (`/api/runs`) | `POST /stream` - stateless run + SSE; `POST /wait` - stateless run + block; `GET /{rid}/messages` - paginated messages by run_id `{data, has_more}` (cursor: `after_seq`/`before_seq`); `GET /{rid}/feedback` - list feedback by run_id | diff --git a/backend/app/gateway/app.py b/backend/app/gateway/app.py index fd1c71b72ec..42ccaca337d 100644 --- a/backend/app/gateway/app.py +++ b/backend/app/gateway/app.py @@ -22,6 +22,7 @@ features, feedback, github_webhooks, + input_polish, mcp, memory, models, @@ -362,6 +363,10 @@ def create_app() -> FastAPI: "name": "suggestions", "description": "Generate follow-up question suggestions for conversations", }, + { + "name": "input-polish", + "description": "Polish composer draft input before sending", + }, { "name": "channels", "description": "Manage IM channel integrations (Feishu, Slack, Telegram)", @@ -445,6 +450,9 @@ def create_app() -> FastAPI: # Suggestions API is mounted at /api/threads/{thread_id}/suggestions app.include_router(suggestions.router) + # Input polishing API is mounted at /api/input-polish + app.include_router(input_polish.router) + # User-facing IM channel connection API is mounted at /api/channels app.include_router(channel_connections.router) diff --git a/backend/app/gateway/routers/__init__.py b/backend/app/gateway/routers/__init__.py index b271a1228d1..e1750469d8d 100644 --- a/backend/app/gateway/routers/__init__.py +++ b/backend/app/gateway/routers/__init__.py @@ -1,6 +1,7 @@ from . import ( artifacts, assistants_compat, + input_polish, mcp, models, scheduled_tasks, @@ -14,6 +15,7 @@ __all__ = [ "artifacts", "assistants_compat", + "input_polish", "mcp", "models", "scheduled_tasks", diff --git a/backend/app/gateway/routers/input_polish.py b/backend/app/gateway/routers/input_polish.py new file mode 100644 index 00000000000..257b529d4b5 --- /dev/null +++ b/backend/app/gateway/routers/input_polish.py @@ -0,0 +1,107 @@ +import logging + +from fastapi import APIRouter, Depends, HTTPException, Request +from pydantic import BaseModel, Field + +import deerflow.utils.llm_text as llm_text +from app.gateway.authz import require_permission +from app.gateway.deps import get_config +from deerflow.config.app_config import AppConfig +from deerflow.utils.oneshot_llm import run_oneshot_llm + +logger = logging.getLogger(__name__) + +router = APIRouter(prefix="/api", tags=["input-polish"]) + + +class InputPolishRequest(BaseModel): + text: str = Field(..., description="Draft text currently shown in the composer") + locale: str | None = Field(default=None, description="Optional UI locale hint") + thread_id: str | None = Field(default=None, description="Optional thread id for tracing only") + + +class InputPolishResponse(BaseModel): + rewritten_text: str = Field(..., description="Polished draft text") + changed: bool = Field(..., description="Whether the model changed the original draft") + + +def _clean_rewritten_text(text: str) -> str: + # The polished draft may legitimately contain a literal "" substring + # (e.g. a draft that asks about the tag), so do NOT truncate at a dangling + # open tag here — that would silently drop the rest of a valid rewrite and + # can produce a spurious 503. Complete ... blocks are still + # removed. + candidate = llm_text.strip_think_blocks(text, truncate_unclosed=False) + candidate = llm_text.strip_markdown_code_fence(candidate) + return candidate.strip() + + +def _build_system_instruction() -> str: + return ( + "You are DeerFlow's pre-send prompt optimizer.\n" + "Rewrite the user's rough draft into a clearer instruction for an AI agent before it is sent.\n" + "Do not answer the task.\n" + "Preserve the user's language, intent, entities, file paths, URLs, code blocks, and any leading slash command prefix exactly.\n" + "Improve the draft by making the goal, scope, constraints, and desired output explicit when they are implied by the draft.\n" + "For vague quality words such as 'better', 'good-looking', or 'polished', translate them into concrete but generic quality criteria.\n" + "Do not invent facts, business context, tools, file names, dates, metrics, or user preferences that are not implied.\n" + "Prefer one concise paragraph or a short bullet list. Keep it under 180 words unless the original draft is longer.\n" + "Output only the rewritten draft, with no markdown wrapper, explanation, or alternatives." + ) + + +def _build_user_content(text: str, locale: str | None) -> str: + locale_hint = locale.strip() if locale else "same language as the draft" + return f"Locale hint: {locale_hint}\n\nRewrite this draft while preserving its intent:\n\n{text}\n" + + +@router.post( + "/input-polish", + response_model=InputPolishResponse, + summary="Polish Composer Input", + description="Rewrite a draft message before it is sent. This does not create a thread run or persist any message.", +) +@require_permission("runs", "create") +async def polish_input( + body: InputPolishRequest, + request: Request, + config: AppConfig = Depends(get_config), +) -> InputPolishResponse: + del request # Required by the auth decorator. + + if not config.input_polish.enabled: + raise HTTPException(status_code=404, detail="Input polishing is disabled") + + # Validate the same normalized view of the input that we send to the model, + # so the user-facing length boundary and the model input cannot disagree + # (e.g. a padded draft passing the check but arriving with stray whitespace). + text = body.text.strip() + if not text: + raise HTTPException(status_code=400, detail="Input text is required") + + max_chars = config.input_polish.max_chars + if len(text) > max_chars: + raise HTTPException(status_code=400, detail=f"Input text exceeds {max_chars} characters") + + model_name = config.input_polish.model_name + try: + raw = await run_oneshot_llm( + system_instruction=_build_system_instruction(), + user_content=_build_user_content(text, body.locale), + run_name="input_polish", + app_config=config, + model_name=model_name, + thread_id=body.thread_id, + ) + rewritten = _clean_rewritten_text(raw) + except Exception as exc: + logger.exception("Failed to polish input: thread_id=%s err=%s", body.thread_id, exc) + raise HTTPException(status_code=503, detail="Failed to polish input") from exc + + if not rewritten: + raise HTTPException(status_code=503, detail="Failed to polish input") + + return InputPolishResponse( + rewritten_text=rewritten, + changed=rewritten != text, + ) diff --git a/backend/app/gateway/routers/suggestions.py b/backend/app/gateway/routers/suggestions.py index c672ce5b2d7..ce001e24a5b 100644 --- a/backend/app/gateway/routers/suggestions.py +++ b/backend/app/gateway/routers/suggestions.py @@ -1,18 +1,14 @@ import json import logging -import os from fastapi import APIRouter, Depends, Request -from langchain_core.messages import HumanMessage, SystemMessage from pydantic import BaseModel, Field import deerflow.utils.llm_text as llm_text from app.gateway.authz import require_permission from app.gateway.deps import get_config from deerflow.config.app_config import AppConfig -from deerflow.models import create_chat_model -from deerflow.runtime.user_context import get_effective_user_id -from deerflow.tracing import inject_langfuse_metadata +from deerflow.utils.oneshot_llm import run_oneshot_llm logger = logging.getLogger(__name__) @@ -38,7 +34,6 @@ class SuggestionsConfigResponse(BaseModel): enabled: bool = Field(..., description="Whether follow-up suggestions are enabled globally") -_extract_response_text = llm_text.extract_response_text _strip_markdown_code_fence = llm_text.strip_markdown_code_fence _strip_think_blocks = llm_text.strip_think_blocks @@ -129,18 +124,14 @@ async def generate_suggestions( user_content = f"Conversation Context:\n{conversation}\n\nGenerate {n} follow-up questions" try: - model = create_chat_model(name=body.model_name, thinking_enabled=False, app_config=config) - invoke_config: dict = {"run_name": "suggest_agent"} - inject_langfuse_metadata( - invoke_config, - thread_id=thread_id, - user_id=get_effective_user_id(), - assistant_id="suggest_agent", + raw = await run_oneshot_llm( + system_instruction=system_instruction, + user_content=user_content, + run_name="suggest_agent", + app_config=config, model_name=body.model_name, - environment=os.environ.get("DEER_FLOW_ENV") or os.environ.get("ENVIRONMENT"), + thread_id=thread_id, ) - response = await model.ainvoke([SystemMessage(content=system_instruction), HumanMessage(content=user_content)], config=invoke_config) - raw = _extract_response_text(response.content) suggestions = _parse_json_string_list(raw) or [] cleaned = [s.replace("\n", " ").strip() for s in suggestions if s.strip()] cleaned = cleaned[:n] diff --git a/backend/packages/harness/deerflow/config/app_config.py b/backend/packages/harness/deerflow/config/app_config.py index 98cab4b4716..1b53fd30edb 100644 --- a/backend/packages/harness/deerflow/config/app_config.py +++ b/backend/packages/harness/deerflow/config/app_config.py @@ -18,6 +18,7 @@ from deerflow.config.database_config import DatabaseConfig from deerflow.config.extensions_config import ExtensionsConfig from deerflow.config.guardrails_config import GuardrailsConfig, load_guardrails_config_from_dict +from deerflow.config.input_polish_config import InputPolishConfig from deerflow.config.loop_detection_config import LoopDetectionConfig from deerflow.config.memory_config import MemoryConfig, load_memory_config_from_dict from deerflow.config.model_config import ModelConfig @@ -167,6 +168,7 @@ class AppConfig(BaseModel): acp_agents: dict[str, ACPAgentConfig] = Field(default_factory=dict, description="ACP-compatible agent configuration") subagents: SubagentsAppConfig = Field(default_factory=SubagentsAppConfig, description="Subagent runtime configuration") guardrails: GuardrailsConfig = Field(default_factory=GuardrailsConfig, description="Guardrail middleware configuration") + input_polish: InputPolishConfig = Field(default_factory=InputPolishConfig, description="Pre-send input polishing configuration.") suggestions: SuggestionsConfig = Field(default_factory=SuggestionsConfig, description="Follow-up suggestions configuration.") circuit_breaker: CircuitBreakerConfig = Field(default_factory=CircuitBreakerConfig, description="LLM circuit breaker configuration") channel_connections: ChannelConnectionsConfig = Field( diff --git a/backend/packages/harness/deerflow/config/input_polish_config.py b/backend/packages/harness/deerflow/config/input_polish_config.py new file mode 100644 index 00000000000..c2745ac8cd3 --- /dev/null +++ b/backend/packages/harness/deerflow/config/input_polish_config.py @@ -0,0 +1,9 @@ +from pydantic import BaseModel, Field + + +class InputPolishConfig(BaseModel): + """Configuration for pre-send input polishing.""" + + enabled: bool = Field(default=True, description="Whether to enable pre-send input polishing in the composer") + max_chars: int = Field(default=4000, ge=1, description="Maximum number of draft characters accepted by the input polishing endpoint") + model_name: str | None = Field(default=None, description="Optional model name override for input polishing") diff --git a/backend/packages/harness/deerflow/utils/llm_text.py b/backend/packages/harness/deerflow/utils/llm_text.py index bd792a3c068..625cb41aa5c 100644 --- a/backend/packages/harness/deerflow/utils/llm_text.py +++ b/backend/packages/harness/deerflow/utils/llm_text.py @@ -10,12 +10,23 @@ _OPEN_THINK_RE = re.compile(r"]*>", re.IGNORECASE) -def strip_think_blocks(text: str) -> str: - """Remove inline reasoning ```` blocks from a model response.""" +def strip_think_blocks(text: str, *, truncate_unclosed: bool = True) -> str: + """Remove inline reasoning ```` blocks from a model response. + + Complete ``...`` blocks are always removed. A dangling, + unclosed ```` open tag is treated as a model that was truncated + mid-thought: when ``truncate_unclosed`` is True (the default, used by JSON + parsers like suggestions/goal where trailing garbage must be dropped) the + text is cut at that tag. Callers that may legitimately echo a literal + ```` substring in their output (e.g. the input polisher rewriting a + draft that mentions the tag) pass ``truncate_unclosed=False`` so the tag is + preserved instead of silently discarding the rest of the text. + """ text = _THINK_BLOCK_RE.sub("", text) - open_match = _OPEN_THINK_RE.search(text) - if open_match: - text = text[: open_match.start()] + if truncate_unclosed: + open_match = _OPEN_THINK_RE.search(text) + if open_match: + text = text[: open_match.start()] return text.strip() diff --git a/backend/packages/harness/deerflow/utils/oneshot_llm.py b/backend/packages/harness/deerflow/utils/oneshot_llm.py new file mode 100644 index 00000000000..c9582116ab8 --- /dev/null +++ b/backend/packages/harness/deerflow/utils/oneshot_llm.py @@ -0,0 +1,72 @@ +"""Shared helper for one-shot, non-graph LLM text requests. + +Several Gateway routes (input polishing, follow-up suggestions, and title-style +rewrites) do the same thing: build a chat model from config, attach Langfuse +trace metadata, invoke it once with a system + user message pair, and pull the +plain text back out of the response. Centralizing that sequence here keeps the +tracing-metadata fields and invocation shape from drifting between routers — a +fix to one (e.g. a new Langfuse field) now applies to all callers instead of +silently regressing in whichever copy was forgotten. + +Response-text *cleaning* (think-block / code-fence stripping, JSON parsing) is +intentionally left to each caller because their post-processing differs; this +helper stops at the extracted raw text. +""" + +from __future__ import annotations + +import os + +from langchain_core.messages import HumanMessage, SystemMessage + +from deerflow.config.app_config import AppConfig +from deerflow.models import create_chat_model +from deerflow.runtime.user_context import get_effective_user_id +from deerflow.tracing import inject_langfuse_metadata +from deerflow.utils.llm_text import extract_response_text + + +def _resolve_environment() -> str | None: + return os.environ.get("DEER_FLOW_ENV") or os.environ.get("ENVIRONMENT") + + +async def run_oneshot_llm( + *, + system_instruction: str, + user_content: str, + run_name: str, + app_config: AppConfig, + model_name: str | None = None, + thread_id: str | None = None, +) -> str: + """Run a single non-graph system+user LLM turn and return the raw text. + + Args: + system_instruction: System message content. + user_content: Human message content. + run_name: LangChain ``run_name`` and Langfuse ``assistant_id`` for the call. + app_config: Application config used to build the model. + model_name: Optional model override; ``None`` uses the default model. + thread_id: Optional thread id, forwarded to Langfuse for tracing only. + + Returns: + The extracted plain-text content of the model response (uncleaned). + """ + model = create_chat_model(name=model_name, thinking_enabled=False, app_config=app_config) + invoke_config: dict = {"run_name": run_name} + inject_langfuse_metadata( + invoke_config, + thread_id=thread_id, + user_id=get_effective_user_id(), + assistant_id=run_name, + model_name=model_name, + environment=_resolve_environment(), + ) + response = await model.ainvoke( + [ + SystemMessage(content=system_instruction), + HumanMessage(content=user_content), + ], + config=invoke_config, + ) + return extract_response_text(response.content) diff --git a/backend/tests/test_input_polish_router.py b/backend/tests/test_input_polish_router.py new file mode 100644 index 00000000000..fe22a070b83 --- /dev/null +++ b/backend/tests/test_input_polish_router.py @@ -0,0 +1,195 @@ +import asyncio +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock + +import pytest +from fastapi import HTTPException + +from app.gateway.routers import input_polish +from deerflow.utils import oneshot_llm + + +def _config( + *, + enabled: bool = True, + max_chars: int = 4000, + model_name: str | None = None, +): + return SimpleNamespace( + input_polish=SimpleNamespace( + enabled=enabled, + max_chars=max_chars, + model_name=model_name, + ), + ) + + +def test_clean_rewritten_text_removes_think_and_fence(): + text = "reasoning\n```text\nrewrite this\n```" + assert input_polish._clean_rewritten_text(text) == "rewrite this" + + +def test_clean_rewritten_text_keeps_literal_think_tag(): + # A polished draft may legitimately mention the tag. The cleaner + # must not truncate at the dangling open tag (which would drop the rest of + # the rewrite and can surface as a spurious 503). + text = "Explain what the tag does in reasoning models." + assert input_polish._clean_rewritten_text(text) == "Explain what the tag does in reasoning models." + + +def test_polish_input_uses_config_model_and_preserves_response(monkeypatch): + request = input_polish.InputPolishRequest( + text="/web-dev 做一个页面", + locale="zh-CN", + thread_id="thread-1", + ) + fake_model = MagicMock() + fake_model.ainvoke = AsyncMock(return_value=MagicMock(content="/web-dev 请设计并实现一个视觉精致的页面。")) + + create_chat_model = MagicMock(return_value=fake_model) + monkeypatch.setattr(oneshot_llm, "create_chat_model", create_chat_model) + config = _config(model_name="polish-model") + + result = asyncio.run( + input_polish.polish_input.__wrapped__( + request, + request=None, + config=config, + ), + ) + + assert result.rewritten_text == "/web-dev 请设计并实现一个视觉精致的页面。" + assert result.changed is True + create_chat_model.assert_called_once_with( + name="polish-model", + thinking_enabled=False, + app_config=config, + ) + fake_model.ainvoke.assert_awaited_once() + assert fake_model.ainvoke.await_args.kwargs["config"]["run_name"] == "input_polish" + + +def test_polish_input_uses_default_model_when_config_model_is_missing(monkeypatch): + request = input_polish.InputPolishRequest(text="make this clearer") + fake_model = MagicMock() + fake_model.ainvoke = AsyncMock(return_value=MagicMock(content="Make this clearer.")) + + create_chat_model = MagicMock(return_value=fake_model) + monkeypatch.setattr(oneshot_llm, "create_chat_model", create_chat_model) + + result = asyncio.run( + input_polish.polish_input.__wrapped__( + request, + request=None, + config=_config(model_name=None), + ), + ) + + assert result.rewritten_text == "Make this clearer." + create_chat_model.assert_called_once() + assert create_chat_model.call_args.kwargs["name"] is None + + +def test_polish_input_returns_404_when_disabled(monkeypatch): + request = input_polish.InputPolishRequest(text="hello") + fake_model = MagicMock() + monkeypatch.setattr(oneshot_llm, "create_chat_model", fake_model) + + with pytest.raises(HTTPException) as exc_info: + asyncio.run( + input_polish.polish_input.__wrapped__( + request, + request=None, + config=_config(enabled=False), + ), + ) + + assert exc_info.value.status_code == 404 + fake_model.assert_not_called() + + +def test_polish_input_rejects_empty_or_too_long_input(monkeypatch): + fake_model = MagicMock() + monkeypatch.setattr(oneshot_llm, "create_chat_model", fake_model) + + with pytest.raises(HTTPException) as empty_exc: + asyncio.run( + input_polish.polish_input.__wrapped__( + input_polish.InputPolishRequest(text=" "), + request=None, + config=_config(), + ), + ) + assert empty_exc.value.status_code == 400 + + with pytest.raises(HTTPException) as long_exc: + asyncio.run( + input_polish.polish_input.__wrapped__( + input_polish.InputPolishRequest(text="hello"), + request=None, + config=_config(max_chars=4), + ), + ) + assert long_exc.value.status_code == 400 + fake_model.assert_not_called() + + +def test_polish_input_returns_503_on_model_error(monkeypatch): + request = input_polish.InputPolishRequest(text="hello") + fake_model = MagicMock() + fake_model.ainvoke = AsyncMock(side_effect=RuntimeError("boom")) + monkeypatch.setattr(oneshot_llm, "create_chat_model", MagicMock(return_value=fake_model)) + + with pytest.raises(HTTPException) as exc_info: + asyncio.run( + input_polish.polish_input.__wrapped__( + request, + request=None, + config=_config(), + ), + ) + + assert exc_info.value.status_code == 503 + + +def test_polish_input_rejects_whitespace_only_draft(monkeypatch): + # A padded draft that is empty after normalization is rejected as empty, + # matching the normalized view used for the model input. + fake_model = MagicMock() + monkeypatch.setattr(oneshot_llm, "create_chat_model", fake_model) + + with pytest.raises(HTTPException) as exc_info: + asyncio.run( + input_polish.polish_input.__wrapped__( + input_polish.InputPolishRequest(text=" \n\t "), + request=None, + config=_config(), + ), + ) + + assert exc_info.value.status_code == 400 + fake_model.assert_not_called() + + +def test_polish_input_validates_and_sends_normalized_text(monkeypatch): + # The length boundary and the model input must agree on one normalized view: + # a draft whose raw length exceeds max_chars only due to padding is accepted + # (strip fits), and the model receives the stripped text, not the padding. + raw_draft = " summarize report " # 22 chars raw, 16 chars stripped + fake_model = MagicMock() + fake_model.ainvoke = AsyncMock(return_value=MagicMock(content="Please summarize the report clearly.")) + monkeypatch.setattr(oneshot_llm, "create_chat_model", MagicMock(return_value=fake_model)) + + result = asyncio.run( + input_polish.polish_input.__wrapped__( + input_polish.InputPolishRequest(text=raw_draft), + request=None, + config=_config(max_chars=len(raw_draft.strip())), + ), + ) + + assert result.rewritten_text == "Please summarize the report clearly." + messages = fake_model.ainvoke.await_args.args[0] + human_content = messages[-1].content + assert "summarize report" in human_content + assert " summarize report " not in human_content diff --git a/backend/tests/test_suggestions_router.py b/backend/tests/test_suggestions_router.py index dcb6bbb8fb2..b50bae59717 100644 --- a/backend/tests/test_suggestions_router.py +++ b/backend/tests/test_suggestions_router.py @@ -6,6 +6,7 @@ from app.gateway.routers import suggestions from deerflow.trace_context import request_trace_context +from deerflow.utils import oneshot_llm @pytest.fixture(autouse=True) @@ -86,7 +87,7 @@ def test_generate_suggestions_strips_inline_think_block(monkeypatch): content = '\nThe user asked about deep learning. Options: maybe [1] frameworks, [2] math basics.\n\n["深度学习和机器学习的区别？", "常用框架有哪些？", "需要什么数学基础？"]' fake_model = MagicMock() fake_model.ainvoke = AsyncMock(return_value=MagicMock(content=content)) - monkeypatch.setattr(suggestions, "create_chat_model", lambda **kwargs: fake_model) + monkeypatch.setattr(oneshot_llm, "create_chat_model", lambda **kwargs: fake_model) result = asyncio.run(suggestions.generate_suggestions.__wrapped__("t1", req, request=None, config=SimpleNamespace(suggestions=SimpleNamespace(enabled=True)))) @@ -113,7 +114,7 @@ def test_generate_suggestions_parses_and_limits(monkeypatch): ) fake_model = MagicMock() fake_model.ainvoke = AsyncMock(return_value=MagicMock(content='```json\n["Q1", "Q2", "Q3", "Q4"]\n```')) - monkeypatch.setattr(suggestions, "create_chat_model", lambda **kwargs: fake_model) + monkeypatch.setattr(oneshot_llm, "create_chat_model", lambda **kwargs: fake_model) # Bypass the require_permission decorator (which needs request + # thread_store) — these tests cover the parsing logic. @@ -141,7 +142,7 @@ def test_generate_suggestions_injects_deerflow_trace_metadata_when_langfuse_enab ) fake_model = MagicMock() fake_model.ainvoke = AsyncMock(return_value=MagicMock(content='["Q1"]')) - monkeypatch.setattr(suggestions, "create_chat_model", lambda **kwargs: fake_model) + monkeypatch.setattr(oneshot_llm, "create_chat_model", lambda **kwargs: fake_model) try: with request_trace_context("suggest-trace-1"): @@ -167,7 +168,7 @@ def test_generate_suggestions_parses_list_block_content(monkeypatch): ) fake_model = MagicMock() fake_model.ainvoke = AsyncMock(return_value=MagicMock(content=[{"type": "text", "text": '```json\n["Q1", "Q2"]\n```'}])) - monkeypatch.setattr(suggestions, "create_chat_model", lambda **kwargs: fake_model) + monkeypatch.setattr(oneshot_llm, "create_chat_model", lambda **kwargs: fake_model) # Bypass the require_permission decorator (which needs request + # thread_store) — these tests cover the parsing logic. @@ -189,7 +190,7 @@ def test_generate_suggestions_parses_output_text_block_content(monkeypatch): ) fake_model = MagicMock() fake_model.ainvoke = AsyncMock(return_value=MagicMock(content=[{"type": "output_text", "text": '```json\n["Q1", "Q2"]\n```'}])) - monkeypatch.setattr(suggestions, "create_chat_model", lambda **kwargs: fake_model) + monkeypatch.setattr(oneshot_llm, "create_chat_model", lambda **kwargs: fake_model) # Bypass the require_permission decorator (which needs request + # thread_store) — these tests cover the parsing logic. @@ -208,7 +209,7 @@ def test_generate_suggestions_returns_empty_on_model_error(monkeypatch): ) fake_model = MagicMock() fake_model.ainvoke = AsyncMock(side_effect=RuntimeError("boom")) - monkeypatch.setattr(suggestions, "create_chat_model", lambda **kwargs: fake_model) + monkeypatch.setattr(oneshot_llm, "create_chat_model", lambda **kwargs: fake_model) # Bypass the require_permission decorator (which needs request + # thread_store) — these tests cover the parsing logic. @@ -232,7 +233,7 @@ def test_generate_suggestions_returns_empty_when_disabled(monkeypatch): fake_model = MagicMock() fake_model.ainvoke = AsyncMock(side_effect=RuntimeError("Model should not be called.")) - monkeypatch.setattr(suggestions, "create_chat_model", lambda **kwargs: fake_model) + monkeypatch.setattr(oneshot_llm, "create_chat_model", lambda **kwargs: fake_model) result = asyncio.run(suggestions.generate_suggestions.__wrapped__("t1", req, request=None, config=mock_config)) diff --git a/config.example.yaml b/config.example.yaml index 400745f763f..7240989a1b4 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -15,7 +15,7 @@ # ============================================================================ # Bump this number when the config schema changes. # Run `make config-upgrade` to merge new fields into your local config.yaml. -config_version: 19 +config_version: 20 # ============================================================================ # Logging @@ -911,6 +911,20 @@ suggestions: enabled: true +# ============================================================================ +# Input Polish Configuration +# ============================================================================ +# Configure whether the composer can rewrite draft input before sending. + +input_polish: + enabled: true + # Maximum draft length accepted by /api/input-polish. + max_chars: 4000 + # Optional fast model for draft polishing. Leave null to use the default chat model. + # For best UX, set this to your lowest-latency inexpensive model. + model_name: null + + # ============================================================================ # Loop Detection Configuration # ============================================================================ diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index 76d5024eeb7..8bb6ea19694 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -53,7 +53,7 @@ The frontend is a stateful chat application. Users create **threads** (conversat - `workspace/` — Chat page components (messages, artifacts, settings) - `landing/` — Landing page sections - `docs/` — Docs / MDX rendering components -- **`core/`** — Business logic, the heart of the app. Domains include `threads/` (creation, streaming, state), `api/` (LangGraph client singleton), `agents/` (custom agents), `auth/` (authentication), `artifacts/`, `channels/` (IM connections), `i18n/` (en-US, zh-CN), `settings/`, `memory/`, `skills/`, `messages/`, `mcp/`, `models/`, `suggestions/`, `tasks/`, `todos/`, `tools/`, `workspace-changes/` (run-scoped changed-file summaries and diff fetching), `config/`, `notification/`, `blog/`, plus rendering helpers (`rehype/`, `streamdown/`) and `utils/`. +- **`core/`** — Business logic, the heart of the app. Domains include `threads/` (creation, streaming, state), `api/` (LangGraph client singleton), `agents/` (custom agents), `auth/` (authentication), `artifacts/`, `channels/` (IM connections), `i18n/` (en-US, zh-CN), `settings/`, `memory/`, `skills/`, `messages/`, `mcp/`, `models/`, `input-polish/` (pre-send draft rewrite API), `suggestions/`, `tasks/`, `todos/`, `tools/`, `workspace-changes/` (run-scoped changed-file summaries and diff fetching), `config/`, `notification/`, `blog/`, plus rendering helpers (`rehype/`, `streamdown/`) and `utils/`. - **`hooks/`** — Shared React hooks - **`lib/`** — Utilities (`cn()` from clsx + tailwind-merge) - **`content/`** — MDX content (blog posts, docs) rendered by the app @@ -63,7 +63,7 @@ The frontend is a stateful chat application. Users create **threads** (conversat ### Data Flow -1. User input → thread hooks (`core/threads/hooks.ts`) → LangGraph SDK streaming +1. Optional composer helpers such as `core/input-polish` can rewrite the local draft before submission; confirmed user input then flows to thread hooks (`core/threads/hooks.ts`) → LangGraph SDK streaming 2. Stream events update thread state (messages, artifacts, todos, goal) 3. Stop actions call the LangGraph SDK stream stop path; `core/threads/hooks.ts` invalidates current-thread, token-usage, and sidebar/search caches immediately and schedules one follow-up refetch because SDK stop may finish via abort + fire-and-forget cancel before backend title finalization commits 4. TanStack Query manages server state; localStorage stores user settings diff --git a/frontend/src/components/workspace/input-box-helpers.ts b/frontend/src/components/workspace/input-box-helpers.ts index fd40848f7c5..0dbadbb1108 100644 --- a/frontend/src/components/workspace/input-box-helpers.ts +++ b/frontend/src/components/workspace/input-box-helpers.ts @@ -189,6 +189,17 @@ export function parseCompactCommand(value: string): boolean { return /^\/(?:compact|context\s+compact)\s*$/i.test(value.trim()); } +export function canPolishInput(value: string): boolean { + const trimmed = value.trim(); + if (!trimmed) { + return false; + } + // Reserved builtin command lines are routed to their own handlers, not the + // LLM, so they must not be rewritten. Reuse the same parsers the composer + // uses to dispatch them instead of maintaining a third parallel list. + return parseGoalCommand(trimmed) === null && !parseCompactCommand(trimmed); +} + export function getInputSubmitAction({ text, fileCount, diff --git a/frontend/src/components/workspace/input-box.tsx b/frontend/src/components/workspace/input-box.tsx index b4523a7636f..a000663cc9d 100644 --- a/frontend/src/components/workspace/input-box.tsx +++ b/frontend/src/components/workspace/input-box.tsx @@ -7,11 +7,13 @@ import { CheckIcon, GraduationCapIcon, LightbulbIcon, + Loader2Icon, PaperclipIcon, PlusIcon, - SparklesIcon, RocketIcon, + SparklesIcon, TargetIcon, + Undo2Icon, XIcon, ZapIcon, } from "lucide-react"; @@ -64,6 +66,8 @@ import { import { fetch } from "@/core/api/fetcher"; import { getBackendBaseURL } from "@/core/config"; import { useI18n } from "@/core/i18n/hooks"; +import { polishInputDraft } from "@/core/input-polish/api"; +import { hasOpenHumanInputRequest } from "@/core/messages/human-input"; import { isHiddenFromUIMessage } from "@/core/messages/utils"; import { useModels } from "@/core/models/hooks"; import { @@ -106,6 +110,7 @@ import { import { abortGoalRequest, beginGoalRequest, + canPolishInput, createGoalRequestState, findSuggestionTemplatePlaceholder, finishGoalRequest, @@ -253,7 +258,7 @@ export function InputBox({ ) => void | Promise; onStop?: () => void; }) { - const { t } = useI18n(); + const { locale, t } = useI18n(); const queryClient = useQueryClient(); const searchParams = useSearchParams(); const [modelDialogOpen, setModelDialogOpen] = useState(false); @@ -269,6 +274,13 @@ export function InputBox({ const textareaRef = useRef(null); const goalRequestStateRef = useRef(createGoalRequestState()); const compactRequestStateRef = useRef(createGoalRequestState()); + const inputPolishRequestRef = useRef<{ + controller: AbortController | null; + sequence: number; + }>({ + controller: null, + sequence: 0, + }); const promptHistoryIndexRef = useRef(null); const promptHistoryDraftRef = useRef(""); @@ -278,6 +290,11 @@ export function InputBox({ const suggestionsEnabled = suggestionsConfig?.enabled; const [followupsHidden, setFollowupsHidden] = useState(false); const [followupsLoading, setFollowupsLoading] = useState(false); + const [polishingInput, setPolishingInput] = useState(false); + const [inputPolishUndo, setInputPolishUndo] = useState<{ + originalText: string; + rewrittenText: string; + } | null>(null); const [textareaFocused, setTextareaFocused] = useState(false); const [skillSuggestionIndex, setSkillSuggestionIndex] = useState(0); const [dismissedSkillSuggestionValue, setDismissedSkillSuggestionValue] = @@ -434,6 +451,7 @@ export function InputBox({ useEffect(() => { promptHistoryIndexRef.current = null; promptHistoryDraftRef.current = ""; + setInputPolishUndo(null); }, [threadId]); useEffect(() => { @@ -445,6 +463,17 @@ export function InputBox({ }; }, [threadId]); + const abortInputPolishRequest = useCallback(() => { + inputPolishRequestRef.current.controller?.abort(); + inputPolishRequestRef.current.controller = null; + inputPolishRequestRef.current.sequence += 1; + setPolishingInput(false); + }, []); + + useEffect(() => { + return () => abortInputPolishRequest(); + }, [abortInputPolishRequest, threadId]); + useEffect(() => { const currentIndex = promptHistoryIndexRef.current; if (currentIndex !== null && currentIndex >= promptHistory.length) { @@ -455,6 +484,9 @@ export function InputBox({ const handleModelSelect = useCallback( (model_name: string) => { + if (disabled || polishingInput) { + return; + } const model = models.find((m) => m.name === model_name); if (!model) { return; @@ -467,11 +499,14 @@ export function InputBox({ }); setModelDialogOpen(false); }, - [onContextChange, context, models], + [disabled, onContextChange, context, models, polishingInput], ); const handleModeSelect = useCallback( (mode: InputMode) => { + if (disabled || polishingInput) { + return; + } onContextChange?.({ ...context, mode: getResolvedMode(mode, supportThinking), @@ -485,17 +520,20 @@ export function InputBox({ : "minimal", }); }, - [onContextChange, context, supportThinking], + [disabled, onContextChange, context, polishingInput, supportThinking], ); const handleReasoningEffortSelect = useCallback( (effort: "minimal" | "low" | "medium" | "high") => { + if (disabled || polishingInput) { + return; + } onContextChange?.({ ...context, reasoning_effort: effort, }); }, - [onContextChange, context], + [disabled, onContextChange, context, polishingInput], ); const handleGoalCommand = useCallback( @@ -702,6 +740,7 @@ export function InputBox({ } promptHistoryIndexRef.current = null; promptHistoryDraftRef.current = ""; + setInputPolishUndo(null); setFollowups([]); setFollowupsHidden(false); setFollowupsLoading(false); @@ -881,6 +920,30 @@ export function InputBox({ slashSkillQuery !== null && skillSuggestions.length > 0 && dismissedSkillSuggestionValue !== textInput.value; + const isComposerDisabled = disabled === true; + const isMockThread = isMock === true; + const hasOpenHumanInputCard = useMemo( + () => + hasOpenHumanInputRequest( + thread.messages, + (message) => !isHiddenFromUIMessage(message), + ), + [thread.messages], + ); + const composerLocked = isComposerDisabled || polishingInput; + const inputPolishUndoAvailable = + !polishingInput && + inputPolishUndo !== null && + (textInput.value ?? "") === inputPolishUndo.rewrittenText; + const inputPolishDisabled = + isComposerDisabled || + isMockThread || + hasOpenHumanInputCard || + polishingInput || + (!inputPolishUndoAvailable && + (status === "streaming" || + slashSkillQuery !== null || + !canPolishInput(textInput.value ?? ""))); useEffect(() => { setSkillSuggestionIndex(0); @@ -967,6 +1030,94 @@ export function InputBox({ [textInput], ); + const handlePolishInput = useCallback(async () => { + if (inputPolishDisabled) { + return; + } + + const originalText = textInput.value ?? ""; + const controller = new AbortController(); + inputPolishRequestRef.current.controller?.abort(); + const sequence = inputPolishRequestRef.current.sequence + 1; + inputPolishRequestRef.current = { + controller, + sequence, + }; + setPolishingInput(true); + + try { + const result = await polishInputDraft( + { + text: originalText, + locale, + thread_id: threadId, + }, + { signal: controller.signal }, + ); + + const isCurrentRequest = + inputPolishRequestRef.current.controller === controller && + inputPolishRequestRef.current.sequence === sequence && + !controller.signal.aborted; + if (!isCurrentRequest || (textInput.value ?? "") !== originalText) { + return; + } + + const rewrittenText = result.rewritten_text.trim(); + if (!rewrittenText || !result.changed) { + toast.info(t.inputBox.inputPolishNoChanges); + return; + } + + // Applying the rewrite replaces the draft outside the textarea change + // handler, so clear any in-progress history browse state; otherwise a + // stale index would let the next ArrowDown overwrite the rewrite. + promptHistoryIndexRef.current = null; + promptHistoryDraftRef.current = ""; + setPromptHistoryValue(rewrittenText); + setInputPolishUndo({ + originalText, + rewrittenText, + }); + } catch (error) { + const isCurrentRequest = + inputPolishRequestRef.current.controller === controller && + inputPolishRequestRef.current.sequence === sequence; + if (isAbortError(error) || !isCurrentRequest) { + return; + } + toast.error( + error instanceof Error ? error.message : t.inputBox.inputPolishFailed, + ); + } finally { + if ( + inputPolishRequestRef.current.controller === controller && + inputPolishRequestRef.current.sequence === sequence + ) { + inputPolishRequestRef.current.controller = null; + setPolishingInput(false); + } + } + }, [ + inputPolishDisabled, + locale, + setPromptHistoryValue, + t.inputBox.inputPolishFailed, + t.inputBox.inputPolishNoChanges, + textInput, + threadId, + ]); + + const handleUndoInputPolish = useCallback(() => { + if (!inputPolishUndoAvailable || inputPolishUndo === null) { + return; + } + promptHistoryIndexRef.current = null; + promptHistoryDraftRef.current = ""; + setPromptHistoryValue(inputPolishUndo.originalText); + setInputPolishUndo(null); + }, [inputPolishUndo, inputPolishUndoAvailable, setPromptHistoryValue]); + const handlePromptHistoryKeyDown = useCallback( (event: KeyboardEvent) => { if ( @@ -1033,9 +1184,11 @@ export function InputBox({ ); const handlePromptTextareaChange = useCallback(() => { + abortInputPolishRequest(); + setInputPolishUndo(null); promptHistoryIndexRef.current = null; promptHistoryDraftRef.current = ""; - }, []); + }, [abortInputPolishRequest]); const showFollowups = !disabled && @@ -1247,14 +1400,22 @@ export function InputBox({ + {polishingInput && ( +