From 9f665fbd7b8a433ee64ef33dee73cada7feed16c Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 31 Aug 2026 02:34:41 -0700 Subject: [PATCH 1/3] Release hygiene: label name, canary auto-close, coverage doc, allowlist honesty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four small fixes ahead of v0.10.0: - .github/release.yml excluded the label `github-actions`, which does not exist in this repo; the real label is `github_actions` (28 merged PRs carry it, none carry the hyphen form). Latent today because the author exclusion catches dependabot, but a human-authored CI PR would have leaked into "Other Changes". - installer-smoke.yml gains a `recover` job: on a fully green run it closes any open "Installer canary failure" issue, using the same exact-title lookup as the notify job. Without it a single blip leaves a permanently open issue — #644 sat open ten days across ten green runs. - API-COVERAGE.md's header had drifted three SDK bumps behind the pin: it claimed v0.12.0 (actual: v0.15.0), still carried the uploads-versions gap that closed with v0.14.0's typed ListVersions (basecamp/basecamp-sdk#683 — the `uploads` row already documented the shipped command), and said the field-keyed 422 fix was "past this pin" when #541 landed inside it at v0.13.0. Summary is now 184/184 with the Blocked status retained at zero. The endpoint-count reconciliation the file defers stays deferred. - check-cli-surface-diff.sh's header said to clear .surface-breaking after each release; five months and several releases in, nobody ever has, RELEASING.md never mentions it, and nothing depends on it. The comment now describes the allowlist as what it is: cumulative. --- .github/release.yml | 2 +- .github/workflows/installer-smoke.yml | 23 ++++++++++ API-COVERAGE.md | 61 ++++++++++++++------------- scripts/check-cli-surface-diff.sh | 5 ++- 4 files changed, 60 insertions(+), 31 deletions(-) diff --git a/.github/release.yml b/.github/release.yml index 89f4acc0d..75e315782 100644 --- a/.github/release.yml +++ b/.github/release.yml @@ -2,7 +2,7 @@ changelog: exclude: labels: - dependencies - - github-actions + - github_actions authors: - dependabot[bot] categories: diff --git a/.github/workflows/installer-smoke.yml b/.github/workflows/installer-smoke.yml index 2110df668..89c1b4705 100644 --- a/.github/workflows/installer-smoke.yml +++ b/.github/workflows/installer-smoke.yml @@ -157,3 +157,26 @@ jobs: # Always emit annotation so the failure is visible in the workflow summary echo "::error::Installer canary failed. See ${RUN_URL}" + + recover: + name: Close canary issue on recovery + needs: [linux, macos, windows] + if: success() + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + issues: write + steps: + - name: Close open canary failure issue + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + TITLE="Installer canary failure" + + # Same title lookup as the notify job: exact-match on the open issue. + existing=$(gh issue list --state open --search "in:title $TITLE" --json number,title --jq '[.[] | select(.title == "'"$TITLE"'")][0].number // empty' 2>/dev/null || true) + if [ -n "$existing" ]; then + gh issue close "$existing" --comment "All installer legs are green again: ${RUN_URL}" || true + fi diff --git a/API-COVERAGE.md b/API-COVERAGE.md index b6622137c..98c31c2fa 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -6,28 +6,25 @@ Coverage of Basecamp 3 API endpoints. Source: [bc3-api/sections](https://github. | Status | Sections | Endpoints | |--------|----------|-----------| -| ✅ Implemented | 49 | 183 | -| ⚠️ Blocked | 1 | 1 | +| ✅ Implemented | 50 | 184 | +| ⚠️ Blocked | 0 | 0 | | ⏭️ Out of scope | 4 | 12 | | **Total tracked** | **54** | **196** | -**183 of 184 tracked in-scope endpoints.** The one gap is `GET -/uploads/:id/versions.json` in the `uploads` section, and it is not a missing -command — the command is written. The SDK's `UploadsService.ListVersions` -decodes the response with the wrong type: the API returns version *events* -(`action`, `details`, `recording_id`), the OpenAPI spec models them as -`[]Upload`, so a caller gets Upload structs with blank `title`/`filename`/ -`status` and an `id` that is the event id and does not resolve as an upload. -Shipping a command on that would mean shipping wrong data, so it is held. -Reported to the SDK; the command lands when `ListVersions` returns an event -type. - -This file previously read "100% coverage of tracked in-scope API (184/184)". -That was wrong, and the matrix had no way to say so — with only ✅ and ⏭️ -available, a partly-covered section had to be recorded as fully covered. Hence -the third status above. It is deliberately narrow: ⚠️ means the CLI cannot -faithfully cover an endpoint for a reason outside the CLI, and it names the -blocker. +**184 of 184 tracked in-scope endpoints.** The last gap — `GET +/uploads/:id/versions.json` — closed with the v0.14.0 SDK bump. The command +(`files versions`) was written earlier but held: the SDK's +`UploadsService.ListVersions` decoded the response as `[]Upload` when the API +returns version *events*, so shipping it would have meant shipping wrong data. +basecamp/basecamp-sdk#683 (v0.14.0) returns a typed version, and the command +ships — see the `uploads` row. + +An earlier revision of this file read "100% coverage of tracked in-scope API +(184/184)" while that gap was open. That was wrong, and the matrix had no way +to say so — with only ✅ and ⏭️ available, a partly-covered section had to be +recorded as fully covered. Hence the third status above, currently marking +nothing. It is deliberately narrow: ⚠️ means the CLI cannot faithfully cover an +endpoint for a reason outside the CLI, and it names the blocker. This is not a complete bc-api parity figure. The five BC5 sections introduced by bc-api#410 that were previously untracked — `my_bookmarks`, `drafts`, `my_notes`, @@ -46,10 +43,15 @@ Out-of-scope sections are excluded from parity totals and scripts: chatbots (dif > Note: the per-row `Endpoints` column in the Coverage by Section table sums higher than the Summary totals above. The discrepancy predates the BC5 baseline; the row count (48 sections) is authoritative for the `Since` column. Reconciling endpoint counts is pre-existing maintenance, tracked separately. -**SDK version:** v0.12.0 — adds 20 exported Go methods over 13 new backend -operations. The extra seven wrap endpoints that already existed but were -reachable only through the raw generated client, which the andon-cord rule -forbids the CLI from calling. +**SDK version:** v0.15.0 (`internal/version/sdk-provenance.json` is +authoritative). The command surface below largely dates to the v0.12.0 bump, +which added 20 exported Go methods over 13 new backend operations; the extra +seven wrapped endpoints that already existed but were reachable only through +the raw generated client, which the andon-cord rule forbids the CLI from +calling. v0.13.0–v0.15.0 corrected shapes and routes (pointerized optional +fields, page-selection semantics, field-keyed 422 payloads) rather than opening +new sections; their additions here are `files replace` and the un-held +`files versions`, both from v0.14.0's upload work (basecamp/basecamp-sdk#683). Those methods land as four new command groups (`bookmarks`, `drafts`, `notes`, `calendars`) and three extensions (`assignments` gains the Up Next verbs, @@ -61,12 +63,13 @@ v0.12.0 also gave 11 `EverythingService` methods a trailing plus the two unpaginated overdue endpoints. The family is 5 unchanged + 11 changed = 16. -One v0.12.0 defect shapes a command rather than just a call: `parseErrorBody` -reads only `error`/`error_description`, so a calendar 422 carrying -`{"errors":{"color":[…]}}` arrives as a bare `validation error` naming neither -field nor value. `calendars update` therefore validates its eleven colors -client-side. The SDK fixes this past this pin (#541 returns a `fieldErrors` -map), so a later bump could surface the server's own message. +One v0.12.0 defect shaped a command rather than just a call: its +`parseErrorBody` read only `error`/`error_description`, so a calendar 422 +carrying `{"errors":{"color":[…]}}` arrived as a bare `validation error` naming +neither field nor value. `calendars update` therefore validates its eleven +colors client-side. The SDK fix is inside the pin as of v0.13.0 +(basecamp/basecamp-sdk#541 returns a field-keyed map); the client-side check +stays as a fast local answer, and the server's own message now backs it up. It carries `EverythingService` (`AccountClient.Everything()`, basecamp/basecamp-sdk#435 and #438), a 16-method account-wide aggregate family diff --git a/scripts/check-cli-surface-diff.sh b/scripts/check-cli-surface-diff.sh index 36b03639e..7ddc368f6 100755 --- a/scripts/check-cli-surface-diff.sh +++ b/scripts/check-cli-surface-diff.sh @@ -3,7 +3,10 @@ # Usage: scripts/check-cli-surface-diff.sh # # Intentional breaking changes can be listed in .surface-breaking (one per line). -# Clear that file after each release. +# The allowlist is cumulative: entries stay after release, recording every +# removal acknowledged since the baseline. No release step clears it, and +# nothing depends on it being cleared — an entry only ever excuses the removal +# it names. set -euo pipefail BASELINE="$1" CURRENT="$2" From 0e59e54875c47d22c3dc410073e3e6dc8d00dd15 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 31 Aug 2026 02:47:04 -0700 Subject: [PATCH 2/3] Address review: label-keyed canary dedup, calendars staleness, allowlist candor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex pointed out the recover job's exact-title lookup breaks the moment a human retitles the issue — the repo already learned this (#607 filed alongside the retitled #602) and built notify-issue.sh, label-keyed and fail-closed, to stop it. The notify job here was carrying a third inline title-based copy of that logic, so both jobs move to the installer-canary label: notify goes through notify-issue.sh, recover closes every open labeled issue on a green run (one worth keeping open sheds the label). Lookup failure skips the close with a warning rather than reading as "nothing open". Copilot caught the fieldErrors correction leaving the same stale claim in three more places: the calendars row here and both comments in calendars.go still said the pinned SDK cannot carry the 422 field message. All three now say what's true — the local check stays for an immediate answer naming the alternatives; the server's field-keyed message has come back since v0.13.0. The allowlist comment also overclaimed: an entry does not excuse only "the removal it names" — it excuses every future removal of that surface line, including one after a reintroduction. Said so, and named the clear-at-release step that would scope it. --- .github/workflows/installer-smoke.yml | 49 +++++++++++++++++---------- API-COVERAGE.md | 2 +- internal/commands/calendars.go | 16 +++++---- scripts/check-cli-surface-diff.sh | 8 +++-- 4 files changed, 47 insertions(+), 28 deletions(-) diff --git a/.github/workflows/installer-smoke.yml b/.github/workflows/installer-smoke.yml index 89c1b4705..8f73d458a 100644 --- a/.github/workflows/installer-smoke.yml +++ b/.github/workflows/installer-smoke.yml @@ -134,29 +134,38 @@ jobs: permissions: issues: write steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - name: File or update canary failure issue env: GH_TOKEN: ${{ github.token }} - GH_REPO: ${{ github.repository }} + REPO_SLUG: ${{ github.repository }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} EVENT_NAME: ${{ github.event_name }} LINUX_RESULT: ${{ needs.linux.result }} MACOS_RESULT: ${{ needs.macos.result }} WINDOWS_RESULT: ${{ needs.windows.result }} run: | - TITLE="Installer canary failure" BODY="The installer canary failed (event: ${EVENT_NAME}). Legs: linux=${LINUX_RESULT}, macos=${MACOS_RESULT}, windows=${WINDOWS_RESULT}. See ${RUN_URL} for details." - # Check for existing open issue before creating a new one - existing=$(gh issue list --state open --search "in:title $TITLE" --json number,title --jq '[.[] | select(.title == "'"$TITLE"'")][0].number // empty' 2>/dev/null || true) - if [ -n "$existing" ]; then - gh issue comment "$existing" --body "$BODY" || true - else - gh issue create --title "$TITLE" --body "$BODY" || true - fi - - # Always emit annotation so the failure is visible in the workflow summary + # Dedup lives in notify-issue.sh, shared with release.yml and + # aur-publish.yml, and keys on the installer-canary label rather + # than the title — a title lookup misses the moment someone + # retitles the issue while triaging it, and this job used to carry + # exactly that inline title-based copy. + notify_status=0 + scripts/notify-issue.sh \ + --repo "$REPO_SLUG" \ + --label installer-canary \ + --title "Installer canary failure" \ + --body "$BODY" || notify_status=$? + + # Always emit the annotation, whatever the notifier did. It fails + # closed rather than filing a duplicate, so on a lookup failure + # this is the only signal left. echo "::error::Installer canary failed. See ${RUN_URL}" + exit "$notify_status" recover: name: Close canary issue on recovery @@ -173,10 +182,16 @@ jobs: GH_REPO: ${{ github.repository }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} run: | - TITLE="Installer canary failure" - - # Same title lookup as the notify job: exact-match on the open issue. - existing=$(gh issue list --state open --search "in:title $TITLE" --json number,title --jq '[.[] | select(.title == "'"$TITLE"'")][0].number // empty' 2>/dev/null || true) - if [ -n "$existing" ]; then - gh issue close "$existing" --comment "All installer legs are green again: ${RUN_URL}" || true + # Same key as the notify job: the installer-canary label, which + # survives retitling. A green canary invalidates every open canary + # issue; one worth keeping open past recovery should shed the label. + # + # Lookup failure is not "nothing open": skip the close and let the + # next green run retry, but say so in the annotations. + if ! matches=$(gh issue list --state open --label installer-canary --json number --jq '.[].number'); then + echo "::warning::Could not list open installer-canary issues; leaving any open issue for the next green run." + exit 0 fi + for number in $matches; do + gh issue close "$number" --comment "All installer legs are green again: ${RUN_URL}" + done diff --git a/API-COVERAGE.md b/API-COVERAGE.md index 98c31c2fa..ec02b5dac 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -220,7 +220,7 @@ cannot faithfully cover at least one endpoint for a reason outside the CLI. A | documents | 8 | `files`, `docs` | ✅ | BC4 | - | list, show, create, update. Create supports `--subscribe`/`--no-subscribe`, `--visible-to-clients` (root vault only) | | attachments | 1 | `uploads`, `attachments` | ✅ | BC4 | - | Upload via `attach`; list embedded attachments via `attachments list` (parses `` from content) | | **Schedule** | -| calendars | 2 | `calendars` | ✅ | BC5 | - | show, update (color only). No index endpoint, so there is no `calendars list` — address one by id or pasted URL. The eleven colors are validated client-side, because the SDK at this pin cannot carry the server's 422 field message | +| calendars | 2 | `calendars` | ✅ | BC5 | - | show, update (color only). No index endpoint, so there is no `calendars list` — address one by id or pasted URL. The eleven colors are validated client-side for an immediate answer that names the alternatives; since v0.13.0 the SDK also carries the server's field-keyed 422 message | | schedules | 2 | `schedule` | ✅ | BC4 | - | Schedule container + settings | | schedule_entries | 5 | `schedule` | ✅ | BC4 | - | list, show, create, update, occurrences. Create supports `--subscribe`/`--no-subscribe` | | events | 1 | `events` | ✅ | BC4 | - | Recording change audit trail | diff --git a/internal/commands/calendars.go b/internal/commands/calendars.go index 5c9f99d27..175f86f29 100644 --- a/internal/commands/calendars.go +++ b/internal/commands/calendars.go @@ -15,11 +15,12 @@ import ( // calendarColors are the colors a calendar accepts. // -// Validated client-side because the SDK cannot report what the server says: at -// v0.12.0 its error parser reads only error/error_description, so a 422 whose -// body is {"errors":{"color":[...]}} degrades to a bare "validation error" with -// no mention of the field, the value, or the alternatives. Rejecting here turns -// that into an answer the caller can act on. +// Client-side validation began as a workaround: at SDK v0.12.0 the error +// parser read only error/error_description, so a 422 whose body is +// {"errors":{"color":[...]}} degraded to a bare "validation error" naming +// neither field nor value. The SDK carries the field-keyed message since +// v0.13.0 (basecamp/basecamp-sdk#541); the local check stays because it +// answers before any request and names the alternatives. var calendarColors = []string{ "white", "red", "orange", "yellow", "green", "blue", "aqua", "purple", "gray", "pink", "brown", @@ -144,8 +145,9 @@ Colors: ` + strings.Join(calendarColors, ", ") + ` } // validateCalendarColor rejects an unknown color before the request, naming the -// alternatives. The server would reject it too, but the SDK cannot carry its -// message back, so an unchecked value fails as a bare "validation error". +// alternatives. The server would reject it too, and since SDK v0.13.0 its +// field-keyed message does come back; checking here still answers without a +// round trip. See calendarColors for the history. func validateCalendarColor(color string) error { if color == "" { return output.ErrUsageHint( diff --git a/scripts/check-cli-surface-diff.sh b/scripts/check-cli-surface-diff.sh index 7ddc368f6..a459fdc08 100755 --- a/scripts/check-cli-surface-diff.sh +++ b/scripts/check-cli-surface-diff.sh @@ -4,9 +4,11 @@ # # Intentional breaking changes can be listed in .surface-breaking (one per line). # The allowlist is cumulative: entries stay after release, recording every -# removal acknowledged since the baseline. No release step clears it, and -# nothing depends on it being cleared — an entry only ever excuses the removal -# it names. +# removal acknowledged since the baseline — no release step clears it. Know the +# cost: an entry excuses every future removal of the same surface line, so a +# command removed, later reintroduced, and removed again passes on the old +# acknowledgement. Scoping entries to one release would take a clear-at-release +# step in RELEASING.md, which does not exist today. set -euo pipefail BASELINE="$1" CURRENT="$2" From 4a54408a8580baa246fe035f94052a49a9536331 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 31 Aug 2026 02:53:31 -0700 Subject: [PATCH 3/3] Grant the notify job contents: read for its checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers caught it: the workflow sets permissions: {} globally, and the checkout added for notify-issue.sh gets no repository access from issues: write alone — the job would die before filing the issue, exactly on the runs it exists for. Same non-inheritance note as release.yml documents for its headless-probe call. --- .github/workflows/installer-smoke.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/installer-smoke.yml b/.github/workflows/installer-smoke.yml index 8f73d458a..25ab97c7d 100644 --- a/.github/workflows/installer-smoke.yml +++ b/.github/workflows/installer-smoke.yml @@ -131,7 +131,11 @@ jobs: if: failure() runs-on: ubuntu-latest timeout-minutes: 5 + # contents: read cannot be inherited — this workflow sets permissions: {} + # at the top, so without it the checkout that fetches notify-issue.sh has + # no repository access and the job dies before it can file the issue. permissions: + contents: read issues: write steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1