fix(skills): copy projected skill files instead of hardlinking (#4825)

This commit is contained in:
Nefelibata 2026-08-17 08:17:13 +08:00 committed by GitHub
parent 062ba9ddfc
commit 9668b35b1a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 91 additions and 29 deletions

View File

@ -5,7 +5,7 @@
- **Loading**: `load_skills()` recursively scans public, per-user custom, global integration, and legacy custom locations for `SKILL.md`, parses metadata, and reads enabled state from extensions_config.json plus per-user skill state for non-public categories; that directory is a package boundary, so no nested `SKILL.md` is registered as a runtime skill. SkillScan has a deliberately narrower packaging rule: known eval fixtures are permitted as support data, while other nested `SKILL.md` files are reported as package defects. It parses runtime metadata and reads enabled state from extensions_config.json.
- **External reload**: `POST /api/skills/reload` is an admin-only, process-local invalidation hook for trusted MinIO/NFS/CSI writes. `SkillStorage` instances do not cache a catalog — `load_skills()` scans on every call — so the route clears all `(app_config, user_id)` entries and the rendered prompt-section LRU, then waits up to the shared refresh timeout for the existing off-loop single-flight refresh. Each invalidation receives a generation-bound result handle; a successful scan atomically replaces the global enabled-skills cache, while a loader-level failure propagates to the HTTP waiter and preserves the last-known-good global cache. Per-user/config scans capture the refresh version and cannot repopulate shared caches if invalidation occurs while they are loading. A timed-out HTTP wait fails generically while the daemon refresh worker continues. Subsequent runs rescan after a successful reload; active runs keep their existing snapshot. Each Uvicorn worker/Kubernetes Pod must be targeted separately. Direct mount writes bypass install/edit validation, SkillScan, and history, so mounted roots are an operator-controlled trust boundary.
- **Tool policy**: Agent `allowed-tools` declarations apply dynamically only to slash-activated skills and skills captured in `ThreadState.skill_context` through configured `read_file` loads; passive enabled skills and custom-agent/subagent skill allowlists remain discoverable without clamping the baseline toolset. Subagents render only skill discovery metadata at startup and reuse the same adjacent `SkillActivationMiddleware` + `SkillToolPolicyMiddleware` pair as the lead; their configured `skills` field limits discovery and activation instead of eagerly loading bodies or unioning policies. Slash policy is dominant for its run, preventing subsequently read skills from widening explicit authority; autonomous captured skills use the existing union only when no slash source exists. `tool_search` and `describe_skill` stay available as framework discovery infrastructure, while every discovered or promoted business tool still requires active-policy permission for schema visibility and execution; `task` likewise requires an explicit declaration. Each active model call intentionally reloads the full live registry so enable/disable changes, frontmatter edits, and custom/public name-shadow winners take effect without a stale TTL or unsafe direct-path cache; all tool calls produced by that model step reuse the resulting source-and-path-signed decision. Registry failures and all-invalid active sets fail closed, while stale individual paths are skipped when another valid skill remains. This is best-effort behavioral scoping, not a hard security boundary: alternate loading paths are not captured and bounded autonomous context may evict entries.
- **Sandbox projection**: `skills/projection.py` materializes enabled-only trees at `{base_dir}/skills_view/public` and `{base_dir}/users/{user_id}/skills_view/{custom,legacy,integrations}`. It hardlinks files when possible and falls back to copies across filesystems. Storage writes, archive installs, deletes, and toggles rebuild under a cross-process lock; Gateway boot ensures only the shared public view, while each user view is repaired lazily on first sandbox acquire. Managed integration packages are global, but their projected category is per-user because enabled state is isolated. Rebuilds stage a complete tree and reconcile it with per-file atomic replacement, so unrelated enabled skills remain continuously visible; disable/delete paths remove only the affected package before mutating to preserve fail-closed behavior. User projection rebuilds re-read global enable state from disk instead of the process singleton, so a toggle handled by another Gateway worker is reflected on the next acquire. Gateway public-skill toggles take the public projection lock before the shared `extensions_config_write_lock`, re-read an existing config from disk, persist the full model shape, and rebuild before responding; keep this as one worker-owned critical section so MCP writes cannot interleave and request cancellation cannot release either lock while the worker still runs. The shared public steady-state signature check runs without the global projection lock; stale/error paths take the lock and re-check before rebuilding or clearing. User-scope checks remain serialized per user. Category root inodes remain stable so live bind mounts observe content changes without sandbox recreation. Projection failures clear the affected view before raising.
- **Sandbox projection**: `skills/projection.py` materializes enabled-only trees at `{base_dir}/skills_view/public` and `{base_dir}/users/{user_id}/skills_view/{custom,legacy,integrations}`. It copies files into the view (`_copy_into_view`) so a sandbox write cannot mutate the canonical skill inode; the operational trade-off is an O(total bytes) I/O and per-user storage multiplier across rebuilds, prioritized for write isolation over zero-copy hardlinks. Steady-state freshness checks combine source and view metadata tree digests, so in-sandbox view tampering is detected and repaired on the next acquire. Storage writes, archive installs, deletes, and toggles rebuild under a cross-process lock; Gateway boot ensures only the shared public view, while each user view is repaired lazily on first sandbox acquire. Managed integration packages are global, but their projected category is per-user because enabled state is isolated. Rebuilds stage a complete tree and reconcile it with per-file atomic replacement, so unrelated enabled skills remain continuously visible; disable/delete paths remove only the affected package before mutating to preserve fail-closed behavior. User projection rebuilds re-read global enable state from disk instead of the process singleton, so a toggle handled by another Gateway worker is reflected on the next acquire. Gateway public-skill toggles take the public projection lock before the shared `extensions_config_write_lock`, re-read an existing config from disk, persist the full model shape, and rebuild before responding; keep this as one worker-owned critical section so MCP writes cannot interleave and request cancellation cannot release either lock while the worker still runs. The shared public steady-state signature check runs without the global projection lock; stale/error paths take the lock and re-check before rebuilding or clearing. User-scope checks remain serialized per user. Category root inodes remain stable so live bind mounts observe content changes without sandbox recreation. Projection failures clear the affected view before raising.
- **Injection (legacy / default)**: Enabled skills are listed in the agent system prompt with full metadata and container paths (`<available_skills>` block). Controlled by `skills.deferred_discovery: false` (default).
- **Deferred discovery** (`skills.deferred_discovery: true`): Skills are listed by name only in a compact `<skill_index>` block, keeping the system prompt prefix-cache friendly. The agent calls the `describe_skill` tool at runtime to fetch full metadata for skills it wants to use, then loads the SKILL.md via `read_file`. Two new modules support this path:
- `skills/catalog.py``SkillCatalog` (immutable, searchable; query forms: `select:a,b`, `+prefix`, free-text regex); `select:` returns all requested skills without a result cap; other modes cap at `MAX_RESULTS=5`.

View File

@ -2,7 +2,6 @@
from __future__ import annotations
import errno
import hashlib
import json
import logging
@ -94,15 +93,12 @@ def _projection_lock(root: Path) -> Iterator[None]:
msvcrt.locking(lock_file.fileno(), msvcrt.LK_UNLCK, 1)
def _link_or_copy(source: str, target: str, *, follow_symlinks: bool = True) -> str:
# Hardlinks share the source inode and provide no write isolation. Any
# read-only guarantee must come from the consuming sandbox or mount.
try:
os.link(source, target, follow_symlinks=follow_symlinks)
except OSError as exc:
if exc.errno not in {errno.EXDEV, errno.EPERM, errno.EACCES, errno.ENOTSUP}:
raise
shutil.copy2(source, target, follow_symlinks=follow_symlinks)
def _copy_into_view(source: str, target: str, *, follow_symlinks: bool = True) -> str:
# Always copy. Hardlinks share the source inode, so a LocalSandbox bash
# write through the projected view would mutate the canonical skill file.
# Isolation must live in the projection itself; PathMapping.read_only is
# only enforced by write_file / update_file, not execute_command.
shutil.copy2(source, target, follow_symlinks=follow_symlinks)
return target
@ -114,7 +110,7 @@ def _stage_skill(source: Path, target: Path, nested_skill_roots: set[Path]) -> N
shutil.copytree(
source,
target,
copy_function=_link_or_copy,
copy_function=_copy_into_view,
symlinks=True,
ignore=_exclude_nested_skills,
dirs_exist_ok=True,
@ -298,20 +294,39 @@ def _read_manifest(scope_root: Path) -> dict | None:
return value if isinstance(value, dict) else None
def _write_manifest(scope_root: Path, source_signature: str) -> None:
def _write_manifest(scope_root: Path, source_signature: str, view_signature: str | None = None) -> None:
scope_root.mkdir(parents=True, exist_ok=True)
target = _manifest_path(scope_root)
fd, temporary_name = tempfile.mkstemp(prefix=".projection-manifest-", suffix=".tmp", dir=scope_root)
temporary = Path(temporary_name)
payload = {
"version": _MANIFEST_VERSION,
"source_signature": source_signature,
}
if view_signature is not None:
payload["view_signature"] = view_signature
try:
with os.fdopen(fd, "w", encoding="utf-8") as stream:
json.dump({"version": _MANIFEST_VERSION, "source_signature": source_signature}, stream, sort_keys=True)
json.dump(payload, stream, sort_keys=True)
temporary.replace(target)
except Exception:
temporary.unlink(missing_ok=True)
raise
def _view_signature(paths: SkillProjectionPaths, scope: str) -> str:
digest = hashlib.sha256()
if scope == "public":
_update_tree_digest(digest, paths.public, "public_view")
elif scope == "user":
_update_tree_digest(digest, paths.custom, "custom_view")
_update_tree_digest(digest, paths.legacy, "legacy_view")
_update_tree_digest(digest, paths.integrations, "integrations_view")
else: # pragma: no cover - internal invariant
raise ValueError(f"Unknown skill projection scope: {scope}")
return digest.hexdigest()
def _load_public_skills(storage: SkillStorage, *, enabled_only: bool) -> list[Skill]:
from deerflow.config.extensions_config import ExtensionsConfig
@ -363,7 +378,7 @@ def _rebuild_public_locked(storage: SkillStorage, paths: SkillProjectionPaths) -
)
after = _source_signature(storage, "public")
if before == after:
_write_manifest(scope_root, after)
_write_manifest(scope_root, after, _view_signature(paths, "public"))
return
raise RuntimeError("Public skills changed repeatedly while rebuilding the sandbox projection")
except Exception:
@ -395,7 +410,7 @@ def _rebuild_user_locked(storage: SkillStorage, paths: SkillProjectionPaths) ->
)
after = _source_signature(storage, "user")
if before == after:
_write_manifest(scope_root, after)
_write_manifest(scope_root, after, _view_signature(paths, "user"))
return
raise RuntimeError("User skills changed repeatedly while rebuilding the sandbox projection")
except Exception:
@ -428,9 +443,10 @@ def _public_projection_is_fresh(storage: SkillStorage, paths: SkillProjectionPat
manifest_before = _read_manifest(paths.public.parent)
if manifest_before is None or manifest_before.get("version") != _MANIFEST_VERSION:
return False
signature = _source_signature(storage, "public")
source_sig = _source_signature(storage, "public")
view_sig = _view_signature(paths, "public")
manifest_after = _read_manifest(paths.public.parent)
return manifest_before == manifest_after and manifest_before.get("source_signature") == signature
return manifest_before == manifest_after and manifest_before.get("source_signature") == source_sig and manifest_before.get("view_signature") == view_sig
def ensure_skill_projections(storage: SkillStorage) -> SkillProjectionPaths:
@ -456,8 +472,17 @@ def ensure_skill_projections(storage: SkillStorage) -> SkillProjectionPaths:
with _projection_lock(paths.custom.parent):
try:
manifest = _read_manifest(paths.custom.parent)
signature = _source_signature(storage, "user")
if not paths.custom.is_dir() or not paths.legacy.is_dir() or not paths.integrations.is_dir() or manifest is None or manifest.get("version") != _MANIFEST_VERSION or manifest.get("source_signature") != signature:
source_sig = _source_signature(storage, "user")
view_sig = _view_signature(paths, "user")
if (
not paths.custom.is_dir()
or not paths.legacy.is_dir()
or not paths.integrations.is_dir()
or manifest is None
or manifest.get("version") != _MANIFEST_VERSION
or manifest.get("source_signature") != source_sig
or manifest.get("view_signature") != view_sig
):
_rebuild_user_locked(storage, paths)
except Exception:
_clear_projection_scope(paths.custom.parent, paths.custom, paths.legacy, paths.integrations)

View File

@ -153,6 +153,27 @@ class TestReadOnlyPath:
sandbox.write_file("/mnt/skills/new_file.py", "content")
assert exc_info.value.errno == errno.EROFS
def test_bash_write_to_projected_copy_does_not_mutate_source(self, tmp_path):
source = tmp_path / "canonical" / "SKILL.md"
view = tmp_path / "skills_view" / "public" / "demo" / "SKILL.md"
source.parent.mkdir(parents=True)
view.parent.mkdir(parents=True)
source.write_text("ORIGINAL\n", encoding="utf-8")
from deerflow.skills.projection import _copy_into_view
_copy_into_view(str(source), str(view))
assert view.stat().st_ino != source.stat().st_ino
sandbox = LocalSandbox(
"test",
[
PathMapping(container_path="/mnt/skills/public/demo", local_path=str(view.parent), read_only=True),
],
)
sandbox.execute_command("python -c \"from pathlib import Path; Path(r'/mnt/skills/public/demo/SKILL.md').write_text('MUTATED\\n', encoding='utf-8')\"")
assert source.read_text(encoding="utf-8") == "ORIGINAL\n"
assert view.read_text(encoding="utf-8") == "MUTATED\n"
def test_write_file_allowed_on_writable_mount(self, tmp_path):
data_dir = tmp_path / "data"
data_dir.mkdir()

View File

@ -2,7 +2,6 @@
from __future__ import annotations
import errno
import shutil
import zipfile
from concurrent.futures import ThreadPoolExecutor
@ -100,20 +99,37 @@ def test_nested_skill_frontmatter_is_supporting_data_inside_parent_package(proje
assert (nested_view / "SKILL.md").is_file()
def test_projection_falls_back_to_copy_when_hardlink_is_unavailable(projection_env, monkeypatch) -> None:
def test_projection_copies_instead_of_hardlinking_source_files(projection_env) -> None:
env = projection_env
source = _write_skill(env.skills_root / "public", "demo-skill")
def _cross_device_link(*_args, **_kwargs):
raise OSError(errno.EXDEV, "cross-device link")
monkeypatch.setattr("deerflow.skills.projection.os.link", _cross_device_link)
projected = rebuild_skill_projections(env.storage)
target = projected.public / "demo-skill" / "SKILL.md"
assert target.read_text(encoding="utf-8") == source.read_text(encoding="utf-8")
assert target.stat().st_ino != source.stat().st_ino
target.write_text("MUTATED\n", encoding="utf-8")
assert source.read_text(encoding="utf-8") != "MUTATED\n"
def test_view_tampering_triggers_automatic_repair_on_ensure(projection_env) -> None:
env = projection_env
source = _write_skill(env.skills_root / "public", "demo-skill")
projected = rebuild_skill_projections(env.storage)
target = projected.public / "demo-skill" / "SKILL.md"
target.write_text("MUTATED\n", encoding="utf-8")
ensure_skill_projections(env.storage)
assert target.read_text(encoding="utf-8") == source.read_text(encoding="utf-8")
env.storage.write_custom_skill("demo-user", "SKILL.md", _skill_content("demo-user", "original"))
projected = rebuild_skill_projections(env.storage)
user_target = projected.custom / "demo-user" / "SKILL.md"
user_target.write_text("MUTATED_USER\n", encoding="utf-8")
ensure_skill_projections(env.storage)
assert user_target.read_text(encoding="utf-8") == _skill_content("demo-user", "original")
def test_atomic_custom_skill_rewrite_refreshes_projection(projection_env) -> None:
env = projection_env
@ -551,10 +567,10 @@ def test_boot_factory_failure_cleanup_waits_for_concurrent_public_rebuild(projec
cleanup_finished = Event()
real_write_manifest = projection_module._write_manifest
def _delayed_write_manifest(scope_root, signature):
def _delayed_write_manifest(*args, **kwargs):
before_manifest.set()
assert release_rebuild.wait(timeout=5)
real_write_manifest(scope_root, signature)
real_write_manifest(*args, **kwargs)
monkeypatch.setattr(projection_module, "_write_manifest", _delayed_write_manifest)
monkeypatch.setattr(