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
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.
|
Review threads: 3 resolved (2 fixed, 1 declined with the reasoning in the thread).
|
There was a problem hiding this comment.
🟡 Changes recommended
The tokenless test can inherit a real token and publish fixture content to the production skills repository.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
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, basecamp-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.
🟡 Changes recommended
Consecutive pre-fix publishers can overwrite the tombstone and resume deleting one another’s skills during migration.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The URL rewrite still exposes the repository token in the Git transport helper’s process arguments.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Review threads: 5 resolved (4 fixed, 1 declined with the reasoning in the thread). Declined:
Fixed, and worth carrying to the sibling copies of the script (basecamp/cli#78, hey-cli#434, fizzy-cli#214), since both scripts are meant to stay identical apart from
Copilot approved 4ec7f53; Codex last reviewed a8bb7f3 and has not re-reviewed since, so a re-request needs a human. |
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.extraheader(AUTHORIZATION: basic …, the form actions/checkout writes) in a private mode-600GIT_CONFIG_GLOBALremoved with the tmpdir, so it appears in neither the remote URL nor any git argv — aninsteadOfrewrite, the first cut here, still hands the credential-bearing URL togit-remote-httpsin argv.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).sync-skillsjob inrelease.ymlnow makes the check the manual workflow 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 this branch's script run against it as basecamp-cli, then the sibling hey-cli branch's script run against the result: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, plus anunset SKILLS_TOKENup front so a token in the caller's environment cannot carry the github.com case to the real target;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 (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 hey-cli carries the same change: basecamp/hey-cli#434