Repository navigation
Conversation
The executable embeds the registry while installing skill contents from main at run time, so catalog changes need a release and the two can drift unnoticed. The plan fetches skill-registry.json at the same commit as the contents, records that commit in plan evidence, pins global apply to the reviewed commit, caches registries for offline status, and removes the embedded copy. A release is then needed only for schema or CLI changes. Decided with the user on 2026-09-30; implementation starts with a spike confirming Skills CLI 1.5.23 installs commit-pinned public sources. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Skills CLI 1.5.23 cannot install `.../tree/<commit>/skills`: it clones with `--branch <ref>`, which rejects a commit hash. An anonymous shallow Git fetch of the commit followed by a local-path `skills add` installs a tree identical to that commit, so public agent-scripts materialization will reuse the authenticated path's commit fetch and gain a Git prerequisite. `git ls-remote` resolves main without API rate limits, and the raw registry at a commit matches `git show`. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The roadmap still described the global rollout as proposed and awaiting per-machine authorization, but its plan records the evidence binding as delivered and assigns per-machine outcomes to their execution artifacts. Today's configured global syncs ran under that contract, so the plan now states its outcome and completion and moves to the completed list. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Step 2 of the remote-registry plan. RegistrySource resolves refs/heads/main from Git's anonymous smart-HTTP ref advertisement, fetches skill-registry.json from the raw URL at that commit, validates it, and records its SHA-256. The ref advertisement has the same protocol and rate-limit profile as `git ls-remote` but keeps the frequent status path free of child processes. Verified registries are cached by commit in a `registry/` namespace that reuses the status cache's bounded reads, lock, and atomic writes. Status reuses a resolution for 24 hours and honors the 15-minute retry cooldown; selection commands always try to refresh. Both fall back to the last verified registry marked stale, while an unsupported or invalid published registry fails closed so a cached copy cannot hide a needed update. The CLI now loads its registry once per invocation instead of three times for plan and apply. Production still reads the embedded registry until step 3 switches loading, pins sources, and records commit evidence together. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Step 3 of the remote-registry plan. The CLI now loads skill-registry.json through RegistrySource once per invocation: plan and apply resolve main afresh, global apply reloads the registry at the commit recorded in the reviewed plan so a push after review cannot change the applied state, status reuses a resolution for 24 hours, and profiles/init fall back to a cached registry with a registry-stale warning. Plan evidence records `sjunepark/agent-scripts@<commit> sha256:<digest>`; plans from embedded-registry releases must be reviewed again. Agent-scripts sources are pinned to the registry's commit. Skills CLI cannot clone a commit, so the materializer fetches public /tree/<commit> sources anonymously with Git and installs from the checkout. Git runs in Skills CLI's own isolated environment so system settings such as core.autocrlf still apply: disabling them checked out LF files on Windows and reported every CRLF-installed skill as an update. Inherited GIT_*, GH_*, askpass, and token variables are stripped so a Git hook environment cannot redirect the forced checkout into the caller's repository. Plan and apply always fetch registry bytes rather than trusting an unverifiable cached file, and a reviewed commit that no longer exists is a conflict. `--no-status-check` also skips the registry cache. CLI integration tests build with the sjskillstest tag, which alone lets the binary read a fixture registry server; release.py clears GOFLAGS so the tag cannot reach a release. A live read-only `plan --global` matches the previous binary's expected-content hashes exactly. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Step 4 of the remote-registry plan. The CLI reads the registry from the published source, so the embedded copy and EmbeddedRegistry are removed. The former data file becomes a frozen Go test fixture under internal/sjskills/testdata: tests assert its catalog counts, so catalog changes on main no longer require test edits. The Node test drops its embedded-copy equality check and keeps validating skill-registry.json against the repository's skills. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Step 5 of the remote-registry plan. docs/skill-registry.md now owns the publication and loading contract: the registry is read from main at run time, agent-scripts skills install from its commit, plans record commit and digest evidence that global apply reloads, older reviewed plans need re-review, cached registries carry a registry-stale warning, and --no-status-check has no offline fallback. The release guide notes that only schema or CLI changes need a release and that plan/apply need Git. The sjskills skill and its references describe the same boundaries. The installed-archive consumer check runs offline and the binary no longer embeds a registry, so test-release.py seeds the registry cache: status uses it, profiles/init fall back with a stale warning, and plan fails because it never uses a cached registry. It passed on windows/amd64. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: sjunepark/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (33)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 SummarySummary by CodeRabbit
WalkthroughThe CLI now reads the skill registry from its published source at runtime. Plans record registry commit and digest evidence, and global apply reloads the registry at the reviewed commit. Commit-pinned public GitHub skills use an anonymous Git fetch path. ChangesPublished registry flow
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant CLI
participant RegistrySource
participant ReviewedPlan
participant Materializer
CLI->>RegistrySource: Resolve registry for planning
RegistrySource-->>CLI: Return registry and commit evidence
CLI->>ReviewedPlan: Record plan with registry evidence
CLI->>ReviewedPlan: Read reviewed registry commit during global apply
CLI->>RegistrySource: Load registry at reviewed commit
RegistrySource-->>CLI: Return registry at reviewed commit
CLI->>Materializer: Prepare using reviewed registry
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains. The published-registry flow is ready to merge subject to normal checks, including the native platform matrix. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 22.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 58 functions across 23 files. (8 skipped: 8 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
Review follow-up for
All 8 checks pass, including native consumer checks on macOS Intel, macOS Apple silicon, and Windows x64. |
sjunepark
added a commit
that referenced
this pull request
Sep 30, 2026
Advance the minor version for PR #26: sjskills reads skill-registry.json from main at run time, pins agent-scripts skills to that commit, and binds global apply to the reviewed registry commit. Later catalog changes need no release. Publish only artifacts that pass the tagged native matrix. Co-Authored-By: Claude Opus 5.5 <[email protected]>
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.
Summary
sjskillsused to embedskill-registry.jsonand install skill contents from.../tree/main/skills. As a result, every catalog change needed a new release, and nothing recorded which commit supplied the installed contents. This PR makes the binary read the registry frommainat run time and install agent-scripts skills from that same commit. After this, only registry schema or CLI changes need a release. The plan is plans/sjskills-remote-registry.md (steps 1–5; step 6 is the release).Behavior
mainonce, using Git's anonymous smart-HTTP ref advertisement (no credentials, no API quota). It then fetches the registry fromraw.githubusercontent.comat that commit and loads it once for plan, status and notices.tree/mainsources are pinned to that commit. Skills CLI can't clone a commit, so public/tree/<commit>sources are fetched with Git (now a prerequisite) and installed from the checkout.core.autocrlf), so installed bytes match Skills CLI's own clones.GIT_*,GH_*, askpass and token variables.sjunepark/agent-scripts@<commit> sha256:<digest>. Global apply reloads the reviewed commit, so pushes after review can't change the applied state. Plans from embedded-registry releases must be re-reviewed. A missing reviewed commit is a conflict.statusreuses a resolution for 24h and follows the retry cooldown.profiles/initfall back to the last verified cached registry and emit aregistry-stalewarning.plan/applyalways fetch.versionfails closed and names the required update.--no-status-checkskips the registry cache and has no offline fallback.sjskillstesttag that reads a fixture registry server.release.pyclearsGOFLAGS, so release builds can't pick up the tag.Validation
go test ./...on Windows passes everything exceptTestDefaultStatusNestedAndUnsafeConfiguration/symlink. That subtest also fails on the prior commit because this Windows account lacks the symlink privilege.go vet ./..., the Node registry/instructions/audit tests, the hook tests,scripts/validate-skillsandrelease_test.pypass. So dorelease.py build/verifyandtest-release.py(windows/amd64, run withPYTHONUTF8=1because of a local cp949 console).plan --globalagainst real GitHub resolvedmain, pinned every agent-scripts source, and produced expected-content hashes identical to the previous binary's (16 unchanged).GIT_DIR/GIT_WORK_TREEredirect risk and a cache-trust gap onplan/apply. Both are fixed and covered by tests.Risks
main'sskill-registry.jsonas the registry the new binary reads. Currently released binaries are unaffected until the next release is installed.🤖 Generated with Claude Code