fix(skills): stop marking persisted env vars missing on remote backends (#3650)
Salvage of PR #3452 (kentimsit). Fixes skill readiness checks on remote backends — persisted env vars are no longer incorrectly marked as missing. Co-Authored-By: kentimsit <kentimsit@users.noreply.github.com>
This commit is contained in:
@@ -63,6 +63,35 @@ class TestSkillViewRegistersPassthrough:
|
||||
assert result["success"] is True
|
||||
assert is_env_passthrough("TENOR_API_KEY")
|
||||
|
||||
def test_remote_backend_persisted_env_vars_registered(self, tmp_path, monkeypatch):
|
||||
"""Remote-backed skills still register locally available env vars."""
|
||||
monkeypatch.setenv("TERMINAL_ENV", "docker")
|
||||
_create_skill(
|
||||
tmp_path,
|
||||
"test-skill",
|
||||
frontmatter_extra=(
|
||||
"required_environment_variables:\n"
|
||||
" - name: TENOR_API_KEY\n"
|
||||
" prompt: Enter your Tenor API key\n"
|
||||
),
|
||||
)
|
||||
monkeypatch.setattr("tools.skills_tool.SKILLS_DIR", tmp_path)
|
||||
|
||||
from hermes_cli.config import save_env_value
|
||||
|
||||
save_env_value("TENOR_API_KEY", "persisted-value-123")
|
||||
monkeypatch.delenv("TENOR_API_KEY", raising=False)
|
||||
|
||||
with patch("tools.skills_tool._secret_capture_callback", None):
|
||||
from tools.skills_tool import skill_view
|
||||
|
||||
result = json.loads(skill_view(name="test-skill"))
|
||||
|
||||
assert result["success"] is True
|
||||
assert result["setup_needed"] is False
|
||||
assert result["missing_required_environment_variables"] == []
|
||||
assert is_env_passthrough("TENOR_API_KEY")
|
||||
|
||||
def test_missing_env_vars_not_registered(self, tmp_path, monkeypatch):
|
||||
"""When a skill declares required_environment_variables but the var is NOT set,
|
||||
it should NOT be registered in the passthrough."""
|
||||
|
||||
@@ -813,6 +813,29 @@ class TestSkillViewPrerequisites:
|
||||
assert result["setup_needed"] is False
|
||||
assert result["missing_required_environment_variables"] == []
|
||||
|
||||
def test_remote_backend_treats_persisted_env_as_available(
|
||||
self, tmp_path, monkeypatch
|
||||
):
|
||||
monkeypatch.setenv("TERMINAL_ENV", "docker")
|
||||
|
||||
with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
|
||||
_make_skill(
|
||||
tmp_path,
|
||||
"remote-ready",
|
||||
frontmatter_extra="prerequisites:\n env_vars: [PERSISTED_REMOTE_KEY]\n",
|
||||
)
|
||||
from hermes_cli.config import save_env_value
|
||||
|
||||
save_env_value("PERSISTED_REMOTE_KEY", "persisted-value")
|
||||
monkeypatch.delenv("PERSISTED_REMOTE_KEY", raising=False)
|
||||
raw = skill_view("remote-ready")
|
||||
|
||||
result = json.loads(raw)
|
||||
assert result["success"] is True
|
||||
assert result["setup_needed"] is False
|
||||
assert result["missing_required_environment_variables"] == []
|
||||
assert result["readiness_status"] == "available"
|
||||
|
||||
def test_no_setup_metadata_when_no_required_envs(self, tmp_path):
|
||||
with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
|
||||
_make_skill(tmp_path, "plain-skill")
|
||||
@@ -878,17 +901,11 @@ class TestSkillViewPrerequisites:
|
||||
assert result["setup_needed"] is True
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"backend,expected_note",
|
||||
[
|
||||
("ssh", "remote environment"),
|
||||
("daytona", "remote environment"),
|
||||
("docker", "docker-backed skills"),
|
||||
("singularity", "singularity-backed skills"),
|
||||
("modal", "modal-backed skills"),
|
||||
],
|
||||
"backend",
|
||||
["ssh", "daytona", "docker", "singularity", "modal"],
|
||||
)
|
||||
def test_remote_backend_keeps_setup_needed_after_local_secret_capture(
|
||||
self, tmp_path, monkeypatch, backend, expected_note
|
||||
def test_remote_backend_becomes_available_after_local_secret_capture(
|
||||
self, tmp_path, monkeypatch, backend
|
||||
):
|
||||
monkeypatch.setenv("TERMINAL_ENV", backend)
|
||||
monkeypatch.delenv("TENOR_API_KEY", raising=False)
|
||||
@@ -926,10 +943,10 @@ class TestSkillViewPrerequisites:
|
||||
result = json.loads(raw)
|
||||
assert result["success"] is True
|
||||
assert len(calls) == 1
|
||||
assert result["setup_needed"] is True
|
||||
assert result["readiness_status"] == "setup_needed"
|
||||
assert result["missing_required_environment_variables"] == ["TENOR_API_KEY"]
|
||||
assert expected_note in result["setup_note"].lower()
|
||||
assert result["setup_needed"] is False
|
||||
assert result["readiness_status"] == "available"
|
||||
assert result["missing_required_environment_variables"] == []
|
||||
assert "setup_note" not in result
|
||||
|
||||
def test_skill_view_surfaces_skill_read_errors(self, tmp_path, monkeypatch):
|
||||
with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
|
||||
|
||||
@@ -355,13 +355,8 @@ def _remaining_required_environment_names(
|
||||
capture_result: Dict[str, Any],
|
||||
*,
|
||||
env_snapshot: Dict[str, str] | None = None,
|
||||
backend: str | None = None,
|
||||
) -> List[str]:
|
||||
if backend is None:
|
||||
backend = _get_terminal_backend_name()
|
||||
missing_names = set(capture_result["missing_names"])
|
||||
if backend in _REMOTE_ENV_BACKENDS:
|
||||
return [entry["name"] for entry in required_env_vars]
|
||||
|
||||
if env_snapshot is None:
|
||||
env_snapshot = load_env()
|
||||
@@ -1076,8 +1071,7 @@ def skill_view(name: str, file_path: str = None, task_id: str = None) -> str:
|
||||
missing_required_env_vars = [
|
||||
e
|
||||
for e in required_env_vars
|
||||
if backend in _REMOTE_ENV_BACKENDS
|
||||
or not _is_env_var_persisted(e["name"], env_snapshot)
|
||||
if not _is_env_var_persisted(e["name"], env_snapshot)
|
||||
]
|
||||
capture_result = _capture_required_environment_variables(
|
||||
skill_name,
|
||||
@@ -1089,7 +1083,6 @@ def skill_view(name: str, file_path: str = None, task_id: str = None) -> str:
|
||||
required_env_vars,
|
||||
capture_result,
|
||||
env_snapshot=env_snapshot,
|
||||
backend=backend,
|
||||
)
|
||||
setup_needed = bool(remaining_missing_required_envs)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user