ci: clean up api-sync workflow (dedup + CI triggers + commit type)#19
Conversation
There was a problem hiding this comment.
CI-only change — no Python code, no CLI commands, no backwards-compat concerns, no design-principle issues.
Three minor points below (all 🟡). Dedup logic and token-fallback pattern are sound.
🟡 Dedup step ordering — the dedup check runs after Install oasdiff, Diff OpenAPI specs, and Check CLI coverage. On the duplicate path, those steps are wasted. Moving the dedup step to run immediately after Fetch upstream OpenAPI spec (it only needs /tmp/new-openapi.json) would short-circuit the expensive work on cache-hit days.
🟡 Race condition in dedup — two simultaneous workflow_dispatch runs could both pass the dedup check before either pushes a branch, producing duplicate PRs. Low risk for a daily-scheduled job, but noting it so it's a known trade-off rather than a surprise.
There was a problem hiding this comment.
This PR is CI-only, so backwards-compatibility and design-principle checks don't apply. One issue worth fixing before merge.
Overall: The three changes are logically sound and the dedup implementation is correct. The RELEASE_TOKEN fallback pattern is appropriate and the chore:/feat: split aligns with semver intent.
Open api-sync PRs were accumulating daily because the workflow did not check whether a prior PR already carried the current upstream spec. Compounding this, GITHUB_TOKEN-authored pushes and PRs do not trigger further workflow runs, so required CI checks never ran and the PRs sat BLOCKED by branch protection. - Add a dedup step that compares the new spec SHA against the spec on each open api-sync PR head; skip branch creation if a match is found. - Check out with RELEASE_TOKEN in detect and implement jobs so pushed commits trigger CI. - Create the PR with RELEASE_TOKEN so required checks run on it.
Spec-only upstream changes (e.g. an internal schema class rename) should not bump the zad-cli version, but squash-merging the PR propagates its title to main, and a feat: prefix triggers a minor release. Detect whether Claude's implement step actually added code under src/ or tests/ and set the PR title prefix accordingly: feat when new CLI surface landed, chore when only api/upstream-openapi.json moved.
c217cd5 to
87ef9c7
Compare
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| NEW_SHA=$(sha256sum /tmp/new-openapi.json | awk '{print $1}') | ||
| echo "new_sha=$NEW_SHA" |
There was a problem hiding this comment.
🟡 Minor: echo "new_sha=$NEW_SHA" logs to stdout but does not write to $GITHUB_OUTPUT. If this is intentional debug output, rename it to avoid looking like an output assignment (e.g. echo "# debug: new_sha=$NEW_SHA"). If it was meant to expose the value as a step output for later use, add >> "$GITHUB_OUTPUT".
Since new_sha is not consumed anywhere in the workflow this is non-blocking, but it will confuse future readers.
| NUM=$(echo "$line" | awk '{print $1}') | ||
| REF=$(echo "$line" | cut -d' ' -f2-) | ||
| # Grab the spec at that branch's tip via the API (no extra clone needed). | ||
| if gh api "repos/$GITHUB_REPOSITORY/contents/api/upstream-openapi.json?ref=$REF" --jq .content 2>/dev/null | base64 -d > /tmp/existing-openapi.json; then |
There was a problem hiding this comment.
🟡 Minor: Without set -o pipefail, the if tests only base64 -d's exit code. If gh api fails (e.g. the file does not exist on the branch), base64 -d receives empty input, exits 0, and the block is entered with an empty /tmp/existing-openapi.json. The SHA then mismatches so there are no false positives in practice, but the failure is silently swallowed.
A cleaner pattern that preserves the intent:
CONTENT=$(gh api "repos/$GITHUB_REPOSITORY/contents/api/upstream-openapi.json?ref=$REF" --jq .content 2>/dev/null) \
&& echo "$CONTENT" | base64 -d > /tmp/existing-openapi.json \This makes gh api failure propagate correctly.
There was a problem hiding this comment.
This PR only touches .github/workflows/api-sync.yml — no CLI commands, client methods, or tests changed, so there are no backwards-compatibility concerns and the CLI Design Principles are not in scope.
The three changes (dedup, RELEASE_TOKEN for CI triggers, chore:/feat: prefix) are all sound:
- Dedup logic: correctly compares SHA256 of the upstream spec at each open api-sync PR branch head. The
has_file_changespropagation means theimplementjob is also correctly skipped when a duplicate is found. - RELEASE_TOKEN: standard pattern for bypassing the GitHub Actions restriction on GITHUB_TOKEN-authored events not triggering further workflows. The fallback to
GITHUB_TOKENis graceful degradation. The dedup step correctly keepsGITHUB_TOKEN(read-only access is sufficient there). chore:vsfeat:prefix:grep -qE '^(src/|tests/)'is correct — the project usessrc/zad_cli/layout (confirmed).
Two minor issues flagged inline (both non-blocking):
echo "new_sha=$NEW_SHA"does not write to$GITHUB_OUTPUT— looks like a debug log, but the naming implies an output variable.gh api ... | base64 -dpipeline silently swallowsgh apifailures withoutpipefail. Functionally safe (SHA mismatch handles it), but not obvious to a future reader.
Summary
Three related fixes to the api-sync workflow, so future runs behave sensibly instead of piling up stuck PRs or cutting accidental minor releases.
1. Dedup open api-sync PRs
Before creating a branch, the workflow now compares the new spec SHA against the spec at the head of each open `api-sync`-labeled PR. If an open PR already carries the same content, we skip instead of opening a duplicate. This is why #13–#17 accumulated as identical clones of #18.
2. Trigger required CI on api-sync PRs
`GITHUB_TOKEN`-authored pushes and PRs deliberately do not trigger further workflow runs. That left the required `test (3.11/3.12/3.13)` and `pre-commit` checks unrun on api-sync PRs, so branch protection kept them `BLOCKED` indefinitely. Both checkouts (`detect`, `implement`) and the `gh pr create` call now use `RELEASE_TOKEN`.
3. Use `chore:` prefix for spec-only syncs
Squash-merging uses the PR title as the commit message on main. A `feat:` title on a spec-only change was causing python-semantic-release to cut a minor version bump for no user-visible change (this is how we ended up at v0.3.0 today). After the `implement` step we now check whether anything under `src/` or `tests/` changed vs `origin/main`. If yes, the PR title uses `feat:`. If not, `chore:` — no release.
Test plan