fix(skill-versioning): consume divergence_tolerance, replace vacuous catalog-isolation guard
- divergence_tolerance was loaded from config but diff_flow_versions never consulted it, so structural_divergence was always plain sequence-equality regardless of the configured tolerance. Now actually applied per design.md D4. - The skill-catalog-subscription isolation guard test asserted 'skills.catalog' not in sys.modules, which is vacuously true since that module doesn't exist anywhere yet. Replaced with a real check against store.py's actual imports. openspec: skill-versioning capability, archived change skill-learning-runtime
This commit is contained in:
@@ -1,7 +1,9 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
import ast
|
||||
import inspect
|
||||
|
||||
from skills_learning import store as skill_store_module
|
||||
from skills_learning.models import (
|
||||
LOCAL_SYNTHESIS_SOURCE,
|
||||
FlowStep,
|
||||
@@ -11,6 +13,25 @@ from skills_learning.models import (
|
||||
from skills_learning.store import SkillStore
|
||||
|
||||
|
||||
def _imported_module_names(module) -> set[str]:
|
||||
"""Statically collect every module name imported by ``module``'s source.
|
||||
|
||||
Covers both ``import x.y`` and ``from x.y import z`` forms so the check
|
||||
actually fails if a catalog-subscription import is ever added, instead
|
||||
of only checking whether some module happened to already be imported by
|
||||
something else in the running interpreter.
|
||||
"""
|
||||
source = inspect.getsource(module)
|
||||
tree = ast.parse(source)
|
||||
names: set[str] = set()
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.Import):
|
||||
names.update(alias.name for alias in node.names)
|
||||
elif isinstance(node, ast.ImportFrom) and node.module:
|
||||
names.add(node.module)
|
||||
return names
|
||||
|
||||
|
||||
def _skill(
|
||||
*,
|
||||
name: str = "search web",
|
||||
@@ -35,7 +56,19 @@ def test_skill_store_enforces_local_synthesis_source_without_catalog_write_path(
|
||||
stored = store.create_version(_skill())
|
||||
|
||||
assert stored.source == LOCAL_SYNTHESIS_SOURCE
|
||||
assert "skills.catalog" not in sys.modules
|
||||
|
||||
imported_modules = _imported_module_names(skill_store_module)
|
||||
catalog_coupled = {
|
||||
name
|
||||
for name in imported_modules
|
||||
if "catalog" in name.lower() or "sync_client" in name.lower()
|
||||
}
|
||||
assert catalog_coupled == set(), (
|
||||
"skills_learning/store.py must not import from the "
|
||||
"skill-catalog-subscription store/sync-client (D6: locally-authored "
|
||||
"skills live in their own store, composed only in prose); found: "
|
||||
f"{catalog_coupled}"
|
||||
)
|
||||
|
||||
|
||||
def test_skill_store_keeps_version_chain_and_returns_latest_by_name() -> None:
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
from __future__ import annotations
|
||||
|
||||
from skills_learning.config import SkillAuthoringConfig
|
||||
from skills_learning.models import FlowStep, FlowTemplateSkill, SkillMetadata
|
||||
from skills_learning.store import SkillStore
|
||||
from skills_learning.versioning import diff_flow_versions, store_synthesized_skill
|
||||
@@ -70,3 +71,62 @@ def test_structural_divergence_creates_new_version_and_preserves_parent() -> Non
|
||||
assert result.skill.parent_version_id == first.id
|
||||
assert store.get_by_id(first.id) == first
|
||||
assert store.get_latest_by_name("search") == result.skill
|
||||
|
||||
|
||||
def _two_step_skill(*, text: str) -> FlowTemplateSkill:
|
||||
return _skill(
|
||||
steps=[
|
||||
FlowStep("tap", {"x": 1}),
|
||||
FlowStep("input_text", {"text": text}),
|
||||
]
|
||||
)
|
||||
|
||||
|
||||
def test_argument_divergence_within_configured_tolerance_does_not_bump_version() -> None:
|
||||
store = SkillStore()
|
||||
first = store.create_version(_two_step_skill(text="coffee"))
|
||||
candidate = _two_step_skill(text="tea")
|
||||
# Half (1 of 2) step positions diverge; tolerance allows up to half.
|
||||
config = SkillAuthoringConfig(divergence_tolerance=0.5)
|
||||
|
||||
diff = diff_flow_versions(first.steps, candidate.steps, config=config)
|
||||
assert diff.structural_divergence is False
|
||||
assert diff.argument_divergence_fraction == 0.5
|
||||
|
||||
result = store_synthesized_skill(store, candidate, config=config)
|
||||
|
||||
assert result.created_new_version is False
|
||||
assert result.skill.version == first.version
|
||||
assert result.skill.id == first.id
|
||||
|
||||
|
||||
def test_argument_divergence_beyond_configured_tolerance_bumps_version() -> None:
|
||||
store = SkillStore()
|
||||
first = store.create_version(_two_step_skill(text="coffee"))
|
||||
candidate = _two_step_skill(text="tea")
|
||||
# Half (1 of 2) step positions diverge; tolerance only allows less than that.
|
||||
config = SkillAuthoringConfig(divergence_tolerance=0.3)
|
||||
|
||||
diff = diff_flow_versions(first.steps, candidate.steps, config=config)
|
||||
assert diff.structural_divergence is True
|
||||
assert diff.argument_divergence_fraction == 0.5
|
||||
|
||||
result = store_synthesized_skill(store, candidate, config=config)
|
||||
|
||||
assert result.created_new_version is True
|
||||
assert result.skill.version == 2
|
||||
assert result.skill.parent_version_id == first.id
|
||||
|
||||
|
||||
def test_argument_divergence_without_config_never_bumps_version() -> None:
|
||||
# Backward-compatible default: no config supplied means unlimited
|
||||
# tolerance, matching pre-existing behavior of absorbing any argument
|
||||
# divergence as a parameter update rather than a new version.
|
||||
store = SkillStore()
|
||||
first = store.create_version(_two_step_skill(text="coffee"))
|
||||
candidate = _two_step_skill(text="tea")
|
||||
|
||||
result = store_synthesized_skill(store, candidate)
|
||||
|
||||
assert result.created_new_version is False
|
||||
assert result.skill.version == first.version
|
||||
|
||||
Reference in New Issue
Block a user