Seed the skills sync with per-source manifests and the skills/ layout - #78
Merged
Merged
Conversation
The sync logic lived three times (scripts/, seed/scripts/, the composite action), and all three diverged from what basecamp/skills actually holds: they published to <cli>/<skill>/ at the target root, a layout never adopted, and tracked ownership in one shared .managed-skills. The real CLIs' scripts write skills/<name>/ and bare names in that shared file, and each deletes every name not its own (basecamp/skills#5). One script now, seed/scripts/sync-skills.sh: the skills/<name>/ layout, a manifest per publishing source (.managed-skills.<source>), removal only of names this source listed and no longer ships and no other source claims, a refusal to publish a name another source owns, no first-run fallback, and the legacy .managed-skills rewritten as a comment-only tombstone so an un-upgraded sibling deletes nothing. The cli repo's own scripts/sync-skills.sh execs it as source `cli`; the composite action runs it from its checkout. seed/scripts/test-sync-skills.sh runs the script as hey-cli and basecamp-cli in turn against a throwaway target and is part of make check in both this repo and the seed.
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. |
![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: f08ae6bd52
ℹ️ 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.
🟡 Changes recommended
Unresolved critical and moderate findings affect retry safety, ownership protection, push destinations, cleanup, and target integrity.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Centralizes skills synchronization with per-source manifests, the skills/<name>/ layout, and expanded integration coverage.
Changes:
- Replaces duplicated sync scripts with the seed implementation and wrappers.
- Adds ownership manifests, collision protection, tombstone migration, and filtering.
- Wires tests into Makefiles and CI; updates workflows and documentation.
File summaries
| File | Reviewed changes |
|---|---|
seed/scripts/test-sync-skills.sh |
Multi-source integration tests; nit (1 vote): documented mutant checks are not executed. |
seed/scripts/sync-skills.sh |
Canonical sync implementation; unresolved critical findings cover retry safety (2 votes), push-URL validation (1), and pipefail ownership checks (1); moderate findings cover dirty targets (2), empty-source cleanup (1), and -n manifest/source emission (1 each). |
seed/Makefile |
Wires sync tests into seed checks. |
seed/.github/workflows/test.yml |
Adds seed sync testing to CI. |
seed/.github/workflows/release.yml |
Updates seed release validation and sync invocation. |
scripts/sync-skills.sh |
Thin wrapper using the repository’s sync identity. |
README.md |
Documents the action and skills synchronization. |
prompts/seed-cli.md |
Updates generated seed files and test instructions. |
Makefile |
Adds repository sync test integration. |
AGENTS.md |
Documents skills-sync architecture and guidance. |
actions/sync-skills/action.yml |
Delegates action execution to the seed implementation. |
.github/workflows/test.yml |
Adds sync coverage to repository CI. |
.github/workflows/sensitive-change-gate.yml |
Extends sensitive-change protection to the canonical script. |
.github/workflows/release.yml |
Removes redundant CLI identity configuration. |
Review details
Suppressed comments (4)
seed/scripts/sync-skills.sh:154
- An empty
skillstree is rejected before the stale-manifest cleanup can run. Both release workflows gate this script onls skills/*/SKILL.md, so deleting a source's last skill will skip the sync and leave its previously published directory and manifest entry forever. Permit an existing-but-empty source tree to run the removal path (while still failing for a missing source), and update the workflow precheck.
[[ ${#skill_names[@]} -gt 0 ]] || die "no skills found under ${SKILLS_SOURCE}/*/SKILL.md"
seed/scripts/sync-skills.sh:96
plain_namepermits skill names such as-n, butecho "$line"treats those as echo options instead of emitting the manifest entry. As a result, ownership checks and stale-removal logic can miss a valid-nentry and overwrite another source's skill. Emit manifest lines withprintf '%s\n' "$line"instead.
if plain_name "$line"; then
echo "$line"
seed/scripts/sync-skills.sh:111
plain_namealso permits a source name such as-n;echo "$other"then emits an empty value, soclaimed_by_otherreports a collision with a blank source and produces an unusable manifest filename in its warning/error path. Useprintf '%s\n' "$other"when returning the source name.
if read_manifest "$file" | grep -qxF -- "$name"; then
echo "$other"
seed/scripts/test-sync-skills.sh:267
- The description says this test runs three deliberate mutants, but the test only invokes the supplied
SYNC_SCRIPT; it never copies or modifies a mutant or executes a second script. A regression that removes the tombstone, collision guard, or dot-directory filter would therefore not fail these assertions. Add the mutation checks or remove that claim.
# --- Verdict ---
echo ""
if [[ "$failures" -eq 0 ]]; then
echo "sync-skills: all assertions passed"
- Files reviewed: 14/14 changed files
- Comments generated: 4
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A fresh clone whose push loses a race to a sibling's release is rejected as "fetch first", not "non-fast-forward", so the retry inherited from hey-cli's script never ran; match both, and give the temp gitconfig the bot identity so the rebase in the retry has a committer. The test now races a sibling against a local bare origin and asserts the retry lands. A SKILLS_TARGET checkout with uncommitted changes is refused, since `git add -A` would sweep them into the sync commit. The remote-URL assert reads the configured URL rather than the insteadOf-rewritten one, which is also what lets the test route pushes to the bare repo. The header names the one rollout case the tombstone leaves behind: a skill a still-pre-fix sibling drops stays in the target until removed by hand.
This was referenced Sep 13, 2026
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcd18c7857
ℹ️ 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".
Rebasing the already-made commit replayed decisions taken against a stale tree: a sibling that claimed a name in the meantime would have had it deleted or double-claimed. Now the rejected commit is dropped, the remote's tip fetched, and the sync applied afresh — collision guard included — before the one retry. The race test asserts that a sibling claiming a name this source ships makes the retry refuse. The remote assert covers a separate push URL, the collision lookup reads each manifest into a variable rather than through a pipe grep -q can close early under pipefail, and the composite action keeps its old no-skills no-op in front of the now strict script.
jeremy
added a commit
to basecamp/fizzy-cli
that referenced
this pull request
Sep 13, 2026
The seed (basecamp/cli#78) went further than the hey-cli copy taken first: a rejected push re-applies the sync from the remote's new tip (matching git's "fetch first" as well as "non-fast-forward"), a name another source's manifest holds is refused before anything is copied, SYNC_SOURCE and DRY_RUN are validated, a dirty checkout is refused, and the token reaches git through a private config rather than the remote URL. basecamp-cli and hey-cli are converging on the same file, so every publisher runs one script; only the CLI_NAME default differs here. The seed's test-sync-skills.sh replaces the bash port of the BATS file: it covers the same ownership cases plus the racing publisher, the publish-side refusal, DRY_RUN=remote and the dirty-tree guard.
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.
jeremy
added a commit
to basecamp/basecamp-cli
that referenced
this pull request
Sep 13, 2026
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17c383af63
ℹ️ 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".
…ards SKILLS_TARGET let the script adopt an existing checkout of basecamp/skills. It existed so the test could run the sync against a prepared history, and it was offered to operators as a convenience, but every release path clones its own target. Each review round then found another corner of "any checkout": a dirty tree, a fork as the remote, a fork-style push URL, several URLs, an insteadOf rewrite, unpublished local commits, a linked worktree. Six guards for a seam production never uses is a sign the seam is the problem. 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. The only commit it can push is the one it made, against the tip it cloned or fetched, so the remote-URL, branch and clean-tree asserts have nothing left to check and are gone with the knob. The retry after a rejected push, the per-source manifests, the collision guard, the tombstone and DRY_RUN validation are unchanged. DRY_RUN=local is now only the offline preview; DRY_RUN=remote still clones and stops before committing. The test points SKILLS_REPO_URL at a local bare repository and reads every result back from a clone of its own, so each case is a real clone, commit and push. The race is staged with a post-commit hook reached through the GIT_CONFIG_* environment, which pushes the sibling's commit between the script's clone and its push; a script that does not retry "fetch first", or that replays the stale commit instead of applying the sync again from the fetched tip, fails those cases. A case also proves the default target is refused without a token before anything is cloned.
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.
jeremy
added a commit
to basecamp/fizzy-cli
that referenced
this pull request
Sep 14, 2026
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.
jeremy
added a commit
to basecamp/hey-cli
that referenced
this pull request
Sep 14, 2026
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.
jeremy
added a commit
to basecamp/basecamp-cli
that referenced
this pull request
Sep 14, 2026
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.
There was a problem hiding this comment.
🟡 Changes recommended
Empty-set cleanup and production remote validation remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
Every case set SYNC_SOURCE to play hey-cli or basecamp-cli, so the one line each CLI's 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 expected source: a CLI's Makefile passes its own name in EXPECTED_SOURCE, the seed reads its CLI_NAME line. A copy left at the seed's placeholder, or naming another CLI, fails here instead of publishing under that source's manifest and identity. The three ways of running the script now go through one run_sync, which is where the release identifiers and the origin URL live.
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved cleanup and remote/branch safety issues remain.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
seed/scripts/sync-skills.sh:144
- Rejecting an empty skill set prevents the manifest cleanup path from handling a source that drops its last skill. The release workflows and the action also skip this script when no
skills/*/SKILL.mdexists, so the last published directory and.managed-skills.<source>remain forever; allow an empty set and run the sync when an existing manifest needs cleanup.
seed/scripts/sync-skills.sh:214 SKILLS_REPO_URLis used directly for the clone and later push, but the script never verifies that the remote is the intendedbasecamp/skillstarget or that the checked-out branch ismain. A typo or misconfigured workflow can therefore send the release token and commit to another repository while the output still saysbasecamp/skills; restore the remote/branch safety assertions while allowing the documented local test URL.
actions/sync-skills/action.yml:53
- This early return also matches a source that previously published skills but has now removed its last one, so the action never invokes manifest cleanup and leaves those skills in
basecamp/skillsindefinitely. Do not skip an empty source when it has prior ownership; let the sync handle an empty set or provide an explicit cleanup path.
if ! compgen -G "${SKILLS_SOURCE}/*/SKILL.md" > /dev/null; then
echo "No skill files found under ${SKILLS_SOURCE}/ — skipping sync"
exit 0
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
… rewrite 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 (GIT_TRACE=1 shows it). 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. A bogus token is rejected with "Authentication failed", so the header is sent and honored. 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.
jeremy
added a commit
to basecamp/hey-cli
that referenced
this pull request
Sep 14, 2026
… 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.
jeremy
added a commit
to basecamp/fizzy-cli
that referenced
this pull request
Sep 14, 2026
… 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0649a4d5e9
ℹ️ 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".
Member
Author
|
Review threads: 3 resolved (0 fixed, 3 declined with the reasoning in each thread). Declined:
Also in 0649a4d: the three fixes basecamp-cli#708 landed during its own review, ported into the seed so the copies stay byte-identical — the token now reaches git as a github.com-scoped |
Member
Author
|
Review threads: 2 more resolved (0 fixed, 2 declined with the reasoning in each thread) — 17 of 17 resolved. Declined:
Bots on 0649a4d: Codex reviewed it (the two threads above). Copilot last reviewed 19b52ed and has not reported on 0649a4d 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 19b52ed is the |
jeremy
added a commit
to basecamp/basecamp-cli
that referenced
this pull request
Sep 14, 2026
* Sync only the skills this CLI owns to basecamp/skills 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. * Check every URL the target remote could push to 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. * Always clone the skills target fresh; retire SKILLS_TARGET and its guards 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. * Say what the private gitconfig replaces, and what it does not 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. * Skip the release-time skills sync for anything but the latest stable 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. * Run the sync as the source its CLI_NAME default names 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. * Clear any inherited SKILLS_TOKEN before the sync test runs * Put the remote dry-run heading back on the target it describes * Say which skills the legacy-manifest tombstone shields, and which it cannot * Send the skills token as a github.com-scoped Authorization header, not a URL rewrite
jeremy
added a commit
to basecamp/fizzy-cli
that referenced
this pull request
Sep 14, 2026
* Publish the fizzy skill to basecamp/skills on release basecamp/skills is becoming the skills repo for every 37signals product, and Fizzy was the one CLI not publishing there. Stable releases now mirror skills/ into it with the same sync-skills.sh basecamp-cli and hey-cli run, which owns only this CLI's skills via a per-source manifest (.managed-skills.fizzy-cli) and never touches a sibling's (basecamp/skills#5). The job is continue-on-error and files a skills-sync issue on failure; sync-skills.yml is the manual recovery. The sibling repos' BATS test is ported to plain bash, since this repo's toolchain has no BATS, and runs as make test-scripts (part of make check) and in CI. * Name the pre-fix sibling test after what it tests * Converge on the seed's sync-skills.sh and its test The seed (basecamp/cli#78) went further than the hey-cli copy taken first: a rejected push re-applies the sync from the remote's new tip (matching git's "fetch first" as well as "non-fast-forward"), a name another source's manifest holds is refused before anything is copied, SYNC_SOURCE and DRY_RUN are validated, a dirty checkout is refused, and the token reaches git through a private config rather than the remote URL. basecamp-cli and hey-cli are converging on the same file, so every publisher runs one script; only the CLI_NAME default differs here. The seed's test-sync-skills.sh replaces the bash port of the BATS file: it covers the same ownership cases plus the racing publisher, the publish-side refusal, DRY_RUN=remote and the dirty-tree guard. * Always clone the skills target fresh; retire SKILLS_TARGET and its guards 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. * Say what the private gitconfig replaces, and what it does not 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. * Skip the release-time skills sync for anything but the latest stable 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. * Run the sync as the source its CLI_NAME default names 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, fizzy-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. * Send the token as a github.com-scoped Authorization header, not a URL 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. * Say that make check runs the skills sync test too
jeremy
added a commit
to basecamp/hey-cli
that referenced
this pull request
Sep 14, 2026
* Sync only the skills this CLI owns to basecamp/skills 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. * Always clone the skills target fresh; retire SKILLS_TARGET and its guards 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. * Say what the private gitconfig replaces, and what it does not 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. * Skip the release-time skills sync for anything but the latest stable 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. * Run the sync as the source its CLI_NAME default names 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. * Send the token as a github.com-scoped Authorization header, not a URL 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.
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.
Fixes the seed's skills sync so future CLIs inherit the fix for basecamp/skills#5 — the hey-cli bot deleting the Basecamp skills and the basecamp-cli bot deleting
hey, on alternate releases.Two ways the seed had drifted from reality
<cli>/<skill>/at the target root. That layout was never adopted: github.com/basecamp/skills holdsskills/<name>/, which is whatnpx skills add basecamp/skillsreads and what basecamp-cli's and hey-cli's real scripts write. The seed now writesskills/<name>/..managed-skills. The real CLIs' scripts (which had diverged from the seed) put bare names in that same file, and each one deletes every name not its own — that is Add CI automation: benchmarks, AI labeler, path labeler #5. And the seed's removal listed the<cli>/directory, so with theskills/<name>layout it needs a manifest to know what it owns at all.The design
Each publishing source owns
.managed-skills.<source>at the target root (hey-cli,basecamp-cli,clifor this repo), one sorted name per line. Askills/<name>directory is removed only when this source's manifest lists it, its current skill set no longer has it, and no other source's manifest claims it — a name two sources claim gets a warning and is left alone, and the script refuses outright to publish a name another source's manifest holds (a collision to settle upstream, never one a release resolves by clobbering). With no manifest yet the first run removes nothing; the old "no manifest ⇒ everyskills/*is mine" fallback is gone. The legacy.managed-skillsis rewritten on every run as a comment-only tombstone: the pre-fix script skips lines it cannot parse as a skill name but treats a missing file as licence to own everything, so the tombstone is what makes the rollout order irrelevant.Identity derives from
SYNC_SOURCE(default<CLI_NAME>-cli): the manifest name,<source>[bot], andSync skills from <source> <tag>. The safety asserts (remote URL, branch), the copy filter (no*.go, no dotfiles),SKILLS_SOURCEand bothDRY_RUNmodes come from hey-cli's script; the token reaches git as a github.com-scopedhttp.extraheaderin a private mode-600 temp gitconfig rather than in the clone URL or aninsteadOfrewrite (which git expands into git-remote-https's argv). The push retry changed shape in review: a rejected push (fetch first— what a fresh clone actually gets — ornon-fast-forward) drops the commit, fetches the remote's tip and applies the whole sync again against it, collision guard included, rather than rebasing decisions made against a stale tree. The script always clones the target fresh — fromSKILLS_REPO_URL, default basecamp/skills — and pushes only the commit it made, so there is no checkout to hand it; the test pointsSKILLS_REPO_URLat a local bare repository, so it exercises real clones, commits and pushes with no network.DRY_RUN=localpreviews the copy offline against an empty tree;DRY_RUN=remoteclones the real target and stops before committing.One script, not three
seed/scripts/sync-skills.shis the implementation.scripts/sync-skills.sh(this repo's own, forskills/rubric-audit) is a thin wrapper that execs it withSYNC_SOURCE=cli. A copy would recreate the drift this PR removes.actions/sync-skills/action.ymlruns it via${{ github.action_path }}/../../seed/scripts/sync-skills.shinstead of carrying an inlined third copy. Nothing consumes this action — not hey-cli, basecamp-cli, fizzy-cli or the seed's ownrelease.yml(allrun: scripts/sync-skills.shdirectly), and a GitHub code search forbasecamp/cli/actions/sync-skillsfinds no uses. Keeping it is now cheap (a mapping of inputs to env vars), but deleting it would be the more honest change; happy to drop it here or in a follow-up.Test
seed/scripts/test-sync-skills.shbuilds a throwawaybasecamp/skillsin the state #5 left it (skills/basecamp,skills/basecamp-doctor, legacy.managed-skillslisting them) and two fixture trees (hey-cli with a nested file, a*.go, a dotfile and a dot-directory that must not be copied; basecamp-cli), then runs the script interleaved — A, B, A, B — asserting after each run that both sources' skills are present, the manifests list exactly their own names,.managed-skillsis the tombstone, and the author is<source>[bot]. Then: hey-cli drops a skill (only that directory goes); a pre-fix sibling rewrites the legacy manifest (nothing of B's goes); both manifests claimbasecamp(survives, with the warning); hey-cli ships a skill namedbasecamp(refused, target untouched);DRY_RUN=remoteand tokenlessDRY_RUN=local; a github.com target without a token, refused before anything is cloned; a real push racing a sibling against a local bare origin (retry lands), the same race where the sibling claims a name this source ships (retry refuses), and the sibling dropping that claim (the next release publishes). Three deliberate mutants of the script (tombstone dropped, collision guard dropped, dot-directory filter dropped) each fail exactly the assertions aimed at them.Wired into
make check/make check-allhere and in the seed Makefile, the Test job in.github/workflows/test.yml, and the seed's test and release workflows.prompts/seed-cli.mdcopies the test alongside the script.Also
.github/workflows/sensitive-change-gate.ymlnow gatesseed/scripts/sync-skills.shtoo, since that is where the push logic lives.CLI_NAME: cliis dropped from this repo's release workflow — the wrapper owns the identity (it would otherwise have read ascli-cli).prompts/seed-cli.md. RUBRIC.md 3A.5 still points atscripts/sync-skills.sh, which stays right for a generated CLI.basecamp-cli and hey-cli are getting the same script in sibling PRs (links to follow).