Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44bb9a7aa7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR adds an automated skill synchronization pipeline that copies skill documentation from basecamp-cli to the basecamp/skills distribution repository on each release. It introduces a shell script for the sync logic, Makefile targets for local/remote dry-run testing, and a new CI job in the release workflow.
Changes:
- Adds
scripts/sync-skills.shwith manifest-driven stale cleanup, URL/branch safety guards, and a single non-fast-forward retry - Appends a
sync-skillsjob to.github/workflows/release.yml, gated on thereleasejob withcontinue-on-error: trueand failure notification via GitHub issues - Adds
make sync-skillsandmake sync-skills-remotetargets for manual dry-run testing
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
scripts/sync-skills.sh |
Core sync script: discovers skills, copies non-Go assets, cleans up stale skills via .managed-skills manifest, commits and pushes with one non-fast-forward retry |
.github/workflows/release.yml |
Adds sync-skills CI job that runs after release, generates an App token scoped to basecamp/skills, runs the sync script, and notifies on failure |
Makefile |
Adds sync-skills (local DRY_RUN) and sync-skills-remote (remote DRY_RUN) targets for manual testing, with entries in the help text |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
bot
added
enhancement
github-actions
bot
added
enhancement
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
bot
added
enhancement
Add scripts/sync-skills.sh that copies skills from this repo into the basecamp/skills distribution repo on each tagged release. The release workflow runs the script after a successful release, using the existing GitHub App for authentication. The script discovers skills via skills/*/SKILL.md, copies them (excluding *.go and dotfiles), removes stale skills tracked by a .managed-skills manifest, and commits with provenance metadata. Safety guards validate the target remote URL (exact github.com host match) and branch. Push failures from non-fast-forward are retried once after rebase; other errors fail immediately. DRY_RUN=local and DRY_RUN=remote modes support offline and authenticated testing respectively. Makefile targets (sync-skills, sync-skills-remote) expose these for manual use. On sync failure, the workflow creates or comments on a deduplicated issue in basecamp/skills and emits a ::error annotation.
- Reject '.' and '..' explicitly in manifest validation before the regex check, which would otherwise accept them - Add SKILLS_TOKEN guard to sync-skills-remote Makefile target - Wrap retry push in error handling so failures produce a clear message
When no .managed-skills manifest exists in the target repo, treat all */SKILL.md directories as previously managed so stale skills are cleaned up on the first run rather than becoming permanent drift. Also widen the manifest entry validator to accept mixed-case names.
- Set timeout-minutes: 10 on sync-skills job to match the convention set by the release job and avoid 6-hour hangs on network issues - Redact credentials from the origin URL in assert_remote_url error messages so tokens are never logged
On first run (no .managed-skills manifest), all */SKILL.md directories in the target are treated as managed. This is intentional — basecamp-cli is the source of truth. DRY_RUN=remote can preview the effect.
Make variables aren't automatically exported as environment variables to subprocesses — pass it explicitly like the other env vars.
github-actions
bot
added
enhancement
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
scripts/sync-skills.sh— copiesskills/*/SKILL.md(and non-Go, non-dotfile assets) intobasecamp/skillson each tagged releasesync-skillsjob to the release workflow, gated onneeds: [release]andcontinue-on-error: truemake sync-skills TAG=v...andmake sync-skills-remote TAG=v...for manual dry-run testingKey behaviors:
.managed-skills) — only deletes skills previously synced by this script, never touches other content in the target repobasecamp/skills, plus::errorannotation as fallbackTest plan
DRY_RUN=local— discovers skills, copies into tmpdir, shows diffDRY_RUN=remote— clones target, copies, shows diff against real state (needs token)v*tag — verifysync-skillsjob in Actions, check commit inbasecamp/skills