Merge pull request #1158 from Rhosmarie/feat/skill-space-black

feat(harness): support per-space skill policies

Author: Rhosmarie

tests/cloud/test_harness_app_contract.py

@@ -53,6 +53,7 @@
     HarnessResourceOverride,
     HarnessResponseMetrics,
     HarnessSelectedSkill,
+    HarnessSkillSpacePolicy,
     InvokeHarnessRequest,
     InvokeHarnessResponse,
     LlmUsageMetrics,
@@ -207,6 +208,50 @@ def test_structured_tools_skills_and_mcp_are_accepted(self):
         )
         assert h.mcp == [HarnessMcpServer(name="db", server_url="http://db.test/mcp")]
 
+    def test_selected_skill_accepts_skill_space_policy(self):
+        h = HarnessOverrides.model_validate(
+            {
+                "selected_skills": [
+                    {
+                        "source": "skillspace",
+                        "skill_space_id": "ss-a",
+                        "skill_space_policy": {
+                            "mode": "deny",
+                            "ids": ["s-blocked"],
+                        },
+                    }
+                ]
+            }
+        )
+
+        assert h.selected_skills == [
+            HarnessSelectedSkill(
+                source="skillspace",
+                skill_space_id="ss-a",
+                skill_space_policy=HarnessSkillSpacePolicy(
+                    mode="deny",
+                    ids=["s-blocked"],
+                ),
+            )
+        ]
+
+    def test_selected_skill_policy_requires_skillspace_source(self):
+        with pytest.raises(ValidationError, match="only supported for skillspace"):
+            HarnessOverrides.model_validate(
+                {
+                    "selected_skills": [
+                        {
+                            "source": "skillhub",
+                            "slug": "team/reporting",
+                            "skill_space_policy": {
+                                "mode": "allow",
+                                "ids": ["s-1"],
+                            },
+                        }
+                    ]
+                }
+            )
+
     def test_structured_registry_is_accepted_and_normalized(self):
         h = HarnessOverrides.model_validate(
             {
@@ -1236,7 +1281,9 @@ def __init__(self, names):
         def custom_tool():
             return "custom"
 
-        def fake_build_skill_toolset(skill_ids, download_dir=None):
+        def fake_build_skill_toolset(
+            skill_ids, download_dir=None, skill_space_policy=None
+        ):
             return FakeSkillToolset(skill_ids) if skill_ids else None
 
         custom_tool.__name__ = "custom_tool"
@@ -1260,6 +1307,95 @@ def fake_build_skill_toolset(skill_ids, download_dir=None):
         assert len(skill_toolsets) == 1
         assert skill_toolsets[0].names == ["team/new-skill"]
 
+    def test_spawn_passes_selected_skill_space_policy(self, monkeypatch):
+        from veadk.cloud.harness_app import utils
+
+        captured = {}
+
+        class FakeSkillToolset:
+            def __init__(self, names):
+                self.names = names
+
+        def fake_build_skill_toolset(
+            skill_ids, download_dir=None, skill_space_policy=None
+        ):
+            captured["skill_ids"] = list(skill_ids)
+            captured["policy"] = (
+                skill_space_policy.to_json() if skill_space_policy else None
+            )
+            return FakeSkillToolset(skill_ids) if skill_ids else None
+
+        monkeypatch.delenv("SKILL_SPACE_POLICY", raising=False)
+        monkeypatch.setattr(utils, "SkillToolset", FakeSkillToolset)
+        monkeypatch.setattr(utils, "build_skill_toolset", fake_build_skill_toolset)
+        base = Agent(model_name="base-model", model_api_key="test-key")
+
+        spawn_harness_agent(
+            base,
+            HarnessOverrides(
+                selected_skills=[
+                    {
+                        "source": "skillspace",
+                        "skill_space_id": "ss-a",
+                        "skill_space_policy": {
+                            "mode": "allow",
+                            "ids": ["s-a2", "s-a1", "s-a2"],
+                        },
+                    },
+                    {
+                        "source": "skillspace",
+                        "skill_space_id": "ss-b",
+                        "skill_space_policy": {
+                            "mode": "deny",
+                            "ids": ["s-b9"],
+                        },
+                    },
+                ]
+            ),
+        )
+
+        assert captured["skill_ids"] == ["space:ss-a", "space:ss-b"]
+        assert captured["policy"] == (
+            '{"spaces":{"ss-a":{"mode":"allow","ids":["s-a1","s-a2"]},'
+            '"ss-b":{"mode":"deny","ids":["s-b9"]}}}'
+        )
+        assert "SKILL_SPACE_POLICY" not in os.environ
+
+    def test_spawn_rejects_conflicting_selected_skill_space_policy(self, monkeypatch):
+        from veadk.cloud.harness_app import utils
+
+        monkeypatch.setattr(
+            utils, "build_skill_toolset", lambda *_args, **_kwargs: None
+        )
+        base = Agent(model_name="base-model", model_api_key="test-key")
+
+        with pytest.raises(ValueError, match="Conflicting skill_space_policy"):
+            spawn_harness_agent(
+                base,
+                HarnessOverrides(
+                    selected_skills=[
+                        {
+                            "source": "skillspace",
+                            "skill_space_id": "ss-a",
+                            "skill_id": "s-a1",
+                            "skill_space_policy": {
+                                "mode": "allow",
+                                "ids": ["s-a1"],
+                            },
+                        },
+                        {
+                            "source": "skillspace",
+                            "skill_space_id": "ss-a",
+                            "skill_id": "s-a2",
+                            "skill_space_policy": {
+                                "mode": "deny",
+                                "ids": ["s-a9"],
+                            },
+                        },
+                    ]
+                ),
+            )
+
     def test_spawn_clears_builtin_tools_and_skills_with_empty_overrides(
         self, monkeypatch
     ):

tests/cloud/test_harness_skill_download.py

@@ -31,6 +31,7 @@
 from google.adk.tools.skill_toolset import SkillToolset
 
 from veadk.cloud.harness_app import utils
+from veadk.skills.policy import parse_skill_space_policy
 from veadk.skills.skill import Skill as VeADKSkill
 
 
@@ -152,6 +153,159 @@ def fail_skillhub_download(*args, **kwargs):
     assert isinstance(toolset._code_executor, UnsafeLocalCodeExecutor)
 
 
+@pytest.mark.parametrize(
+    ("policy", "expected_names"),
+    [
+        ('{"mode":"allow","ids":["s-allow"]}', ["allowed-skill"]),
+        ('{"mode":"deny","ids":["s-deny"]}', ["allowed-skill"]),
+    ],
+)
+def test_build_skill_toolset_applies_skill_space_policy(
+    monkeypatch,
+    tmp_path,
+    policy,
+    expected_names,
+):
+    remote_skills = [
+        VeADKSkill(
+            name="allowed-skill",
+            description="Allowed skill.",
+            path="skills/s-allow/v1/allowed-skill.zip",
+            skill_space_id="ss-test",
+            bucket_name="bucket",
+            id="s-allow",
+        ),
+        VeADKSkill(
+            name="denied-skill",
+            description="Denied skill.",
+            path="skills/s-deny/v1/denied-skill.zip",
+            skill_space_id="ss-test",
+            bucket_name="bucket",
+            id="s-deny",
+        ),
+    ]
+    materialized: list[str] = []
+
+    monkeypatch.setenv("SKILL_SPACE_POLICY", policy)
+    monkeypatch.setattr(
+        utils,
+        "_load_skills_from_space_id",
+        lambda _skill_space_id: remote_skills,
+    )
+
+    def fake_materialize_remote_skill(
+        skill: VeADKSkill,
+        *,
+        cache_dir=None,
+    ):
+        materialized.append(skill.name)
+        skill_dir = tmp_path / skill.name
+        _write_adk_skill(skill_dir, name=skill.name)
+        return skill_dir
+
+    monkeypatch.setattr(
+        utils,
+        "materialize_remote_skill",
+        fake_materialize_remote_skill,
+    )
+
+    toolset = utils.build_skill_toolset(["space:ss-test"], download_dir=tmp_path)
+
+    assert materialized == expected_names
+    assert [skill.name for skill in toolset._list_skills()] == expected_names
+
+
+def test_build_skill_toolset_allows_empty_skill_space_policy(
+    monkeypatch,
+    tmp_path,
+):
+    remote_skills = [
+        VeADKSkill(
+            name="remote-skill",
+            description="Remote skill.",
+            path="skills/s-1/v1/remote-skill.zip",
+            skill_space_id="ss-test",
+            bucket_name="bucket",
+            id="s-1",
+        )
+    ]
+
+    monkeypatch.setenv("SKILL_SPACE_POLICY", '{"mode":"allow","ids":[]}')
+    monkeypatch.setattr(
+        utils,
+        "_load_skills_from_space_id",
+        lambda _skill_space_id: remote_skills,
+    )
+    monkeypatch.setattr(
+        utils,
+        "materialize_remote_skill",
+        lambda *_args, **_kwargs: pytest.fail(
+            "filtered skills must not be materialized"
+        ),
+    )
+
+    assert utils.build_skill_toolset(["space:ss-test"], download_dir=tmp_path) is None
+
+
+def test_build_skill_toolset_prefers_explicit_policy_over_env(
+    monkeypatch,
+    tmp_path,
+):
+    remote_skills = [
+        VeADKSkill(
+            name="explicit-skill",
+            description="Explicit skill.",
+            path="skills/s-explicit/v1/explicit-skill.zip",
+            skill_space_id="ss-test",
+            bucket_name="bucket",
+            id="s-explicit",
+        ),
+        VeADKSkill(
+            name="env-skill",
+            description="Env skill.",
+            path="skills/s-env/v1/env-skill.zip",
+            skill_space_id="ss-test",
+            bucket_name="bucket",
+            id="s-env",
+        ),
+    ]
+    materialized: list[str] = []
+
+    monkeypatch.setenv("SKILL_SPACE_POLICY", '{"mode":"allow","ids":["s-env"]}')
+    monkeypatch.setattr(
+        utils,
+        "_load_skills_from_space_id",
+        lambda _skill_space_id: remote_skills,
+    )
+
+    def fake_materialize_remote_skill(
+        skill: VeADKSkill,
+        *,
+        cache_dir=None,
+    ):
+        materialized.append(skill.name)
+        skill_dir = tmp_path / skill.name
+        _write_adk_skill(skill_dir, name=skill.name)
+        return skill_dir
+
+    monkeypatch.setattr(
+        utils,
+        "materialize_remote_skill",
+        fake_materialize_remote_skill,
+    )
+
+    toolset = utils.build_skill_toolset(
+        ["space:ss-test"],
+        download_dir=tmp_path,
+        skill_space_policy=parse_skill_space_policy(
+            '{"mode":"allow","ids":["s-explicit"]}'
+        ),
+    )
+
+    assert materialized == ["explicit-skill"]
+    assert [skill.name for skill in toolset._list_skills()] == ["explicit-skill"]
+
+
 def test_replace_skills_replaces_existing_skill_toolset(monkeypatch, tmp_path):
     existing_dir = tmp_path / "existing"
     incoming_dir = tmp_path / "incoming"
@@ -170,7 +324,7 @@ def test_replace_skills_replaces_existing_skill_toolset(monkeypatch, tmp_path):
     agent = SimpleNamespace(tools=[marker_tool, existing_toolset])
     calls: list[tuple[list[str], object]] = []
 
-    def fake_build_skill_toolset(skill_ids, download_dir=None):
+    def fake_build_skill_toolset(skill_ids, download_dir=None, skill_space_policy=None):
         calls.append((skill_ids, download_dir))
         return incoming_toolset
 

tests/frontend/server/skills/test_reviews.py

@@ -42,11 +42,16 @@
 def package(name: str, text: str) -> bytes:
     data = io.BytesIO()
     with zipfile.ZipFile(data, "w") as archive:
-        archive.writestr(
-            f"{name}/SKILL.md",
-            f"---\nname: {name}\ndescription: Review test\n---\n{text}",
-        )
-        archive.writestr(f"{name}/references/data.txt", text)
+        for path, content in [
+            (
+                f"{name}/SKILL.md",
+                f"---\nname: {name}\ndescription: Review test\n---\n{text}",
+            ),
+            (f"{name}/references/data.txt", text),
+        ]:
+            info = zipfile.ZipInfo(path)
+            info.date_time = (2025, 1, 1, 0, 0, 0)
+            archive.writestr(info, content)
     return data.getvalue()