Repository navigation
feat(core): fetch the skills bundle from an immutable pin - #123
Open
dg-coreylweathers wants to merge 4 commits into
Open
dg-coreylweathers wants to merge 4 commits into
dg-coreylweathers wants to merge 4 commits into
Conversation
Add deepctl_core.skill_bundle. It downloads the deepgram/skills tarball from a pinned commit SHA, verifies the tarball sha256, extracts it with strict member checks, and returns skill folders whose names are single plain path segments. The cache is published atomically. Nothing calls it yet.
A concurrent fetch that already published a valid cache now counts as success instead of failing. Staging is kept whenever it holds the only copy of the old cache. Windows reserved device names and trailing dots or spaces are rejected in tar members and skill names. The ref length cap drops to 100 to stay under MAX_PATH.
dg-coreylweathers
force-pushed
the
goal/bg-1-skills-bundle-fetch
branch
4 times, most recently
from
October 5, 2026 13:52
e7d1a0f to
1344774
Compare
The staging cleanup re-checks that the moved-aside copy carries the deepctl marker. A target that changed between the check and the move is refused: a directory is put back, and anything else (a file or a symlink, dangling or not) is kept in staging and named in the error, so nothing is overwritten. Refs with a segment starting with '.' are rejected. Windows superscript COM/LPT and CONIN$/CONOUT$ names are rejected, the cache name cap drops to 120 bytes for MAX_PATH, and errors read as one sentence.
dg-coreylweathers
force-pushed
the
goal/bg-1-skills-bundle-fetch
branch
from
October 5, 2026 13:59
1344774 to
60f9630
Compare
dg-coreylweathers
marked this pull request as ready for review
October 5, 2026 14:05
This was referenced Oct 5, 2026
GregHolmes
reviewed
Oct 7, 2026
GregHolmes
left a comment
Contributor
There was a problem hiding this comment.
Review: deepgram/cli #123, feat(core): fetch the skills bundle from an immutable pin
Classification: code
Verdict: approve
Intent
Add an unused deepctl_core.skill_bundle module that downloads the Deepgram skills bundle from a full commit SHA, verifies the default archive hash, safely extracts it, validates manifest skill names, and atomically publishes a cache. It must not change any command or existing install behavior.
Blocking
None.
Should-fix
None.
Nits
None.
Verified (evidence)
- PASS: The default source is the full
0fc13fad726fb78e17fb1f05ba5942f0d022990fcommit and the checked-in SHA-256 matches the codeload archive.git ls-remoteresolvesdeepgram-skills-v1.7.0to that commit; the downloaded archive hashes to5b7f975378110372c8ae3a3c712b72ba2fa43b06f2d1d93497abf87262a23980. - PASS:
packages/deepctl-core/src/deepctl_core/skill_bundle.pyrejects unsafe archive members, non-regular types, unsafe Windows components, unsafe refs, and manifest names before constructing destination paths. - PASS: The cache only serves the pinned ref after structural manifest validation; tampered downloaded archives are rejected before extraction or cache publication.
- PASS: Docker verification with the repository lockfile passed: Ruff format/check, strict mypy for
skill_bundle.py, andpackages/deepctl-core/tests/unit/test_skill_bundle.py(140 passed).
Needs human
None.
Developer-facing messaging
The internal module is deliberately not wired into any command yet, so this PR introduces no new developer-facing behavior or documentation claim.
GregHolmes
approved these changes
Oct 7, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First PR of the five that replace #111. It adds one new module,
deepctl_core.skill_bundle, plus its tests. Nothing calls it yet.What it does
0fc13fad726fb78e17fb1f05ba5942f0d022990f(labeleddeepgram-skills-v1.7.0). Its tarball must match sha2565b7f9753…3980, and a mismatch raisesSkillFetchError. The release tag is lightweight and could be moved, so it is kept only as a human-readable label and is never fetched.--refandDEEPCTL_SKILLS_REFaccept only[A-Za-z0-9._/-], must start with a letter or digit, and may not contain..,//or/., or end in.or/. The cap is 100 characters, and a cache dir name may be at most 120 bytes, which keeps a typical Windows cache path under MAX_PATH. A user-supplied ref has no known hash, so it is not hash-checked, unless it equals the pinned commit.SkillRefNotFoundError, and an invalid ref raisesSkillRefInvalidError...,\,:(drive letters, UNC paths, alternate data streams), symlinks, hardlinks and devices.CON,nul.txt,COM1,COM¹,CONOUT$…) and path components that end in a dot or a space.xb, so an existing file is never overwritten.plugins[name=="deepgram"].skillsfrom.claude-plugin/marketplace.json. Each entry must fully match(?:\./)?skills/<name>. The name must match^[A-Za-z0-9][A-Za-z0-9._-]*$and must not be a Windows reserved name, end in a dot, or duplicate another name by case. All of this is checked before anyPathis built.~/.deepctl/skills/repo_cache/.mkdtempsibling, validates, then swaps withos.replace.dgprocess publishes a valid cache for the same ref during the final rename, that copy is used instead of failing. Other overlaps fail cleanly and keep the old copy.What it does not do yet
It has no caller. It doesn't change
skill_generator.pyor any command, and it doesn't touch the README. Folder install and thedg skillscommands come in goal 2. Concurrent fetches of the same ref don't lose data, but they aren't serialized. Goal 5's lock coversskills.jsonwrites, not this cache. Returned paths are valid until the next fetch of the same ref replaces the cache, so goal 2's installer must treat a source folder that vanishes mid-copy as a retryable fetch error.Greg's findings this closes (from the 2026-10-05 review on #111)
TestPinnedDefault::test_tampered_archive_is_refused_and_leaves_nothingand::test_tampered_archive_keeps_the_previous_cache.TestManifesttests are parametrized over..,a/b,a\b,/abs,C:\x,C:x,.,con,NUL,com1and more.Ownership proof for every filesystem delete or move
rmtree: it removes only the staging dir this call created withtempfile.mkdtempinside the cache root. Before deleting, it re-checks that the moved-aside old copy carries the marker. Staging is kept whenever it holds the only copy of the old cache, and the error names it (a bare Ctrl-C has no error to name it in).How to review
Read
skill_bundle.py(399 lines). Its sections, in order:RepoSkillvalidate_ref,resolve_skills_ref)fetch_skill_bundle_publishCheck the pin yourself:
git ls-remote https://github.com/deepgram/skills refs/tags/deepgram-skills-v1.7.0curl -sSL https://codeload.github.com/deepgram/skills/tar.gz/0fc13fad726fb78e17fb1f05ba5942f0d022990f | shasum -a 256Repeated downloads returned the same bytes.
Skim the tests. They are grouped as
TestPinnedDefault(S3),TestRefs,TestDownload,TestTarSafety,TestManifest(B4) andTestPublish, and none of them use the network.Verification
git diff --numstatexcluding tests.Stacked series: 1/5. Tracking PR: #111.
🤖 Generated with Claude Code