Skip to content

Commit 527b7d0

Browse files
author
Ovtcharov
committed
fix(skills): validate every clause of a compound version range
The parse-time gate probed a range with `matches("0.0.0", spec)`, and `matches` is `all()` over a generator — it short-circuits on the first clause the probe version fails. Any real lower bound fails against `0.0.0`, so in `">=1.2.0, 1.2.x"` the unreadable second clause was never parsed and the manifest was accepted. That reintroduced the machine-dependent verdict the gate exists to remove: the bad clause then surfaced at load only where the skill was installed at a version passing clause one, and elsewhere was misreported as an ordinary "does not satisfy the pin". `RemoteSkill.resolve` had the same hole. Walk the clauses directly instead of probing through `matches`, leaving match semantics untouched everywhere else. Also corrects the `version`/`required` rows of the skill-format field table, which still described a pin as recorded-not-resolved twenty lines above the note saying it is enforced, and asserts the skip reason in the optional-violation test rather than only that the skill was skipped.
1 parent a8f1b51 commit 527b7d0

4 files changed

Lines changed: 50 additions & 5 deletions

File tree

docs/plans/skill-format.mdx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,8 +195,8 @@ Each entry in `skills` or a set's list is either a **skill name** or a mapping:
195195
| Key | Type | Required | Description |
196196
|---|---|---|---|
197197
| `name` | string | yes | A skill name per [naming](#naming). |
198-
| `version` | string | no | SemVer or range. **Recorded, not resolved** in this phase — see below. |
199-
| `required` | bool | no | Default `true`. A missing `required: false` skill is logged and skipped; a missing required one fails the launch. |
198+
| `version` | string | no | SemVer or range, **enforced at load** against the installed skill's version — see below. |
199+
| `required` | bool | no | Default `true`. A `required: false` skill that is missing *or version-incompatible* is logged and skipped; either failure on a required one fails the launch. |
200200

201201
**Set names** use the same slug shape as skill names, capped at 32 chars — a
202202
lowercase alphanumeric start and end with internal hyphens:

src/gaia/skills/versions.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,15 @@ def validate_spec(spec: Optional[str]) -> None:
158158
Raises:
159159
SkillValidationError: naming the clause that could not be read.
160160
"""
161-
matches("0.0.0", spec)
161+
# Every clause, deliberately not via ``matches``: its ``all()`` short-circuits
162+
# on the first clause a probe version fails, which would leave a later
163+
# unreadable clause of a conjunction unchecked.
164+
normalized = (spec or "").strip()
165+
if normalized.lower() in ANY_SPECS:
166+
return
167+
for clause in normalized.split(","):
168+
if clause.strip():
169+
_matches_clause("0.0.0", clause)
162170

163171

164172
def highest(versions: Iterable[str]) -> Optional[str]:

tests/unit/test_skill_sets.py

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
from __future__ import annotations
1414

15+
import logging
1516
from pathlib import Path
1617
from unittest.mock import MagicMock, patch
1718

@@ -604,16 +605,26 @@ def test_agent_required_pin_violation_fails_the_launch(tmp_path, pinned):
604605
assert agent.active_skill_set is None
605606

606607

607-
def test_agent_optional_pin_violation_is_skipped_with_a_reason(tmp_path, pinned):
608+
def test_agent_optional_pin_violation_is_skipped_with_a_reason(
609+
tmp_path, pinned, caplog
610+
):
608611
agent = _pinned_agent(
609612
tmp_path,
610613
pinned,
611614
{"name": "inbox-triage", "version": ">=2.0.0", "required": False},
612615
)
613616

614-
assert agent.load_skill_set() == {}
617+
with caplog.at_level(logging.INFO):
618+
assert agent.load_skill_set() == {}
615619
assert agent.active_skill_set == "work"
616620

621+
# "Skipped" has to be visible, not invisible: the reason names the pin and
622+
# the version on disk, or the agent quietly runs without the capability.
623+
skipped = "\n".join(
624+
r.getMessage() for r in caplog.records if "inbox-triage" in r.getMessage()
625+
)
626+
assert ">=2.0.0" in skipped and "1.4.0" in skipped
627+
617628

618629
def test_agent_pin_against_an_unversioned_skill_is_refused(tmp_path, pinned):
619630
"""Unsatisfiable by unknowability: absence of a version is not a match."""

tests/unit/test_skills_marketplace.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,36 @@ def test_validate_spec_accepts_every_supported_range_shape():
143143
"~=1.2",
144144
"!=1.0.0",
145145
">=1.2.0, <2.0.0",
146+
">=1.2.3-beta.1, <2.0.0",
147+
">= 1.0.0 , < 2.0.0", # whitespace around the conjunction
146148
):
147149
validate_spec(spec)
148150

149151

152+
@pytest.mark.parametrize(
153+
"spec",
154+
[
155+
">=1.2.0, 1.2.x",
156+
">=1.2.0, >=v2",
157+
">=1.2.0, <2.0.0.0",
158+
"<2.0.0, junk",
159+
],
160+
)
161+
def test_validate_spec_checks_every_clause_of_a_conjunction(spec):
162+
"""The gate must not stop at the first clause a probe version fails.
163+
164+
``matches`` is ``all()`` over a generator, so it short-circuits: probing
165+
with a sentinel meant `>=1.2.0` returned False and the unreadable clause
166+
after it was never parsed. The manifest was then accepted, and the failure
167+
resurfaced at load only on machines with that skill installed at a version
168+
passing clause one — the machine-dependent verdict this gate exists to kill.
169+
"""
170+
from gaia.skills.versions import validate_spec
171+
172+
with pytest.raises(SkillValidationError, match="does not name a version number"):
173+
validate_spec(spec)
174+
175+
150176
# ---------------------------------------------------------------------------
151177
# Tiers
152178
# ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)