fix(cli-go): sync the API client with the current Management API spec - #6519
fix(cli-go): sync the API client with the current Management API spec#6519avallete wants to merge 5 commits into
Conversation
The Codegen check fetched the live staging spec on every pull request and diffed the result against the committed `pkg/api`. That conflated two questions: whether the pull request left generated code inconsistent, and whether staging had drifted ahead of the repository. The second dominated, so a spec change turned every open pull request touching `apps/cli-go` red at once, none of which the authors caused or could fix, and it made all of those runs depend on staging being reachable. Pin the spec instead. `go generate` now reads `api/v1-openapi.yaml`, making the per-pull-request check hermetic and reproducible offline: it fails only when `pkg/api` no longer matches the snapshot it was generated from. The API Sync workflow becomes the sole reader of the live spec, refreshing the snapshot and regenerating the client together in one pull request, and its diff now shows the upstream API change rather than only generated Go. This restores the model `packages/api` already uses, where `pnpm generate` runs in the sync workflow and per-pull-request drift is checked against the committed `openapi.json`. Its change detection covers the snapshot as well as `pkg/api`, so an upstream edit that codegen ignores is committed rather than refetched and discarded on every run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKDPB9bwSx5WM7juDAM7CT
Seeding the OpenAPI snapshot surfaced drift that the previous live-fetch codegen had never committed: `StorageConfigResponseOutput.MigrationVersion` is now nullable, so assigning it straight to the `string` field `storage.TargetMigration` no longer compiles. Unwrap it with the same `Get()` guard `FromRemoteAuthConfig` already uses for nullable remote fields, which leaves the local value untouched when the platform omits one -- matching the surrounding intent that unset config should not change platform defaults. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKDPB9bwSx5WM7juDAM7CT
The repository carries roughly 3.5 MB of generated output -- the Go Management API client, the `@supabase/api` client and OpenAPI document, the docs config schemas, and the lockfiles -- none of it marked. It skews GitHub's language statistics toward whichever language the largest generator emits, and it expands in pull request diffs as though it were code to review. Mark it `linguist-generated`. The attribute is GitHub-only, so git, CI, and local diffs are unaffected: the files are collapsed behind a click rather than hidden, and dropped from the language bar. turbo.json's `generate` outputs and the `go generate` directives are the source of truth for the list, so only generated files are marked -- the hand-written codegen inputs (`pkg/api/*.cfg.yaml`, `api/overlay.yaml`) are left alone, as are large but authored files such as the integration test suites and `legacy-local-config-values.ts`. `apps/docs/content/docs/commands/` is generated but gitignored, so it needs no entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKDPB9bwSx5WM7juDAM7CT
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@b88f403626eedbdd3c4e0630c824799452a2be9ePreview package for commit |
avallete
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Review
Only Claude's independent review was available; Codex did not complete. All five findings were verified and confirmed, although claude-1 was narrowed because the alleged checkout/diff mismatch is false.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | .gitattributes:16 |
reviewability |
claude | Marking the OpenAPI snapshot as generated collapses its GitHub diff, conflicting with the sync workflow's promise that the snapshot diff exposes upstream API changes. |
| 🟡 MINOR | .github/workflows/cli-go-api-sync.yml:31 |
ci |
claude | After per-PR codegen stops reading the live specification, API drift has no fallback detection if repository_dispatch ceases working. |
| ⚪ NIT | .github/workflows/cli-go-api-sync.yml:44 |
ci |
claude | The snapshot change check ignores trailing-space-only upstream changes, causing those changes to be repeatedly fetched but never committed. |
| ⚪ NIT | apps/cli-go/pkg/config/storage.go:134 |
test-coverage |
claude | The new nullable migration-version branch has no direct regression test, and its current call path cannot expose the assignment through the resulting configuration diff. |
| ⚪ NIT | apps/cli-go/api/README.md:19 |
documentation |
claude | The README does not identify the staging specification as the snapshot's authoritative upstream, while prominently linking the production Swagger UI. |
Stats
Claude findings: 5 · Codex findings: 0 · Confirmed: 5 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| # Only the generated files are listed, not their hand-written inputs | ||
| # (pkg/api/*.cfg.yaml, api/overlay.yaml). apps/docs/content/docs/commands/ is | ||
| # generated too but gitignored, so it needs no entry. | ||
| apps/cli-go/api/v1-openapi.yaml linguist-generated=true |
There was a problem hiding this comment.
🟡 MINOR · reviewability · source: claude
Marking the OpenAPI snapshot as generated collapses its GitHub diff, conflicting with the sync workflow's promise that the snapshot diff exposes upstream API changes.
Evidence: .gitattributes:7-16 explicitly says generated files are collapsed and marks apps/cli-go/api/v1-openapi.yaml; .github/workflows/cli-go-api-sync.yml:72 says the snapshot diff shows the upstream change.
Suggested fix: Do not mark apps/cli-go/api/v1-openapi.yaml as linguist-generated, or revise the workflow messaging if collapsed review is intentional.
| run: curl -fsSL "$API_SPEC_URL" -o api/v1-openapi.yaml | ||
| env: | ||
| API_SPEC_URL: https://api.supabase.green/api/v1-yaml |
There was a problem hiding this comment.
🟡 MINOR · ci · source: claude
After per-PR codegen stops reading the live specification, API drift has no fallback detection if repository_dispatch ceases working.
Evidence: .github/workflows/cli-go-api-sync.yml:3-7 has only repository_dispatch and workflow_dispatch, while lines 26-33 establish this workflow as the sole live-spec reader. trusted/docs/openapi-sync.md documents a daily schedule as the fallback for lost dispatches.
Suggested fix: Add a scheduled trigger, such as a daily run, as a fallback to repository_dispatch.
| id: check | ||
| run: | | ||
| if git diff --ignore-space-at-eol --exit-code --quiet pkg; then | ||
| if git diff --ignore-space-at-eol --exit-code --quiet api/v1-openapi.yaml pkg; then |
There was a problem hiding this comment.
⚪ NIT · ci · source: claude
The snapshot change check ignores trailing-space-only upstream changes, causing those changes to be repeatedly fetched but never committed.
Evidence: .github/workflows/cli-go-api-sync.yml:44 applies --ignore-space-at-eol to api/v1-openapi.yaml as well as pkg.
Suggested fix: Check the snapshot with an ordinary git diff and retain --ignore-space-at-eol only for generated Go files, or document that such whitespace drift is intentionally ignored.
| if value, err := remoteConfig.MigrationVersion.Get(); err == nil { | ||
| s.TargetMigration = value | ||
| } |
There was a problem hiding this comment.
⚪ NIT · test-coverage · source: claude
The new nullable migration-version branch has no direct regression test, and its current call path cannot expose the assignment through the resulting configuration diff.
Evidence: apps/cli-go/pkg/config/storage.go:13 excludes TargetMigration from TOML and JSON; lines 157-169 call FromRemoteStorageConfig only on a clone serialized to TOML. Repository search found no other caller or direct storage test.
Suggested fix: Add a focused test showing that null preserves TargetMigration and a present value replaces it.
| The specification is generated from our NestJS middleware. The latest release is | ||
| viewable as [Swagger UI](https://api.supabase.com/api/v1). |
There was a problem hiding this comment.
⚪ NIT · documentation · source: claude
The README does not identify the staging specification as the snapshot's authoritative upstream, while prominently linking the production Swagger UI.
Evidence: apps/cli-go/api/README.md:19-20 links api.supabase.com; the actual refresh source appears only in .github/workflows/cli-go-api-sync.yml:33 as https://api.supabase.green/api/v1-yaml.
Suggested fix: State explicitly that API Sync refreshes the snapshot from the staging api.supabase.green specification and distinguish that source from the production Swagger UI.
Review findings on the snapshot-pinning change, in descending order of consequence. Making the per-pull-request Codegen check hermetic removed the repository's only accidental drift detector, leaving the API Sync workflow as the sole reader of the live specification -- triggered exclusively by `repository_dispatch`. A dropped dispatch would leave drift undetected indefinitely. Add the daily cron fallback that docs/openapi-sync.md already documents for exactly this failure. Compare the snapshot byte-for-byte in the sync workflow's change detection. Under `--ignore-space-at-eol` a trailing-whitespace-only upstream edit is refetched and discarded on every run -- the loop that check was added to prevent. `pkg` keeps the flag, being oapi-codegen output rather than upstream bytes. Reconcile the generated-file marking with the sync workflow's claim that the snapshot diff shows the upstream change: both now say that collapsed is not hidden, and the sync pull request points reviewers at the diff behind the click. State in api/README.md that staging is the snapshot's authoritative upstream. It linked only the production Swagger UI, which lags the snapshot and is not what codegen reads. Cover the nullable migration-version mapping directly. `TargetMigration` is excluded from TOML and JSON, so `DiffWithRemote` cannot observe it through the serialized clone it compares. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKDPB9bwSx5WM7juDAM7CT
Red checks on
|
| Job | Window | Duration |
|---|---|---|
Run integration tests (dceccab) |
13:16:08 → 13:26:08 | 10:00 |
Analyze (javascript-typescript) (b88f403) |
14:12:50 → 14:22:50 | 10:00 |
Run stack end-to-end tests (native) (b88f403) |
14:13:01 → 14:23:01 | 10:00 |
Check code quality (b88f403) |
14:14:00 → 14:24:00 | 10:00 |
Every one of them:
- ran on
blacksmith-8vcpu-ubuntu-2404 - was killed at exactly 10:00, to the second, while no
timeout-minutesis set anywhere intest.ymlorcodeql.yml - reports no
stepsarray at all in the jobs API, while every sibling job in the same run reports full step timings - has no log archive — the logs endpoint returns a persistent 404, and the check run's
outputis empty
That combination means the runner disappeared before GitHub received step records or logs, so there is no test output because no test result was ever reported.
Two independent controls support that reading:
Run integration testswas re-run on the identical commitdceccaband passed in 7m16s with complete step telemetry. Same code, same tree; the only variable was the runner.Run stack end-to-end tests (container)— the sibling matrix leg of the job that failed native — passed in 3m37s onb88f403.
Worth noting the failure mode is visually indistinguishable from a real failure, which is the same class of problem this PR addresses: CI going red for reasons the author neither caused nor can fix.
Analyze (javascript-typescript) has been re-run. Run stack end-to-end tests (native) and Check code quality cannot be re-run yet — GitHub returns 403 This workflow is already running while CLI e2e shard 1/3 is still in flight on that run — and will be re-run as soon as it completes.
I'd suggest not papering over this with workflow-level retries; the fix belongs on the runner side.
Everything else on b88f403 is green, including the Go Test job that exercises the new pkg/config/storage_test.go cases, Run unit and integration tests, Analyze (go), Codegen, and the other three e2e legs.
Generated by Claude Code
2 participants
Summary
The
Codegencheck fetched the live staging spec on every pull request and diffed the result against the committedpkg/api. That conflated two questions: whether the pull request left generated code inconsistent, and whether staging had drifted ahead of the repository. The second dominated, so a spec change turned every open pull request touchingapps/cli-gored at once — none of which the authors caused or could fix — and it made all of those runs depend on staging being reachable.This pins the spec instead:
apps/cli-go/api/v1-openapi.yamlis a committed snapshot of the staging spec, andgo generatereads it instead ofhttps://api.supabase.green/api/v1-yaml. The per-PRCodegencheck is now hermetic: it fails only whenpkg/apino longer matches the snapshot it was generated from, and it reproduces offline.pkg/api, so an upstream edit that codegen ignores (a description, an example) is committed rather than refetched and discarded on every run.api/README.mdis rewritten. It still documented the pre-URLbeta.yamlflow, and its links to the generated files were broken relative paths.This restores the model
packages/apialready uses, wherepnpm generateruns only in the sync workflow and per-PR drift is checked against the committedopenapi.json.Drift the snapshot exposed
Seeding the snapshot was expected to regenerate
pkg/apibyte-identically. It did not: staging had drifted ahead of the committed client, which is exactly the accumulation the old check could never land. Two changes came in, and they are worth a look:StorageConfigResponseOutput.MigrationVersionis nownullable.Nullable[string]. Assigning it to thestringfieldstorage.TargetMigrationno longer compiles, soFromRemoteStorageConfigunwraps it with the sameGet()guardFromRemoteAuthConfigalready uses. Behavior change worth noting: when the platform omits the field, the local value is left untouched rather than overwritten with"", consistent with the surrounding intent that unset config should not change platform defaults.UpdateCustomHostnameResponseOutputresult fields became optional pointers. Nothing outsidepkg/apiconsumes that type, so no call sites needed changes.Generated-file marking
Committing a 468 KB spec snapshot raised the question of generated-file marking, and
.gitattributescarried nolinguist-*entries at all, so the repository's roughly 3.5 MB of generated output was skewing GitHub's language statistics and expanding in diffs. All of it is now markedlinguist-generated: the Go client and spec snapshot,packages/api/src/generated/, the docs config schemas, and the lockfiles.turbo.json'sgenerateoutputs and thego generatedirectives are the source of truth for that list, so only generated files are marked. Hand-written codegen inputs (pkg/api/*.cfg.yaml,api/overlay.yaml) are untouched, as are large but authored files such as the integration test suites. The attribute is GitHub-only — git, CI, and local diffs behave identically, and the files are collapsed behind a click rather than hidden, so the sync PR's spec diff and lockfile changes remain reviewable.Linked issue
N/A — CI reliability change, no linked issue.
Checklist
Reviewer notes
One open decision: the snapshot's file name.
v1-openapi.yamlmatches the/api/v1-yamlendpoint;beta.yamlwas the name before codegen moved to the live URL, if continuity is preferred.🤖 Generated with Claude Code
https://claude.ai/code/session_01TKDPB9bwSx5WM7juDAM7CT