fix(config): retire push-capability pruning from ProjectConfig (CLI-2314) - #6469
Conversation
…er projection change
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@0676789936a20d45f59a17dd2184153a4b290a35Preview package for commit |
There was a problem hiding this comment.
🤖 AI Review
All 13 deduplicated findings were verified and confirmed. The most consequential issues are silently skipped email-template bodies when local Auth is disabled and newly pushable stock-template values that can disable or overwrite hosted configuration. The remaining findings concern inaccurate documentation, stale test fixtures, and missing positive coverage.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | packages/config/src/project-config/project-config.ts:753 |
compatibility |
claude | A stock [storage.analytics] enabled = false declaration now becomes an update that disables a remotely enabled Analytics/Iceberg catalog. |
| 🟠 MAJOR | apps/cli/src/legacy/commands/config/push/push.encoders.ts:1387 |
compatibility |
claude | The stock [auth.oauth_server] enabled = false declaration can now disable a hosted OAuth server that was enabled outside the CLI. |
| 🟠 MAJOR | apps/cli/src/legacy/commands/config/push/push.plan.ts:188 |
compatibility |
claude | Users who set the local-only auth.enabled toggle false can now push all other stock Auth declarations, including local-development URLs, over hosted values. |
| 🟠 MAJOR | apps/cli/src/legacy/commands/config/push/push.handler.ts:316 |
correctness |
claude+codex | Auth email template and notification bodies are silently skipped when auth.enabled is false even though the Auth resource and its other declared fields are now pushed. |
| 🟡 MINOR | apps/cli/src/legacy/commands/config/push/SIDE_EFFECTS.md:102 |
documentation |
claude | The push side-effects documentation still says auth.enabled and storage.enabled gate their resources, contradicting the new implementation. |
| 🟡 MINOR | apps/cli/src/legacy/commands/config/push/push.handler.ts:568 |
documentation |
claude | The handler comment incorrectly says only db.network_restrictions can receive disabled status; undeclared db.ssl_enforcement does too. |
| 🟡 MINOR | apps/cli/src/legacy/commands/config/pull/pull.handler.ts:242 |
documentation |
claude | The claim that every prune depends on the written path's own state and that the unpushable branch has no live trigger is false for the SMTP-to-email-rate-limit presence cascade. |
| 🟡 MINOR | apps/cli/src/legacy/commands/config/pull/pull.integration.test.ts:599 |
test-coverage |
claude | The pull unpushable-warning path no longer has positive integration or rendering coverage after the OAuth-server test was changed to negative assertions. |
| 🟡 MINOR | apps/cli/src/legacy/commands/config/config.format.ts:193 |
user-output |
claude+codex | The unmanaged-path message falsely says every omitted path belongs to a disabled section. |
| 🟡 MINOR | packages/config/src/project-config/project-config.ts:799 |
documentation |
claude | The DISABLED_SENTINEL_PRUNES header still describes obsolete legacy-push behavior and attributes rules to the wrong constant. |
| ⚪ NIT | packages/config/src/config-diff.ts:89 |
documentation |
claude | The ConfigChangeSet.unmanaged docstring still defines the concept specifically as state config push cannot communicate, despite the PR's actor-neutral semantics. |
| ⚪ NIT | packages/config/src/project-config/project-config.ts:857 |
documentation |
codex | The SMTP sentinel comment says host is excluded from dropKeys, but the rule includes it. |
| ⚪ NIT | apps/cli/src/legacy/commands/config/push/push.format.unit.test.ts:656 |
test-quality |
claude | Formatter tests still use auth.oauth_server.enabled as an unmanaged path and a gated-off Auth resource, states the production pipeline can no longer produce. |
Findings outside the diff
- 🟠 MAJOR
apps/cli/src/legacy/commands/config/push/push.handler.ts:316— Auth email template and notification bodies are silently skipped whenauth.enabledis false even though the Auth resource and its other declared fields are now pushed. - ⚪ NIT
apps/cli/src/legacy/commands/config/push/push.format.unit.test.ts:656— Formatter tests still useauth.oauth_server.enabledas an unmanaged path and a gated-off Auth resource, states the production pipeline can no longer produce.
Stats
Claude findings: 12 · Codex findings: 3 · Confirmed: 13 · 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.
- push.handler.ts: stop gating auth email template/notification content loading on auth.enabled, which silently pushed empty content over real hosted customization now that the auth resource is always considered - fix stale docs that still described the pre-CLI-2314 auth/storage gate, the wrong resource-disabled example, and the wrong constant for hook/SMS sentinel rules - fix a comment that claimed host was excluded from the SMTP dropKeys list when it is included - reword the unmanaged-path message to not assume every omission is caused by a disabled section (SMTP-linked rate-limit and unselected SMS provider credentials are omitted for other reasons) - add positive integration coverage for the pull "unpushable" warning, which had none since the oauth_server case was rewritten to negative assertions; corrects the accompanying docstrings, which understated the real (narrower) trigger condition once actually verified - repoint stale oauth_server.enabled/disabled-resource fixtures in formatter unit tests to states the pipeline can still produce
Review round follow-up: still framed unmanaged paths as specifically what config push cannot communicate, and didn't mention the cross-path smtp/rate_limit_email_sent case.
|
Addressed the AI review round (13 findings). Two of them were reported "outside the diff" with no inline thread to reply to directly:
The other 11 findings each got a reply on their own thread. 8 are fixed and those threads are resolved. 3 (the storage.analytics/oauth_server/auth-stock-declarations "compatibility" findings) are confirmed accurate but are the direct, intended consequence of what this PR set out to do — I've left those open with the existing mitigation (push's per-resource interactive confirmation) explained, flagged for your call rather than deciding unilaterally on a design we worked out together. Commits: |
…note CI's fmt:check caught oxfmt table-alignment drift from an earlier manual edit. Also corrected the Files Read table's auth email template row, which still said content is only read "when auth.enabled" - that gate was removed in the CLI-2314 review-round fix (e46fdbd).
…eed risk Review round decision (not a code change): the interactive per-resource confirmation is accepted as sufficient mitigation for previously-invisible fields (storage.analytics, auth.oauth_server, auth's stock dev values) becoming genuinely pushable. Spelling out in --help that a non-interactive run defaults to proceeding without an explicit answer, so a script or agent driving this command should check `config diff` first rather than relying on a prompt it may not observe.
Nothing in this repo consumes it via the root entrypoint — the only real usage is packages/config's own internal import from ./project-config/hosted-sections.ts, which predates this PR and is untouched. It was added purely on the assumption that Studio (a separate repo) would want it; add it back if and when that repo actually asks for it.
Summary
fromConfigDocumentbaked the legacyconfig pushpipeline's write-capability envelope into theshared
ProjectConfigrepresentation (ADR 0021's original "push convergence" ruling). That wasright when the CLI's own push command was the only reader; it's wrong now that Studio reads the
same package for drift detection via the Management API, an actor with none of push's write-path
limitations. Concretely, this hid real hosted customization from every consumer — not just push —
behind three families of unconditional/blanket omissions.
Builds on #6454 (CLI-2313, merged into
develop) — this branch has already been rebased ontodeveloppost-merge. Nothing here depends on the v2 PATCH endpoint (BRA-245/CLI-2317); everythingworks over the v1 write endpoints #6454 already wired up.
Linear: CLI-2314.
What changed
applyPushUnmanagedOmissionsdeleted — this unconditionally dropped the wholeauth.oauth_serversubtree and droppedstorage.analytics/storage.vectorentirely wheneverenabled: false, because legacy push never sent those shapes. That pipeline no longer exists(CLI-2313 deleted
config-sync/*.sync.ts); push now consumesProjectConfigdirectly.DISABLED_SENTINEL_PRUNESre-derived from the platform's actual data model, not from whatlegacy push happened to send. Every surviving entry's docstring now states a real, verified
reason a sibling field goes inert server-side when its container is off (e.g.
api.enabledisthe same wire fact as
db_schema.length > 0, not an independent field the API arm doesn'talready gate on its own). A new cross-arm symmetry test makes this machine-checked: a disabled
document projection and the equivalent disabled API-response projection must be byte-identical.
auth.enabled/storage.enabledblanket section-prunes. These had nothing to dowith the platform —
auth.enabledis literally "Enable the local GoTrue service" with nohosted/registry equivalent, yet disabling it wiped the entire rest of the
authsection (SMTP,external providers, captcha, everything) from the shared representation for anyone reading it.
Fixed a real bug this exposed:
push.plan.ts'slegacyPushResourceEnabledwas gating theentire
auth/storageresource dispatch on that same local toggle, so a declared change couldbe silently dropped.
auth/storagenow follow the same no-gate patternapi/db.settingsalready used. No new "default comparison" machinery was needed —
diffProjectConfig's existingdeclaredflag (computed generically from raw TOML presence for every comparable path) alreadykeeps an undeclared default value from ever being pushed over a real hosted customization.
auth.oauth_server.*wired up as genuinely pushable. It's writable viaUpdateAuthConfigBody(confirmed against the OpenAPI spec) — push's own
SIDE_EFFECTS.mdhad been carrying it in theunsupported-prefix list as an explicitly-labeled placeholder for this exact issue.
ConfigAbsencePolicy("absent-is-default"/"absent-is-hands-off") for the twooperand policies
fromConfigDocumentalways implicitly implemented depending on which overloadyou called. Documents the one genuinely hazardous cell (an absent field masquerading as a real
declaration when diffing in dense mode) and the
declaredRoot = {}cliff a caller hits byomitting
document— exactly the calling shape Studio would use.disabled" instead of "cannot be pushed").
ProjectConfigas a shared multi-actor representation, plusmatching touch-ups to ADR 0022/0023 and cleanup of stale references to the deleted function.
Verification (post-rebase onto
develop)packages/config:bun --bun vitest run --project unit— 1264 passed.apps/cli:pnpm types:checkclean;pnpm test:unit— 5984 passed, 1 skipped;pnpm test:integration— 3438 passed.Follow-ups (out of scope here)
local.auth.enabled/local.storage.enablednamespace sosupabase start's "run thisDocker service" concept is structurally separate from hosted management, instead of overloading
auth.enabled/storage.enabled.LEGACY_PUSH_UNSUPPORTED_PREFIXES's remainingdb.poolerentry against the OpenAPI spec —pool_mode/default_pool_sizeare writable viaUpdateSupavisorConfigBody, onlymax_client_conngenuinely isn't.