fix(config): correct ProjectConfig's hosted-field coverage (CLI-2316) - #6451
Merged
Coly010 merged 5 commits intoSep 4, 2026
Conversation
…tConfig (CLI-2316) fromConfigDocument copied each hosted section wholesale, so fields with no live hosted counterpart under any CLI command (local dev ports, db.major_version, the whole db.pooler/realtime sections, db.migrations/seed, and most of experimental) leaked into ProjectConfig — contradicting its own contract of predicting state after a push. fromApiProjectConfig is untouched: db.major_version and 3 of db.pooler's fields still populate from real, read-only remote state. Verified against the actual config-sync push mappers, seed buckets, db inspect, and start's local bootstrap rather than just the schema, per CLI-2316.
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@bd456b06ab6155c42f744b3a50a1245fc36517f7Preview package for commit |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🤖 AI Review
Nine deduplicated findings are confirmed. The two major issues are loss of comparison/pull behavior for remote database fields and a new test that does not type-check. The remaining findings concern missing integrity coverage, stale ADRs, and documentation precision.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | packages/config/src/project-config/project-config.ts:337 |
correctness |
claude+codex | Excluding db.major_version and db.pooler removes four remote-backed fields from normal comparison and pull planning, leaving stock projects with a permanent unmanaged caveat and no value-level mismatch report. |
| 🟠 MAJOR | packages/config/src/project-config/project-config.unit.test.ts:1582 |
type-checking |
codex | The new runtime-shape test fails TypeScript overload resolution because Record<string, unknown> is not assignable to a fromConfigDocument operand. |
| 🟡 MINOR | packages/config/src/project-config/project-config.ts:330 |
maintainability |
claude | No integrity guard tracks the overlap between DOCUMENT_ONLY_LOCAL_PATHS and registry-managed paths, so future registry rows beneath an excluded prefix could silently become uncomparable. |
| 🟡 MINOR | packages/config/src/project-config/project-config.ts:234 |
documentation |
claude | The new document-arm omission family is not recorded in ADR 0021 or ADR 0022, leaving their enumerations of convergence behavior and unmanaged paths incomplete. |
| ⚪ NIT | packages/config/src/project-config/project-config.ts:251 |
documentation |
claude | The rationale for excluding api.port, api.tls, and api.external_url omits their use by the auth mapper to derive comparison-only external_url and jwt_issuer values. |
| ⚪ NIT | packages/config/src/project-config/project-config.unit.test.ts:575 |
test-coverage |
claude | The exhaustive test's anti-vacuity comment overstates its protection: decoding defaults materialize 12 of the 18 paths even if the handwritten probe omits them. |
| ⚪ NIT | packages/config/src/project-config/project-config.ts:208 |
naming |
claude | copyHostedValueWithoutSecrets now applies both secret stripping and document-only local exclusions, so its name no longer describes its behavior. |
| ⚪ NIT | packages/config/src/project-config/project-config.ts:314 |
documentation |
claude | The storage.buckets exception overlooks each bucket's local-only objects_path child. |
| ⚪ NIT | packages/config/src/project-config/project-config.ts:318 |
public-content |
codex | The public source comment unnecessarily includes internal Slack attribution and an individual's name. |
Stats
Claude findings: 7 · Codex findings: 3 · Confirmed: 9 · 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.
…6-supabaseconfig-fromconfigdocument-leaks-cli-only-fields
…omparable
The MAJOR correctness finding was right: db.major_version and 3 of
db.pooler's fields are real, v2GetProjectConfig-reported hosted facts
(confirmed against the OpenAPI-generated schema directly), and ProjectConfig
is consumed by config diff/pull today, not config push (which still runs on
the legacy v1 config-sync mappers with zero ProjectConfig involvement).
Excluding them from the document arm made them permanently unmanaged for
every stock project, silently blocking config pull from ever syncing them.
Only db.pooler.{enabled,port} stay excluded — v2GetProjectConfig reports
neither field at all.
Also addresses the review's other findings:
- Added an integrity test asserting no comparableProjectConfigPaths entry
can ever be a DOCUMENT_ONLY_LOCAL_PATHS member, which would have caught
the major_version/pooler regression automatically.
- Renamed copyHostedValueWithoutSecrets to copyHostedValueForDocument (it
now does more than secret-stripping, and has a single, document-arm-only
caller).
- Documented the auth mapper's comparison-only use of api.port/tls/
external_url, and storage.buckets' own local-only objects_path child.
- Corrected the exhaustive test's anti-vacuity comment: only 6 of the 18
excluded paths are optionalKey and therefore truly guarded by the probe;
the other 12 decode to schema defaults regardless.
- Recorded the new document-arm omission family in ADR 0021 (explicitly
distinguishing it from the existing unmanaged-by-push family it looks
similar to) and cross-referenced it from ADR 0022.
- Removed internal Slack/individual attribution from the public source
comment.
The type-checking finding (Record<string, unknown> not assignable to
fromConfigDocument) does not reproduce: verified via pnpm types:check and a
deliberate error-injection sanity check that tsc does check that exact line
and passes it cleanly. No change made there.
…ontent_sid
Follow-up audit against a non-exhaustive user-provided list of fields
believed to be missing hosted coverage. Verified each against the actual
config-sync push code and the OpenAPI-generated V1GetAuthServiceConfigOutput
contract (plus live recorded fixtures for defaults), the same methodology
used for CLI-2316's exclusion list.
Genuine gaps fixed:
- auth.external.figma: the schema had no figma provider at all, even though
the API genuinely has external_figma_{enabled,client_id,secret,
email_optional} — already flagged as a known gap in registry-auth.ts's own
comments. Also closes a latent secret leak: external_figma_secret had no
isSecret coverage before this and could have surfaced a raw value via
unmappedApiFields.
- auth.sms.otp_length / auth.sms.otp_expiry: new config-schema fields for
pre-existing real GoTrue fields (sms_otp_length/sms_otp_exp) that neither
the legacy shell's config-sync nor Go's own `sms` struct ever modeled —
confirmed via live fixture defaults (60s/6 chars), not guessed.
- auth.sms.twilio.content_sid: new config-schema field + registry row for a
pre-existing real GoTrue field (sms_twilio_content_sid), gated the same as
its account_sid/message_service_sid siblings.
The rest of the reported list was already working correctly (just needed
gating fields like enabled/smtp.enabled set to see them) or is deliberately
NOT hosted, verified per-item against the real API:
- skip_nonce_check has no API field for any provider except google.
- Plain "slack" (vs. slack_oidc) is a Go-parity deprecation this schema
intentionally never modeled (io.ts strips [auth.external.slack] with a
warning) — a live API field existing doesn't by itself justify reversing
that.
- experimental.s3_host/s3_region/s3_access_key/s3_secret_key have no API
counterpart anywhere (grepped the full generated contract) — CLI-2316's
exclusion of these was correct.
- auth.oauth_server.* is correctly mapped on the API arm but unconditionally
dropped from the document arm by the pre-existing applyPushUnmanagedOmissions
— this is explicitly CLI-2314's planned scope (retire push-capability
pruning), not addressed here.
Fixed two shared mock fixtures (pull/diff integration tests) whose
sms_otp_exp: 600 now surfaces as real drift against the new mapping,
breaking unrelated exact-count assertions — changed to the schema default
(60) to match a "clean baseline" project.
Coly010
changed the title
Contributor
Author
Follow-up audit: missing hosted fieldsPer request, audited the ~130-field list against the actual push code and the OpenAPI-generated contracts ( Genuine gaps, fixed (ecc61eb):
Confirmed NOT gaps (no change, verified per-item against the real contract):
Also fixed two shared mock fixtures ( |
…tion CliConfig["auth"]["sms"] gained otp_length/otp_expiry (packages/config CLI-2316 follow-up); this test's LegacyBuildGotrueEnvInput.sms literal needed the two new required fields to keep type-checking. gotrue.service.ts itself still hardcodes GOTRUE_SMS_OTP_EXP/LENGTH rather than reading input.sms.*, same as before this change — wiring the local stack up to the new config fields is a separate follow-up, not addressed here.
jgoux
approved these changes
Sep 4, 2026
Coly010
deleted the
columferry/cli-2316-supabaseconfig-fromconfigdocument-leaks-cli-only-fields
branch
September 4, 2026 10:18
Coly010
added a commit
that referenced
this pull request
Sep 4, 2026
Absorbs #6451's ProjectConfig coverage correction: the new comparable auth.sms.otp_length/otp_expiry leaves and twilio content_sid gain encoder mappings (content_sid as an optional companion so a project that never set it can still push its twilio settings), the new figma provider joins the exhaustive fixtures and pins, and the shared v2 fixture carries develop's sms_otp_exp value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ProjectConfigis meant to describe "what a Supabase project looks like on the platform." This PR corrects its coverage in both directions, verified against the actualconfig-syncpush code and the OpenAPI-generated API contracts (packages/api/src/generated/contracts.ts) rather than assumption:Removes CLI-only fields that leaked in (
fromConfigDocument'sDOCUMENT_ONLY_LOCAL_PATHS): local dev ports (api.port,db.port,db.shadow_port),api.tls/api.external_url,db.health_timeout,db.pooler.{enabled,port},db.migrations,db.seed, all ofrealtime.*, and most ofexperimental.*(OrioleDB/S3 storage-engine config,pgdelta,inspect) — none has any live hosted counterpart under any CLI command.db.major_versionanddb.pooler.{pool_mode,default_pool_size,max_client_conn}were initially excluded too but restored after review: they're real fieldsv2GetProjectConfiggenuinely reports, andProjectConfigis consumed byconfig diff/config pulltoday (notconfig push, which still runs on legacy v1 endpoints) — excluding them made them permanently unclassifiable, silently blockingconfig pullfrom ever syncing them.Adds genuinely missing hosted fields (follow-up audit against a broader field list):
auth.external.figma(the schema had nofigmaprovider at all, despite a realexternal_figma_*API surface — already flagged as a known gap in the registry's own comments),auth.sms.otp_length/auth.sms.otp_expiry(new fields for pre-existing real GoTrue settings neither the legacy shell nor Go's own CLI ever modeled), andauth.sms.twilio.content_sid(same story, Twilio-only).Everything else audited was either already working correctly (just needed a gating field like
enabled/smtp.enabledset to observe it) or is deliberately not hosted, confirmed per-item against the real API:skip_nonce_checkhas no API field for any provider except google; plainslack(vs.slack_oidc) is an existing Go-parity deprecation this schema intentionally never modeled;experimental.s3_host/s3_region/the S3 credential pair have no API counterpart anywhere.auth.oauth_server.*is correctly mapped on the API arm but unconditionally dropped from the document arm by the pre-existingapplyPushUnmanagedOmissions— that's explicitly Linear CLI-2314's planned scope (retire push-capability pruning), not addressed here.Added a standing integrity test asserting no registry-mapped field can ever land in the CLI-only exclusion list, which would have caught the
db.major_version/db.poolerregression automatically. ADR 0021/0022 updated to record the corrected family and the invariant.Linear: CLI-2316