fix(distribution): path-aware allowlist; preserve legacy copy-everything when omitted
Follow-ups to the previous commit (#74414 by @webtecnica, re #74373): - When distribution_owned is OMITTED, restore the legacy contract: every staged entry outside USER_OWNED_EXCLUDE is copied. The cherry-picked filter consulted owned_paths(), which silently narrowed omitted-list distributions to DEFAULT_DIST_OWNED and dropped undeclared payload (extra top-level files/dirs existing distributions legitimately ship). - Make explicit allowlists path-aware so documented nested entries like skills/research/ and cron/digest.json select exactly that subtree/file instead of being dropped by the top-level name comparison. Traversal segments (.., absolute) and USER_OWNED_EXCLUDE roots are still rejected. - Regression tests: omitted-list legacy behavior + nested-path allowlist.
This commit is contained in:
parent
a42e3e8ba4
commit
ee5a66ae3e
|
|
@ -67,7 +67,7 @@ import subprocess
|
|||
import tempfile
|
||||
from dataclasses import dataclass, field
|
||||
from datetime import datetime, timezone
|
||||
from pathlib import Path
|
||||
from pathlib import Path, PurePosixPath
|
||||
from typing import Any, Dict, List, Optional, Tuple
|
||||
|
||||
from agent.skill_utils import is_excluded_skill_path
|
||||
|
|
@ -557,26 +557,16 @@ def _copy_dist_payload(
|
|||
``preserve_config`` is False (fresh install or ``--force-config`` update).
|
||||
``.env.template`` is renamed to ``.env.EXAMPLE`` in the target to avoid
|
||||
shadowing a real ``.env``.
|
||||
|
||||
When the manifest declares an explicit ``distribution_owned`` list, only
|
||||
those paths are copied (path-aware: nested entries such as
|
||||
``skills/research`` or ``cron/digest.json`` are honoured). When the list
|
||||
is omitted the legacy behaviour is preserved: every staged entry outside
|
||||
``USER_OWNED_EXCLUDE`` is copied.
|
||||
"""
|
||||
target.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
owned = set(manifest.owned_paths())
|
||||
|
||||
for entry in staged.iterdir():
|
||||
name = entry.name
|
||||
|
||||
if name in USER_OWNED_EXCLUDE:
|
||||
continue
|
||||
if name not in owned:
|
||||
continue
|
||||
if name == ENV_TEMPLATE_FILENAME:
|
||||
shutil.copy2(entry, target / ENV_EXAMPLE_FILENAME)
|
||||
continue
|
||||
if name == "config.yaml" and preserve_config and (target / "config.yaml").exists():
|
||||
# Leave user's config.yaml alone on update
|
||||
continue
|
||||
|
||||
dest = target / name
|
||||
def _copy_entry(entry: Path, dest: Path) -> None:
|
||||
if entry.is_dir():
|
||||
if dest.exists():
|
||||
shutil.rmtree(dest)
|
||||
|
|
@ -593,6 +583,50 @@ def _copy_dist_payload(
|
|||
else:
|
||||
shutil.copy2(entry, dest)
|
||||
|
||||
explicit_owned = [p.strip().strip("/") for p in manifest.distribution_owned]
|
||||
explicit_owned = [p for p in explicit_owned if p]
|
||||
|
||||
if explicit_owned:
|
||||
# Path-aware allowlist: copy exactly the declared paths.
|
||||
for rel in explicit_owned:
|
||||
rel_parts = PurePosixPath(rel).parts
|
||||
if not rel_parts or rel_parts[0] in USER_OWNED_EXCLUDE:
|
||||
continue
|
||||
if ".." in rel_parts or PurePosixPath(rel).is_absolute():
|
||||
continue
|
||||
src = staged.joinpath(*rel_parts)
|
||||
if not src.exists():
|
||||
continue
|
||||
if len(rel_parts) == 1:
|
||||
name = rel_parts[0]
|
||||
if name == ENV_TEMPLATE_FILENAME:
|
||||
shutil.copy2(src, target / ENV_EXAMPLE_FILENAME)
|
||||
continue
|
||||
if name == "config.yaml" and preserve_config and (target / "config.yaml").exists():
|
||||
# Leave user's config.yaml alone on update
|
||||
continue
|
||||
dest = target.joinpath(*rel_parts)
|
||||
dest.parent.mkdir(parents=True, exist_ok=True)
|
||||
_copy_entry(src, dest)
|
||||
else:
|
||||
# Legacy behaviour: no explicit allowlist means the whole staged
|
||||
# payload (minus USER_OWNED_EXCLUDE) is distribution-owned. Do NOT
|
||||
# narrow to DEFAULT_DIST_OWNED here — existing distributions ship
|
||||
# arbitrary extra top-level paths without declaring them.
|
||||
for entry in staged.iterdir():
|
||||
name = entry.name
|
||||
|
||||
if name in USER_OWNED_EXCLUDE:
|
||||
continue
|
||||
if name == ENV_TEMPLATE_FILENAME:
|
||||
shutil.copy2(entry, target / ENV_EXAMPLE_FILENAME)
|
||||
continue
|
||||
if name == "config.yaml" and preserve_config and (target / "config.yaml").exists():
|
||||
# Leave user's config.yaml alone on update
|
||||
continue
|
||||
|
||||
_copy_entry(entry, target / name)
|
||||
|
||||
# Emit .env.EXAMPLE from manifest if the staged tree didn't ship one
|
||||
if manifest.env_requires and not (target / ENV_EXAMPLE_FILENAME).exists():
|
||||
(target / ENV_EXAMPLE_FILENAME).write_text(
|
||||
|
|
|
|||
|
|
@ -267,6 +267,49 @@ class TestInstall:
|
|||
assert full.exists() or full.is_dir(), \
|
||||
f"DEFAULT_DIST_OWNED '{path}' not found in target"
|
||||
|
||||
def test_install_omitted_allowlist_copies_everything(self, profile_env):
|
||||
"""Legacy contract: when distribution_owned is OMITTED, every staged
|
||||
entry outside USER_OWNED_EXCLUDE is copied — the omitted list must NOT
|
||||
silently narrow to DEFAULT_DIST_OWNED."""
|
||||
staged = _make_staging_dir(profile_env, "legacy_all")
|
||||
# Extra top-level payload not covered by DEFAULT_DIST_OWNED
|
||||
(staged / "extra.txt").write_text("bonus\n")
|
||||
(staged / "tools").mkdir()
|
||||
(staged / "tools" / "helper.py").write_text("# helper\n")
|
||||
|
||||
plan = install_distribution(str(staged), name="legacy_all")
|
||||
assert (plan.target_dir / "extra.txt").read_text() == "bonus\n", \
|
||||
"omitted distribution_owned must keep copying undeclared files"
|
||||
assert (plan.target_dir / "tools" / "helper.py").exists(), \
|
||||
"omitted distribution_owned must keep copying undeclared dirs"
|
||||
|
||||
def test_install_allowlist_supports_nested_paths(self, profile_env):
|
||||
"""Documented nested entries like skills/research/ and cron/digest.json
|
||||
must select exactly that subtree/file, not be silently dropped."""
|
||||
mf = DistributionManifest(
|
||||
name="nested",
|
||||
version="0.1.0",
|
||||
distribution_owned=["SOUL.md", "skills/research/", "cron/digest.json"],
|
||||
)
|
||||
staged = _make_staging_dir(profile_env, "nested", manifest=mf)
|
||||
(staged / "skills" / "research").mkdir()
|
||||
(staged / "skills" / "research" / "SKILL.md").write_text(
|
||||
"---\nname: research\ndescription: r\n---\n# R\n"
|
||||
)
|
||||
(staged / "cron" / "digest.json").write_text('{"schedule": "0 8 * * *"}')
|
||||
|
||||
plan = install_distribution(str(staged), name="nested")
|
||||
# Nested allowlisted paths are installed
|
||||
assert (plan.target_dir / "skills" / "research" / "SKILL.md").exists()
|
||||
assert (plan.target_dir / "cron" / "digest.json").exists()
|
||||
# Sibling paths under the same parents are NOT dragged along
|
||||
assert not (plan.target_dir / "skills" / "demo").exists(), \
|
||||
"skills/demo is not allowlisted and must not be copied"
|
||||
assert not (plan.target_dir / "cron" / "daily.json").exists(), \
|
||||
"cron/daily.json is not allowlisted and must not be copied"
|
||||
# Unrelated top-level entries stay out too
|
||||
assert not (plan.target_dir / "mcp.json").exists()
|
||||
|
||||
def test_update_respects_distribution_owned_allowlist(self, profile_env):
|
||||
"""Update must only copy paths listed in distribution_owned."""
|
||||
# 1. Install with full default distribution_owned
|
||||
|
|
|
|||
Loading…
Reference in New Issue