From e967fb5922a6530a38af4506ac0a0d4ebbca7657 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 17:09:19 -0700 Subject: [PATCH 1/6] Sync only the skills this CLI owns to basecamp/skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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., and reads only that file to decide what to remove. A skills/ 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. --- .github/workflows/release.yml | 2 +- .github/workflows/sync-skills.yml | 10 +- .github/workflows/test.yml | 3 + Makefile | 11 +- RELEASING.md | 27 ++- scripts/sync-skills.sh | 391 ++++++++++++++++++++---------- scripts/test-sync-skills.sh | 326 +++++++++++++++++++++++++ tests/e2e/sync_skills.bats | 4 +- 8 files changed, 634 insertions(+), 140 deletions(-) create mode 100755 scripts/test-sync-skills.sh diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index da12acce..03e42384 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -66,7 +66,7 @@ jobs: - name: Run release quality gate run: | - make check-release-lockstep fmt-check vet lint test test-e2e tidy-check check-surface + make check-release-lockstep fmt-check vet lint test test-e2e test-sync-skills tidy-check check-surface go test -race -count=1 ./... - name: Run govulncheck diff --git a/.github/workflows/sync-skills.yml b/.github/workflows/sync-skills.yml index 4a387664..cb8b91af 100644 --- a/.github/workflows/sync-skills.yml +++ b/.github/workflows/sync-skills.yml @@ -6,8 +6,10 @@ # needs a way back without cutting a new tag. This is it. # # scripts/sync-skills.sh mirrors the skills/ tree at the given ref into -# basecamp/skills and no-ops when the content already matches, so re-running it -# for an already-synced release is safe. +# basecamp/skills — only this CLI's skills, tracked in its own manifest there +# (.managed-skills.hey-cli); other CLIs' skills are never touched — and no-ops +# when the content already matches, so re-running it for an already-synced +# release is safe. name: Sync skills on: @@ -53,8 +55,8 @@ jobs: exit 1 fi - # sync-skills.sh mirrors the tree wholesale, so syncing an older tag - # would roll basecamp/skills back. The only reason to run this by + # sync-skills.sh mirrors this CLI's skills wholesale, so syncing an + # older tag would roll them back in basecamp/skills. The only reason to run this by # hand is that the newest release failed to sync — so require exactly # that release. latest=$(gh release view --repo "${GITHUB_REPOSITORY}" --json tagName --jq .tagName) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 0cc80074..e1e1a812 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -171,6 +171,9 @@ jobs: - name: Run bats suite run: make test-e2e + - name: Test the skills sync + run: make test-sync-skills + installer-bash32: name: Installer (bash 3.2) runs-on: macos-latest diff --git a/Makefile b/Makefile index c6f7e56c..7ac2316b 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build test test-unit test-e2e test-smoke preview-callback coverage fmt fmt-check vet lint tidy tidy-check \ +.PHONY: build test test-unit test-e2e test-sync-skills test-smoke preview-callback coverage fmt fmt-check vet lint tidy tidy-check \ race-test vuln gosec secrets replace-check check-toolchain check security \ release-check release test-release bench bench-save bench-compare \ check-surface update-surface check-surface-compat check-size check-lint-lockstep \ @@ -25,6 +25,7 @@ help: @echo " make test-unit Run unit tests" @echo " make test Alias for test-unit" @echo " make test-e2e Run the bats suite (installer and script contracts)" + @echo " make test-sync-skills Run the skills sync as two CLIs against a throwaway target" @echo " make test-smoke Run smoke tests against a live server" @echo " make preview-callback Preview the OAuth callback screens in a browser" @echo " make coverage Run cross-package coverage and enforce the 70.8% floor" @@ -43,7 +44,7 @@ help: @echo " make secrets Run gitleaks secret scan" @echo " make replace-check Guard against replace directives in go.mod" @echo "" - @echo " make check fmt-check + vet + lint + test-unit + tidy-check" + @echo " make check fmt-check + vet + lint + test-unit + test-sync-skills + tidy-check" @echo " make security lint + vuln + gosec + secrets" @echo " make release-check check + replace-check + vuln + gosec + race-test" @echo " make release Run release preflight and tag (VERSION=v1.2.3 [DRY_RUN=1])" @@ -103,6 +104,10 @@ preview-callback: check-toolchain test-e2e: @./tests/e2e/run.sh +# Run the skills sync against a throwaway basecamp/skills, as two CLIs publishing in turn +test-sync-skills: + scripts/test-sync-skills.sh + # Run smoke tests against a live HEY server. # Requires: a running server (default http://app.hey.localhost:3003) and Chrome. # Override defaults: make test-smoke HEY_SMOKE_BASE_URL=... HEY_SMOKE_EMAIL=... HEY_SMOKE_PASSWORD=... @@ -171,7 +176,7 @@ replace-check: fi # Local CI gate -check: fmt-check vet lint test-unit tidy-check check-surface check-release-lockstep +check: fmt-check vet lint test-unit test-sync-skills tidy-check check-surface check-release-lockstep # Verify every workflow lints with the same golangci-lint version check-lint-lockstep: diff --git a/RELEASING.md b/RELEASING.md index 13d41d65..dde9cba3 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -216,13 +216,26 @@ account, store the private key as `AUR_KEY`. ## Skills sync -Stable releases mirror `skills/` into `basecamp/skills`. If that job fails, a -`skills-sync`-labeled issue is filed; recover with the `Sync skills` workflow -(`workflow_dispatch`, stable tag, optional dry run). It refuses anything but the -latest stable release so it cannot roll the distribution repo back, and it runs -the sync script from the dispatching branch against the tag's skills tree — so -when the failure was a defect in `sync-skills.sh` itself, merge the fix to main -and dispatch; no new release needed. +Stable releases mirror `skills/` into [basecamp/skills](https://github.com/basecamp/skills), +which several CLIs share. `scripts/sync-skills.sh` owns only this CLI's skills there: +it records the names it published in `.managed-skills.hey-cli` at the target root and +removes a `skills/` only when that manifest lists it, the release no longer ships +it, and no other CLI's `.managed-skills.*` claims it (a collision is warned about and +left alone), and refuses outright to publish a name another CLI's manifest holds. A +target with no `.managed-skills.hey-cli` yet is a first run: nothing is removed. A push +rejected because another CLI published first is retried by applying the whole sync +again from the remote's new tip, not by replaying the stale commit. The legacy shared `.managed-skills` is rewritten as a comment-only tombstone +so a CLI still on the pre-fix script — which deleted everything its own tree lacked — +deletes nothing (basecamp/skills#5). `scripts/test-sync-skills.sh` (`make test-sync-skills`, +in `make check`) pins the contract by running the script as both CLIs against a +throwaway target. + +If that job fails, a `skills-sync`-labeled issue is filed; recover with the +`Sync skills` workflow (`workflow_dispatch`, stable tag, optional dry run). It +refuses anything but the latest stable release so it cannot roll the distribution +repo back, and it runs the sync script from the dispatching branch against the +tag's skills tree — so when the failure was a defect in `sync-skills.sh` itself, +merge the fix to main and dispatch; no new release needed. ## Local dry runs diff --git a/scripts/sync-skills.sh b/scripts/sync-skills.sh index d44a63bc..2a912f78 100755 --- a/scripts/sync-skills.sh +++ b/scripts/sync-skills.sh @@ -1,42 +1,139 @@ #!/usr/bin/env bash -set -euo pipefail - -# Sync skills from hey-cli to basecamp/skills distribution repo. +# sync-skills.sh — Publish this CLI's skills to the basecamp/skills distribution repo. +# +# Runs from CI on a release tag. Mirrors each skills// tree (SKILL.md and its +# supporting files; no *.go, no dotfiles) into skills// at the root of +# basecamp/skills — the layout `npx skills add basecamp/skills` reads — then commits +# as [bot] and pushes. +# +# Several CLIs publish into that one repo, so each owns a manifest of its own at the +# target root, .managed-skills., listing the skill names it has published, +# one per line. A skills/ directory is removed only when all of these hold: +# this source's manifest lists it, this source's current skill set no longer has +# it, and no other source's manifest claims it. A name two sources claim is a +# collision to settle upstream, never one a release resolves by deletion — the +# script warns and leaves the directory, and refuses outright to publish a name +# another source's manifest holds. Nothing else in the target is ever deleted: with +# no manifest yet, the first run publishes and removes nothing. # -# Env vars: -# SKILLS_TOKEN - GitHub token with push access to basecamp/skills (required unless DRY_RUN=local) -# RELEASE_TAG - Release tag, e.g. v1.2.3 (required) -# SOURCE_SHA - Source commit SHA (required) -# SKILLS_SOURCE - Optional: directory holding the skills tree (default: skills) -# DRY_RUN - Optional: "local" (no network) or "remote" (clone but skip push) +# The shared manifest the pre-fix scripts kept, .managed-skills, is rewritten on +# every run as a comment-only tombstone. The pre-fix script skips any line it cannot +# parse as a skill name, but treats a missing file as licence to own every skills/* +# directory — so the tombstone is what stops an un-upgraded sibling from deleting +# anyone's skills, whichever CLI upgrades first (basecamp/skills#5). One case is +# accepted: a skill a still-pre-fix sibling drops after the tombstone exists stays +# behind in the target (that script has no names left to delete by, and the +# sibling's own first run here removes nothing) — a lingering directory to remove +# by hand, which beats guessing ownership from the legacy file. +# +# Required env vars: +# RELEASE_TAG — the release tag (e.g. v1.2.3) +# SOURCE_SHA — the source commit SHA +# SKILLS_TOKEN — GitHub token with push access to basecamp/skills; not needed +# for DRY_RUN=local, nor when SKILLS_TARGET is set +# +# Optional env vars: +# CLI_NAME — this CLI's name; the publishing source is -cli +# SYNC_SOURCE — the publishing repo's name (default: -cli). Names the +# manifest, the bot and the commit; the test sets it to play +# another CLI +# SKILLS_SOURCE — directory holding the skills tree (default: skills). A manual +# recovery workflow can point it at a checkout of the release tag +# so the sync logic comes from a newer ref than the content +# SKILLS_TARGET — an existing checkout of basecamp/skills to sync into instead of +# cloning; the remote-URL and branch asserts still run against it +# DRY_RUN — "local": no network. Without SKILLS_TARGET, copy into an empty +# tmpdir and print what would be published; with it, apply and +# commit there but do not push. +# "remote": clone (or use SKILLS_TARGET), apply, print the diff, +# and stop before committing +# + +set -euo pipefail +CLI_NAME="${CLI_NAME:-hey}" +SYNC_SOURCE="${SYNC_SOURCE:-${CLI_NAME}-cli}" RELEASE_TAG="${RELEASE_TAG:?RELEASE_TAG is required}" SOURCE_SHA="${SOURCE_SHA:?SOURCE_SHA is required}" +SKILLS_SOURCE="${SKILLS_SOURCE:-skills}" +SKILLS_TARGET="${SKILLS_TARGET:-}" +SKILLS_TOKEN="${SKILLS_TOKEN:-}" DRY_RUN="${DRY_RUN:-}" -# The skills tree to mirror. The manual recovery workflow (sync-skills.yml) -# points this at a separate checkout of the release tag, so the sync logic can -# come from a newer ref (with fixes) than the content it mirrors. -SKILLS_SOURCE="${SKILLS_SOURCE:-skills}" TARGET_REPO="basecamp/skills" TARGET_BRANCH="main" SKILLS_SUBDIR="skills" +LEGACY_MANIFEST=".managed-skills" +MANIFEST="${LEGACY_MANIFEST}.${SYNC_SOURCE}" +# The commit's provenance line; GITHUB_REPOSITORY is exact in CI, the default holds +# for the basecamp org's naming. +SOURCE_REPO="${GITHUB_REPOSITORY:-basecamp/${SYNC_SOURCE}}" # --- Helpers --- die() { echo "ERROR: $*" >&2; exit 1; } +warn() { echo "WARNING: $*" >&2; } + +# A skill directory name or a source name: nothing a path could smuggle in. +plain_name() { + [[ "$1" != "." && "$1" != ".." && "$1" =~ ^[a-zA-Z0-9._-]+$ ]] +} + +in_list() { + local needle="$1" item + shift + for item in "$@"; do + [[ "$item" == "$needle" ]] && return 0 + done + return 1 +} +# Print the skill names a manifest lists, one per line. Blank and comment lines +# are skipped silently; anything else that is not a plain name, with a warning. +read_manifest() { + local file="$1" line + [[ -f "$file" ]] || return 0 + while IFS= read -r line || [[ -n "$line" ]]; do + [[ -z "$line" || "$line" == \#* ]] && continue + if plain_name "$line"; then + echo "$line" + else + warn "skipping invalid entry in ${file##*/}: $line" + fi + done < "$file" +} + +# Print the other source whose manifest claims a name, if any. +claimed_by_other() { + local name="$1" file other listed + for file in "${target}/${LEGACY_MANIFEST}".*; do + [[ -f "$file" ]] || continue + other="${file##*/"${LEGACY_MANIFEST}".}" + [[ "$other" == "$SYNC_SOURCE" ]] && continue + listed=$(read_manifest "$file") + if grep -qxF -- "$name" <<< "$listed"; then + echo "$other" + return 0 + fi + done + return 1 +} + +# The URLs as configured: `remote get-url` would show them after any insteadOf +# rewrite, so an operator's rewrite could pass this check with a repo that is not +# the target. A push URL of its own is where `git push origin` would actually go. assert_remote_url() { - local url - url=$(git -C "$1" remote get-url origin) - local stripped - stripped=$(echo "$url" | sed -E 's/\.git$//') - case "$stripped" in - https://github.com/"$TARGET_REPO") ;; - https://x-access-token:*@github.com/"$TARGET_REPO") ;; - git@github.com:"$TARGET_REPO") ;; - *) die "origin remote '$(echo "$url" | sed -E 's#(https://[^:@]+:)[^@]*@#\1***@#')' does not point to github.com/$TARGET_REPO" ;; - esac + local kind url + for kind in url pushurl; do + url=$(git -C "$1" config --get "remote.origin.${kind}" || true) + [[ -z "$url" && "$kind" == pushurl ]] && continue + case "${url%.git}" in + "https://github.com/${TARGET_REPO}") ;; + https://x-access-token:*@github.com/"${TARGET_REPO}") ;; + "git@github.com:${TARGET_REPO}") ;; + *) die "origin ${kind} '$(echo "$url" | sed -E 's#(https://[^:@]+:)[^@]*@#\1***@#')' does not point to github.com/${TARGET_REPO}" ;; + esac + done } assert_branch() { @@ -45,128 +142,168 @@ assert_branch() { [[ "$branch" == "$TARGET_BRANCH" ]] || die "checked-out branch is '$branch', expected '$TARGET_BRANCH'" } +# --- Validate the knobs --- + +plain_name "$SYNC_SOURCE" || die "SYNC_SOURCE '$SYNC_SOURCE' is not a plain name" +case "$DRY_RUN" in + ""|local|remote) ;; + *) die "DRY_RUN must be unset, 'local' or 'remote', not '$DRY_RUN'" ;; +esac + # --- Discover skills --- -skill_dirs=() +skill_names=() for skill_md in "$SKILLS_SOURCE"/*/SKILL.md; do [[ -f "$skill_md" ]] || continue - skill_dirs+=("$(dirname "$skill_md")") + name=$(basename "$(dirname "$skill_md")") + plain_name "$name" || die "skill directory '$name' is not a plain name" + skill_names+=("$name") done -[[ ${#skill_dirs[@]} -gt 0 ]] || die "no skills found under $SKILLS_SOURCE/*/SKILL.md" -echo "Found ${#skill_dirs[@]} skill(s): ${skill_dirs[*]}" +[[ ${#skill_names[@]} -gt 0 ]] || die "no skills found under ${SKILLS_SOURCE}/*/SKILL.md" +echo "Found ${#skill_names[@]} skill(s) in ${SKILLS_SOURCE}/: ${skill_names[*]}" -# --- Copy skills into target, excluding *.go and dotfiles --- +# --- Copy skills, excluding *.go and dotfiles, preserving subdirectories --- copy_skills() { - local target_dir="$1" - for skill_dir in "${skill_dirs[@]}"; do - local name - name=$(basename "$skill_dir") - rm -rf "${target_dir:?}/${name}" - mkdir -p "$target_dir/$name" - find "$skill_dir" -mindepth 1 \ - ! -name '*.go' \ - ! -name '.*' \ - ! -path '*/.*' \ - -type f \ - -exec bash -c ' - src="$1"; skill_dir="$2"; target_dir="$3" - rel="${src#$skill_dir/}" - mkdir -p "$(dirname "$target_dir/$rel")" - cp "$src" "$target_dir/$rel" - ' _ {} "$skill_dir" "$target_dir/$name" \; + local skills_dir="$1" name dest + for name in "${skill_names[@]}"; do + dest="${skills_dir}/${name}" + rm -rf "${dest:?}" + mkdir -p "$dest" + (cd "${SKILLS_SOURCE}/${name}" && find . -type f ! -name '*.go' ! -name '.*' ! -path '*/.*/*' -print0) | + while IFS= read -r -d '' file; do + mkdir -p "${dest}/$(dirname "$file")" + cp "${SKILLS_SOURCE}/${name}/${file}" "${dest}/${file}" + done done } -# --- DRY_RUN=local: copy into tmpdir, diff against empty baseline --- +tmpdir=$(mktemp -d) +trap 'rm -rf "$tmpdir"' EXIT + +# --- DRY_RUN=local without a target: what would be published --- -if [[ "$DRY_RUN" == "local" ]]; then - tmpdir=$(mktemp -d) - trap 'rm -rf "$tmpdir"' EXIT - echo "DRY_RUN=local: copying skills into $tmpdir" - copy_skills "$tmpdir/$SKILLS_SUBDIR" +if [[ "$DRY_RUN" == "local" && -z "$SKILLS_TARGET" ]]; then + preview="${tmpdir}/preview" + echo "DRY_RUN=local: copying skills into ${preview}" + copy_skills "${preview}/${SKILLS_SUBDIR}" echo "" echo "=== Skills copied ===" - find "$tmpdir" -type f | sort | while read -r f; do - echo " ${f#"$tmpdir/"}" + find "$preview" -type f | LC_ALL=C sort | while read -r file; do + echo " ${file#"${preview}/"}" done echo "" echo "=== Diff (against empty baseline) ===" - git -C "$tmpdir" init -q - git -C "$tmpdir" add -A - git -C "$tmpdir" diff --cached --stat + git -C "$preview" init -q + git -C "$preview" add -A + git -C "$preview" diff --cached --stat echo "" echo "DRY_RUN=local complete. No network operations performed." exit 0 fi -# --- Clone target repo --- - -[[ -n "${SKILLS_TOKEN:-}" ]] || die "SKILLS_TOKEN is required (set DRY_RUN=local for offline testing)" - -tmpdir=$(mktemp -d) -trap 'rm -rf "$tmpdir"' EXIT - -echo "Cloning $TARGET_REPO into $tmpdir/skills..." -git clone --depth 1 --branch "$TARGET_BRANCH" \ - "https://x-access-token:${SKILLS_TOKEN}@github.com/${TARGET_REPO}.git" \ - "$tmpdir/skills" +# --- Git configuration for the target --- +# +# A private global config for every git call below: the bot is the identity for +# the commit and for the rebase a retried push needs, 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. +export GIT_CONFIG_GLOBAL="${tmpdir}/gitconfig" +cat > "$GIT_CONFIG_GLOBAL" <> "$GIT_CONFIG_GLOBAL" <&2 + for name in ${previously_published[@]+"${previously_published[@]}"}; do + in_list "$name" "${skill_names[@]}" && continue + if other=$(claimed_by_other "$name"); then + warn "skills/${name} is no longer in ${SYNC_SOURCE}'s skills but ${other} lists it in ${LEGACY_MANIFEST}.${other}; leaving it in place" continue fi - previously_managed_names+=("$entry") - done < "$target/$MANIFEST" -else - for candidate in "$target/$SKILLS_SUBDIR"/*/SKILL.md; do - [[ -f "$candidate" ]] || continue - previously_managed_names+=("$(basename "$(dirname "$candidate")")") + if [[ -d "${target}/${SKILLS_SUBDIR}/${name}" ]]; then + echo "Removing stale skill: ${name}" + rm -rf "${target:?}/${SKILLS_SUBDIR}/${name}" + fi done -fi -for previously_managed in "${previously_managed_names[@]}"; do - found=0 - for name in "${source_skill_names[@]}"; do - [[ "$name" == "$previously_managed" ]] && found=1 && break - done - if [[ "$found" -eq 0 && -d "$target/$SKILLS_SUBDIR/$previously_managed" ]]; then - echo "Removing stale skill: $previously_managed" - rm -rf "${target:?}/$SKILLS_SUBDIR/$previously_managed" - fi -done + # This source's manifest, and the legacy tombstone + printf '%s\n' "${skill_names[@]}" | LC_ALL=C sort > "${target}/${MANIFEST}" + cat > "${target}/${LEGACY_MANIFEST}" <<'TOMBSTONE' +# Superseded by the per-source manifests (.managed-skills.), one per publishing CLI. +# Each CLI deletes only the skill directories listed in its own manifest. +# Kept so a CLI still running the pre-fix sync script deletes nothing: that script skips +# every line it cannot parse as a skill name and only deletes names it can. +TOMBSTONE + + git -C "$target" add -A +} -# Write current manifest -printf '%s\n' "${source_skill_names[@]}" | sort > "$target/$MANIFEST" +commit_sync() { + git -C "$target" commit -q -m "$(cat <&1 } if ! output=$(push_target); then - if echo "$output" | grep -qi "non-fast-forward"; then - echo "Push rejected (non-fast-forward). Pulling with rebase and retrying..." - git -C "$target" pull --rebase origin "$TARGET_BRANCH" - if ! retry_output=$(push_target); then - echo "$retry_output" >&2 - die "Push failed after retry" - fi - else + if ! echo "$output" | grep -Eqi "fetch first|non-fast-forward"; then echo "$output" >&2 die "Push failed" fi + echo "Push rejected (the remote has moved). Applying the sync again from its new tip..." + git -C "$target" fetch -q origin "$TARGET_BRANCH" + git -C "$target" reset -q --hard FETCH_HEAD + apply_sync + if git -C "$target" diff --cached --quiet; then + echo "Nothing left to publish: the remote already holds these skills." + exit 0 + fi + commit_sync + if ! retry_output=$(push_target); then + echo "$retry_output" >&2 + die "Push failed after retry" + fi fi echo "" -echo "Skills synced to $TARGET_REPO ($TARGET_BRANCH) from $RELEASE_TAG" +echo "Skills synced to ${TARGET_REPO} (${TARGET_BRANCH}) from ${SYNC_SOURCE} ${RELEASE_TAG}" diff --git a/scripts/test-sync-skills.sh b/scripts/test-sync-skills.sh new file mode 100755 index 00000000..9f746893 --- /dev/null +++ b/scripts/test-sync-skills.sh @@ -0,0 +1,326 @@ +#!/usr/bin/env bash +# test-sync-skills.sh — Run sync-skills.sh as two CLIs against one throwaway +# basecamp/skills checkout and prove neither deletes the other's skills. +# +# The target starts in the state basecamp/skills#5 left it: basecamp-cli's skills +# and the shared .managed-skills listing them. Then hey-cli and basecamp-cli sync +# in turn, one loses a skill, a pre-fix sibling rewrites the legacy manifest, and +# two manifests claim one name — after each step both sources' skills must be +# where they belong. Everything runs with DRY_RUN=local and SKILLS_TARGET, so no +# network and no token. +# +# Usage: scripts/test-sync-skills.sh (tests scripts/sync-skills.sh) +# SYNC_SCRIPT=path/to/sync-skills.sh scripts/test-sync-skills.sh + +set -euo pipefail + +here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +SYNC_SCRIPT="${SYNC_SCRIPT:-${here}/sync-skills.sh}" +[[ -x "$SYNC_SCRIPT" ]] || { echo "ERROR: ${SYNC_SCRIPT} is not an executable script" >&2; exit 1; } + +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT + +# The test's own git calls see only this config; the script brings its own. +export GIT_CONFIG_GLOBAL="${work}/gitconfig" GIT_CONFIG_NOSYSTEM=1 +printf '[user]\n\tname = test\n\temail = test@example.com\n' > "$GIT_CONFIG_GLOBAL" + +target="${work}/target" +out="${work}/out" +failures=0 + +# --- Assertions --- + +ok() { echo "ok - $*"; } +not_ok() { echo "not ok - $*"; failures=$((failures + 1)); } + +assert() { # description, command... + local desc="$1" + shift + if "$@"; then ok "$desc"; else not_ok "$desc"; fi +} + +assert_skill() { assert "skills/$1 present" test -f "${target}/skills/$1/SKILL.md"; } +assert_no_skill() { assert "skills/$1 absent" test ! -e "${target}/skills/$1"; } +assert_no_path() { assert "$1 not published" test ! -e "${target}/skills/$1"; } +assert_content() { # path, expected content + assert "$1 holds '$2'" test "$(cat "${target}/$1")" = "$2" +} +assert_manifest() { # source, names... + local source="$1" want + shift + want=$(printf '%s\n' "$@") + assert ".managed-skills.${source} lists exactly: $*" test "$(cat "${target}/.managed-skills.${source}")" = "$want" +} +assert_no_manifest() { assert ".managed-skills.$1 absent" test ! -e "${target}/.managed-skills.$1"; } +assert_tombstone() { + if [[ -f "${target}/.managed-skills" ]] && ! grep -qv '^#' "${target}/.managed-skills" && grep -q 'Superseded' "${target}/.managed-skills"; then + ok ".managed-skills is the comment-only tombstone" + else + not_ok ".managed-skills is the comment-only tombstone" + fi +} +assert_author() { assert "last commit authored by $1[bot]" test "$(git -C "$target" log -1 --format=%an)" = "$1[bot]"; } +assert_clean() { assert "target working tree committed clean" test -z "$(git -C "$target" status --porcelain)"; } +assert_output() { assert "output says: $1" grep -q -- "$1" "$out"; } +assert_head() { # expected sha, description + assert "$2" test "$(git -C "$target" rev-parse HEAD)" = "$1" +} + +# --- Running the script --- + +sync() { # source, fixture, [VAR=value...] + local source="$1" fixture="$2" + shift 2 + if env "$@" SYNC_SOURCE="$source" SKILLS_SOURCE="${fixture}/skills" \ + RELEASE_TAG=v9.9.9 SOURCE_SHA=0123abcd "$SYNC_SCRIPT" > "$out" 2>&1; then + ok "sync as ${source} succeeded" + else + not_ok "sync as ${source} succeeded" + sed 's/^/ /' "$out" + fi +} + +sync_local() { sync "$1" "$2" DRY_RUN=local SKILLS_TARGET="$target"; } + +sync_expecting_failure() { # source, fixture, [VAR=value...] + local source="$1" fixture="$2" + shift 2 + if env "$@" SYNC_SOURCE="$source" SKILLS_SOURCE="${fixture}/skills" \ + RELEASE_TAG=v9.9.9 SOURCE_SHA=0123abcd "$SYNC_SCRIPT" > "$out" 2>&1; then + not_ok "sync as ${source} refused" + sed 's/^/ /' "$out" + else + ok "sync as ${source} refused" + fi +} + +# --- Fixtures --- + +write_skill() { # dir, content + mkdir -p "$1" + echo "$2" > "$1/SKILL.md" +} + +a="${work}/hey-cli" +b="${work}/basecamp-cli" + +write_skill "${a}/skills/hey" "hey v2" +mkdir -p "${a}/skills/hey/reference" "${a}/skills/hey/.cache" +echo "nested" > "${a}/skills/hey/reference/commands.md" +echo "package hey" > "${a}/skills/hey/embed.go" +echo "secret" > "${a}/skills/hey/.env" +echo "cached" > "${a}/skills/hey/.cache/index" +write_skill "${a}/skills/hey-doctor" "hey-doctor v2" + +write_skill "${b}/skills/basecamp" "basecamp v2" +write_skill "${b}/skills/basecamp-doctor" "basecamp-doctor v2" + +# The target as basecamp/skills#5 left it: only basecamp-cli's skills survive, +# and the shared manifest names them. +git init -q -b main "$target" +git -C "$target" remote add origin https://github.com/basecamp/skills.git +write_skill "${target}/skills/basecamp" "basecamp v1" +write_skill "${target}/skills/basecamp-doctor" "basecamp-doctor v1" +printf 'basecamp\nbasecamp-doctor\n' > "${target}/.managed-skills" +echo "# skills" > "${target}/README.md" +git -C "$target" add -A +git -C "$target" commit -q -m "State after basecamp/skills#5" + +# --- Interleaved syncs: A, B, A, B --- + +echo "# hey-cli syncs first: restores its skills, touches nothing else" +sync_local hey-cli "$a" +assert_skill hey +assert_skill hey-doctor +assert_skill basecamp +assert_skill basecamp-doctor +assert_content skills/basecamp/SKILL.md "basecamp v1" +assert_content skills/hey/reference/commands.md "nested" +assert_no_path hey/embed.go +assert_no_path hey/.env +assert_no_path hey/.cache +assert_manifest hey-cli hey hey-doctor +assert_no_manifest basecamp-cli +assert_tombstone +assert_author hey-cli +assert_clean +assert_output "first run for hey-cli, removing nothing" + +echo "# basecamp-cli syncs: refreshes its skills, leaves hey-cli's" +sync_local basecamp-cli "$b" +assert_skill hey +assert_skill hey-doctor +assert_skill basecamp +assert_skill basecamp-doctor +assert_content skills/basecamp/SKILL.md "basecamp v2" +assert_manifest hey-cli hey hey-doctor +assert_manifest basecamp-cli basecamp basecamp-doctor +assert_tombstone +assert_author basecamp-cli +assert_clean + +echo "# both sync again with nothing new: no commits, nothing lost" +head_before=$(git -C "$target" rev-parse HEAD) +sync_local hey-cli "$a" +assert_output "No changes to commit" +sync_local basecamp-cli "$b" +assert_output "No changes to commit" +assert_head "$head_before" "HEAD unchanged by the no-op syncs" +assert_skill hey +assert_skill hey-doctor +assert_skill basecamp +assert_skill basecamp-doctor +assert_manifest hey-cli hey hey-doctor +assert_manifest basecamp-cli basecamp basecamp-doctor +assert_tombstone + +# --- hey-cli drops a skill: only that directory goes --- + +echo "# hey-cli drops hey-doctor" +rm -rf "${a}/skills/hey-doctor" +sync_local hey-cli "$a" +assert_output "Removing stale skill: hey-doctor" +assert_no_skill hey-doctor +assert_skill hey +assert_skill basecamp +assert_skill basecamp-doctor +assert_manifest hey-cli hey +assert_manifest basecamp-cli basecamp basecamp-doctor +assert_author hey-cli + +# --- A pre-fix sibling rewrote the legacy manifest: still nothing of B's goes --- + +echo "# a pre-fix basecamp-cli rewrites .managed-skills with its own names" +printf 'basecamp\nbasecamp-doctor\n' > "${target}/.managed-skills" +git -C "$target" commit -q -am "Sync skills from basecamp-cli v0.0.0 (pre-fix script)" +sync_local hey-cli "$a" +assert_skill basecamp +assert_skill basecamp-doctor +assert_skill hey +assert_tombstone +assert_manifest basecamp-cli basecamp basecamp-doctor + +# --- Two manifests claim one name: removal is refused with a warning --- + +echo "# hey-cli's manifest also lists basecamp, which basecamp-cli owns" +printf 'basecamp\nhey\n' > "${target}/.managed-skills.hey-cli" +git -C "$target" commit -q -am "Collision: hey-cli claims basecamp" +sync_local hey-cli "$a" +assert_skill basecamp +assert_content skills/basecamp/SKILL.md "basecamp v2" +assert_output "WARNING: skills/basecamp is no longer in hey-cli's skills but basecamp-cli lists it" +assert_manifest hey-cli hey +assert_manifest basecamp-cli basecamp basecamp-doctor + +# --- Publishing a name another source owns is refused before anything changes --- + +echo "# hey-cli ships a skill named basecamp" +write_skill "${a}/skills/basecamp" "hey-cli's basecamp" +head_before=$(git -C "$target" rev-parse HEAD) +sync_expecting_failure hey-cli "$a" DRY_RUN=local SKILLS_TARGET="$target" +assert_output "ERROR: skills/basecamp is published by basecamp-cli" +assert_content skills/basecamp/SKILL.md "basecamp v2" +assert_head "$head_before" "HEAD unchanged by the refused sync" +assert_clean +rm -rf "${a}/skills/basecamp" + +# --- DRY_RUN=remote applies and shows the diff but commits nothing --- + +echo "# DRY_RUN=remote against the checkout" +echo "hey v3" > "${a}/skills/hey/SKILL.md" +head_before=$(git -C "$target" rev-parse HEAD) +sync hey-cli "$a" DRY_RUN=remote SKILLS_TARGET="$target" +assert_output "DRY_RUN=remote: skipping commit and push" +assert_output "+hey v3" +assert_head "$head_before" "HEAD unchanged by DRY_RUN=remote" +git -C "$target" reset -q --hard + +# --- DRY_RUN=local with no target: no network, lists what would be published --- + +echo "# DRY_RUN=local without SKILLS_TARGET" +sync hey-cli "$a" DRY_RUN=local +assert_output "skills/hey/SKILL.md" +assert_output "skills/hey/reference/commands.md" +assert_output "No network operations performed" +if grep -q "embed.go" "$out"; then not_ok "preview leaves out embed.go"; else ok "preview leaves out embed.go"; fi + +# --- Another publisher pushes first: the push is retried after a rebase --- + +echo "# a concurrent publisher wins the race to origin" +origin="${work}/origin.git" +git init -q --bare -b main "$origin" +git -C "$target" push -q "$origin" main +sibling="${work}/sibling" +git clone -q "$origin" "$sibling" +echo "# skills (sibling)" > "${sibling}/README.md" +git -C "$sibling" commit -q -am "Sync skills from basecamp-cli v0.0.1 (concurrent)" +git -C "$sibling" push -q origin main +echo "hey v4" > "${a}/skills/hey/SKILL.md" +# A real push, with github.com/basecamp/skills routed to the local bare repo through +# the environment — the script's private gitconfig cannot hide that. +sync hey-cli "$a" SKILLS_TARGET="$target" \ + GIT_CONFIG_COUNT=1 "GIT_CONFIG_KEY_0=url.${origin}.insteadOf" GIT_CONFIG_VALUE_0=https://github.com/basecamp/skills.git +assert_output "Push rejected" +assert_output "Skills synced to basecamp/skills" +assert "origin main holds the sibling's commit then the sync" \ + test "$(git -C "$origin" log --format=%s -2 main | tr '\n' '|')" = "Sync skills from hey-cli v9.9.9|Sync skills from basecamp-cli v0.0.1 (concurrent)|" +assert_content skills/hey/SKILL.md "hey v4" +assert_content README.md "# skills (sibling)" +assert_clean + +echo "# a concurrent publisher claims a name this source ships: the retry refuses" +git -C "$sibling" pull -q origin main +printf 'basecamp\nbasecamp-doctor\nhey\n' > "${sibling}/.managed-skills.basecamp-cli" +git -C "$sibling" commit -q -am "Collision: basecamp-cli claims hey (concurrent)" +git -C "$sibling" push -q origin main +echo "hey v5" > "${a}/skills/hey/SKILL.md" +sync_expecting_failure hey-cli "$a" SKILLS_TARGET="$target" \ + GIT_CONFIG_COUNT=1 "GIT_CONFIG_KEY_0=url.${origin}.insteadOf" GIT_CONFIG_VALUE_0=https://github.com/basecamp/skills.git +assert_output "Push rejected" +assert_output "ERROR: skills/hey is published by basecamp-cli" +assert "origin main tip is the sibling's commit" \ + test "$(git -C "$origin" log -1 --format=%s main)" = "Collision: basecamp-cli claims hey (concurrent)" +assert_content skills/hey/SKILL.md "hey v4" +assert_clean +git -C "$sibling" checkout -q HEAD~1 -- .managed-skills.basecamp-cli +git -C "$sibling" commit -q -am "basecamp-cli drops its claim on hey" +git -C "$sibling" push -q origin main +git -C "$target" fetch -q "$origin" main +git -C "$target" reset -q --hard FETCH_HEAD + +# --- Safety asserts on the checkout --- + +echo "# a checkout with uncommitted changes is refused" +echo "stray" > "${target}/stray.txt" +sync_expecting_failure hey-cli "$a" DRY_RUN=local SKILLS_TARGET="$target" +assert_output "has uncommitted changes" +rm "${target}/stray.txt" + +echo "# a checkout that is not basecamp/skills on main is refused" +wrong="${work}/wrong-remote" +git init -q -b main "$wrong" +git -C "$wrong" remote add origin https://github.com/basecamp/other.git +git -C "$wrong" commit -q --allow-empty -m "init" +sync_expecting_failure hey-cli "$a" DRY_RUN=local SKILLS_TARGET="$wrong" +assert_output "does not point to github.com/basecamp/skills" + +git -C "$target" remote set-url --push origin https://github.com/someone/skills.git +sync_expecting_failure hey-cli "$a" DRY_RUN=local SKILLS_TARGET="$target" +assert_output "origin pushurl 'https://github.com/someone/skills.git' does not point to github.com/basecamp/skills" +git -C "$target" config --unset remote.origin.pushurl + +git -C "$target" checkout -q -b not-main +sync_expecting_failure hey-cli "$a" DRY_RUN=local SKILLS_TARGET="$target" +assert_output "checked-out branch is 'not-main', expected 'main'" +git -C "$target" checkout -q main + +# --- Verdict --- + +echo "" +if [[ "$failures" -eq 0 ]]; then + echo "sync-skills: all assertions passed" +else + echo "sync-skills: ${failures} assertion(s) failed" >&2 + exit 1 +fi diff --git a/tests/e2e/sync_skills.bats b/tests/e2e/sync_skills.bats index 34c52c77..99215cde 100644 --- a/tests/e2e/sync_skills.bats +++ b/tests/e2e/sync_skills.bats @@ -31,7 +31,7 @@ teardown() { @test "defaults to the skills/ tree and excludes *.go and dotfiles" { run "$SYNC" [ "$status" -eq 0 ] - [[ "$output" == *"Found 1 skill(s): skills/hey"* ]] + [[ "$output" == *"Found 1 skill(s) in skills/: hey"* ]] [[ "$output" == *"skills/hey/SKILL.md"* ]] [[ "$output" == *"skills/hey/reference/api.md"* ]] [[ "$output" != *"tool.go"* ]] @@ -41,7 +41,7 @@ teardown() { @test "SKILLS_SOURCE points the sync at a tagged checkout" { SKILLS_SOURCE=release/skills run "$SYNC" [ "$status" -eq 0 ] - [[ "$output" == *"Found 1 skill(s): release/skills/hey"* ]] + [[ "$output" == *"Found 1 skill(s) in release/skills/: hey"* ]] [[ "$output" == *"skills/hey/SKILL.md"* ]] [[ "$output" != *"reference/api.md"* ]] } From 7cfcf583997852cbced55065a221afaa936c2772 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 20:13:36 -0700 Subject: [PATCH 2/6] Always clone the skills target fresh; retire SKILLS_TARGET and its guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- RELEASING.md | 7 +- scripts/sync-skills.sh | 106 +++++++------------- scripts/test-sync-skills.sh | 193 ++++++++++++++++++++---------------- 3 files changed, 144 insertions(+), 162 deletions(-) diff --git a/RELEASING.md b/RELEASING.md index dde9cba3..4d1b59af 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -226,9 +226,10 @@ target with no `.managed-skills.hey-cli` yet is a first run: nothing is removed. rejected because another CLI published first is retried by applying the whole sync again from the remote's new tip, not by replaying the stale commit. The legacy shared `.managed-skills` is rewritten as a comment-only tombstone so a CLI still on the pre-fix script — which deleted everything its own tree lacked — -deletes nothing (basecamp/skills#5). `scripts/test-sync-skills.sh` (`make test-sync-skills`, -in `make check`) pins the contract by running the script as both CLIs against a -throwaway target. +deletes nothing (basecamp/skills#5). The script always clones the target fresh and pushes +only the commit it made, so there is no checkout to hand it. `scripts/test-sync-skills.sh` +(`make test-sync-skills`, in `make check`) pins the contract by running the script as both +CLIs against a local bare repository — real clones, commits and pushes, no network. If that job fails, a `skills-sync`-labeled issue is filed; recover with the `Sync skills` workflow (`workflow_dispatch`, stable tag, optional dry run). It diff --git a/scripts/sync-skills.sh b/scripts/sync-skills.sh index 2a912f78..e3b9d6cd 100755 --- a/scripts/sync-skills.sh +++ b/scripts/sync-skills.sh @@ -1,10 +1,12 @@ #!/usr/bin/env bash # sync-skills.sh — Publish this CLI's skills to the basecamp/skills distribution repo. # -# Runs from CI on a release tag. Mirrors each skills// tree (SKILL.md and its -# supporting files; no *.go, no dotfiles) into skills// at the root of -# basecamp/skills — the layout `npx skills add basecamp/skills` reads — then commits -# as [bot] and pushes. +# Runs from CI on a release tag. Clones basecamp/skills fresh into a temp directory, +# mirrors each skills// tree (SKILL.md and its supporting files; no *.go, no +# dotfiles) into skills// at its root — the layout `npx skills add +# basecamp/skills` reads — then commits as [bot], pushes, and removes the +# clone. The script never adopts an existing checkout: the only commit it can push +# is the one it made, against the tip it cloned or fetched. # # Several CLIs publish into that one repo, so each owns a manifest of its own at the # target root, .managed-skills., listing the skill names it has published, @@ -27,26 +29,27 @@ # by hand, which beats guessing ownership from the legacy file. # # Required env vars: -# RELEASE_TAG — the release tag (e.g. v1.2.3) -# SOURCE_SHA — the source commit SHA -# SKILLS_TOKEN — GitHub token with push access to basecamp/skills; not needed -# for DRY_RUN=local, nor when SKILLS_TARGET is set +# RELEASE_TAG — the release tag (e.g. v1.2.3) +# SOURCE_SHA — the source commit SHA +# SKILLS_TOKEN — GitHub token with push access to basecamp/skills; not needed +# for DRY_RUN=local, nor when SKILLS_REPO_URL is not on github.com # # Optional env vars: -# CLI_NAME — this CLI's name; the publishing source is -cli -# SYNC_SOURCE — the publishing repo's name (default: -cli). Names the -# manifest, the bot and the commit; the test sets it to play -# another CLI -# SKILLS_SOURCE — directory holding the skills tree (default: skills). A manual -# recovery workflow can point it at a checkout of the release tag -# so the sync logic comes from a newer ref than the content -# SKILLS_TARGET — an existing checkout of basecamp/skills to sync into instead of -# cloning; the remote-URL and branch asserts still run against it -# DRY_RUN — "local": no network. Without SKILLS_TARGET, copy into an empty -# tmpdir and print what would be published; with it, apply and -# commit there but do not push. -# "remote": clone (or use SKILLS_TARGET), apply, print the diff, -# and stop before committing +# CLI_NAME — this CLI's name; the publishing source is -cli +# SYNC_SOURCE — the publishing repo's name (default: -cli). Names the +# manifest, the bot and the commit; the test sets it to play +# another CLI +# SKILLS_SOURCE — directory holding the skills tree (default: skills). A manual +# recovery workflow can point it at a checkout of the release tag +# so the sync logic comes from a newer ref than the content +# 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 +# would be published. +# "remote": clone, apply, print the diff, and stop before +# committing # set -euo pipefail @@ -56,12 +59,12 @@ SYNC_SOURCE="${SYNC_SOURCE:-${CLI_NAME}-cli}" RELEASE_TAG="${RELEASE_TAG:?RELEASE_TAG is required}" SOURCE_SHA="${SOURCE_SHA:?SOURCE_SHA is required}" SKILLS_SOURCE="${SKILLS_SOURCE:-skills}" -SKILLS_TARGET="${SKILLS_TARGET:-}" SKILLS_TOKEN="${SKILLS_TOKEN:-}" DRY_RUN="${DRY_RUN:-}" TARGET_REPO="basecamp/skills" TARGET_BRANCH="main" +SKILLS_REPO_URL="${SKILLS_REPO_URL:-https://github.com/${TARGET_REPO}.git}" SKILLS_SUBDIR="skills" LEGACY_MANIFEST=".managed-skills" MANIFEST="${LEGACY_MANIFEST}.${SYNC_SOURCE}" @@ -119,29 +122,6 @@ claimed_by_other() { return 1 } -# The URLs as configured: `remote get-url` would show them after any insteadOf -# rewrite, so an operator's rewrite could pass this check with a repo that is not -# the target. A push URL of its own is where `git push origin` would actually go. -assert_remote_url() { - local kind url - for kind in url pushurl; do - url=$(git -C "$1" config --get "remote.origin.${kind}" || true) - [[ -z "$url" && "$kind" == pushurl ]] && continue - case "${url%.git}" in - "https://github.com/${TARGET_REPO}") ;; - https://x-access-token:*@github.com/"${TARGET_REPO}") ;; - "git@github.com:${TARGET_REPO}") ;; - *) die "origin ${kind} '$(echo "$url" | sed -E 's#(https://[^:@]+:)[^@]*@#\1***@#')' does not point to github.com/${TARGET_REPO}" ;; - esac - done -} - -assert_branch() { - local branch - branch=$(git -C "$1" rev-parse --abbrev-ref HEAD) - [[ "$branch" == "$TARGET_BRANCH" ]] || die "checked-out branch is '$branch', expected '$TARGET_BRANCH'" -} - # --- Validate the knobs --- plain_name "$SYNC_SOURCE" || die "SYNC_SOURCE '$SYNC_SOURCE' is not a plain name" @@ -182,9 +162,9 @@ copy_skills() { tmpdir=$(mktemp -d) trap 'rm -rf "$tmpdir"' EXIT -# --- DRY_RUN=local without a target: what would be published --- +# --- DRY_RUN=local: what would be published --- -if [[ "$DRY_RUN" == "local" && -z "$SKILLS_TARGET" ]]; then +if [[ "$DRY_RUN" == "local" ]]; then preview="${tmpdir}/preview" echo "DRY_RUN=local: copying skills into ${preview}" copy_skills "${preview}/${SKILLS_SUBDIR}" @@ -206,7 +186,7 @@ fi # --- Git configuration for the target --- # # A private global config for every git call below: the bot is the identity for -# the commit and for the rebase a retried push needs, the token goes in as a URL +# 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. export GIT_CONFIG_GLOBAL="${tmpdir}/gitconfig" @@ -216,30 +196,19 @@ cat > "$GIT_CONFIG_GLOBAL" <> "$GIT_CONFIG_GLOBAL" < "$GIT_CONFIG_GLOBAL" +# A file:// URL rather than a path: git ignores --depth for a path, and the +# script's clone is shallow, so the retry has to fetch as it would from GitHub. +origin="${work}/origin.git" +origin_url="file://${origin}" target="${work}/target" out="${work}/out" failures=0 @@ -61,38 +68,54 @@ assert_tombstone() { fi } assert_author() { assert "last commit authored by $1[bot]" test "$(git -C "$target" log -1 --format=%an)" = "$1[bot]"; } -assert_clean() { assert "target working tree committed clean" test -z "$(git -C "$target" status --porcelain)"; } assert_output() { assert "output says: $1" grep -q -- "$1" "$out"; } assert_head() { # expected sha, description - assert "$2" test "$(git -C "$target" rev-parse HEAD)" = "$1" + assert "$2" test "$(git -C "$origin" rev-parse main)" = "$1" } +origin_head() { git -C "$origin" rev-parse main; } + # --- Running the script --- +# +# Every run clones origin afresh, as a release would clone basecamp/skills, and +# the target clone is brought to origin's tip afterwards for the assertions. + +refresh_target() { + git -C "$target" fetch -q origin main + git -C "$target" reset -q --hard FETCH_HEAD +} sync() { # source, fixture, [VAR=value...] local source="$1" fixture="$2" shift 2 - if env "$@" SYNC_SOURCE="$source" SKILLS_SOURCE="${fixture}/skills" \ + if env SKILLS_REPO_URL="$origin_url" "$@" SYNC_SOURCE="$source" SKILLS_SOURCE="${fixture}/skills" \ RELEASE_TAG=v9.9.9 SOURCE_SHA=0123abcd "$SYNC_SCRIPT" > "$out" 2>&1; then ok "sync as ${source} succeeded" else not_ok "sync as ${source} succeeded" sed 's/^/ /' "$out" fi + refresh_target } -sync_local() { sync "$1" "$2" DRY_RUN=local SKILLS_TARGET="$target"; } - sync_expecting_failure() { # source, fixture, [VAR=value...] local source="$1" fixture="$2" shift 2 - if env "$@" SYNC_SOURCE="$source" SKILLS_SOURCE="${fixture}/skills" \ + if env SKILLS_REPO_URL="$origin_url" "$@" SYNC_SOURCE="$source" SKILLS_SOURCE="${fixture}/skills" \ RELEASE_TAG=v9.9.9 SOURCE_SHA=0123abcd "$SYNC_SCRIPT" > "$out" 2>&1; then not_ok "sync as ${source} refused" sed 's/^/ /' "$out" else ok "sync as ${source} refused" fi + refresh_target +} + +# Commit the target's working tree and push it, as a hand-made or pre-fix commit +publish() { # message + git -C "$target" add -A + git -C "$target" commit -q -m "$1" + git -C "$target" push -q origin main } # --- Fixtures --- @@ -118,19 +141,20 @@ write_skill "${b}/skills/basecamp-doctor" "basecamp-doctor v2" # The target as basecamp/skills#5 left it: only basecamp-cli's skills survive, # and the shared manifest names them. +git init -q --bare -b main "$origin" git init -q -b main "$target" -git -C "$target" remote add origin https://github.com/basecamp/skills.git +git -C "$target" remote add origin "$origin_url" write_skill "${target}/skills/basecamp" "basecamp v1" write_skill "${target}/skills/basecamp-doctor" "basecamp-doctor v1" printf 'basecamp\nbasecamp-doctor\n' > "${target}/.managed-skills" echo "# skills" > "${target}/README.md" -git -C "$target" add -A -git -C "$target" commit -q -m "State after basecamp/skills#5" +publish "State after basecamp/skills#5" # --- Interleaved syncs: A, B, A, B --- echo "# hey-cli syncs first: restores its skills, touches nothing else" -sync_local hey-cli "$a" +sync hey-cli "$a" +assert_output "Skills synced to basecamp/skills" assert_skill hey assert_skill hey-doctor assert_skill basecamp @@ -144,11 +168,12 @@ assert_manifest hey-cli hey hey-doctor assert_no_manifest basecamp-cli assert_tombstone assert_author hey-cli -assert_clean assert_output "first run for hey-cli, removing nothing" +assert "origin main is the seed commit then hey-cli's sync" \ + test "$(git -C "$origin" log --format=%s -2 main | tr '\n' '|')" = "Sync skills from hey-cli v9.9.9|State after basecamp/skills#5|" echo "# basecamp-cli syncs: refreshes its skills, leaves hey-cli's" -sync_local basecamp-cli "$b" +sync basecamp-cli "$b" assert_skill hey assert_skill hey-doctor assert_skill basecamp @@ -158,15 +183,14 @@ assert_manifest hey-cli hey hey-doctor assert_manifest basecamp-cli basecamp basecamp-doctor assert_tombstone assert_author basecamp-cli -assert_clean echo "# both sync again with nothing new: no commits, nothing lost" -head_before=$(git -C "$target" rev-parse HEAD) -sync_local hey-cli "$a" +head_before=$(origin_head) +sync hey-cli "$a" assert_output "No changes to commit" -sync_local basecamp-cli "$b" +sync basecamp-cli "$b" assert_output "No changes to commit" -assert_head "$head_before" "HEAD unchanged by the no-op syncs" +assert_head "$head_before" "origin main unchanged by the no-op syncs" assert_skill hey assert_skill hey-doctor assert_skill basecamp @@ -179,7 +203,7 @@ assert_tombstone echo "# hey-cli drops hey-doctor" rm -rf "${a}/skills/hey-doctor" -sync_local hey-cli "$a" +sync hey-cli "$a" assert_output "Removing stale skill: hey-doctor" assert_no_skill hey-doctor assert_skill hey @@ -193,8 +217,8 @@ assert_author hey-cli echo "# a pre-fix basecamp-cli rewrites .managed-skills with its own names" printf 'basecamp\nbasecamp-doctor\n' > "${target}/.managed-skills" -git -C "$target" commit -q -am "Sync skills from basecamp-cli v0.0.0 (pre-fix script)" -sync_local hey-cli "$a" +publish "Sync skills from basecamp-cli v0.0.0 (pre-fix script)" +sync hey-cli "$a" assert_skill basecamp assert_skill basecamp-doctor assert_skill hey @@ -205,8 +229,8 @@ assert_manifest basecamp-cli basecamp basecamp-doctor echo "# hey-cli's manifest also lists basecamp, which basecamp-cli owns" printf 'basecamp\nhey\n' > "${target}/.managed-skills.hey-cli" -git -C "$target" commit -q -am "Collision: hey-cli claims basecamp" -sync_local hey-cli "$a" +publish "Collision: hey-cli claims basecamp" +sync hey-cli "$a" assert_skill basecamp assert_content skills/basecamp/SKILL.md "basecamp v2" assert_output "WARNING: skills/basecamp is no longer in hey-cli's skills but basecamp-cli lists it" @@ -217,103 +241,96 @@ assert_manifest basecamp-cli basecamp basecamp-doctor echo "# hey-cli ships a skill named basecamp" write_skill "${a}/skills/basecamp" "hey-cli's basecamp" -head_before=$(git -C "$target" rev-parse HEAD) -sync_expecting_failure hey-cli "$a" DRY_RUN=local SKILLS_TARGET="$target" +head_before=$(origin_head) +sync_expecting_failure hey-cli "$a" assert_output "ERROR: skills/basecamp is published by basecamp-cli" assert_content skills/basecamp/SKILL.md "basecamp v2" -assert_head "$head_before" "HEAD unchanged by the refused sync" -assert_clean +assert_head "$head_before" "origin main unchanged by the refused sync" rm -rf "${a}/skills/basecamp" -# --- DRY_RUN=remote applies and shows the diff but commits nothing --- +# --- DRY_RUN=remote clones and shows the diff but commits nothing --- -echo "# DRY_RUN=remote against the checkout" +echo "# DRY_RUN=remote against origin" echo "hey v3" > "${a}/skills/hey/SKILL.md" -head_before=$(git -C "$target" rev-parse HEAD) -sync hey-cli "$a" DRY_RUN=remote SKILLS_TARGET="$target" +head_before=$(origin_head) +sync hey-cli "$a" DRY_RUN=remote assert_output "DRY_RUN=remote: skipping commit and push" assert_output "+hey v3" -assert_head "$head_before" "HEAD unchanged by DRY_RUN=remote" -git -C "$target" reset -q --hard +assert_head "$head_before" "origin main unchanged by DRY_RUN=remote" +assert_content skills/hey/SKILL.md "hey v2" -# --- DRY_RUN=local with no target: no network, lists what would be published --- +# --- DRY_RUN=local: no clone at all, lists what would be published --- -echo "# DRY_RUN=local without SKILLS_TARGET" -sync hey-cli "$a" DRY_RUN=local +echo "# DRY_RUN=local never reaches the repository" +sync hey-cli "$a" DRY_RUN=local SKILLS_REPO_URL="file://${work}/nowhere.git" assert_output "skills/hey/SKILL.md" assert_output "skills/hey/reference/commands.md" assert_output "No network operations performed" if grep -q "embed.go" "$out"; then not_ok "preview leaves out embed.go"; else ok "preview leaves out embed.go"; fi -# --- Another publisher pushes first: the push is retried after a rebase --- +# --- Without a token, github.com is refused before anything is cloned --- + +echo "# the default target needs SKILLS_TOKEN" +sync_expecting_failure hey-cli "$a" SKILLS_REPO_URL=https://github.com/basecamp/skills.git +assert_output "ERROR: SKILLS_TOKEN is required" +if grep -q "Cloning" "$out"; then not_ok "refused before cloning"; else ok "refused before cloning"; fi + +# --- Another publisher pushes first: the sync is applied again from its tip --- +# +# The script clones afresh, so the sibling's push has to land after that clone and +# before the push. A post-commit hook, reached through the GIT_CONFIG_* environment +# the script's private gitconfig cannot hide, pushes the sibling's commit at exactly +# that moment; the script's push is then rejected as "fetch first". -echo "# a concurrent publisher wins the race to origin" -origin="${work}/origin.git" -git init -q --bare -b main "$origin" -git -C "$target" push -q "$origin" main sibling="${work}/sibling" -git clone -q "$origin" "$sibling" +git clone -q "$origin_url" "$sibling" +hooks="${work}/hooks" +mkdir -p "$hooks" +printf '#!/usr/bin/env bash\ngit -C "%s" push -q origin main\n' "$sibling" > "${hooks}/post-commit" +chmod +x "${hooks}/post-commit" +racing() { # source, fixture: sync with the sibling pushing between clone and push + sync "$1" "$2" GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.hooksPath "GIT_CONFIG_VALUE_0=${hooks}" +} +racing_expecting_failure() { + sync_expecting_failure "$1" "$2" GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.hooksPath "GIT_CONFIG_VALUE_0=${hooks}" +} + +echo "# a concurrent publisher wins the race to origin" echo "# skills (sibling)" > "${sibling}/README.md" git -C "$sibling" commit -q -am "Sync skills from basecamp-cli v0.0.1 (concurrent)" -git -C "$sibling" push -q origin main +sibling_head=$(git -C "$sibling" rev-parse HEAD) echo "hey v4" > "${a}/skills/hey/SKILL.md" -# A real push, with github.com/basecamp/skills routed to the local bare repo through -# the environment — the script's private gitconfig cannot hide that. -sync hey-cli "$a" SKILLS_TARGET="$target" \ - GIT_CONFIG_COUNT=1 "GIT_CONFIG_KEY_0=url.${origin}.insteadOf" GIT_CONFIG_VALUE_0=https://github.com/basecamp/skills.git +racing hey-cli "$a" assert_output "Push rejected" assert_output "Skills synced to basecamp/skills" assert "origin main holds the sibling's commit then the sync" \ test "$(git -C "$origin" log --format=%s -2 main | tr '\n' '|')" = "Sync skills from hey-cli v9.9.9|Sync skills from basecamp-cli v0.0.1 (concurrent)|" +assert "the sync commit was made on the sibling's tip" test "$(git -C "$origin" rev-parse main^)" = "$sibling_head" assert_content skills/hey/SKILL.md "hey v4" assert_content README.md "# skills (sibling)" -assert_clean +assert_author hey-cli echo "# a concurrent publisher claims a name this source ships: the retry refuses" git -C "$sibling" pull -q origin main printf 'basecamp\nbasecamp-doctor\nhey\n' > "${sibling}/.managed-skills.basecamp-cli" git -C "$sibling" commit -q -am "Collision: basecamp-cli claims hey (concurrent)" -git -C "$sibling" push -q origin main +sibling_head=$(git -C "$sibling" rev-parse HEAD) echo "hey v5" > "${a}/skills/hey/SKILL.md" -sync_expecting_failure hey-cli "$a" SKILLS_TARGET="$target" \ - GIT_CONFIG_COUNT=1 "GIT_CONFIG_KEY_0=url.${origin}.insteadOf" GIT_CONFIG_VALUE_0=https://github.com/basecamp/skills.git +racing_expecting_failure hey-cli "$a" assert_output "Push rejected" assert_output "ERROR: skills/hey is published by basecamp-cli" -assert "origin main tip is the sibling's commit" \ - test "$(git -C "$origin" log -1 --format=%s main)" = "Collision: basecamp-cli claims hey (concurrent)" +assert_head "$sibling_head" "origin main tip is the sibling's commit" assert_content skills/hey/SKILL.md "hey v4" -assert_clean + +echo "# the sibling drops its claim: the next release publishes" git -C "$sibling" checkout -q HEAD~1 -- .managed-skills.basecamp-cli git -C "$sibling" commit -q -am "basecamp-cli drops its claim on hey" git -C "$sibling" push -q origin main -git -C "$target" fetch -q "$origin" main -git -C "$target" reset -q --hard FETCH_HEAD - -# --- Safety asserts on the checkout --- - -echo "# a checkout with uncommitted changes is refused" -echo "stray" > "${target}/stray.txt" -sync_expecting_failure hey-cli "$a" DRY_RUN=local SKILLS_TARGET="$target" -assert_output "has uncommitted changes" -rm "${target}/stray.txt" - -echo "# a checkout that is not basecamp/skills on main is refused" -wrong="${work}/wrong-remote" -git init -q -b main "$wrong" -git -C "$wrong" remote add origin https://github.com/basecamp/other.git -git -C "$wrong" commit -q --allow-empty -m "init" -sync_expecting_failure hey-cli "$a" DRY_RUN=local SKILLS_TARGET="$wrong" -assert_output "does not point to github.com/basecamp/skills" - -git -C "$target" remote set-url --push origin https://github.com/someone/skills.git -sync_expecting_failure hey-cli "$a" DRY_RUN=local SKILLS_TARGET="$target" -assert_output "origin pushurl 'https://github.com/someone/skills.git' does not point to github.com/basecamp/skills" -git -C "$target" config --unset remote.origin.pushurl - -git -C "$target" checkout -q -b not-main -sync_expecting_failure hey-cli "$a" DRY_RUN=local SKILLS_TARGET="$target" -assert_output "checked-out branch is 'not-main', expected 'main'" -git -C "$target" checkout -q main +sync hey-cli "$a" +assert_output "Skills synced to basecamp/skills" +assert_content skills/hey/SKILL.md "hey v5" +assert_manifest basecamp-cli basecamp basecamp-doctor +assert_manifest hey-cli hey # --- Verdict --- From eee78095b7fe3dd8c9fdf3c6c05f564694daa514 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 18:50:57 -0700 Subject: [PATCH 3/6] Say what the private gitconfig replaces, and what it does not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- scripts/sync-skills.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/scripts/sync-skills.sh b/scripts/sync-skills.sh index e3b9d6cd..954a423a 100755 --- a/scripts/sync-skills.sh +++ b/scripts/sync-skills.sh @@ -186,9 +186,11 @@ fi # --- Git configuration for the target --- # # 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 commit, and for the one a rejected push makes again, and the token goes in as +# a URL rewrite so it never appears in argv or in the remote URL. Only the user's +# global file is replaced (~/.gitconfig: identity, signing, credential helpers, hooks +# path); the system config and any GIT_CONFIG_COUNT/GIT_CONFIG_KEY_* settings in the +# environment still apply — the test's race case injects a hooks path that way. export GIT_CONFIG_GLOBAL="${tmpdir}/gitconfig" cat > "$GIT_CONFIG_GLOBAL" < Date: Sun, 13 Sep 2026 18:50:57 -0700 Subject: [PATCH 4/6] Skip the release-time skills sync for anything but the latest stable release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/release.yml | 23 +++++++++++++++++++++++ RELEASING.md | 5 ++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 03e42384..dc0a2fc5 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -565,7 +565,29 @@ jobs: with: persist-credentials: false + # An older stable release can reach this job after a newer one has + # published — its run stalled, or its failed sync was rerun later — and + # the script mirrors the skills wholesale, so it would roll basecamp/skills + # back. The concurrency group serialises the jobs but does not order them, + # so this is the check the manual Sync skills workflow makes: only the + # latest stable release publishes. A failed lookup fails the job, which + # files the issue below rather than skipping in silence. + - name: Confirm this is the latest stable release + id: latest + env: + GH_TOKEN: ${{ github.token }} + TAG: ${{ github.ref_name }} + run: | + latest=$(gh release view --repo "${GITHUB_REPOSITORY}" --json tagName --jq .tagName) + if [ "$TAG" = "$latest" ]; then + echo "publish=true" >> "$GITHUB_OUTPUT" + else + echo "::warning::Skipping the skills sync for ${TAG}: the latest stable release is ${latest}, and syncing an older tag would roll basecamp/skills back." + echo "publish=false" >> "$GITHUB_OUTPUT" + fi + - name: Generate token for skills repo + if: steps.latest.outputs.publish == 'true' id: skills-token uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 with: @@ -576,6 +598,7 @@ jobs: permission-contents: write - name: Sync skills to distribution repo + if: steps.latest.outputs.publish == 'true' id: sync env: SKILLS_TOKEN: ${{ steps.skills-token.outputs.token }} diff --git a/RELEASING.md b/RELEASING.md index 4d1b59af..cfe73a09 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -236,7 +236,10 @@ If that job fails, a `skills-sync`-labeled issue is filed; recover with the refuses anything but the latest stable release so it cannot roll the distribution repo back, and it runs the sync script from the dispatching branch against the tag's skills tree — so when the failure was a defect in `sync-skills.sh` itself, -merge the fix to main and dispatch; no new release needed. +merge the fix to main and dispatch; no new release needed. The release-time job makes +the same check before it publishes, 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 it back. ## Local dry runs From 6c8647288911f0c6dc3233223ffae631fa225c09 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 19:17:36 -0700 Subject: [PATCH 5/6] Run the sync as the source its CLI_NAME default names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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@19b52ed6fa30993d34017cd19ef57c79a9545bff. --- Makefile | 2 +- scripts/test-sync-skills.sh | 57 ++++++++++++++++++++++++++++++++----- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index 7ac2316b..f65605a9 100644 --- a/Makefile +++ b/Makefile @@ -106,7 +106,7 @@ test-e2e: # Run the skills sync against a throwaway basecamp/skills, as two CLIs publishing in turn test-sync-skills: - scripts/test-sync-skills.sh + EXPECTED_SOURCE=hey-cli scripts/test-sync-skills.sh # Run smoke tests against a live HEY server. # Requires: a running server (default http://app.hey.localhost:3003) and Chrome. diff --git a/scripts/test-sync-skills.sh b/scripts/test-sync-skills.sh index 3e18ba3d..84b77347 100755 --- a/scripts/test-sync-skills.sh +++ b/scripts/test-sync-skills.sh @@ -8,12 +8,16 @@ # from a clone of its own. It starts in the state basecamp/skills#5 left it: # basecamp-cli's skills and the shared .managed-skills listing them. Then # hey-cli and basecamp-cli sync in turn, one loses a skill, a pre-fix sibling -# rewrites the legacy manifest, two manifests claim one name, and a sibling -# wins the race to push — after each step both sources' skills must be where -# they belong. No network and no token. +# rewrites the legacy manifest, two manifests claim one name, a sibling wins +# the race to push, and the script runs as the source its own CLI_NAME default +# names — after each step both sources' skills must be where they belong. No +# network and no token. # # Usage: scripts/test-sync-skills.sh (tests scripts/sync-skills.sh) # SYNC_SCRIPT=path/to/sync-skills.sh scripts/test-sync-skills.sh +# EXPECTED_SOURCE=-cli scripts/test-sync-skills.sh +# (the source the script publishes as when nothing names one; a CLI's +# Makefile passes its own, the default is read off the script's CLI_NAME line) set -euo pipefail @@ -85,11 +89,17 @@ refresh_target() { git -C "$target" reset -q --hard FETCH_HEAD } +# The script against origin with the identifiers a release carries; the caller's +# VAR=value pairs go in front of the fixed ones, so only SKILLS_REPO_URL can be +# overridden (DRY_RUN=local points it nowhere to prove it is never reached). +run_sync() { # [VAR=value...] + env SKILLS_REPO_URL="$origin_url" "$@" RELEASE_TAG=v9.9.9 SOURCE_SHA=0123abcd "$SYNC_SCRIPT" > "$out" 2>&1 +} + sync() { # source, fixture, [VAR=value...] local source="$1" fixture="$2" shift 2 - if env SKILLS_REPO_URL="$origin_url" "$@" SYNC_SOURCE="$source" SKILLS_SOURCE="${fixture}/skills" \ - RELEASE_TAG=v9.9.9 SOURCE_SHA=0123abcd "$SYNC_SCRIPT" > "$out" 2>&1; then + if run_sync "$@" SYNC_SOURCE="$source" SKILLS_SOURCE="${fixture}/skills"; then ok "sync as ${source} succeeded" else not_ok "sync as ${source} succeeded" @@ -101,8 +111,7 @@ sync() { # source, fixture, [VAR=value...] sync_expecting_failure() { # source, fixture, [VAR=value...] local source="$1" fixture="$2" shift 2 - if env SKILLS_REPO_URL="$origin_url" "$@" SYNC_SOURCE="$source" SKILLS_SOURCE="${fixture}/skills" \ - RELEASE_TAG=v9.9.9 SOURCE_SHA=0123abcd "$SYNC_SCRIPT" > "$out" 2>&1; then + if run_sync "$@" SYNC_SOURCE="$source" SKILLS_SOURCE="${fixture}/skills"; then not_ok "sync as ${source} refused" sed 's/^/ /' "$out" else @@ -111,6 +120,20 @@ sync_expecting_failure() { # source, fixture, [VAR=value...] refresh_target } +# The script with neither SYNC_SOURCE nor CLI_NAME set, so the source is the one +# the CLI_NAME default line names — the line each CLI edits, which the other +# runs here never reach because they set SYNC_SOURCE to play another CLI. +sync_as_default() { # fixture + local fixture="$1" + if (unset CLI_NAME SYNC_SOURCE; run_sync SKILLS_SOURCE="${fixture}/skills"); then + ok "sync as the default source succeeded" + else + not_ok "sync as the default source succeeded" + sed 's/^/ /' "$out" + fi + refresh_target +} + # Commit the target's working tree and push it, as a hand-made or pre-fix commit publish() { # message git -C "$target" add -A @@ -332,6 +355,26 @@ assert_content skills/hey/SKILL.md "hey v5" assert_manifest basecamp-cli basecamp basecamp-doctor assert_manifest hey-cli hey +# --- The CLI_NAME default: the one line each CLI's copy of the script changes --- +# +# A copy whose default names another CLI, or still names the seed's placeholder, +# fails here rather than publishing under that source's manifest and bot identity. +# The CLI's Makefile says which source to expect; the seed expects what its own +# CLI_NAME line says. Last, because in hey-cli's or basecamp-cli's repository this +# is that CLI's own sync, which rightly rewrites its manifest from the new tree. + +echo "# with nothing set, the script publishes as the source its CLI_NAME default names" +default_source="${EXPECTED_SOURCE:-$(sed -n 's/^CLI_NAME=.*CLI_NAME:-\([a-z0-9-]*\)}.*/\1/p' "$SYNC_SCRIPT")-cli}" +assert "a default source is known (${default_source})" test "$default_source" != "-cli" +c="${work}/default" +write_skill "${c}/skills/default-skill" "default-skill v1" +sync_as_default "$c" +assert_output "Skills synced to basecamp/skills (main) from ${default_source} v9.9.9" +assert_skill default-skill +assert_manifest "$default_source" default-skill +assert_author "$default_source" +assert_tombstone + # --- Verdict --- echo "" From 6918171ee6610049ba71358c084a8ec711fe7e53 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 22:07:18 -0700 Subject: [PATCH 6/6] Send the token as a github.com-scoped Authorization header, not a URL rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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:@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. --- scripts/sync-skills.sh | 30 +++++++++++++++++++----------- scripts/test-sync-skills.sh | 5 +++++ 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/scripts/sync-skills.sh b/scripts/sync-skills.sh index 954a423a..953ad2b0 100755 --- a/scripts/sync-skills.sh +++ b/scripts/sync-skills.sh @@ -22,11 +22,16 @@ # every run as a comment-only tombstone. The pre-fix script skips any line it cannot # parse as a skill name, but treats a missing file as licence to own every skills/* # directory — so the tombstone is what stops an un-upgraded sibling from deleting -# anyone's skills, whichever CLI upgrades first (basecamp/skills#5). One case is -# accepted: a skill a still-pre-fix sibling drops after the tombstone exists stays -# behind in the target (that script has no names left to delete by, and the -# sibling's own first run here removes nothing) — a lingering directory to remove -# by hand, which beats guessing ownership from the legacy file. +# this source's skills, whichever CLI upgrades first (basecamp/skills#5). It shields +# only the sources that have upgraded: a pre-fix sibling still writes its own names +# to .managed-skills, and a second pre-fix sibling still deletes those — the #5 +# clobber, confined to the CLIs yet to upgrade and gone once each has; nothing the +# target holds can make that script delete less, since a file it cannot read widens +# its reach to every skills/* directory. One more case is accepted: a skill a +# still-pre-fix sibling drops after the tombstone exists stays behind in the target +# (that script has no names left to delete by, and the sibling's own first run here +# removes nothing) — a lingering directory to remove by hand, which beats guessing +# ownership from the legacy file. # # Required env vars: # RELEASE_TAG — the release tag (e.g. v1.2.3) @@ -187,10 +192,13 @@ fi # # 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, and the token goes in as -# a URL rewrite so it never appears in argv or in the remote URL. Only the user's -# global file is replaced (~/.gitconfig: identity, signing, credential helpers, hooks -# path); the system config and any GIT_CONFIG_COUNT/GIT_CONFIG_KEY_* settings in the -# environment still apply — the test's race case injects a hooks path that way. +# an Authorization header scoped to github.com, the way actions/checkout sends it — +# not as a URL rewrite, which git expands before handing the URL to git-remote-https +# in argv. The remote URL stays clean; the token is only in this file, mode 600, +# removed with the tmpdir. Only the user's global file is replaced (~/.gitconfig: +# identity, signing, credential helpers, hooks path); the system config and any +# GIT_CONFIG_COUNT/GIT_CONFIG_KEY_* settings in the environment still apply — the +# test's race case injects a hooks path that way. export GIT_CONFIG_GLOBAL="${tmpdir}/gitconfig" cat > "$GIT_CONFIG_GLOBAL" <> "$GIT_CONFIG_GLOBAL" < "$GIT_CONFIG_GLOBAL" +# The token-required case points the script at the real basecamp/skills; a token +# inherited from the caller's environment would let it clone and publish the +# fixtures there. +unset SKILLS_TOKEN + # A file:// URL rather than a path: git ignores --depth for a path, and the # script's clone is shallow, so the retry has to fetch as it would from GitHub. origin="${work}/origin.git"