Seed the skills sync with per-source manifests and the skills/ layout - #78
Open
jeremy wants to merge 5 commits into
Open
Seed the skills sync with per-source manifests and the skills/ layout#78jeremy wants to merge 5 commits into
jeremy wants to merge 5 commits into
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.
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 stays in a private temp gitconfiginsteadOfrewrite (the seed's approach) rather than in the clone URL. 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.SKILLS_TARGET=<checkout>syncs into an existing checkout instead of cloning (refused if dirty, or if its fetch or push URL is not basecamp/skills), which is what the test uses.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 real push racing a sibling against a local bare origin (retry lands), and the same race where the sibling claims a name this source ships (retry refuses); a dirty checkout; and the remote-URL, push-URL and branch asserts. 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).