fix: normalize against tools.skills_tool.SKILLS_DIR, the root skill_view enforces

The extracted normalize_skill_lookup_name() resolved trusted roots via
agent.skill_utils.get_skills_dir(), but skill_view() enforces
tools.skills_tool.SKILLS_DIR — a separate module attribute that callers
and 60+ existing tests patch directly. With the helper reading a
different symbol than the enforcer, any SKILLS_DIR patch (or future
divergence between the two resolvers) makes normalization disagree with
enforcement and absolute-path loads regress silently. Read SKILLS_DIR at
call time (deferred import, cycle-safe) with get_skills_dir() as the
fallback, and align the new tests to patch the enforced symbol.

Follow-up to the salvage of #59829 by @HexLab98.
This commit is contained in:
kshitijk4poor 2026-07-07 11:29:19 +05:30 committed by kshitij
parent e7082ea99f
commit 713e50e7d2
3 changed files with 20 additions and 5 deletions

View file

@ -524,7 +524,19 @@ def normalize_skill_lookup_name(identifier: str) -> str:
if not identifier_path.is_absolute():
return raw_identifier.lstrip("/")
trusted_roots = [get_skills_dir()]
# Look the primary skills root up on tools.skills_tool at CALL time
# (not via get_skills_dir()): callers and tests patch
# ``tools.skills_tool.SKILLS_DIR`` and skill_view() itself resolves
# against that module attribute, so normalization must agree with the
# exact root skill_view() will enforce. Import deferred to avoid a
# module cycle (tools.skills_tool imports agent.skill_utils).
try:
from tools import skills_tool as _skills_tool
primary_root = Path(_skills_tool.SKILLS_DIR)
except Exception:
primary_root = get_skills_dir()
trusted_roots = [primary_root]
try:
trusted_roots.extend(get_external_skills_dirs())
except Exception:
@ -542,7 +554,7 @@ def normalize_skill_lookup_name(identifier: str) -> str:
continue
try:
return str(identifier_path.resolve().relative_to(get_skills_dir().resolve()))
return str(identifier_path.resolve().relative_to(primary_root.resolve()))
except Exception:
return raw_identifier

View file

@ -348,7 +348,9 @@ class TestNormalizeSkillLookupName:
skills_dir = tmp_path / "skills"
skill_dir = skills_dir / "category" / "my-skill"
skill_dir.mkdir(parents=True)
monkeypatch.setattr("agent.skill_utils.get_skills_dir", lambda: skills_dir)
# Patch the root skill_view() itself enforces — normalization reads
# tools.skills_tool.SKILLS_DIR at call time so the two stay in sync.
monkeypatch.setattr("tools.skills_tool.SKILLS_DIR", skills_dir)
assert normalize_skill_lookup_name(str(skill_dir)) == "category/my-skill"
def test_absolute_via_symlink_uses_lexical_relative_path(self, tmp_path, monkeypatch):
@ -363,12 +365,13 @@ class TestNormalizeSkillLookupName:
link.symlink_to(external)
except OSError:
pytest.skip("Symlinks not supported")
monkeypatch.setattr("agent.skill_utils.get_skills_dir", lambda: skills_dir)
monkeypatch.setattr("tools.skills_tool.SKILLS_DIR", skills_dir)
assert normalize_skill_lookup_name(str(link)) == "my-skill"
def test_untrusted_absolute_returned_unchanged(self, tmp_path, monkeypatch):
from agent.skill_utils import normalize_skill_lookup_name
monkeypatch.setattr("tools.skills_tool.SKILLS_DIR", tmp_path / "skills")
monkeypatch.setattr("agent.skill_utils.get_skills_dir", lambda: tmp_path / "skills")
outside = str(tmp_path / "outside" / "skill")
assert normalize_skill_lookup_name(outside) == outside

View file

@ -2902,7 +2902,7 @@ class TestBuildJobPromptAbsoluteSkillPath:
return json.dumps({"success": True, "content": "# Alpha\nDo alpha."})
return json.dumps({"success": False, "error": f"Skill '{name}' not found."})
with patch("agent.skill_utils.get_skills_dir", return_value=skills_dir), \
with patch("tools.skills_tool.SKILLS_DIR", skills_dir), \
patch("tools.skills_tool.skill_view", side_effect=_skill_view):
result = _build_job_prompt({"skills": [absolute_path], "prompt": "go"})