Skip to content

Commit b22ded7

Browse files
fix(skills): make the new skills errors picklable, and say how to change a bad DEEPCTL_SKILLS_REF
Review round 9 on PR #111: - SkillRefNotFoundError, SkillsStateLockTimeout and SkillOwnershipError rebuild from their own arguments, so pickle and copy round-trip them - a missing ref that came from DEEPCTL_SKILLS_REF ends by saying to set it to another ref, or unset it to use the pinned release Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 15cc4e0 commit b22ded7

6 files changed

Lines changed: 139 additions & 6 deletions

File tree

‎packages/deepctl-cmd-skills/src/deepctl_cmd_skills/command.py‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -637,10 +637,16 @@ def _fetch_skills(
637637
except SkillRefNotFoundError as exc:
638638
# The ref does not exist upstream, so waiting for the network
639639
# cannot help. Core's message already names where the ref came
640-
# from; only a ref the user did not type needs the way to
641-
# choose another.
640+
# from. A ref typed with --ref needs nothing more; one from the
641+
# environment needs the way out of it, as login and the plugin
642+
# refresh give; any other needs --ref.
642643
advice = "No skills were installed."
643-
if exc.ref_source not in ("--ref", REF_ENV_VAR):
644+
if exc.ref_source == REF_ENV_VAR:
645+
advice += (
646+
f" Set {REF_ENV_VAR} to another ref, or unset it to use "
647+
"the pinned release."
648+
)
649+
elif exc.ref_source != "--ref":
644650
advice += " Pass --ref to choose another deepgram/skills ref."
645651
raise click.ClickException(f"{exc}\n\n{advice}")
646652
except SkillFetchError as exc:

‎packages/deepctl-cmd-skills/tests/unit/test_skills_command.py‎

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -298,10 +298,15 @@ def test_fetch_error_becomes_a_click_exception(self, message):
298298
@pytest.mark.parametrize(
299299
("ref_source", "last_line"),
300300
[
301-
# The user typed the ref, or set the env var: core's message
302-
# already says which, so there is nothing to add.
301+
# The user typed the ref: core's message already says so, so
302+
# there is nothing to add.
303303
("--ref", "No skills were installed."),
304-
(REF_ENV_VAR, "No skills were installed."),
304+
# The env var: how to get out of it, as login and plugin say.
305+
(
306+
REF_ENV_VAR,
307+
f"No skills were installed. Set {REF_ENV_VAR} to another ref, "
308+
"or unset it to use the pinned release.",
309+
),
305310
# A ref the user did not type: --ref is the way to another.
306311
(
307312
PINNED_REF_SOURCE,
@@ -329,6 +334,54 @@ def test_a_missing_ref_ends_without_network_advice(self, ref_source, last_line):
329334
assert "network" not in rendered
330335
assert "Retry" not in rendered
331336

337+
@pytest.mark.parametrize("subcommand", ["install", "update"])
338+
def test_a_missing_env_var_ref_says_how_to_change_it(
339+
self, monkeypatch, capsys, subcommand
340+
):
341+
"""It ended with a bare "No skills were installed." and no way out."""
342+
import urllib.error
343+
344+
monkeypatch.setenv(REF_ENV_VAR, "no-such-ref-zz9")
345+
generator = MagicMock()
346+
generator.cli_name = "claude"
347+
generator.display_name = "Claude Code"
348+
generator.detect.return_value = True
349+
generator.install_conflicts.return_value = []
350+
not_found = urllib.error.HTTPError("https://x", 404, "Not Found", {}, None)
351+
state = {"installed_skills": {"claude": {"paths": [], "skills_ref": "main"}}}
352+
cmd = SkillsCommand()
353+
with (
354+
patch(
355+
"deepctl_core.skill_generator.get_all_generators",
356+
return_value=[generator],
357+
),
358+
patch(
359+
"deepctl_core.skill_generator.collect_command_metadata",
360+
return_value=[],
361+
),
362+
patch("deepctl_core.skill_generator.get_skills_state", return_value=state),
363+
patch("deepctl_core.skill_generator.save_skills_state") as save,
364+
patch(
365+
"deepctl_core.skill_bundle.urllib.request.urlopen",
366+
side_effect=not_found,
367+
),
368+
pytest.raises(click.ClickException) as excinfo,
369+
):
370+
if subcommand == "install":
371+
cmd._handle_install(cli_name="claude")
372+
else:
373+
cmd._handle_update()
374+
375+
rendered = str(excinfo.value)
376+
assert f"which came from {REF_ENV_VAR}." in rendered
377+
assert rendered.splitlines()[-1] == (
378+
f"No skills were installed. Set {REF_ENV_VAR} to another ref, "
379+
"or unset it to use the pinned release."
380+
)
381+
assert "Pass --ref" not in rendered
382+
generator.install_skills.assert_not_called()
383+
save.assert_not_called()
384+
332385
def test_a_network_error_keeps_the_network_advice(self):
333386
exc = SkillFetchError(
334387
"Could not download deepgram/skills@main from https://x: "

‎packages/deepctl-core/src/deepctl_core/skill_bundle.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,11 @@ def __init__(self, ref: str, url: str, ref_source: str | None = None) -> None:
138138
f"{SKILLS_REPO} has no ref {ref!r} (HTTP 404 from {url}). {where}"
139139
)
140140

141+
def __reduce__(self) -> tuple[Any, ...]:
142+
# BaseException pickles and copies by calling the class with
143+
# ``self.args``, which here is only the formatted message.
144+
return (type(self), (self.ref, self.url, self.ref_source))
145+
141146
def advice(self, command: str) -> str:
142147
"""What to do instead of retrying, for a caller that takes no --ref.
143148

‎packages/deepctl-core/src/deepctl_core/skill_generator.py‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,17 @@ class SkillsStateLockTimeout(SkillsStateError):
104104

105105
def __init__(self, lock_path: Path, holder: str = "process") -> None:
106106
self.lock_path = lock_path
107+
self.holder = holder
107108
super().__init__(
108109
f"Another deepctl {holder} holds the skill records lock at "
109110
f"{lock_path}. Wait for it to finish and retry; the lock is "
110111
"released automatically when that process exits."
111112
)
112113

114+
def __reduce__(self) -> tuple[Any, ...]:
115+
# BaseException rebuilds from ``self.args`` (the message alone).
116+
return (type(self), (self.lock_path, self.holder))
117+
113118

114119
class SkillOwnershipError(Exception):
115120
"""A destination already exists and deepctl did not put it there.
@@ -134,6 +139,10 @@ def __init__(self, conflicts: Sequence[tuple[str, Path]]) -> None:
134139
"was installed."
135140
)
136141

142+
def __reduce__(self) -> tuple[Any, ...]:
143+
# BaseException rebuilds from ``self.args`` (the message alone).
144+
return (type(self), (self.conflicts,))
145+
137146

138147
# ---------------------------------------------------------------------------
139148
# Data model

‎packages/deepctl-core/tests/unit/test_skill_bundle.py‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
"""Unit tests for the upstream skill bundle fetcher."""
22

3+
import copy
34
import http.client
45
import importlib.metadata
56
import io
67
import json
78
import os
9+
import pickle
810
import sys
911
import tarfile
1012
import time
@@ -828,3 +830,25 @@ def test_pruning_tolerates_a_missing_root_and_non_directories(self, tmp_path):
828830
os.utime(stale_file, (two_hours_ago, two_hours_ago))
829831
skill_bundle._prune_stale_dirs(cache)
830832
assert stale_file.is_file()
833+
834+
835+
class TestSkillRefNotFoundErrorRoundTrips:
836+
"""Its __init__ takes three arguments, but BaseException rebuilds a
837+
copy or an unpickled error from ``args``, which held only the message."""
838+
839+
@pytest.mark.parametrize("ref_source", [None, "--ref", REF_ENV_VAR])
840+
@pytest.mark.parametrize(
841+
"clone",
842+
[lambda e: pickle.loads(pickle.dumps(e)), copy.copy, copy.deepcopy],
843+
ids=["pickle", "copy", "deepcopy"],
844+
)
845+
def test_ref_not_found_error_survives_pickle_and_copy(self, clone, ref_source):
846+
exc = SkillRefNotFoundError("no-such-ref-zz9", "https://x/y", ref_source)
847+
848+
again = clone(exc)
849+
850+
assert type(again) is SkillRefNotFoundError
851+
assert again.ref == "no-such-ref-zz9"
852+
assert again.url == "https://x/y"
853+
assert again.ref_source == ref_source
854+
assert str(again) == str(exc)

‎packages/deepctl-core/tests/unit/test_skill_generator.py‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
"""Unit tests for skill generator module."""
22

3+
import copy
34
import json
45
import multiprocessing
56
import os
7+
import pickle
68
import re
79
import shutil
810
import threading
@@ -3062,3 +3064,37 @@ class TestWithAdvice:
30623064
)
30633065
def test_joins_with_exactly_one_full_stop(self, detail, joined):
30643066
assert skill_generator.with_advice(detail, "Retry.").endswith(joined)
3067+
3068+
3069+
_CLONES = pytest.mark.parametrize(
3070+
"clone",
3071+
[lambda e: pickle.loads(pickle.dumps(e)), copy.copy, copy.deepcopy],
3072+
ids=["pickle", "copy", "deepcopy"],
3073+
)
3074+
3075+
3076+
class TestSkillErrorsRoundTrip:
3077+
"""BaseException rebuilds a copy or an unpickled error from ``args``,
3078+
which for these held only the formatted message, not the arguments."""
3079+
3080+
@_CLONES
3081+
def test_lock_timeout_survives_pickle_and_copy(self, clone, tmp_path):
3082+
exc = SkillsStateLockTimeout(tmp_path / "skills.lock", holder="thread")
3083+
3084+
again = clone(exc)
3085+
3086+
assert type(again) is SkillsStateLockTimeout
3087+
assert again.lock_path == tmp_path / "skills.lock"
3088+
assert again.holder == "thread"
3089+
assert str(again) == str(exc)
3090+
3091+
@_CLONES
3092+
def test_ownership_error_survives_pickle_and_copy(self, clone, tmp_path):
3093+
conflicts = [("api", tmp_path / "api"), ("docs", tmp_path / "docs")]
3094+
exc = SkillOwnershipError(conflicts)
3095+
3096+
again = clone(exc)
3097+
3098+
assert type(again) is SkillOwnershipError
3099+
assert again.conflicts == conflicts
3100+
assert str(again) == str(exc)

0 commit comments

Comments
 (0)