Sync only the skills this CLI owns to basecamp/skills - #708
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. |
a8bb7f3 to
8985dfc
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Claimant detection can fail under pipefail, and active ownership collisions can still overwrite another publisher’s skill.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prevents multiple CLIs from deleting each other’s distributed skills by introducing source-specific ownership manifests.
Changes:
- Adds per-source manifests, collision safeguards, and legacy tombstone handling.
- Adds comprehensive interleaved-publisher E2E coverage.
- Updates manual recovery 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 source-scoped synchronization. |
e2e/sync_skills.bats |
Tests ownership and migration behavior. |
.github/workflows/sync-skills.yml |
Separates recovery logic and release content checkouts. |
RELEASING.md |
Documents synchronization and recovery. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Active and concurrent ownership collisions can still overwrite or delete another publisher’s skill.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
scripts/sync-skills.sh:220
- Current-source names bypass the only cross-manifest check here. Since
copy_skillshas already removed and recopied every current name, a second publisher that currently ships the same name silently overwrites the existing skill and leaves both manifests claiming it; no collision warning is emitted. This contradicts the stated “warn and leave it alone” collision contract. Check other manifests before copying current skills and skip or fail on an active collision; the collision test should cover both source trees shipping the same name.
in_source_set "$previously_managed" && continue
- Files reviewed: 4/4 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
basecamp-cli/scripts/sync-skills.sh
Line 135 in a8bb7f3
Check other claims before replacing a current skill
When a current source skill's name is already listed in another publisher's manifest, this unconditional rm -rf replaces that publisher's directory before other_claimants is ever consulted. A sequential sync then creates a second claim for the same name without warning, and subsequent releases alternate the distributed content according to which publisher ran last rather than leaving the collision alone. Check the other manifests for every current source name before copying, and abort or preserve the existing directory when a collision is found.
ℹ️ 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".
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 whichever CLI released last (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 bin/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 script is the shared CLI seed's, byte-identical apart from the CLI_NAME default. It also gains SKILLS_SOURCE, as hey-cli's already had, and the manual recovery workflow adopts hey-cli's two-checkout pattern: sync logic from the dispatching ref, content from the release tag.
8985dfc to
4285688
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Remote validation can miss additional push URLs and allow commits to reach an unintended destination.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced
A remote can carry several url and pushurl entries, and git pushes to all of them, but the target guard read one value per kind (`git config --get` reports the last), so a supplied checkout with an extra push destination could pass the check and send the sync commit there too. Read every value and refuse on any that is not basecamp/skills; the test covers a second push URL and a second fetch URL, foreign one first so a single-value read cannot pass it. Two comments still described the rebase a retried push used to do. Mirrors the seed (basecamp/cli#78 @ 17c383a); the script stays identical apart from the CLI_NAME default.
There was a problem hiding this comment.
🟡 Changes recommended
The remote validation can be bypassed by an ambient Git URL rewrite, allowing pushes to an unintended destination.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
…ards Mirrors basecamp/cli@966966e (the seed is the source of truth; only the CLI_NAME default differs here). 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.
There was a problem hiding this comment.
🟡 Changes recommended
Manifest symlinks can expose private configuration or overwrite another CLI’s skill content.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced
| apply_sync() { | ||
| local name other | ||
| local previously_published=() |
| # SKILLS_REPO_URL — where basecamp/skills is cloned from and pushed to (default: | ||
| # https://github.com/basecamp/skills.git). The test points it at | ||
| # a local bare repository so a real push lands somewhere it can | ||
| # read back | ||
| # DRY_RUN — "local": no network; copy into an empty tmpdir and print what |
| # A private global config for every git call below: the bot is the identity for | ||
| # the commit, and for the one a rejected push makes again, the token goes in as a URL | ||
| # rewrite so it never appears in argv or in the remote URL, and nothing from the | ||
| # ambient environment (signing, hooks, defaults) reaches the target. |
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_TARGETpoints the script at an existing checkout instead of cloning, and withDRY_RUN=localapplies and commits without pushing. The remote-URL and branch asserts and the copy filter are unchanged.The script is byte-identical to the shared CLI seed's (
seed/scripts/sync-skills.shin basecamp/cli#78 at 5aec9a1) 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, the checkout must be clean beforegit add -Asweeps it into a commit, and the token reaches git through a privateGIT_CONFIG_GLOBALinsteadOfrewrite instead of the clone URL (so it appears in neither argv nor the remote URL, and the remote-URL assert now checks the configuredurlandpushurlrather than the rewritten view).SKILLS_SOURCE, which hey-cli's already had, so the two scripts are now identical apart from the default source name, and the manualSync skillsworkflow adopts hey-cli's two-checkout pattern (sync logic from the dispatching ref, content from the release tag).Migration walkthrough
A clone of basecamp/skills
mainas of today, then this branch's script run against it as basecamp-cli, then the sibling hey-cli branch's script run against the result, both withSKILLS_TARGET+DRY_RUN=local(apply and commit, no push):So a hey-cli release after the sibling PR 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/bin/ciand the test workflow's e2e job) 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; a dirty checkout, a wrong remote/pushurl and a wrong branch are refused; 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 hey-cli carries the same change: basecamp/hey-cli#434