From a279441aeef8a46f3f10750235d23cbe156fe4f4 Mon Sep 17 00:00:00 2001 From: Jerry Yan <792602257@qq.com> Date: Tue, 7 Jul 2026 08:31:11 +0800 Subject: [PATCH] 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 --- skills_learning/versioning.py | 93 ++++++++++++++++++++++++---------- tests/test_skill_store.py | 37 +++++++++++++- tests/test_skill_versioning.py | 60 ++++++++++++++++++++++ 3 files changed, 161 insertions(+), 29 deletions(-) diff --git a/skills_learning/versioning.py b/skills_learning/versioning.py index e709498..09c80cd 100644 --- a/skills_learning/versioning.py +++ b/skills_learning/versioning.py @@ -3,6 +3,7 @@ from __future__ import annotations from dataclasses import dataclass from typing import Any +from skills_learning.config import SkillAuthoringConfig from skills_learning.models import FlowStep, FlowTemplateSkill from skills_learning.store import SkillStore @@ -13,6 +14,68 @@ class VersionDiff: stored_sequence: list[str] executed_sequence: list[str] argument_differences: list[tuple[int, str, Any, Any]] + argument_divergence_fraction: float + + +def diff_flow_versions( + stored_steps: list[FlowStep], + executed_steps: list[FlowStep], + *, + config: SkillAuthoringConfig | None = None, +) -> VersionDiff: + """Compare a stored skill's steps against a newly executed run. + + Per design.md D4: a change to the tool-name *sequence* itself + (insertion/deletion/reorder) always triggers a new version, with no + tolerance applied. When the sequence matches (same skeleton), the + fraction of step positions whose arguments differ is compared against + ``config.divergence_tolerance`` (a fraction in ``[0, 1]``): a + "materially different parameter set" that exceeds that tolerance is + also treated as divergence worth a new version, per proposal.md's + "differ beyond a configured tolerance ... or a materially different + parameter set" trigger. When no ``config`` is supplied, tolerance is + treated as unlimited (matching the pre-existing, backward-compatible + behavior of never bumping a version for argument-only differences). + """ + stored_sequence = [step.tool_name for step in stored_steps] + executed_sequence = [step.tool_name for step in executed_steps] + sequence_diverged = stored_sequence != executed_sequence + + differences: list[tuple[int, str, Any, Any]] = [] + diverged_positions = 0 + total_positions = 0 + if not sequence_diverged: + for index, (stored_step, executed_step) in enumerate( + zip(stored_steps, executed_steps, strict=True) + ): + total_positions += 1 + keys = set(stored_step.args) | set(executed_step.args) + position_diverged = False + for key in sorted(keys): + stored_value = stored_step.args.get(key) + executed_value = executed_step.args.get(key) + if stored_value != executed_value: + differences.append((index, key, stored_value, executed_value)) + position_diverged = True + if position_diverged: + diverged_positions += 1 + + argument_divergence_fraction = ( + diverged_positions / total_positions if total_positions else 0.0 + ) + beyond_tolerance = ( + not sequence_diverged + and config is not None + and argument_divergence_fraction > config.divergence_tolerance + ) + + return VersionDiff( + structural_divergence=sequence_diverged or beyond_tolerance, + stored_sequence=stored_sequence, + executed_sequence=executed_sequence, + argument_differences=differences, + argument_divergence_fraction=argument_divergence_fraction, + ) @dataclass(frozen=True) @@ -21,41 +84,17 @@ class VersioningResult: created_new_version: bool -def diff_flow_versions( - stored_steps: list[FlowStep], - executed_steps: list[FlowStep], -) -> VersionDiff: - stored_sequence = [step.tool_name for step in stored_steps] - executed_sequence = [step.tool_name for step in executed_steps] - structural = stored_sequence != executed_sequence - differences: list[tuple[int, str, Any, Any]] = [] - if not structural: - for index, (stored_step, executed_step) in enumerate( - zip(stored_steps, executed_steps, strict=True) - ): - keys = set(stored_step.args) | set(executed_step.args) - for key in sorted(keys): - stored_value = stored_step.args.get(key) - executed_value = executed_step.args.get(key) - if stored_value != executed_value: - differences.append((index, key, stored_value, executed_value)) - return VersionDiff( - structural_divergence=structural, - stored_sequence=stored_sequence, - executed_sequence=executed_sequence, - argument_differences=differences, - ) - - def store_synthesized_skill( store: SkillStore, candidate: FlowTemplateSkill, + *, + config: SkillAuthoringConfig | None = None, ) -> VersioningResult: latest = store.get_latest_by_name(candidate.name) if latest is None: return VersioningResult(store.create_version(candidate), True) - diff = diff_flow_versions(latest.steps, candidate.steps) + diff = diff_flow_versions(latest.steps, candidate.steps, config=config) if diff.structural_divergence: return VersioningResult(store.create_version(candidate, parent=latest), True) diff --git a/tests/test_skill_store.py b/tests/test_skill_store.py index a4b1baf..34398b0 100644 --- a/tests/test_skill_store.py +++ b/tests/test_skill_store.py @@ -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: diff --git a/tests/test_skill_versioning.py b/tests/test_skill_versioning.py index 9621c18..5643721 100644 --- a/tests/test_skill_versioning.py +++ b/tests/test_skill_versioning.py @@ -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