Sync only the skills this CLI owns to basecamp/skills - #434
Conversation
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
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. |
587c410 to
0e923e8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Active cross-publisher name collisions can still silently overwrite another publisher’s skill.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Introduces per-publisher skill manifests to prevent CLIs from deleting each other’s distributed skills.
Changes:
- Adds source-specific ownership and legacy-manifest migration.
- Adds interleaved publisher and collision tests.
- Updates workflow and release documentation.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
scripts/sync-skills.sh |
Implements per-source synchronization. |
tests/e2e/sync_skills.bats |
Tests ownership and migration behavior. |
RELEASING.md |
Documents the new contract. |
.github/workflows/sync-skills.yml |
Clarifies workflow behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
basecamp/skills is shared by several CLIs, and every one of them ran the same sync script against one shared .managed-skills: publish skills/*, then delete every name in that file that the publisher's own tree lacks. So hey-cli's v0.1.1 release deleted skills/basecamp and skills/basecamp-doctor (basecamp/skills@08ef7ea), and basecamp-cli's v0.10.0 and v0.11.0 deleted skills/hey (728a916, 42716d7). Today the distribution repo holds only the basecamp skills; HEY's is gone (basecamp/skills#5). Each publisher now owns a manifest of its own, .managed-skills.<source>, and reads only that file to decide what to remove. A skills/<name> goes only when this source's manifest lists it, this release no longer ships it, and no other source's manifest claims it — a name two manifests list is a collision to settle upstream, so it is warned about and left, and a name another source's manifest holds is refused for publishing. A target with no manifest for this source yet removes nothing; the old "no manifest, so everything under skills/ is ours" fallback is gone, since it is exactly what deleted the sibling's skills. The legacy .managed-skills stays, rewritten on every run as a comment-only tombstone. The pre-fix script skips every line it cannot parse as a skill name, so a sibling still running it deletes nothing; had the file been removed, that script's fallback would have claimed every skills/* directory. That is what makes the rollout order across CLIs irrelevant. A push rejected because a sibling published first ("fetch first" on a depth-1 clone, or non-fast-forward) drops the stale commit and applies the sync again from the remote's new tip, collision guard included. The token reaches git through a private GIT_CONFIG_GLOBAL insteadOf rewrite rather than the clone URL, and the checkout must be clean before the sync sweeps it into a commit. SYNC_SOURCE overrides the source name (the bot identity and commit message derive from it) so a test can play the other CLI, and SKILLS_TARGET points the script at an existing checkout instead of cloning; with DRY_RUN=local it applies and commits but skips the push. scripts/test-sync-skills.sh (make test-sync-skills, in make check and CI) builds a target reproducing basecamp/skills as the history above left it and runs the script interleaved as both CLIs, through a rejected push included. The existing bats tests keep the source-discovery contract, adapted to the script's new "Found" line. The script is the shared CLI seed's, byte-identical apart from the CLI_NAME default. The first stable release after this restores skills/hey.
0e923e8 to
e967fb5
Compare
…ards Mirrors basecamp/cli@966966e (the seed is the source of truth; only the CLI_NAME default differs here), and picks up the two seed commits this copy was behind: the remote-URL assert that read every configured URL, and two comment rewordings — both now moot, since the assert is gone. SKILLS_TARGET let the script adopt an existing checkout of basecamp/skills. Every release path clones its own target, and each review round found another corner of "any checkout" to guard. The script now always clones into a temp directory from SKILLS_REPO_URL (default https://github.com/basecamp/skills.git, the token carried through the same insteadOf rewrite as before), applies, pushes and cleans up, so the only commit it can push is the one it made. The remote-URL, branch and clean-tree asserts are gone with the knob; the retry from the fetched tip, the per-source manifests, the collision guard, the tombstone and DRY_RUN validation are unchanged. The test points SKILLS_REPO_URL at a local bare repository and reads every result back from a clone of its own; the race is staged with a post-commit hook that pushes the sibling's commit between the script's clone and push.
Mirrors the seed (basecamp/cli#78 at 1df3bfe5). GIT_CONFIG_GLOBAL stands in for the user's global file only. The system config and any GIT_CONFIG_COUNT settings in the environment still reach every git call the script makes — the race test injects its hooks path through exactly that — so the comment no longer claims the ambient environment is kept out.
…release The sync-skills job's concurrency group serialises the syncs but does not order them by version. An older stable release's job can reach the sync after a newer one has published — its run stalled, or its failed sync was rerun later, which is what the failure issue tells the operator to do — and the script mirrors the skills wholesale, so that run would roll basecamp/skills back to the older skill. The job now makes the check the manual Sync skills workflow already makes: the tag must be the repository's latest stable release, or the sync is skipped with a warning. A failed lookup fails the job, so the failure issue is filed rather than the sync skipped in silence.
There was a problem hiding this comment.
🟡 Changes recommended
The Git URL rewrite still exposes the repository token through the remote helper’s process arguments.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
![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: 94175636ea
ℹ️ 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".
Every case set SYNC_SOURCE to play hey-cli or basecamp-cli, so the one line this copy of the script changes — the CLI_NAME default — never ran. A last case runs the script with neither SYNC_SOURCE nor CLI_NAME set and checks the manifest it writes, the bot it commits as and the summary line against the source the Makefile expects, hey-cli. A copy left at the seed's placeholder, or naming another CLI, fails here instead of publishing under that source's manifest and identity. Mirrors seed/scripts/test-sync-skills.sh from basecamp/cli@19b52ed.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes sensitive cross-repository publishing, deletion, authentication, and concurrency behavior that warrants final human review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c86472889
ℹ️ 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".
… rewrite Mirrors the seed (basecamp/cli#78 at 0649a4d); scripts/sync-skills.sh stays byte-identical apart from the CLI_NAME default, scripts/test-sync-skills.sh byte-identical. The insteadOf rewrite kept the token out of the outer git argv and the remote URL, but git expands the rewrite before invoking git-remote-https, so that helper still received https://x-access-token:<token>@github.com/... as an argument. The private gitconfig now carries http.https://github.com/.extraheader with the basic-auth form actions/checkout writes: the helper sees the plain URL, and the token lives only in the mode-600 file until the tmpdir goes. The test unsets SKILLS_TOKEN up front: its token-required case points the script at the real basecamp/skills, and a token inherited from the caller's environment would have let it clone and publish the fixtures there. The header now says exactly what the tombstone shields — the sources that have upgraded — rather than "anyone's skills": a pre-fix sibling still rewrites .managed-skills with its own names and a second pre-fix sibling still deletes those, the basecamp/skills#5 clobber confined to the CLIs yet to upgrade.
|
Review threads: 3 resolved (1 fixed, 2 declined with the reasoning in each thread). Declined:
6918171 mirrors the seed (basecamp/cli#78 at 0649a4d): |
|
Bots on 6918171: Codex reviewed it with no findings. Copilot last reviewed 6c86472 and has not reported on 6918171 after 35 minutes, though it reviewed each earlier push within minutes; a re-review cannot be requested by API, so a human can request it from the Reviewers panel if wanted. The diff from 6c86472 is the seed mirror — |
The bug
basecamp/skills is shared by several CLIs, and each one's
scripts/sync-skills.shrecorded what it published in the same file,.managed-skills, then deleted every name in that file its ownskills/tree lacked. So the CLIs took turns deleting each other's skills (basecamp/skills#5):skills/basecampandskills/basecamp-doctorskills/heyskills/heyagainToday the distribution repo holds only the basecamp skills; HEY's is gone.
The fix
.managed-skills.<source>at the target root (basecamp-cli,hey-cli, …), one sorted name per line, and reads only its own file to decide what to remove.skills/<name>is removed only when this source's manifest lists it, this release no longer ships it, and no other.managed-skills.*claims it. Two publishers claiming one name is a collision to settle upstream, so it is warned about and left alone. Nothing else ever callsrm -rfon the target besides the per-skill refresh (a name in this source's set, immediately re-copied)..managed-skills.<source>yet removes nothing and writes the manifest from the current set. The old "no manifest, so everything underskills/is ours" fallback is gone — it is exactly what deleted the sibling's skills..managed-skillsstays but is rewritten on every run as a comment-only file. The pre-fix script validates each line against^[a-zA-Z0-9._-]+$and skips the rest, so a CLI still running it deletes nothing; had the file been deleted instead, that script's fallback would have claimed everyskills/*. This is what makes the rollout order across CLIs irrelevant.SYNC_SOURCEoverrides the source name (bot identity and commit message derive from it) so the test can play the other CLI;SKILLS_REPO_URLsays where the target is cloned from and pushed to (the test points it at a local bare repository). The script always clones the target fresh and pushes only the commit it made — there is no checkout to hand it.DRY_RUN=localpreviews the copy offline against an empty tree;DRY_RUN=remoteclones the real target and stops before committing. The copy filter is unchanged.The script is byte-identical to the shared CLI seed's (
seed/scripts/sync-skills.shin basecamp/cli#78 at 1df3bfe5) apart from theCLI_NAMEdefault, which is what carries the per-repoSYNC_SOURCE. The seed went three steps beyond the design, and this PR carries them:fetch firston a depth-1 clone (the oldnon-fast-forwardmatch never fires there) ornon-fast-forward— drops the stale commit, resets to the remote's new tip and applies the whole sync again, collision guard included, before pushing once more; no rebase of decisions made against a stale tree.SYNC_SOURCEandDRY_RUNare validated, and the token reaches git as a github.com-scopedhttp.extraheaderin a private mode-600GIT_CONFIG_GLOBALinstead of the clone URL or aninsteadOfrewrite (which git expands into git-remote-https's argv), so it appears in neither argv nor the remote URL.SKILLS_SOURCE, which this one already had). The pre-existingtests/e2e/sync_skills.batsstays (defaultskills/discovery from the working directory is the one contract the seed test does not exercise), with its twoFound …assertions adapted to the script's new line.sync-skillsjob inrelease.ymlnow makes the check the manualSync skillsworkflow already makes — the tag must be the repository's latest stable release — so an older release whose run stalls, or whose failed sync is rerun after a newer release has shipped, skips the sync instead of rolling basecamp/skills back. The concurrency group serialises the syncs but never ordered them.Migration walkthrough
A local bare clone of basecamp/skills
mainas of today as the target (SKILLS_REPO_URLpointed at it, so every clone and push lands there and nowhere else), then the sibling basecamp-cli branch's script run against it, then this branch's script run against the result:So the first hey-cli release after this merges restores HEY's skill to basecamp/skills, and no release of either CLI touches the other's skills again.
Test
scripts/test-sync-skills.sh(the seed's test, unchanged;make test-sync-skills, inmake check, the CIe2ejob and the release gate) builds a throwaway target reproducing basecamp/skills as the history above left it — the basecamp skills present,skills/heygone, the legacy.managed-skillslisting the basecamp names, plus a README that must survive — and two fixture trees (one with a nested file, a*.go, a dotfile and a dot-directory that must not be copied). It runs the script interleaved hey-cli, basecamp-cli, hey-cli, basecamp-cli, asserting after each run that both sources' skills are present, each manifest lists exactly its own names and.managed-skillsis the tombstone. Then: a skill dropped from one tree removes only that directory; a stale legacy manifest listing the sibling's names (a pre-fix sibling having run) deletes nothing; a name listed in both manifests survives with a warning; publishing a name another source owns is refused before anything changes;DRY_RUN=remoteshows the diff and commits nothing; theDRY_RUN=localpreview stays offline; a concurrent publisher winning the race to origin (a real push into a local bare repo) is absorbed by the fetch-first retry, and a concurrent claim on a name this source ships makes that retry refuse (and the sibling dropping the claim lets the next release publish); a github.com target without a token is refused before anything is cloned; and the commit author is<source>[bot].scripts/sync-skills.shis on the sensitive-change list, so expect the gate label on this PR.The sibling PR in basecamp-cli carries the same change: basecamp/basecamp-cli#708