From 5ed19d80c51d0379e0261052a9d117db99f66896 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 17:28:42 -0700 Subject: [PATCH 1/9] Publish the fizzy skill to basecamp/skills on release basecamp/skills is becoming the skills repo for every 37signals product, and Fizzy was the one CLI not publishing there. Stable releases now mirror skills/ into it with the same sync-skills.sh basecamp-cli and hey-cli run, which owns only this CLI's skills via a per-source manifest (.managed-skills.fizzy-cli) and never touches a sibling's (basecamp/skills#5). The job is continue-on-error and files a skills-sync issue on failure; sync-skills.yml is the manual recovery. The sibling repos' BATS test is ported to plain bash, since this repo's toolchain has no BATS, and runs as make test-scripts (part of make check) and in CI. --- .github/workflows/release.yml | 94 ++++++ .github/workflows/sensitive-change-gate.yml | 1 + .github/workflows/sync-skills.yml | 119 +++++++ .github/workflows/test.yml | 4 + AGENTS.md | 2 +- Makefile | 13 +- RELEASING.md | 35 ++- e2e/sync_skills_test.sh | 332 ++++++++++++++++++++ scripts/sync-skills.sh | 305 ++++++++++++++++++ 9 files changed, 898 insertions(+), 7 deletions(-) create mode 100644 .github/workflows/sync-skills.yml create mode 100755 e2e/sync_skills_test.sh create mode 100755 scripts/sync-skills.sh diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 3dc5a710..51ddfc1a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -150,3 +150,97 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} AUR_KEY: ${{ secrets.AUR_KEY }} + + # Mirrors skills/ into basecamp/skills, the distribution repo shared by every + # CLI. The script owns only this CLI's skills there (.managed-skills.fizzy-cli) + # and never touches a sibling's. continue-on-error: a stale skills repo must + # not fail a release that has already shipped; the failure step files an + # issue instead, and sync-skills.yml is the manual way back. + sync-skills: + name: Sync skills + needs: [release] + if: startsWith(github.ref, 'refs/tags/v') && !contains(github.ref_name, '-') + continue-on-error: true + concurrency: + group: sync-skills + cancel-in-progress: false + runs-on: ubuntu-latest + environment: release + timeout-minutes: 10 + permissions: + contents: read + issues: write + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Generate token for skills repo + id: skills-token + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + client-id: ${{ vars.RELEASE_CLIENT_ID }} + private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} + owner: basecamp + repositories: skills + permission-contents: write + + - name: Sync skills to distribution repo + id: sync + env: + SKILLS_TOKEN: ${{ steps.skills-token.outputs.token }} + RELEASE_TAG: ${{ github.ref_name }} + SOURCE_SHA: ${{ github.sha }} + run: scripts/sync-skills.sh + + - name: Notify on sync failure + if: failure() + env: + GH_TOKEN: ${{ github.token }} + REPO_SLUG: ${{ github.repository }} + REF_NAME: ${{ github.ref_name }} + RUN_ID: ${{ github.run_id }} + run: | + RUN_URL="https://github.com/${REPO_SLUG}/actions/runs/${RUN_ID}" + LABEL="skills-sync" + TITLE="Skills sync failure" + BODY="The automatic skills sync to basecamp/skills for [${REF_NAME}](${RUN_URL}) failed. Check the workflow run for details; once fixed, rerun the failed jobs in that run or dispatch the Sync skills workflow for ${REF_NAME}." + + # Filed on this repo (the only one github.token can write to), one + # open issue per outage. Deduped by label, not title: a title lookup + # misses the moment someone retitles the issue while triaging it. + # The lookup fails closed — an API error must not read as "no open + # issue" and file a duplicate; the annotation below still records + # the failure. + notify_status=0 + if ! matches=$(gh issue list --repo "$REPO_SLUG" --state open --label "$LABEL" \ + --json number --jq '[.[].number] | join(" ")'); then + echo "::error::Could not list open '${LABEL}' issues in ${REPO_SLUG}. Filing nothing rather than risk a duplicate." + notify_status=2 + else + read -ra issues <<< "$matches" + case "${#issues[@]}" in + 0) + echo "No open '${LABEL}' issue; filing one." + # --force makes this idempotent, so the label exists before the + # first issue carries it (gh issue create refuses an unknown label). + gh label create "$LABEL" --repo "$REPO_SLUG" --force \ + --color d93f0b --description "The release-time sync to basecamp/skills failed" \ + && gh issue create --repo "$REPO_SLUG" --title "$TITLE" --body "$BODY" --label "$LABEL" \ + || notify_status=$? + ;; + 1) + echo "Commenting on existing '${LABEL}' issue #${issues[0]}." + gh issue comment --repo "$REPO_SLUG" "${issues[0]}" --body "$BODY" || notify_status=$? + ;; + *) + # Picking one arbitrarily would scatter one outage's history + # across issues a human already decided to keep separate. + echo "::error::${#issues[@]} open issues carry the '${LABEL}' label (${issues[*]}). Refusing to guess which one to update — consolidate them, then re-run." + notify_status=3 + ;; + esac + fi + + echo "::error::Skills sync to basecamp/skills failed for ${REF_NAME}. See ${RUN_URL}" + exit "$notify_status" diff --git a/.github/workflows/sensitive-change-gate.yml b/.github/workflows/sensitive-change-gate.yml index d2023eb3..78e18f5c 100644 --- a/.github/workflows/sensitive-change-gate.yml +++ b/.github/workflows/sensitive-change-gate.yml @@ -12,6 +12,7 @@ jobs: with: extra-patterns: | scripts/publish-aur.sh + scripts/sync-skills.sh permissions: contents: read pull-requests: write diff --git a/.github/workflows/sync-skills.yml b/.github/workflows/sync-skills.yml new file mode 100644 index 00000000..82eb287e --- /dev/null +++ b/.github/workflows/sync-skills.yml @@ -0,0 +1,119 @@ +# Manual skills sync, for when the release-time sync could not run. +# +# The sync-skills job in release.yml is `needs: [release]`, so anything that +# fails the release job after publication skips the sync entirely and leaves +# basecamp/skills stale against a shipped release. A skipped or failed sync +# 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 — only this CLI's skills, tracked in its own manifest there +# (.managed-skills.fizzy-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: + workflow_dispatch: + inputs: + tag: + description: 'Release tag to sync from, with the v prefix (e.g. v4.1.0)' + required: true + type: string + dry_run: + description: 'Preview the sync without pushing' + required: false + default: false + type: boolean + +permissions: {} + +# Shared with release.yml's sync-skills job so a manual run cannot interleave +# with an automatic one. +concurrency: + group: sync-skills + cancel-in-progress: false + +jobs: + sync: + name: Sync skills + runs-on: ubuntu-latest + timeout-minutes: 10 + environment: release + permissions: + contents: read + steps: + - name: Validate tag input + env: + TAG: ${{ inputs.tag }} + GH_TOKEN: ${{ github.token }} + run: | + # No prerelease suffix: the automatic sync in release.yml excludes + # prereleases deliberately, and this path must not smuggle prerelease + # content into the distribution repo's main branch. + if [[ ! "$TAG" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + echo "::error::Invalid tag '${TAG}' — expected a stable v-prefixed semver tag (e.g. v4.1.0)" + exit 1 + fi + + # 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) + if [ "$TAG" != "$latest" ]; then + echo "::error::Refusing to sync ${TAG}: the latest stable release is ${latest}. Syncing an older tag would roll basecamp/skills back." + exit 1 + fi + + # Two checkouts: sync logic from the dispatching ref, skills content from + # the release tag. A defect in scripts/sync-skills.sh is one of the ways + # the automatic sync fails, and running the tagged copy here would just + # re-run the defective script — recovery must be able to run a fix merged + # to main without cutting a new tag. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + # The content still mirrors the skills tree as it was released, even if + # main has moved on since: SKILLS_SOURCE points the script at this + # checkout. Fully qualified: actions/checkout resolves a bare name as a + # branch before a tag, so a branch sharing the tag's name would win and + # mirror unreleased content. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: refs/tags/${{ inputs.tag }} + path: release + persist-credentials: false + + # Needed for dry runs too: the honest preview clones the target, so it + # cannot run tokenless. Dry runs get a read-only token, which is also what + # stops DRY_RUN=remote from pushing even if the script were wrong. + - name: Generate token for skills repo + id: skills-token + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + client-id: ${{ vars.RELEASE_CLIENT_ID }} + private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} + owner: basecamp + repositories: skills + permission-contents: ${{ inputs.dry_run && 'read' || 'write' }} + + # github.sha is the dispatching ref's SHA (main), not the tag's, so + # resolve the tagged commit from its own checkout — otherwise the sync + # records the wrong provenance for the release it claims to mirror. + - name: Resolve the tagged commit + id: source + env: + TAG: ${{ inputs.tag }} + run: echo "sha=$(git -C release rev-parse --verify "refs/tags/${TAG}^{commit}")" >> "$GITHUB_OUTPUT" + + # DRY_RUN=remote clones the actual target and stops before the push, so + # the preview shows the real diff including deletions. + - name: Sync skills to distribution repo + env: + SKILLS_TOKEN: ${{ steps.skills-token.outputs.token }} + RELEASE_TAG: ${{ inputs.tag }} + SOURCE_SHA: ${{ steps.source.outputs.sha }} + SKILLS_SOURCE: release/skills + DRY_RUN: ${{ inputs.dry_run && 'remote' || '' }} + run: scripts/sync-skills.sh diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index bc874b72..15fce360 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -34,6 +34,10 @@ jobs: - name: Run tests run: go test -v ./... + # Shell scripts the release workflow runs, tested without a Go harness. + - name: Run shell script tests + run: e2e/sync_skills_test.sh + - name: Build run: go build -o bin/fizzy ./cmd/fizzy diff --git a/AGENTS.md b/AGENTS.md index a7a74cd2..f0efc1b7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,7 +13,7 @@ fizzy-cli/ │ ├── mcpserver/ # `fizzy mcp` MCP server (catalog/ synced from fizzy-mcp-server) │ └── render/ # Output rendering (styled, markdown, columns) ├── e2e/ # Go integration tests -├── skills/ # Agent skills +├── skills/ # Agent skills (mirrored to basecamp/skills on release) └── .claude-plugin/ # Claude Code integration ``` diff --git a/Makefile b/Makefile index 29495b59..90c69406 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: test test-unit test-e2e e2e test-go test-file e2e-file test-run e2e-run build clean tidy help \ +.PHONY: test test-unit test-scripts test-e2e e2e test-go test-file e2e-file test-run e2e-run build clean tidy help \ check-toolchain fmt fmt-check vet lint tidy-check race-test vuln secrets \ replace-check security check release-check release tools \ surface-snapshot surface-check lint-actions @@ -28,6 +28,7 @@ help: @echo "Usage:" @echo " make build Build the CLI" @echo " make test-unit Run unit tests (no API required)" + @echo " make test-scripts Run shell script tests (no API required)" @echo " make e2e Run owner-only CLI contract e2e tests" @echo " make test-e2e Alias for e2e" @echo " make test Alias for e2e" @@ -50,7 +51,7 @@ help: @echo "" @echo " make lint-actions Lint GitHub Actions workflows" @echo " make security lint + vuln + secrets" - @echo " make check fmt-check + vet + lint + test-unit + tidy-check" + @echo " make check fmt-check + vet + lint + tidy-check + race-test + test-scripts" @echo " make release-check check + replace-check + vuln + race-test" @echo " make release Run release preflight and tag" @echo " make tools Install dev tools" @@ -92,6 +93,10 @@ build: check-toolchain test-unit: check-toolchain go test -v ./internal/... +# Run shell script tests (no API required) +test-scripts: + e2e/sync_skills_test.sh + # Run e2e tests (requires API credentials) e2e: build @if [ -z "$$FIZZY_TEST_TOKEN" ]; then echo "Error: FIZZY_TEST_TOKEN not set"; exit 1; fi @@ -175,8 +180,8 @@ replace-check: # Security suite security: lint vuln secrets -# Local CI gate (fmt, vet, lint, tidy, race-test) -check: fmt-check vet lint tidy-check race-test +# Local CI gate (fmt, vet, lint, tidy, race-test, shell script tests) +check: fmt-check vet lint tidy-check race-test test-scripts # Release preflight release-check: check replace-check vuln diff --git a/RELEASING.md b/RELEASING.md index 46efbe4e..1b2ebc6d 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -20,6 +20,7 @@ Pushing the tag triggers the GitHub Actions release workflow, which: 7. For stable tags only, publishes the Homebrew cask to `basecamp/homebrew-tap` 8. For stable tags only, publishes the Scoop manifest to `basecamp/homebrew-tap` 9. For stable tags only, publishes to AUR (if `AUR_KEY` configured) +10. For stable tags only, mirrors `skills/` into [basecamp/skills](https://github.com/basecamp/skills) ## Versioning @@ -45,18 +46,19 @@ Prerelease behavior is intentionally conservative so existing package-manager us | Homebrew | Updates the normal `basecamp/tap/fizzy` cask. `brew upgrade fizzy` can move users to `v4.0.0`. | Does not update the normal cask (`skip_upload: auto`). Existing `brew upgrade fizzy` users stay on the latest stable cask. | | Scoop | Updates the normal `fizzy` manifest. `scoop update fizzy` can move users to `v4.0.0`. | Does not update the normal manifest (`skip_upload: auto`). Existing Scoop users stay on the latest stable manifest. | | AUR | Updates the normal `fizzy-cli` package if `AUR_KEY` is configured. | Skips the AUR publish job. Existing AUR users stay on the latest stable package. | +| basecamp/skills | Mirrors `skills/fizzy` into the shared skills repo. | Skips the skills sync job. `npx skills add basecamp/skills` keeps serving the latest stable skill. | | Go install | The git tag exists for users who explicitly request it. | The prerelease tag exists for users who explicitly request it; no package-manager manifest is updated. | Technical testers can install prereleases explicitly from the GitHub release assets, for example by downloading the asset for their OS/architecture from `https://github.com/basecamp/fizzy-cli/releases/tag/v4.0.0-beta1`. ## CI Secrets -All release credentials live in the `release` environment (`Settings > Environments > release`), so they are only exposed to jobs that pass the environment's required-reviewer gate. There are no repository-level release secrets. `HOMEBREW_TAP_TOKEN` does not exist as a stored secret — it is minted per-run from the `cli-release-bot` GitHub App credentials. +All release credentials live in the `release` environment (`Settings > Environments > release`), so they are only exposed to jobs that pass the environment's required-reviewer gate. There are no repository-level release secrets. `HOMEBREW_TAP_TOKEN` and the skills sync token do not exist as stored secrets — each is minted per-run from the `cli-release-bot` GitHub App credentials, scoped to the one repo that job pushes to (`homebrew-tap`, `skills`). The app must be installed on both repos with contents write access. | Name | Type | Purpose | |------|------|---------| | `RELEASE_CLIENT_ID` | variable | GitHub App client ID for `cli-release-bot` | -| `RELEASE_APP_PRIVATE_KEY` | secret | GitHub App private key for tap push | +| `RELEASE_APP_PRIVATE_KEY` | secret | GitHub App private key for tap and skills push | | `AUR_KEY` | secret | ed25519 SSH private key for AUR (optional) | | `MACOS_SIGN_P12` | secret | Base64-encoded Developer ID Application .p12 | | `MACOS_SIGN_PASSWORD` | secret | Password for the .p12 certificate | @@ -74,6 +76,7 @@ Set a secret with `gh secret set --env release -R basecamp/fizzy-cli`; th | Homebrew | `basecamp/homebrew-tap` Casks/fizzy.rb | GoReleaser (stable tags only) | | Scoop | `basecamp/homebrew-tap` fizzy.json | GoReleaser (stable tags only) | | AUR | `aur.archlinux.org/packages/fizzy-cli` | `publish-aur.sh` (stable tags only) | +| Skills | `basecamp/skills` skills/fizzy | `sync-skills.sh` (stable tags only) | | Go install | `go install github.com/basecamp/fizzy-cli/cmd/fizzy@latest` | Go module proxy | | curl installer | `scripts/install.sh` | Manual | @@ -93,6 +96,34 @@ rm fizzy-tmp goreleaser release --snapshot --clean ``` +## Skills sync + +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.fizzy-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). A target with no `.managed-skills.fizzy-cli` yet is a first run: nothing is +removed. 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). `e2e/sync_skills_test.sh` pins the contract; it +runs under `make test-scripts` (part of `make check`) and in CI. + +The copy drops `*.go` and dotfiles, so `skills/embed.go` stays here and only +`skills/fizzy/**` is published. Preview what a release would publish, offline: + +```bash +DRY_RUN=local RELEASE_TAG=v0.0.0 SOURCE_SHA=$(git rev-parse HEAD) scripts/sync-skills.sh +``` + +The job is `continue-on-error`, so a failed sync never fails a release that has +already shipped. If it 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 master and dispatch; no new release needed. + ## AUR Setup 1. Generate ed25519 SSH keypair: `ssh-keygen -t ed25519 -f aur_key` diff --git a/e2e/sync_skills_test.sh b/e2e/sync_skills_test.sh new file mode 100755 index 00000000..93ee5430 --- /dev/null +++ b/e2e/sync_skills_test.sh @@ -0,0 +1,332 @@ +#!/usr/bin/env bash +# +# Ownership contract for scripts/sync-skills.sh against the shared +# basecamp/skills distribution repo (basecamp/skills#5). +# +# Several CLIs publish into one target. Each keeps its own manifest there +# (.managed-skills.) and may delete only the skill directories that +# manifest lists — never a sibling's. The pre-fix script kept one shared +# .managed-skills, so every release pruned whatever the previous publisher had +# put there: hey-cli's v0.1.1 sync deleted skills/basecamp and +# skills/basecamp-doctor (08ef7ea), and basecamp-cli's next two releases +# deleted skills/hey (728a916, 42716d7). +# +# The fixture reproduces basecamp/skills as that history left it: the basecamp +# skills present and the legacy manifest listing the basecamp names. fizzy-cli +# has never published there, so its first sync is the first-run path. The +# script's default SYNC_SOURCE plays this repo; the sibling CLI is played by +# setting SYNC_SOURCE explicitly (the two sync_* helpers below are the only +# lines that differ between this file and its twins in the sibling repos). +# SKILLS_TARGET points the script at the fixture checkout and DRY_RUN=local +# applies and commits without pushing, so nothing here touches the network. +# +# This is the BATS file the sibling repos carry, run without BATS: this repo's +# toolchain has none, so a small harness at the bottom plays its part. Each +# test_* function runs in a subshell under `set -e` with an ERR trap, so the +# first failing command ends the test and names its line; `run` captures a +# command's merged output and status the way BATS' does. + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +SYNC="$REPO_ROOT/scripts/sync-skills.sh" + +setup() { + WORK="$(mktemp -d)" + TARGET="$WORK/target" + + # Keep the operator's git identity, signing and hooks out of the fixture. + export GIT_CONFIG_NOSYSTEM=1 + export GIT_CONFIG_GLOBAL=/dev/null + export RELEASE_TAG=v4.1.0 + export SOURCE_SHA=0123456789abcdef0123456789abcdef01234567 + export DRY_RUN=local + + # basecamp/skills today. README.md and skills/orphan stand in for content no + # publisher owns; both must survive every run, orphan in particular because it + # is exactly what the dropped first-run fallback used to claim and delete. + mkdir -p "$TARGET/skills/basecamp" "$TARGET/skills/basecamp-doctor" "$TARGET/skills/orphan" + printf '# basecamp (stale)\n' > "$TARGET/skills/basecamp/SKILL.md" + printf '# basecamp-doctor (stale)\n' > "$TARGET/skills/basecamp-doctor/SKILL.md" + printf '# orphan\n' > "$TARGET/skills/orphan/SKILL.md" + printf '# basecamp/skills\n' > "$TARGET/README.md" + printf 'basecamp\nbasecamp-doctor\n' > "$TARGET/.managed-skills" + git -C "$TARGET" init -q -b main + git -C "$TARGET" remote add origin https://github.com/basecamp/skills.git + git -C "$TARGET" -c user.name=t -c user.email=t@t add -A + git -C "$TARGET" -c user.name=t -c user.email=t@t commit -q -m "Sync skills from basecamp-cli v0.11.0" + + # basecamp-cli's tree. One skill carries a nested file plus a *.go and a + # dotfile that the copy filter must drop. + BASECAMP="$WORK/basecamp/skills" + mkdir -p "$BASECAMP/basecamp/reference" "$BASECAMP/basecamp-doctor" + printf '# basecamp\n' > "$BASECAMP/basecamp/SKILL.md" + printf 'reference\n' > "$BASECAMP/basecamp/reference/api.md" + printf 'package skills\n' > "$BASECAMP/basecamp/embed.go" + printf 'secret\n' > "$BASECAMP/basecamp/.hidden" + printf '# basecamp-doctor\n' > "$BASECAMP/basecamp-doctor/SKILL.md" + + # fizzy-cli's tree. + FIZZY="$WORK/fizzy/skills" + mkdir -p "$FIZZY/fizzy" + printf '# fizzy\n' > "$FIZZY/fizzy/SKILL.md" + + 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.' +} + +teardown() { + rm -rf "$WORK" +} + +sync_basecamp() { + run env SYNC_SOURCE=basecamp-cli SKILLS_SOURCE="$BASECAMP" SKILLS_TARGET="$TARGET" "$SYNC" + [ "$status" -eq 0 ] +} + +sync_fizzy() { + run env SKILLS_SOURCE="$FIZZY" SKILLS_TARGET="$TARGET" "$SYNC" + [ "$status" -eq 0 ] +} + +# Both CLIs' skills present, each manifest listing exactly its own names, the +# legacy file a tombstone, and the unowned content untouched. +assert_shared_state() { + [ -f "$TARGET/skills/basecamp/SKILL.md" ] + [ -f "$TARGET/skills/basecamp-doctor/SKILL.md" ] + [ -f "$TARGET/skills/fizzy/SKILL.md" ] + [ "$(cat "$TARGET/.managed-skills.basecamp-cli")" = $'basecamp\nbasecamp-doctor' ] + [ "$(cat "$TARGET/.managed-skills.fizzy-cli")" = "fizzy" ] + [ "$(cat "$TARGET/.managed-skills")" = "$TOMBSTONE" ] + assert_unowned_intact +} + +assert_unowned_intact() { + [ -f "$TARGET/README.md" ] + [ -f "$TARGET/skills/orphan/SKILL.md" ] +} + +# Every run leaves the fixture committed: the next run must see a clean tree, +# as the real clone does. +assert_clean_tree() { + [ -z "$(git -C "$TARGET" status --porcelain)" ] +} + +test_interleaved_releases_keep_both_clis_skills() { + # basecamp-cli's first run: no .managed-skills.basecamp-cli yet, so nothing + # is removed and the manifest claims the current set. + sync_basecamp + [[ "$output" == *"first run for basecamp-cli"* ]] + [[ "$output" != *"Removing stale skill"* ]] + [ "$(cat "$TARGET/skills/basecamp/SKILL.md")" = "# basecamp" ] + [ -f "$TARGET/skills/basecamp/reference/api.md" ] + [ ! -e "$TARGET/skills/basecamp/embed.go" ] + [ ! -e "$TARGET/skills/basecamp/.hidden" ] + [ "$(cat "$TARGET/.managed-skills.basecamp-cli")" = $'basecamp\nbasecamp-doctor' ] + [ ! -e "$TARGET/.managed-skills.fizzy-cli" ] + [ "$(cat "$TARGET/.managed-skills")" = "$TOMBSTONE" ] + assert_unowned_intact + assert_clean_tree + + sync_fizzy + [[ "$output" == *"first run for fizzy-cli"* ]] + assert_shared_state + assert_clean_tree + + sync_basecamp + [[ "$output" != *"Removing stale skill"* ]] + assert_shared_state + assert_clean_tree + + sync_fizzy + [[ "$output" != *"Removing stale skill"* ]] + assert_shared_state + assert_clean_tree +} + +test_a_rerun_with_nothing_changed_commits_nothing() { + sync_basecamp + sync_fizzy + sync_basecamp + [[ "$output" == *"No changes to commit"* ]] + assert_shared_state + assert_clean_tree +} + +test_removes_only_a_skill_dropped_from_its_own_tree() { + sync_basecamp + sync_fizzy + rm -rf "$BASECAMP/basecamp-doctor" + + sync_basecamp + [[ "$output" == *"Removing stale skill: basecamp-doctor"* ]] + [ ! -e "$TARGET/skills/basecamp-doctor" ] + [ -f "$TARGET/skills/basecamp/SKILL.md" ] + [ -f "$TARGET/skills/fizzy/SKILL.md" ] + [ "$(cat "$TARGET/.managed-skills.basecamp-cli")" = "basecamp" ] + [ "$(cat "$TARGET/.managed-skills.fizzy-cli")" = "fizzy" ] + assert_unowned_intact + assert_clean_tree +} + +test_a_stale_legacy_manifest_from_a_prefix_sibling_deletes_nothing() { + sync_basecamp + sync_fizzy + # The pre-fix script rewrites .managed-skills with its own names after every + # run. The new script must read only its own manifest and put the tombstone + # back. + printf 'basecamp\nbasecamp-doctor\n' > "$TARGET/.managed-skills" + git -C "$TARGET" -c user.name=t -c user.email=t@t commit -q -am "Sync skills from basecamp-cli v0.11.1 (pre-fix)" + + sync_fizzy + [[ "$output" != *"Removing stale skill"* ]] + assert_shared_state + assert_clean_tree +} + +test_a_name_another_manifest_claims_survives_with_a_warning() { + sync_basecamp + sync_fizzy + # Collision: basecamp-cli's manifest lists fizzy, which its tree does not + # ship and fizzy-cli's manifest also lists. Deleting it would take fizzy-cli's + # skill; leave it and say so. + printf 'basecamp\nbasecamp-doctor\nfizzy\n' > "$TARGET/.managed-skills.basecamp-cli" + git -C "$TARGET" -c user.name=t -c user.email=t@t commit -q -am "collide" + + sync_basecamp + [[ "$output" == *"WARNING: not removing skill 'fizzy': also listed by fizzy-cli"* ]] + [[ "$output" != *"Removing stale skill"* ]] + assert_shared_state + assert_clean_tree +} + +test_a_first_run_against_a_legacy_only_target_removes_nothing() { + # fizzy-cli's first release with a sync: the target has never seen a + # .managed-skills.fizzy-cli, and the legacy manifest lists only basecamp's + # names. The pre-fix script would have claimed and deleted both basecamp + # skills here, as hey-cli's did (08ef7ea). + sync_fizzy + [[ "$output" != *"Removing stale skill"* ]] + [ -f "$TARGET/skills/basecamp/SKILL.md" ] + [ -f "$TARGET/skills/basecamp-doctor/SKILL.md" ] + [ -f "$TARGET/skills/fizzy/SKILL.md" ] + [ "$(cat "$TARGET/.managed-skills.fizzy-cli")" = "fizzy" ] + [ ! -e "$TARGET/.managed-skills.basecamp-cli" ] + [ "$(cat "$TARGET/.managed-skills")" = "$TOMBSTONE" ] + assert_unowned_intact + assert_clean_tree +} + +test_commits_as_source_bot_with_the_source_in_the_message() { + sync_basecamp + [ "$(git -C "$TARGET" log -1 --format=%an)" = "basecamp-cli[bot]" ] + [ "$(git -C "$TARGET" log -1 --format=%ae)" = "basecamp-cli[bot]@users.noreply.github.com" ] + [ "$(git -C "$TARGET" log -1 --format=%s)" = "Sync skills from basecamp-cli v4.1.0" ] + [[ "$(git -C "$TARGET" log -1 --format=%b)" == *"basecamp/basecamp-cli@${SOURCE_SHA}"* ]] + + sync_fizzy + [ "$(git -C "$TARGET" log -1 --format=%an)" = "fizzy-cli[bot]" ] + [ "$(git -C "$TARGET" log -1 --format=%ae)" = "fizzy-cli[bot]@users.noreply.github.com" ] + [ "$(git -C "$TARGET" log -1 --format=%s)" = "Sync skills from fizzy-cli v4.1.0" ] + [[ "$(git -C "$TARGET" log -1 --format=%b)" == *"basecamp/fizzy-cli@${SOURCE_SHA}"* ]] +} + +test_still_refuses_a_target_that_is_not_basecamp_skills_on_main() { + git -C "$TARGET" remote set-url origin https://github.com/basecamp/other.git + run env SKILLS_SOURCE="$FIZZY" SKILLS_TARGET="$TARGET" "$SYNC" + [ "$status" -ne 0 ] + [[ "$output" == *"does not point to github.com/basecamp/skills"* ]] + + git -C "$TARGET" remote set-url origin https://github.com/basecamp/skills.git + git -C "$TARGET" checkout -q -b topic + run env SKILLS_SOURCE="$FIZZY" SKILLS_TARGET="$TARGET" "$SYNC" + [ "$status" -ne 0 ] + [[ "$output" == *"checked-out branch is 'topic'"* ]] +} + +test_dry_run_local_without_a_target_still_previews_offline() { + cd "$WORK/basecamp" + run "$SYNC" + [ "$status" -eq 0 ] + [[ "$output" == *"Found 2 skill(s):"* ]] + [[ "$output" == *"skills/basecamp/SKILL.md"* ]] + [[ "$output" == *"skills/basecamp-doctor/SKILL.md"* ]] + [[ "$output" == *"skills/basecamp/reference/api.md"* ]] + [[ "$output" != *"embed.go"* ]] + [[ "$output" != *".hidden"* ]] + [[ "$output" == *"No network operations performed"* ]] +} + +test_skills_source_points_the_sync_at_another_checkout() { + cd "$WORK/basecamp" + run env SKILLS_SOURCE="$FIZZY" "$SYNC" + [ "$status" -eq 0 ] + [[ "$output" == *"Found 1 skill(s): $FIZZY/fizzy"* ]] + [[ "$output" != *"basecamp"* ]] +} + +test_dies_when_the_source_tree_has_no_skills() { + run env SKILLS_SOURCE="$WORK/empty" "$SYNC" + [ "$status" -ne 0 ] + [[ "$output" == *"no skills found under $WORK/empty/*/SKILL.md"* ]] +} + +# --- Harness --- + +# Captures a command's merged stdout+stderr in $output and its exit status in +# $status without tripping `set -e`, as BATS' run does. The ERR trap is +# suspended too: it fires on any nonzero status, `set +e` or not, and a +# refusal under test is not a failure of the test. +run() { + local saved_trap + saved_trap=$(trap -p ERR) + set +e + trap - ERR + output="$("$@" 2>&1)" + status=$? + eval "$saved_trap" + set -e +} + +# ERR trap for a test's subshell: names the failing line and command, and +# shows the last captured output so a failed assertion on it is diagnosable. +fail_report() { + # BASH_LINENO[0] is the line that tripped the trap; LINENO here would be + # this function's own. + echo " failed at line ${BASH_LINENO[0]}: $BASH_COMMAND" + if [[ -n "${output:-}" ]]; then + printf ' --- last output ---\n%s\n' "$output" | sed 's/^/ /' + fi +} + +main() { + local passed=0 failed=0 test result + for test in $(declare -F | awk '$3 ~ /^test_/ { print $3 }'); do + setup + # A subshell inside an `if` would have `set -e` ignored, so run it plainly + # and read the status afterwards. + set +e + ( + set -eE + trap fail_report ERR + "$test" + ) + result=$? + set -e + teardown + if [[ $result -eq 0 ]]; then + echo "ok ${test#test_}" + passed=$((passed + 1)) + else + echo "FAIL ${test#test_}" + failed=$((failed + 1)) + fi + done + echo + echo "$passed passed, $failed failed" + [[ $failed -eq 0 ]] +} + +main "$@" diff --git a/scripts/sync-skills.sh b/scripts/sync-skills.sh new file mode 100755 index 00000000..4520922d --- /dev/null +++ b/scripts/sync-skills.sh @@ -0,0 +1,305 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Sync this CLI's skills to the basecamp/skills distribution repo. +# +# basecamp/skills is shared: every CLI (basecamp-cli, hey-cli, ...) publishes +# its skills as skills// at the target root. Each publisher owns one +# manifest there, .managed-skills., listing the names it published. On +# a sync it removes a skills/ only when its own manifest lists the name, +# its current skills tree no longer has it, and no other publisher's manifest +# claims it. A publisher with no manifest yet removes nothing. That is what +# stops one CLI's release from deleting another's skills (basecamp/skills#5: +# the CLIs used to share a single .managed-skills and each pruned the other). +# +# The legacy .managed-skills is rewritten on every run as a comment-only +# tombstone. A CLI still running the pre-fix script skips every line it cannot +# parse as a skill name, so it deletes nothing — whereas if the file were +# removed, that script would fall back to claiming every skills/*/ as its own. +# +# Env vars: +# SKILLS_TOKEN - GitHub token with push access to basecamp/skills (required +# when cloning, i.e. unless DRY_RUN=local or SKILLS_TARGET is set) +# 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) +# SKILLS_TARGET - Optional: existing basecamp/skills checkout to sync into +# instead of cloning (tests). The remote and branch asserts +# still apply to it. +# SYNC_SOURCE - Optional: the publishing repo's name (default: fizzy-cli). +# Names the manifest, the bot and the commit; tests use it to +# play another CLI. +# DRY_RUN - Optional: "local" (no network) or "remote" (clone but skip push). +# With SKILLS_TARGET, "local" applies and commits but skips the push. + +RELEASE_TAG="${RELEASE_TAG:?RELEASE_TAG is required}" +SOURCE_SHA="${SOURCE_SHA:?SOURCE_SHA is required}" +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}" +SKILLS_TARGET="${SKILLS_TARGET:-}" +SYNC_SOURCE="${SYNC_SOURCE:-fizzy-cli}" +TARGET_REPO="basecamp/skills" +TARGET_BRANCH="main" +SKILLS_SUBDIR="skills" +LEGACY_MANIFEST=".managed-skills" +MANIFEST="$LEGACY_MANIFEST.$SYNC_SOURCE" + +# --- Helpers --- + +die() { echo "ERROR: $*" >&2; exit 1; } + +assert_remote_url() { + local url + url=$(git -C "$1" remote get-url origin) + local stripped + stripped=$(echo "$url" | sed -E 's/\.git$//') + # Validate host + owner/repo for both HTTPS and SSH forms + 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 +} + +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'" +} + +# Prints the skill names listed in a manifest, one per line, skipping (with a +# warning) any line that is not a plain name — the tombstone's comment lines +# included. The pre-fix script parses .managed-skills the same way, which is +# what makes the tombstone inert for it. +manifest_names() { + local file="$1" entry label + label=$(basename "$file") + while IFS= read -r entry; do + [[ -z "$entry" ]] && continue + if [[ "$entry" == "." || "$entry" == ".." || ! "$entry" =~ ^[a-zA-Z0-9._-]+$ ]]; then + echo "WARNING: skipping invalid entry in $label: $entry" >&2 + continue + fi + echo "$entry" + done < "$file" +} + +# Prints the source names whose manifests (other than this source's) list $2. +other_claimants() { + local target="$1" name="$2" file names + for file in "$target/$LEGACY_MANIFEST".*; do + [[ -f "$file" ]] || continue + [[ "$(basename "$file")" == "$MANIFEST" ]] && continue + # Read the whole manifest first: under pipefail, `grep -q` closing the pipe + # early could fail the pipeline on a match and hide the claimant. + names=$(manifest_names "$file" 2>/dev/null) + if grep -qxF -- "$name" <<< "$names"; then + echo "${file##*/"$LEGACY_MANIFEST".}" + fi + done +} + +# --- Discover skills --- + +skill_dirs=() +for skill_md in "$SKILLS_SOURCE"/*/SKILL.md; do + [[ -f "$skill_md" ]] || continue + skill_dirs+=("$(dirname "$skill_md")") +done + +[[ ${#skill_dirs[@]} -gt 0 ]] || die "no skills found under $SKILLS_SOURCE/*/SKILL.md" +echo "Found ${#skill_dirs[@]} skill(s): ${skill_dirs[*]}" + +source_skill_names=() +for skill_dir in "${skill_dirs[@]}"; do + source_skill_names+=("$(basename "$skill_dir")") +done + +in_source_set() { + local name + for name in "${source_skill_names[@]}"; do + [[ "$name" == "$1" ]] && return 0 + done + return 1 +} + +# --- Copy skills into target, excluding *.go and dotfiles --- + +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" + # Copy files, excluding *.go and dotfiles + 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" \; + done +} + +# --- DRY_RUN=local without a target: copy into tmpdir, diff against empty baseline --- + +if [[ "$DRY_RUN" == "local" && -z "$SKILLS_TARGET" ]]; then + tmpdir=$(mktemp -d) + trap 'rm -rf "$tmpdir"' EXIT + echo "DRY_RUN=local: copying skills into $tmpdir" + copy_skills "$tmpdir/$SKILLS_SUBDIR" + echo "" + echo "=== Skills copied ===" + find "$tmpdir" -type f | sort | while read -r f; do + echo " ${f#"$tmpdir/"}" + done + echo "" + echo "=== Diff (against empty baseline) ===" + # Initialize as empty git repo to get a clean diff + git -C "$tmpdir" init -q + git -C "$tmpdir" add -A + git -C "$tmpdir" diff --cached --stat + echo "" + echo "DRY_RUN=local complete. No network operations performed." + exit 0 +fi + +# --- Target: an existing checkout, or a fresh clone --- + +if [[ -n "$SKILLS_TARGET" ]]; then + [[ -e "$SKILLS_TARGET/.git" ]] || die "SKILLS_TARGET '$SKILLS_TARGET' is not a git checkout" + target="$SKILLS_TARGET" + echo "Syncing into existing checkout $target" +else + [[ -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" + + target="$tmpdir/skills" +fi + +# --- Safety checks --- + +assert_remote_url "$target" +assert_branch "$target" + +# --- Copy skills --- + +echo "Copying skills into target..." +copy_skills "$target/$SKILLS_SUBDIR" + +# --- Remove skills this source no longer ships --- +# Only names in this source's own manifest are candidates. No manifest means a +# first run since the per-source manifests arrived (or a brand-new publisher): +# remove nothing, and let the manifest written below claim the current set. + +if [[ -f "$target/$MANIFEST" ]]; then + previously_managed_names=() + while IFS= read -r entry; do + previously_managed_names+=("$entry") + done < <(manifest_names "$target/$MANIFEST") + + for previously_managed in "${previously_managed_names[@]}"; do + in_source_set "$previously_managed" && continue + [[ -d "$target/$SKILLS_SUBDIR/$previously_managed" ]] || continue + claimants=$(other_claimants "$target" "$previously_managed") + if [[ -n "$claimants" ]]; then + # Two publishers claiming one name is a collision to settle upstream, not + # something a release should resolve by deletion. + echo "WARNING: not removing skill '$previously_managed': also listed by $(echo "$claimants" | paste -sd ' ' -)" >&2 + continue + fi + echo "Removing stale skill: $previously_managed" + rm -rf "${target:?}/$SKILLS_SUBDIR/$previously_managed" + done +else + echo "No $MANIFEST in target (first run for $SYNC_SOURCE): removing nothing." +fi + +# --- Write this source's manifest and the legacy tombstone --- + +printf '%s\n' "${source_skill_names[@]}" | sort > "$target/$MANIFEST" + +cat > "$target/$LEGACY_MANIFEST" <<'EOF' +# 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. +EOF + +# --- Commit --- + +git -C "$target" add -A + +if git -C "$target" diff --cached --quiet; then + echo "No changes to commit. Skills are already up to date." + exit 0 +fi + +echo "" +echo "=== Changes ===" +git -C "$target" diff --cached --stat +echo "" + +if [[ "$DRY_RUN" == "remote" ]]; then + echo "DRY_RUN=remote: skipping commit and push." + echo "" + echo "=== Full diff ===" + git -C "$target" diff --cached + exit 0 +fi + +git -C "$target" \ + -c user.name="${SYNC_SOURCE}[bot]" \ + -c user.email="${SYNC_SOURCE}[bot]@users.noreply.github.com" \ + commit -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 + echo "$output" >&2 + die "Push failed" + fi +fi + +echo "" +echo "Skills synced to $TARGET_REPO ($TARGET_BRANCH) from $RELEASE_TAG" From f2fc0f82a405a6ff8527ff100876c83a8748c256 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 17:36:50 -0700 Subject: [PATCH 2/9] Name the pre-fix sibling test after what it tests --- e2e/sync_skills_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/sync_skills_test.sh b/e2e/sync_skills_test.sh index 93ee5430..1336aba8 100755 --- a/e2e/sync_skills_test.sh +++ b/e2e/sync_skills_test.sh @@ -171,7 +171,7 @@ test_removes_only_a_skill_dropped_from_its_own_tree() { assert_clean_tree } -test_a_stale_legacy_manifest_from_a_prefix_sibling_deletes_nothing() { +test_a_stale_legacy_manifest_from_a_pre_fix_sibling_deletes_nothing() { sync_basecamp sync_fizzy # The pre-fix script rewrites .managed-skills with its own names after every From 63fd04986ac26b5b6d8291f5a02289a8a2500968 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 17:44:40 -0700 Subject: [PATCH 3/9] Converge on the seed's sync-skills.sh and its test The seed (basecamp/cli#78) went further than the hey-cli copy taken first: a rejected push re-applies the sync from the remote's new tip (matching git's "fetch first" as well as "non-fast-forward"), a name another source's manifest holds is refused before anything is copied, SYNC_SOURCE and DRY_RUN are validated, a dirty checkout is refused, and the token reaches git through a private config rather than the remote URL. basecamp-cli and hey-cli are converging on the same file, so every publisher runs one script; only the CLI_NAME default differs here. The seed's test-sync-skills.sh replaces the bash port of the BATS file: it covers the same ownership cases plus the racing publisher, the publish-side refusal, DRY_RUN=remote and the dirty-tree guard. --- .github/workflows/test.yml | 5 +- Makefile | 16 +- RELEASING.md | 23 +- e2e/sync_skills_test.sh | 332 -------------------------- scripts/sync-skills.sh | 448 ++++++++++++++++++++---------------- scripts/test-sync-skills.sh | 326 ++++++++++++++++++++++++++ 6 files changed, 604 insertions(+), 546 deletions(-) delete mode 100755 e2e/sync_skills_test.sh create mode 100755 scripts/test-sync-skills.sh diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 15fce360..ae8c4562 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -34,9 +34,8 @@ jobs: - name: Run tests run: go test -v ./... - # Shell scripts the release workflow runs, tested without a Go harness. - - name: Run shell script tests - run: e2e/sync_skills_test.sh + - name: Test the skills sync + run: make test-sync-skills - name: Build run: go build -o bin/fizzy ./cmd/fizzy diff --git a/Makefile b/Makefile index 90c69406..0ddacdc5 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: test test-unit test-scripts test-e2e e2e test-go test-file e2e-file test-run e2e-run build clean tidy help \ +.PHONY: test test-unit test-sync-skills test-e2e e2e test-go test-file e2e-file test-run e2e-run build clean tidy help \ check-toolchain fmt fmt-check vet lint tidy-check race-test vuln secrets \ replace-check security check release-check release tools \ surface-snapshot surface-check lint-actions @@ -28,7 +28,7 @@ help: @echo "Usage:" @echo " make build Build the CLI" @echo " make test-unit Run unit tests (no API required)" - @echo " make test-scripts Run shell script tests (no API required)" + @echo " make test-sync-skills Test the skills sync script (no API required)" @echo " make e2e Run owner-only CLI contract e2e tests" @echo " make test-e2e Alias for e2e" @echo " make test Alias for e2e" @@ -51,7 +51,7 @@ help: @echo "" @echo " make lint-actions Lint GitHub Actions workflows" @echo " make security lint + vuln + secrets" - @echo " make check fmt-check + vet + lint + tidy-check + race-test + test-scripts" + @echo " make check fmt-check + vet + lint + tidy-check + race-test + test-sync-skills" @echo " make release-check check + replace-check + vuln + race-test" @echo " make release Run release preflight and tag" @echo " make tools Install dev tools" @@ -93,9 +93,9 @@ build: check-toolchain test-unit: check-toolchain go test -v ./internal/... -# Run shell script tests (no API required) -test-scripts: - e2e/sync_skills_test.sh +# Test the skills sync script against a throwaway basecamp/skills checkout (no network) +test-sync-skills: + scripts/test-sync-skills.sh # Run e2e tests (requires API credentials) e2e: build @@ -180,8 +180,8 @@ replace-check: # Security suite security: lint vuln secrets -# Local CI gate (fmt, vet, lint, tidy, race-test, shell script tests) -check: fmt-check vet lint tidy-check race-test test-scripts +# Local CI gate (fmt, vet, lint, tidy, race-test, skills sync test) +check: fmt-check vet lint tidy-check race-test test-sync-skills # Release preflight release-check: check replace-check vuln diff --git a/RELEASING.md b/RELEASING.md index 1b2ebc6d..bcedea60 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -99,15 +99,20 @@ goreleaser release --snapshot --clean ## Skills sync 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.fizzy-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). A target with no `.managed-skills.fizzy-cli` yet is a first run: nothing is -removed. 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). `e2e/sync_skills_test.sh` pins the contract; it -runs under `make test-scripts` (part of `make check`) and in CI. +which several CLIs share. `scripts/sync-skills.sh` is the script every publishing CLI +runs (the copy in the `basecamp/cli` seed is canonical; only the `CLI_NAME` default +differs here). It owns only this CLI's skills there: it records the names it published +in `.managed-skills.fizzy-cli` at the target root, refuses to publish a name another +CLI's `.managed-skills.*` already holds, and removes a `skills/` only when its own +manifest lists it, the release no longer ships it, and no other manifest claims it (a +claimed name is warned about and left alone). A target with no `.managed-skills.fizzy-cli` +yet is a first run: nothing is removed. 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). When another CLI +pushes to basecamp/skills between the clone and the push, the sync is applied again +from the remote's new tip (ownership checks included) and pushed once more. +`scripts/test-sync-skills.sh` pins the contract, racing publisher included; it runs as +`make test-sync-skills` (part of `make check`) and in CI. The copy drops `*.go` and dotfiles, so `skills/embed.go` stays here and only `skills/fizzy/**` is published. Preview what a release would publish, offline: diff --git a/e2e/sync_skills_test.sh b/e2e/sync_skills_test.sh deleted file mode 100755 index 1336aba8..00000000 --- a/e2e/sync_skills_test.sh +++ /dev/null @@ -1,332 +0,0 @@ -#!/usr/bin/env bash -# -# Ownership contract for scripts/sync-skills.sh against the shared -# basecamp/skills distribution repo (basecamp/skills#5). -# -# Several CLIs publish into one target. Each keeps its own manifest there -# (.managed-skills.) and may delete only the skill directories that -# manifest lists — never a sibling's. The pre-fix script kept one shared -# .managed-skills, so every release pruned whatever the previous publisher had -# put there: hey-cli's v0.1.1 sync deleted skills/basecamp and -# skills/basecamp-doctor (08ef7ea), and basecamp-cli's next two releases -# deleted skills/hey (728a916, 42716d7). -# -# The fixture reproduces basecamp/skills as that history left it: the basecamp -# skills present and the legacy manifest listing the basecamp names. fizzy-cli -# has never published there, so its first sync is the first-run path. The -# script's default SYNC_SOURCE plays this repo; the sibling CLI is played by -# setting SYNC_SOURCE explicitly (the two sync_* helpers below are the only -# lines that differ between this file and its twins in the sibling repos). -# SKILLS_TARGET points the script at the fixture checkout and DRY_RUN=local -# applies and commits without pushing, so nothing here touches the network. -# -# This is the BATS file the sibling repos carry, run without BATS: this repo's -# toolchain has none, so a small harness at the bottom plays its part. Each -# test_* function runs in a subshell under `set -e` with an ERR trap, so the -# first failing command ends the test and names its line; `run` captures a -# command's merged output and status the way BATS' does. - -set -euo pipefail - -REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" -SYNC="$REPO_ROOT/scripts/sync-skills.sh" - -setup() { - WORK="$(mktemp -d)" - TARGET="$WORK/target" - - # Keep the operator's git identity, signing and hooks out of the fixture. - export GIT_CONFIG_NOSYSTEM=1 - export GIT_CONFIG_GLOBAL=/dev/null - export RELEASE_TAG=v4.1.0 - export SOURCE_SHA=0123456789abcdef0123456789abcdef01234567 - export DRY_RUN=local - - # basecamp/skills today. README.md and skills/orphan stand in for content no - # publisher owns; both must survive every run, orphan in particular because it - # is exactly what the dropped first-run fallback used to claim and delete. - mkdir -p "$TARGET/skills/basecamp" "$TARGET/skills/basecamp-doctor" "$TARGET/skills/orphan" - printf '# basecamp (stale)\n' > "$TARGET/skills/basecamp/SKILL.md" - printf '# basecamp-doctor (stale)\n' > "$TARGET/skills/basecamp-doctor/SKILL.md" - printf '# orphan\n' > "$TARGET/skills/orphan/SKILL.md" - printf '# basecamp/skills\n' > "$TARGET/README.md" - printf 'basecamp\nbasecamp-doctor\n' > "$TARGET/.managed-skills" - git -C "$TARGET" init -q -b main - git -C "$TARGET" remote add origin https://github.com/basecamp/skills.git - git -C "$TARGET" -c user.name=t -c user.email=t@t add -A - git -C "$TARGET" -c user.name=t -c user.email=t@t commit -q -m "Sync skills from basecamp-cli v0.11.0" - - # basecamp-cli's tree. One skill carries a nested file plus a *.go and a - # dotfile that the copy filter must drop. - BASECAMP="$WORK/basecamp/skills" - mkdir -p "$BASECAMP/basecamp/reference" "$BASECAMP/basecamp-doctor" - printf '# basecamp\n' > "$BASECAMP/basecamp/SKILL.md" - printf 'reference\n' > "$BASECAMP/basecamp/reference/api.md" - printf 'package skills\n' > "$BASECAMP/basecamp/embed.go" - printf 'secret\n' > "$BASECAMP/basecamp/.hidden" - printf '# basecamp-doctor\n' > "$BASECAMP/basecamp-doctor/SKILL.md" - - # fizzy-cli's tree. - FIZZY="$WORK/fizzy/skills" - mkdir -p "$FIZZY/fizzy" - printf '# fizzy\n' > "$FIZZY/fizzy/SKILL.md" - - 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.' -} - -teardown() { - rm -rf "$WORK" -} - -sync_basecamp() { - run env SYNC_SOURCE=basecamp-cli SKILLS_SOURCE="$BASECAMP" SKILLS_TARGET="$TARGET" "$SYNC" - [ "$status" -eq 0 ] -} - -sync_fizzy() { - run env SKILLS_SOURCE="$FIZZY" SKILLS_TARGET="$TARGET" "$SYNC" - [ "$status" -eq 0 ] -} - -# Both CLIs' skills present, each manifest listing exactly its own names, the -# legacy file a tombstone, and the unowned content untouched. -assert_shared_state() { - [ -f "$TARGET/skills/basecamp/SKILL.md" ] - [ -f "$TARGET/skills/basecamp-doctor/SKILL.md" ] - [ -f "$TARGET/skills/fizzy/SKILL.md" ] - [ "$(cat "$TARGET/.managed-skills.basecamp-cli")" = $'basecamp\nbasecamp-doctor' ] - [ "$(cat "$TARGET/.managed-skills.fizzy-cli")" = "fizzy" ] - [ "$(cat "$TARGET/.managed-skills")" = "$TOMBSTONE" ] - assert_unowned_intact -} - -assert_unowned_intact() { - [ -f "$TARGET/README.md" ] - [ -f "$TARGET/skills/orphan/SKILL.md" ] -} - -# Every run leaves the fixture committed: the next run must see a clean tree, -# as the real clone does. -assert_clean_tree() { - [ -z "$(git -C "$TARGET" status --porcelain)" ] -} - -test_interleaved_releases_keep_both_clis_skills() { - # basecamp-cli's first run: no .managed-skills.basecamp-cli yet, so nothing - # is removed and the manifest claims the current set. - sync_basecamp - [[ "$output" == *"first run for basecamp-cli"* ]] - [[ "$output" != *"Removing stale skill"* ]] - [ "$(cat "$TARGET/skills/basecamp/SKILL.md")" = "# basecamp" ] - [ -f "$TARGET/skills/basecamp/reference/api.md" ] - [ ! -e "$TARGET/skills/basecamp/embed.go" ] - [ ! -e "$TARGET/skills/basecamp/.hidden" ] - [ "$(cat "$TARGET/.managed-skills.basecamp-cli")" = $'basecamp\nbasecamp-doctor' ] - [ ! -e "$TARGET/.managed-skills.fizzy-cli" ] - [ "$(cat "$TARGET/.managed-skills")" = "$TOMBSTONE" ] - assert_unowned_intact - assert_clean_tree - - sync_fizzy - [[ "$output" == *"first run for fizzy-cli"* ]] - assert_shared_state - assert_clean_tree - - sync_basecamp - [[ "$output" != *"Removing stale skill"* ]] - assert_shared_state - assert_clean_tree - - sync_fizzy - [[ "$output" != *"Removing stale skill"* ]] - assert_shared_state - assert_clean_tree -} - -test_a_rerun_with_nothing_changed_commits_nothing() { - sync_basecamp - sync_fizzy - sync_basecamp - [[ "$output" == *"No changes to commit"* ]] - assert_shared_state - assert_clean_tree -} - -test_removes_only_a_skill_dropped_from_its_own_tree() { - sync_basecamp - sync_fizzy - rm -rf "$BASECAMP/basecamp-doctor" - - sync_basecamp - [[ "$output" == *"Removing stale skill: basecamp-doctor"* ]] - [ ! -e "$TARGET/skills/basecamp-doctor" ] - [ -f "$TARGET/skills/basecamp/SKILL.md" ] - [ -f "$TARGET/skills/fizzy/SKILL.md" ] - [ "$(cat "$TARGET/.managed-skills.basecamp-cli")" = "basecamp" ] - [ "$(cat "$TARGET/.managed-skills.fizzy-cli")" = "fizzy" ] - assert_unowned_intact - assert_clean_tree -} - -test_a_stale_legacy_manifest_from_a_pre_fix_sibling_deletes_nothing() { - sync_basecamp - sync_fizzy - # The pre-fix script rewrites .managed-skills with its own names after every - # run. The new script must read only its own manifest and put the tombstone - # back. - printf 'basecamp\nbasecamp-doctor\n' > "$TARGET/.managed-skills" - git -C "$TARGET" -c user.name=t -c user.email=t@t commit -q -am "Sync skills from basecamp-cli v0.11.1 (pre-fix)" - - sync_fizzy - [[ "$output" != *"Removing stale skill"* ]] - assert_shared_state - assert_clean_tree -} - -test_a_name_another_manifest_claims_survives_with_a_warning() { - sync_basecamp - sync_fizzy - # Collision: basecamp-cli's manifest lists fizzy, which its tree does not - # ship and fizzy-cli's manifest also lists. Deleting it would take fizzy-cli's - # skill; leave it and say so. - printf 'basecamp\nbasecamp-doctor\nfizzy\n' > "$TARGET/.managed-skills.basecamp-cli" - git -C "$TARGET" -c user.name=t -c user.email=t@t commit -q -am "collide" - - sync_basecamp - [[ "$output" == *"WARNING: not removing skill 'fizzy': also listed by fizzy-cli"* ]] - [[ "$output" != *"Removing stale skill"* ]] - assert_shared_state - assert_clean_tree -} - -test_a_first_run_against_a_legacy_only_target_removes_nothing() { - # fizzy-cli's first release with a sync: the target has never seen a - # .managed-skills.fizzy-cli, and the legacy manifest lists only basecamp's - # names. The pre-fix script would have claimed and deleted both basecamp - # skills here, as hey-cli's did (08ef7ea). - sync_fizzy - [[ "$output" != *"Removing stale skill"* ]] - [ -f "$TARGET/skills/basecamp/SKILL.md" ] - [ -f "$TARGET/skills/basecamp-doctor/SKILL.md" ] - [ -f "$TARGET/skills/fizzy/SKILL.md" ] - [ "$(cat "$TARGET/.managed-skills.fizzy-cli")" = "fizzy" ] - [ ! -e "$TARGET/.managed-skills.basecamp-cli" ] - [ "$(cat "$TARGET/.managed-skills")" = "$TOMBSTONE" ] - assert_unowned_intact - assert_clean_tree -} - -test_commits_as_source_bot_with_the_source_in_the_message() { - sync_basecamp - [ "$(git -C "$TARGET" log -1 --format=%an)" = "basecamp-cli[bot]" ] - [ "$(git -C "$TARGET" log -1 --format=%ae)" = "basecamp-cli[bot]@users.noreply.github.com" ] - [ "$(git -C "$TARGET" log -1 --format=%s)" = "Sync skills from basecamp-cli v4.1.0" ] - [[ "$(git -C "$TARGET" log -1 --format=%b)" == *"basecamp/basecamp-cli@${SOURCE_SHA}"* ]] - - sync_fizzy - [ "$(git -C "$TARGET" log -1 --format=%an)" = "fizzy-cli[bot]" ] - [ "$(git -C "$TARGET" log -1 --format=%ae)" = "fizzy-cli[bot]@users.noreply.github.com" ] - [ "$(git -C "$TARGET" log -1 --format=%s)" = "Sync skills from fizzy-cli v4.1.0" ] - [[ "$(git -C "$TARGET" log -1 --format=%b)" == *"basecamp/fizzy-cli@${SOURCE_SHA}"* ]] -} - -test_still_refuses_a_target_that_is_not_basecamp_skills_on_main() { - git -C "$TARGET" remote set-url origin https://github.com/basecamp/other.git - run env SKILLS_SOURCE="$FIZZY" SKILLS_TARGET="$TARGET" "$SYNC" - [ "$status" -ne 0 ] - [[ "$output" == *"does not point to github.com/basecamp/skills"* ]] - - git -C "$TARGET" remote set-url origin https://github.com/basecamp/skills.git - git -C "$TARGET" checkout -q -b topic - run env SKILLS_SOURCE="$FIZZY" SKILLS_TARGET="$TARGET" "$SYNC" - [ "$status" -ne 0 ] - [[ "$output" == *"checked-out branch is 'topic'"* ]] -} - -test_dry_run_local_without_a_target_still_previews_offline() { - cd "$WORK/basecamp" - run "$SYNC" - [ "$status" -eq 0 ] - [[ "$output" == *"Found 2 skill(s):"* ]] - [[ "$output" == *"skills/basecamp/SKILL.md"* ]] - [[ "$output" == *"skills/basecamp-doctor/SKILL.md"* ]] - [[ "$output" == *"skills/basecamp/reference/api.md"* ]] - [[ "$output" != *"embed.go"* ]] - [[ "$output" != *".hidden"* ]] - [[ "$output" == *"No network operations performed"* ]] -} - -test_skills_source_points_the_sync_at_another_checkout() { - cd "$WORK/basecamp" - run env SKILLS_SOURCE="$FIZZY" "$SYNC" - [ "$status" -eq 0 ] - [[ "$output" == *"Found 1 skill(s): $FIZZY/fizzy"* ]] - [[ "$output" != *"basecamp"* ]] -} - -test_dies_when_the_source_tree_has_no_skills() { - run env SKILLS_SOURCE="$WORK/empty" "$SYNC" - [ "$status" -ne 0 ] - [[ "$output" == *"no skills found under $WORK/empty/*/SKILL.md"* ]] -} - -# --- Harness --- - -# Captures a command's merged stdout+stderr in $output and its exit status in -# $status without tripping `set -e`, as BATS' run does. The ERR trap is -# suspended too: it fires on any nonzero status, `set +e` or not, and a -# refusal under test is not a failure of the test. -run() { - local saved_trap - saved_trap=$(trap -p ERR) - set +e - trap - ERR - output="$("$@" 2>&1)" - status=$? - eval "$saved_trap" - set -e -} - -# ERR trap for a test's subshell: names the failing line and command, and -# shows the last captured output so a failed assertion on it is diagnosable. -fail_report() { - # BASH_LINENO[0] is the line that tripped the trap; LINENO here would be - # this function's own. - echo " failed at line ${BASH_LINENO[0]}: $BASH_COMMAND" - if [[ -n "${output:-}" ]]; then - printf ' --- last output ---\n%s\n' "$output" | sed 's/^/ /' - fi -} - -main() { - local passed=0 failed=0 test result - for test in $(declare -F | awk '$3 ~ /^test_/ { print $3 }'); do - setup - # A subshell inside an `if` would have `set -e` ignored, so run it plainly - # and read the status afterwards. - set +e - ( - set -eE - trap fail_report ERR - "$test" - ) - result=$? - set -e - teardown - if [[ $result -eq 0 ]]; then - echo "ok ${test#test_}" - passed=$((passed + 1)) - else - echo "FAIL ${test#test_}" - failed=$((failed + 1)) - fi - done - echo - echo "$passed passed, $failed failed" - [[ $failed -eq 0 ]] -} - -main "$@" diff --git a/scripts/sync-skills.sh b/scripts/sync-skills.sh index 4520922d..385b8abd 100755 --- a/scripts/sync-skills.sh +++ b/scripts/sync-skills.sh @@ -1,252 +1,309 @@ #!/usr/bin/env bash -set -euo pipefail - -# Sync this CLI's skills to the 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. +# +# 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. # -# basecamp/skills is shared: every CLI (basecamp-cli, hey-cli, ...) publishes -# its skills as skills// at the target root. Each publisher owns one -# manifest there, .managed-skills., listing the names it published. On -# a sync it removes a skills/ only when its own manifest lists the name, -# its current skills tree no longer has it, and no other publisher's manifest -# claims it. A publisher with no manifest yet removes nothing. That is what -# stops one CLI's release from deleting another's skills (basecamp/skills#5: -# the CLIs used to share a single .managed-skills and each pruned the other). +# 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 # -# The legacy .managed-skills is rewritten on every run as a comment-only -# tombstone. A CLI still running the pre-fix script skips every line it cannot -# parse as a skill name, so it deletes nothing — whereas if the file were -# removed, that script would fall back to claiming every skills/*/ as its own. +# 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 # -# Env vars: -# SKILLS_TOKEN - GitHub token with push access to basecamp/skills (required -# when cloning, i.e. unless DRY_RUN=local or SKILLS_TARGET is set) -# 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) -# SKILLS_TARGET - Optional: existing basecamp/skills checkout to sync into -# instead of cloning (tests). The remote and branch asserts -# still apply to it. -# SYNC_SOURCE - Optional: the publishing repo's name (default: fizzy-cli). -# Names the manifest, the bot and the commit; tests use it to -# play another CLI. -# DRY_RUN - Optional: "local" (no network) or "remote" (clone but skip push). -# With SKILLS_TARGET, "local" applies and commits but skips the push. +set -euo pipefail + +CLI_NAME="${CLI_NAME:-fizzy}" +SYNC_SOURCE="${SYNC_SOURCE:-${CLI_NAME}-cli}" RELEASE_TAG="${RELEASE_TAG:?RELEASE_TAG is required}" SOURCE_SHA="${SOURCE_SHA:?SOURCE_SHA is required}" -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}" SKILLS_TARGET="${SKILLS_TARGET:-}" -SYNC_SOURCE="${SYNC_SOURCE:-fizzy-cli}" +SKILLS_TOKEN="${SKILLS_TOKEN:-}" +DRY_RUN="${DRY_RUN:-}" + TARGET_REPO="basecamp/skills" TARGET_BRANCH="main" SKILLS_SUBDIR="skills" LEGACY_MANIFEST=".managed-skills" -MANIFEST="$LEGACY_MANIFEST.$SYNC_SOURCE" +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; } -assert_remote_url() { - local url - url=$(git -C "$1" remote get-url origin) - local stripped - stripped=$(echo "$url" | sed -E 's/\.git$//') - # Validate host + owner/repo for both HTTPS and SSH forms - 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 +# A skill directory name or a source name: nothing a path could smuggle in. +plain_name() { + [[ "$1" != "." && "$1" != ".." && "$1" =~ ^[a-zA-Z0-9._-]+$ ]] } -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'" +in_list() { + local needle="$1" item + shift + for item in "$@"; do + [[ "$item" == "$needle" ]] && return 0 + done + return 1 } -# Prints the skill names listed in a manifest, one per line, skipping (with a -# warning) any line that is not a plain name — the tombstone's comment lines -# included. The pre-fix script parses .managed-skills the same way, which is -# what makes the tombstone inert for it. -manifest_names() { - local file="$1" entry label - label=$(basename "$file") - while IFS= read -r entry; do - [[ -z "$entry" ]] && continue - if [[ "$entry" == "." || "$entry" == ".." || ! "$entry" =~ ^[a-zA-Z0-9._-]+$ ]]; then - echo "WARNING: skipping invalid entry in $label: $entry" >&2 - continue +# 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 - echo "$entry" done < "$file" } -# Prints the source names whose manifests (other than this source's) list $2. -other_claimants() { - local target="$1" name="$2" file names - for file in "$target/$LEGACY_MANIFEST".*; do +# 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 - [[ "$(basename "$file")" == "$MANIFEST" ]] && continue - # Read the whole manifest first: under pipefail, `grep -q` closing the pipe - # early could fail the pipeline on a match and hide the claimant. - names=$(manifest_names "$file" 2>/dev/null) - if grep -qxF -- "$name" <<< "$names"; then - echo "${file##*/"$LEGACY_MANIFEST".}" + 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 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" +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[*]}" -source_skill_names=() -for skill_dir in "${skill_dirs[@]}"; do - source_skill_names+=("$(basename "$skill_dir")") -done - -in_source_set() { - local name - for name in "${source_skill_names[@]}"; do - [[ "$name" == "$1" ]] && return 0 - done - return 1 -} - -# --- 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" - # Copy files, excluding *.go and dotfiles - 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 without a target: 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" && -z "$SKILLS_TARGET" ]]; then - tmpdir=$(mktemp -d) - trap 'rm -rf "$tmpdir"' EXIT - echo "DRY_RUN=local: copying skills into $tmpdir" - copy_skills "$tmpdir/$SKILLS_SUBDIR" + 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) ===" - # Initialize as empty git repo to get a clean diff - 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 -# --- Target: an existing checkout, or a fresh clone --- +# --- 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 - continue +# --- Apply the sync to the target's working tree --- +# +# Every decision here is made against the tree as it stands, so a retry after a +# rejected push runs this again from the remote's new tip instead of replaying +# decisions made against a stale one. + +apply_sync() { + local name other + local previously_published=() + + # Refuse a name another source has published + for name in "${skill_names[@]}"; do + if other=$(claimed_by_other "$name"); then + die "skills/${name} is published by ${other} (listed in ${LEGACY_MANIFEST}.${other}); rename the skill or settle ownership upstream" fi - echo "Removing stale skill: $previously_managed" - rm -rf "${target:?}/$SKILLS_SUBDIR/$previously_managed" done -else - echo "No $MANIFEST in target (first run for $SYNC_SOURCE): removing nothing." -fi -# --- Write this source's manifest and the legacy tombstone --- + echo "Copying skills into ${target}/${SKILLS_SUBDIR}/..." + copy_skills "${target}/${SKILLS_SUBDIR}" + + # Remove what this source published before and no longer has + while IFS= read -r name; do + previously_published+=("$name") + done < <(read_manifest "${target}/${MANIFEST}") -printf '%s\n' "${source_skill_names[@]}" | sort > "$target/$MANIFEST" + if [[ ! -f "${target}/${MANIFEST}" ]]; then + echo "No ${MANIFEST} yet: first run for ${SYNC_SOURCE}, removing nothing" + fi + + 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 + if [[ -d "${target}/${SKILLS_SUBDIR}/${name}" ]]; then + echo "Removing stale skill: ${name}" + rm -rf "${target:?}/${SKILLS_SUBDIR}/${name}" + fi + done -cat > "$target/$LEGACY_MANIFEST" <<'EOF' + # 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. -EOF +TOMBSTONE + + git -C "$target" add -A +} -# --- Commit --- +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 From ed326de328db2aa012434d4adcfc12f9658b07d2 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 20:13:44 -0700 Subject: [PATCH 4/9] 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, 145 insertions(+), 161 deletions(-) diff --git a/RELEASING.md b/RELEASING.md index bcedea60..39e43068 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -111,8 +111,11 @@ as a comment-only tombstone so a CLI still on the pre-fix script — which delet everything its own tree lacked — deletes nothing (basecamp/skills#5). When another CLI pushes to basecamp/skills between the clone and the push, the sync is applied again from the remote's new tip (ownership checks included) and pushed once more. -`scripts/test-sync-skills.sh` pins the contract, racing publisher included; it runs as -`make test-sync-skills` (part of `make check`) and in CI. +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` pins the contract, racing +publisher included, by running the script as both CLIs against a local bare repository +— real clones, commits and pushes, no network; it runs as `make test-sync-skills` (part +of `make check`) and in CI. The copy drops `*.go` and dotfiles, so `skills/embed.go` stays here and only `skills/fizzy/**` is published. Preview what a release would publish, offline: diff --git a/scripts/sync-skills.sh b/scripts/sync-skills.sh index 385b8abd..31b0cd90 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 981ae20e8783b22c22fed2765f059782df14d71d Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 18:50:57 -0700 Subject: [PATCH 5/9] 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 31b0cd90..4541a3fe 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 6/9] 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 51ddfc1a..c88a947e 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -175,7 +175,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: @@ -186,6 +208,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 39e43068..977aac29 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -130,7 +130,10 @@ the `Sync skills` workflow (`workflow_dispatch`, stable tag, optional dry run). 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 master and dispatch; no new release needed. +merge the fix to master 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. ## AUR Setup From a917e941c9c9a09fc80d36bd6753816da60aacc4 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 19:17:37 -0700 Subject: [PATCH 7/9] 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, fizzy-cli. A copy left at the seed's placeholder, or naming another CLI, fails here instead of publishing under that source's manifest and identity. Mirrors seed/scripts/test-sync-skills.sh from basecamp/cli@19b52ed6fa30993d34017cd19ef57c79a9545bff. --- Makefile | 2 +- scripts/test-sync-skills.sh | 57 ++++++++++++++++++++++++++++++++----- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index 0ddacdc5..a8aaeac0 100644 --- a/Makefile +++ b/Makefile @@ -95,7 +95,7 @@ test-unit: check-toolchain # Test the skills sync script against a throwaway basecamp/skills checkout (no network) test-sync-skills: - scripts/test-sync-skills.sh + EXPECTED_SOURCE=fizzy-cli scripts/test-sync-skills.sh # Run e2e tests (requires API credentials) e2e: build 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 540866d23a0ffd97b17029971b966dd735d55ac9 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 22:07:26 -0700 Subject: [PATCH 8/9] 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 4541a3fe..1e6f184e 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" From 14d039fbee3855d26b08d33f9e26d9fe85a3476f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 22:07:26 -0700 Subject: [PATCH 9/9] Say that make check runs the skills sync test too --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index f0efc1b7..1a057f2d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,7 +113,7 @@ Token-based via personal access tokens. Run `fizzy setup` for interactive config ## Checks -`make check` runs `fmt-check vet lint tidy-check race-test`. There is no `surface-check` +`make check` runs `fmt-check vet lint tidy-check race-test test-sync-skills`. There is no `surface-check` in that list, but the surface gate still runs: `race-test` is `go test -race -count=1 ./internal/...`, which includes `internal/commands.TestSurfaceSnapshot`.