feat(cli): add config diff command - #6295
Merged
Coly010 merged 26 commits intoSep 2, 2026
Merged
Conversation
Base automatically changed from
kanad-claude/config-default-values-mapping-ced354
to
develop
August 24, 2026 14:23
Coly010
force-pushed
the
kanadgupta/cli-2156-add-supabase-config-diff-to-the-cli
branch
from
August 24, 2026 14:23
8bc72ee to
0181e6c
Compare
Adds the pure comparison engine for supabase config diff: a managed-surface table (defined by the v2 project-config translation, so unmapped schema paths are unmanaged by construction), a change-set classifier with update / remote_only / local_only classes, order-insensitive type-aware equality, byte-size canonicalization, masked-secret transparency, and env-var name threading through the interpolation pipeline onto value origins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… ADR 0019 (CLI-2156) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Read-only drift report between supabase/config.toml and the effective
configuration GET /v2/projects/{ref}/config reports for a target project or
branch. Target resolution via --target (branch name/UUID/ref, link-style
acceptance) or --project-ref or the linked ref; matching [remotes.*] blocks
become the merged local operand per ADR 0018. Text, --output-format
json/stream-json, and Go-compat -o encodings share one structured payload;
--exit-code flips exit 1 on drift after the payload is out. Hoists the branch
name/UUID resolver to legacy/shared with injected error mappers. Adds ADR
0019, SIDE_EFFECTS.md, a go-cli-divergences entry, 26 integration tests
(handler at 100% branch coverage), format unit tests, and a live golden path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Colum confirmed on the ticket that net-new commands carry no Go parity contract, so the Go-compat -o/--output flag is now rejected outright (every value, pretty included) with an error pointing at --output-format, failing fast before target resolution or any network call. Drops the four Go-encoder emit branches, simplifies the JSON payload to always carry explicit nulls for unset sides, and updates SIDE_EFFECTS.md, the divergences entry, and the tests. Ticket acceptance criteria amended accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kanadgupta
force-pushed
the
kanadgupta/cli-2156-add-supabase-config-diff-to-the-cli
branch
from
August 24, 2026 20:03
0181e6c to
24607dc
Compare
…iff-to-the-cli Resolution notes beyond the textual conflicts: - ADR renumbered 0019 -> 0022 (develop took 0019-0021). - The env-var-name threading on value origins re-applied to the relocated CliConfigValueOrigin (config-document.ts); the loader body kept it via auto-merge. - Mechanical adaptation to the CliConfig rename and entrypoint split (loadCliConfig via @supabase/config/effect, CLI_CONFIG_SCHEMA_URL, EffectiveConfig, CliConfigParseError, mockLegacyCliSettings). - diff.live.test.ts rewritten for the new fixture-based live harness. The config-diff translation tables still exist at this commit; the follow-up commit consolidates them onto CLI-2230's registry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…156) CLI-2230 (#6339) landed the registry-driven ProjectConfig convergence normalizers with config diff as their intended consumer (ADR 0021), which made this branch's self-contained translation tables a parallel implementation of the same mapping. The classifier now takes two ProjectConfig projections — fromConfigDocument({config, document}) locally (raw-presence-masked) and fromApiProjectConfig(response) remotely — walks the union of their leaves filtered by isComparableProjectConfigPath, and keeps the declared-set-driven classes, masked transparency (registry isSecret rows), and env naming. remote_only suppression baselines on the default config's projection, falling back to the raw default value for push-gated containers (network restrictions' allow-all) and then the zero value. Deletes config-diff.{managed,auth,read}.ts (~900 lines); scope reporting moves to the command layer off the raw response attributes; ADR 0022 rewritten to record the consolidation; --target registered in the CLI-1896 value-consuming flag guard; purity-pin allowlists extended. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Merged
|
kanadgupta
marked this pull request as ready for review
August 27, 2026 22:25
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cae9c14a97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@5ead3bbfc9196ffdc60e0888849dbdea0ba28dcdPreview package for commit |
Coly010
requested changes
Aug 28, 2026
Coly010
left a comment
Coly010
left a comment
Contributor
There was a problem hiding this comment.
Ran an adversarial review of this PR: four independent passes (implementation, architecture, security, DX) plus live runs from source against staging (api.supabase.green) with a real linked project. Every finding was reproduced by running code, not read off the diff.
Verdict: request changes. The design underneath — the registry-driven managed surface, the secret triple-gating, convergence-projection operands — held up under genuinely hostile probing (16 secret-leak probes across all output modes failed; unknown future secret API fields fail safe; read-only is proven by mtime+content pinning; machine stdout stays payload-pure; the branch-resolver hoist is behaviour-identical with all 88 branches tests green). The blockers are concentrated in the classifier's equality/suppression edges, the flag wiring, and documented claims the code doesn't keep.
Blockers (all inline)
--exit-codeis accidentally required → plainsupabase config diff(the help's own first example) errors out. One-line fix; needs a parser-level test.- Order-insensitive array equality false-negatives on
api.schemas/api.extra_search_path→--exit-codeexits 0 on real drift. - The
remote_onlysuppression baseline misses canonicalized zeros and platform-default subjects → untouched projects report drift. Live on staging: 15 of 18 remote-only entries on a near-default project were this noise ("0s"session values + 13 template/notification subjects + 3 storage defaults). - False clean: a declared
auth.oauth_server.enableddisagreeing with the remote prints "No config differences found."
Confirmed live against staging
Beyond blocker 1, the command hard-fails on staging today: SchemaError(Missing key at ["data"]["attributes"]["storage"]["database_pool_mode"]) — the generated contract requires every block key, so the documented "partial responses degrade, never error" behaviour is unreachable (inline on SIDE_EFFECTS.md). I only obtained a successful run by locally patching the contract to make that key optional. With that patch, the happy path works well end-to-end: 29 classified changes, correct counts, (from env VAR) annotations, masked-credentials note, machine payload and exit codes all as designed.
Majors (all inline)
remote_onlyerases the local value it just compared — the output can't answer "what wouldconfig pushchange?".--workdiris silently ignored (config pushshares the bug).- Telemetry flush + linked-project cache skipped on every pre-resolution failure path (Legacy Shell Invariant #1).
- ANSI/control-character injection via unsanitized path segments and names in text output (
legacySanitizeInlineNameexists for exactly this and is used 14 lines away). --exit-codeconflates drift with failure (both exit 1).- JSON
schema_versionis the user's$schemaURL, not a payload contract version. - Response-decode failures mislabeled as network errors, dropping the upstream suggestion and bypassing the purpose-built actionability adapter.
- Dotted-path round-tripping silently drops record keys containing
..
Smaller items not carried by an inline thread
DiffProjectConfigOptionsasks forlocalanddeclaredseparately — two params that must come from the same load, with nothing enforcing it. Consider accepting the loaded pair and deriving both.countsis derived state computed in three places (config-diff.ts, both formatters +changes.lengthin the handler); one will drift. Either drop it from the package type or maketotalpart of it and use it everywhere.- No docs-site overlay (
config pushhasdocs/supabase/config/push.md), so the published reference page for a semantically subtle command falls back to one sentence — nothing on--exit-code,(unset)vs(not returned), masking, or the fact that rendered local values are convergence projections (a user who writes"1m"and sees"1m0s"will grep their file and file an issue; worth a one-line note in the output or docs). - One concept, three spellings:
[remote only]/remote-only/remote_onlyacross label, summary, and JSON. AndN difference(s)where the count is known at render time. --target <uuid>is echoed as a quoted display name (Comparing against '1111…-…'); the branch's actual name is never shown.SIDE_EFFECTS.md"config.toml is read before any network call" is false for--target <branch>— branch resolution runs first, so a broken TOML burns an API round-trip, and in a fresh directory the "runsupabase link" error wins over the friendlier "runsupabase init" one.- JSON
scopelists only present blocks; consumers must re-derive the missing set from a hardcoded list — considerscope: {present, missing}. - ADR 0022 ships as
proposed(README row too) while its body says it "was first accepted", and it's silent on three shipped decisions:--target, the-orejection, and theremote_onlylocal-value nulling.
Test-suite structure (why the suite missed the blockers)
The parser is never exercised (blocker 1); the auth: {} fixture means the largest, most transform-heavy mapping surface never runs end-to-end (blocker 3); the live test asserts only exit 0 where its own comment says cleanliness is the point; and no test asserts a secret string is absent from output. Details inline on the fixture.
What's genuinely good here
The (from env VAR) annotation, the (unset)/(not returned)/null distinctions, byte sizes rendered in the user's units, masked secrets surfaced-but-never-counted, the read-only proof in tests, and the registry consolidation over a parallel translation table are all exactly right. This is close to a really good command — it just can't currently be invoked, and each of its two core promises (no false drift, no false clean) has a reproduced counterexample.
The generated contract required every block and block key of V2ProjectConfigResponse, so a platform that reports a subset — staging predates storage.database_pool_mode; a permission-truncated response can omit whole blocks — failed the typed decode inside the API client before any consumer-side leniency could run. config diff hard-failed on every staging invocation with a SchemaError, and the documented "partially populated responses degrade, never error" behavior (ADR 0022) was unreachable. Relax all 13 object-level required arrays under data.attributes through the established openapi-overrides.json mechanism (test+replace pairs, same pattern as the SAML attribute_mapping and custom-hostname entries). The envelope (data/type/id/attributes) and array-item shapes stay strict: a partial response omits fields, not halves of array elements. @supabase/config's lenient mirror (ProjectConfigApiAttributes) already modeled every block as optional, so only the drift-guard test needed NonNullable on the generated side. A new client test pins the exact staging shape (missing storage.database_pool_mode and whole blocks) decoding successfully. Addresses PR #6295 review (Coly010): SIDE_EFFECTS.md contract thread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four classifier fixes from the PR #6295 review, all expressed as registry row knowledge instead of type-level inference: - Array equality is per-field wire semantics: rows gain `arrayEquality`, defaulting to SEQUENCE (api.schemas' first entry is PostgREST's default schema; api.extra_search_path is a literal search_path), with auth.additional_redirect_urls opting into set semantics. Local ["public","extensions"] vs remote "extensions,public" now registers as drift instead of exiting 0. - remote_only suppression no longer infers "unconfigured" from JS zeros — canonicalization turns GoTrue's sessions_timebox: 0 into the string "0s", which escaped the zero check and flagged every untouched project. Rows now declare the platform's `unconfiguredValue` (sessions "0s", the 13 provisioning-default mailer subjects pinned by the recorded config_auth fixtures, notification toggles false per supabase/auth's defaults); with no baseline at any tier the value is reported rather than guessed. A registry-driven test walks every comparable path whose config-side baseline is undefined and pins the choice. - A declared path the local projection drops (auth.oauth_server, disabled storage.analytics/vector, sentinel-pruned siblings, …) surfaces in a new `unmanaged` bucket — rendered like the masked note — instead of printing a false "No config differences found" while the file disagrees with the remote. - ConfigChange paths are segment arrays end-to-end (a test_otp phone key containing "." previously round-tripped to undefined and the drift was silently dropped); joining is display-only in diff.format.ts. The JSON payload emits paths as arrays for the same reason. Structural cleanups riding along: remote_only entries keep the materialized local default plus a `declared` flag (text mode renders "1000 (schema default — not declared in config.toml)"); the dead `defaults` option is deleted; DiffProjectConfigOptions takes the loaded {config, document, valueOrigins} pair so the projection and declared set cannot come from different loads (env references derive from the same pair — CliConfigValueOrigin.envVariables is now a list, never a comma-joined string); counts are computed once in the package and carry `total`. The handler keeps ProjectConfigParseError in the typed channel for both normalizer calls, preserving its suggestion and its purpose-built actionability adapter instead of mislabeling response problems as network errors. Addresses PR #6295 review (Coly010): array-equality, zero-suppression, oauth_server false-clean, remote_only local-nulling, dotted-path, counts, paired-operands, and response-decode threads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two flag-surface fixes from the PR #6295 review: - `Flag.boolean("exit-code")` without `Flag.withDefault(false)` is a REQUIRED flag, so plain `supabase config diff` — the help's own first example — failed with `required flag(s) "exit-code" not set`. The integration suite hands the handler a pre-built flags object and never parses, so a new diff.e2e.test.ts pins the parser at the subprocess boundary. - The global `-o/--output` flag was rejected outright, violating Legacy Shell Invariant #6 ("both --output and --output-format must be honored"). It is now honored with --output taking priority, following the backups/list pattern: `-o json|yaml|toml|env` encode the same structured payload the --output-format json envelope carries through the shared encoders, `pretty` falls through to the text renderer, and stdout stays payload-pure (root.ts already swaps in the quiet-progress layer for machine formats). This also retires the rejection error, its three papercuts (help advertising a flag the handler killed, the unactionable --debug suggestion, the missing suggestion field), and the entry the review flagged in the frozen go-cli-divergences.md record — that file is restored to develop's version, undoing the table reflow. Addresses PR #6295 review (Coly010 blockers/threads; Codex P1s): the required exit-code flag, -o handling, and the frozen-record row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…project-ref
`--target` re-invented vocabulary `link` already settled (CLI-2167): it
was a strict superset of `--project-ref`, the two were mutually
exclusive, and its description omitted the 20-lowercase-letters rule, so
a branch named like a ref silently resolved as a project. The command
now has one flag — `--project-ref` accepting a project ref or the name
(or UUID) of a branch of the linked project, with link's exact
description sentence — keeping diff flag-compatible with config push and
retiring the conflict error.
The resolution pipeline is restructured around it:
- The local config is loaded and validated BEFORE any network call: a
fresh directory gets `supabase init` instead of the resolver's
not-linked error, and a malformed TOML no longer burns a
branch-resolution round trip. Configs declaring [remotes.*] reload
once the target ref is known so the overlay stays keyed by the
RESOLVED ref; remotes-free configs load exactly once.
- The parent project ref is passed to the branch resolver lazily and
evaluated only for branch-NAME lookups, so a UUID --project-ref works
in an unlinked directory (`GET /v1/branches/{id}` needs no parent).
- Branch resolution runs under an output.task, matching the config
fetch's own progress treatment.
- A UUID target echoes as `branch <uuid> (project ref <ref>)` instead of
being quoted as if it were a display name.
- Telemetry now flushes on EVERY invocation (Legacy Shell Invariant #1)
— load failures and branch-resolution failures included — while the
linked-project cache write fires exactly when a ref resolved.
Telemetry alignment rides along: diff logs `--project-ref` verbatim only
when ref-shaped (link's guard — a branch name must never reach PostHog),
config push gains the same-family safe logging its ref-only flag always
qualified for, and the documented safe list in apps/cli/CLAUDE.md now
names the config family and the branch-accepting guard rule.
Addresses PR #6295 review (Coly010: --target vocabulary, safeFlags
drift, TOML-before-network, telemetry-flush threads; Codex: UUID-without-
link, branch-resolution progress, pre-resolution telemetry).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The remaining handler/formatter fixes from the PR #6295 review: - --workdir is honored: config loading resolves against cliSettings.workdir (the same root the project-ref resolver and linked-project cache use) instead of process.cwd(), so `config diff --workdir ../other` compares ../other's config.toml against ../other's linked project. config push shared the bug and gets the same fix. - --exit-code drift exits 2, with 1 reserved for errors (terraform plan -detailed-exitcode's convention) — `config diff --exit-code || alert` no longer fires on an expired token. - Text output is injection-safe: every non-constant string (path segments — [remotes.*] names and sms.test_otp keys are unconstrained TOML keys —, env-var names, branch/remote names, the project ref) goes through legacySanitizeInlineName, so a hostile name can no longer emit raw ANSI or forge a "No config differences found." line. Pinned by an integration test with an ESC-carrying remotes name. - The machine payload is contract-clean: `schema_version` is now an integer version of the payload shape itself (1) with the user's `$schema` URL moved to `config_schema`; `scope` is `{present, missing}` with the block set owned by @supabase/config (exported projectConfigApiBlockKeys, derived from its response mirror) instead of hand-copied; an EMPTY block record counts as not-returned, so a permission-truncated `auth: {}` can't be claimed compared while 38 auth keys print local-only; and the json/stream-json message carries the masked/unmanaged caveats so echoing it never reports "in sync" on a project whose SMTP password may have drifted. - A 404 from /v2/projects/{ref}/config classifies as invalid input (the ref names a user-selected resource), matching the branch-resolve error and the ref-addressed push.errors.ts convention. - One spelling per concept: labels and summary both say remote-only / local-only (JSON keeps snake_case remote_only), and counts pluralize properly now that they're known at render time. Addresses PR #6295 review (Coly010: workdir, exit-code conflation, ANSI injection, schema_version, scope machinery, JSON message caveat, naming threads). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The review traced every blocker it found to a hole in the test suite's
structure; this closes them:
- The shared v2 fixture's `auth: {}` meant the largest, most
transform-heavy mapping surface (~200 GoTrue keys: durations,
inversions, unconfigured sentinels, provisioning-default subjects)
never ran end to end. It now carries a realistic fresh-project auth
record at platform defaults, and the existing clean-config test proves
the whole surface classifies cleanly against an empty config.toml. The
empty-block case keeps its own test pinning the "(not returned: auth)"
scope report.
- diff.live.test.ts asserted only exit 0 while its own comment named
auth-record cleanliness as the one thing mocks can't prove — it now
asserts no `auth.` change lines on a fresh project.
- The masking scenario seeded a secret and an HMAC-shaped remote value
without asserting ABSENCE; both streams (and the JSON payload +
message) now pin `not.toContain(<secret>)` against formatter changes.
- Failure paths assert telemetry flushed (branch 404, missing config,
malformed TOML with a branch target — the last also pinning zero API
requests) and that the linked-project cache stays untouched when no
ref resolved.
Addresses PR #6295 review (Coly010): the test-suite-structure section
and the four inline test-addition asks.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add the docs-site overlay (docs/supabase/config/diff.md, the published
reference page config push already had) covering branch targeting, the
update/remote-only/local-only classes, (unset) vs (not returned),
masking and unpushable-declared notes, the convergence-projection
rendering ("1m" renders as "1m0s"), the 0/1/2 exit-code contract, and
the machine output modes.
- ADR 0022 moves to accepted and records the review-driven revisions its
body previously contradicted or omitted: the unconfiguredValue baseline
tier replacing type-level zero inference, registry-declared array
equality (sequence default), the unmanaged bucket, segment-array paths
with declared and materialized-local on change entries, the
branch-accepting --project-ref surface, honoring -o per Legacy Shell
Invariant #6, the 0/1/2 exit codes, the versioned payload contract, and
the relaxed response contract behind the scope note.
Addresses PR #6295 review (Coly010): docs-overlay and ADR-status threads.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4dc035bac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Member
Author
|
Thank you for the adversarial review — every blocker reproduced against running code made this round very direct to act on. All feedback is addressed in seven follow-up commits (no force pushes):
Verification: One deliberate non-change, called out inline as well: the linked-project cache write for an explicit Also worth noting: the three |
Contributor
|
/ai-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Both independent reviews completed. After code and convention verification, all 13 deduplicated findings are confirmed: one critical telemetry privacy issue, one major branch-resolution bug, six minor concerns, and five nits.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🔴 CRITICAL | apps/cli/src/legacy/commands/config/push/push.command.ts:39 |
telemetry-privacy |
claude+codex | config push sends any supplied --project-ref string to telemetry verbatim, including invalid values that may contain user data or secrets. |
| 🟠 MAJOR | apps/cli/src/legacy/commands/config/diff/diff.handler.ts:137 |
correctness |
claude | Branch-name resolution uses the linked branch ref as the parent, causing config diff --project-ref <branch-name> to fail after linking directly to a branch. |
| 🟡 MINOR | apps/cli/src/legacy/commands/config/diff/diff.handler.ts:171 |
user-experience |
claude | Configs containing [remotes.*] are loaded twice, duplicating load-time deprecation warnings. |
| 🟡 MINOR | apps/cli/src/legacy/commands/config/diff/diff.handler.ts:186 |
forward-compatibility |
codex | The command passes the response through strict generated decoding before its lenient config projection, so new API enum values can fail before fromApiProjectConfig runs. |
| 🟡 MINOR | packages/config/src/config-diff.ts:239 |
correctness |
codex | The set array mode actually implements multiset equality, producing drift when duplicate counts differ even though membership is identical. |
| 🟡 MINOR | apps/cli/src/legacy/commands/config/diff/diff.format.ts:236 |
api-contract |
codex | The versioned payload always emits target.branch: null, although its documented contract says the field is optional. |
| 🟡 MINOR | packages/config/src/config-diff.unit.test.ts:438 |
code-quality |
codex | The TypeScript test source contains a literal NUL byte. |
| 🟡 MINOR | apps/cli/src/legacy/commands/config/push/push.integration.test.ts:426 |
test-coverage |
claude | The subdirectory config-push test no longer exercises subdirectory discovery because it changes only the mocked runtime cwd while the handler reads cliSettings.workdir. |
| ⚪ NIT | apps/cli/src/legacy/shared/legacy-db-target-flags.ts:149 |
dead-code |
claude | target is a stale value-consuming flag registration even though no legacy command declares --target. |
| ⚪ NIT | apps/cli/src/legacy/commands/config/diff/diff.e2e.test.ts:18 |
test-cleanup |
claude+codex | The e2e test leaks its temporary working directory. |
| ⚪ NIT | apps/cli/src/legacy/commands/config/diff/diff.format.ts:7 |
module-boundaries |
claude | The pure formatter imports a regex from a service-bearing resolver module, unnecessarily pulling Effect and API-service code into its dependency graph. |
| ⚪ NIT | apps/cli/src/legacy/commands/config/diff/diff.format.ts:81 |
dead-code |
claude | LEGACY_CONFIG_DIFF_PAYLOAD_VERSION is exported despite having no external consumer. |
| ⚪ NIT | packages/api/scripts/openapi-overrides.json:629 |
diff-noise |
claude | Two unrelated pre-existing comments were changed from literal em dashes to JSON Unicode escapes. |
Findings outside the diff
- 🟡 MINOR
apps/cli/src/legacy/commands/config/push/push.integration.test.ts:426— The subdirectory config-push test no longer exercises subdirectory discovery because it changes only the mocked runtime cwd while the handler readscliSettings.workdir.
Stats
Claude findings: 9 · Codex findings: 6 · Confirmed: 13 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
…iff-to-the-cli Conflict resolution and adaptation for develop's CLI-2234 public-surface trim + compiled build and the upstream Management API spec sync: - packages/config/src/index.ts resolves to develop's trimmed public surface; the diff engine's exports (diffProjectConfig, the ConfigChange* types, projectConfigApiBlockKeys) land on the apps/cli-only ./internal subpath instead — the CLI is their only consumer today and the change-set shape will move again with config pull, so they stay out of the semver-covered contract. isEqualConfigValue leaves the barrels entirely (the package's own tests import the module file directly). entrypoint-purity snapshots regenerated accordingly. - The generated API files (openapi.json, contracts.ts, effect-client.ts) and openapi-overrides.json are taken from develop WHOLESALE: upstream's spec sync renamed V2ProjectConfigResponse to V2ProjectConfigResponse_ Output, so this branch's 26 contract-relaxation override entries now point at a schema that no longer exists, and the textual auto-merge of two large JSON rewrites is not trustworthy. The relaxation is not re-pointed but DROPPED (with its partial-decode client test and the drift-guard NonNullable adaptations): the immediate follow-up commit moves the config fetch to executeRaw per ADR 0019 rule 2, which makes the strict generated contract irrelevant to this command and restores the ADR's intended division — strict generated contract, leniency in @supabase/config's mirror. - apps/cli's diff handler/formatter import the moved symbols from @supabase/config/internal, including the goViperCompat-typed loadCliConfig develop already routes config push through. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The diff handler decoded the response through the generated typed client before the lenient config projection ever saw it — ADR 0019 rule 2 calls routing through executeRaw "required, not incidental": the generated Schema.Struct decode drops excess properties and rejects unknown enum members (pooler.pool_mode is three closed literals there), so a new platform enum value failed the command before fromApiProjectConfig's leniency could run, and a lenient decode layered on the strict output would have nothing left to be lenient about. The fetch now follows the established executeRaw pattern (projects/list): the handler checks the status itself (non-200 maps to the existing read-status error with a sanitized body; JSON-parse failures keep the decode-flagged network error) and hands the raw envelope to fromApiProjectConfig, which ADR 0019 built for exactly this input. A new integration test pins the degradation: a novel pool_mode classifies clean instead of failing the decode. This supersedes the contract relaxation from the previous review round (dropped in the merge commit): with the strict generated contract off this command's code path, partial responses AND unknown enum values both degrade through the one lenient mirror, matching ADR 0019's intended division of labor. Addresses PR #6295 AI review: the forward-compatibility finding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two findings from the PR #6295 AI review: - CRITICAL telemetry-privacy: config push marked --project-ref safe unconditionally, but nothing validates the flag before instrumentation fires — an arbitrary string (a typo, a wrong-clipboard paste) reached PostHog verbatim. The wiring now gates the whitelist on PROJECT_REF_PATTERN, same as link and config diff, and is exported (legacyConfigPushHandler, link's precedent) so integration tests drive the exact Command.withHandler wiring: verbatim for a ref-shaped value, "<redacted>" otherwise. The context-merging analytics mock those tests need existed as three identical local copies (link, functions/download, the instrumentation unit test) — hoisted to tests/helpers/mocks.ts as mockContextualAnalytics per the hoist-before-you-duplicate rule. - MAJOR correctness: config diff resolved the branch-name lookup's parent with resolver.resolve, which returns whatever .temp/project-ref holds — after `link <branch>` that is the BRANCH's own ref, which the parent-scoped branches endpoint rejects. The lazy parent now comes from legacyResolveParentScopedProjectRef (the branches family's resolver, which prefers the linked-project.json parent recovery), pinned by a test where project-ref holds a branch ref. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Configs declaring [remotes.*] only reload (for the resolved-ref overlay) when a remote's project_id actually MATCHES — checked on the already-loaded, env-interpolated document — so load-time deprecation warnings no longer repeat for every remotes-carrying config. - Set-mode array equality is true membership equality (dedupe through a Set): a repeated redirect URL is the same allow list, not drift. Duplicate-membership coverage added. - The machine payload omits target.branch when no branch was targeted, matching the documented optional contract, and the integration test now consumes LEGACY_CONFIG_DIFF_PAYLOAD_VERSION so the exported constant is the payload contract's single pinned source. - The ref/UUID patterns move to a service-free legacy-ref-patterns.ts (re-exported by the resolver), so the pure formatter no longer pulls Effect and API-service code into its dependency graph. - The stale `target` entry leaves the value-consuming flag registry (no legacy command declares --target anymore), the diff e2e test removes its temp directory in a finally, the push subdirectory test exercises workdir-based project-root discovery again (it varied the now-unused mocked cwd), and a literal NUL byte in the config-diff unit test source becomes the escaped backslash-u0000 spelling. Addresses PR #6295 AI review: the six minor/nit findings not covered by the preceding commits (the em-dash diff noise disappeared with the override revert in the merge commit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Develop is merged in (merge commit The merge, and one deliberate reversal. Develop's CLI-2234 surface trim moved the diff engine's exports ( Finding → commit map:
Verification: |
…rom diff classification Declared-but-unmanaged paths were double-reported: listed in the unmanaged 'not compared' bucket while still classifying as remote_only with a false 'local: (unset)' — exit 2 forever on in-sync projects. The unmanaged set is now computed before the classification loop and structurally excluded. Mailer template/notification subject suppression no longer string-matches pinned platform copy (which staging's reworded defaults already broke): their rows declare platformRendered, suppressing any remote value while the local projection is silent. Declared subjects classify normally.
A successful `config diff --exit-code` drift signal (exit 2) was collapsed to exit_code 1 with an unknown-category ProcessControlledFailure — polluting the error KPIs with intended outcomes. Effect success + exit 2 on config diff now records exit_code 2 with no failure metadata; every other combination keeps the prior behavior.
…dered mechanisms The develop merge dropped the openapi-overrides contract loosening, and the pinned mailer-subject baselines were replaced by structural platformRendered suppression — the ADR now records the executeRaw access-path decision and the internal-only scope of the diff engine instead of the mechanisms that no longer ship.
…faces - -o table/csv fall through to text like the rest of the family instead of silently env-encoding under live spinner ANSI; only -o env hits encodeEnv. - The summary message and text output carry a not-compared caveat when the API response is missing blocks, so a scoped token can no longer produce an unqualified 'No config differences found.' on a partial comparison. - Branch-NAME targets resolve the linked parent BEFORE the spinner and fail with link's shared not-linked/invalid messages (naming the passed value) instead of the generic resolver prompt/error; resolved branch refs are shape-guarded so a provisioning branch never reaches /v2/projects//config. - Parse failures name the actual config file (config.toml or config.json); 401/403/404 on the config read get purpose-written messages. - --exit-code drift prints a stderr reason line in text mode before exit 2. - The project-ref pattern has one definition (legacy-ref-patterns.ts); telemetry redaction wiring for diff is pinned by tests like push/link. - legacySanitizeInlineName also strips C1, bidi, and line-separator controls. - e2e parser pin gains a positive anchor so it can no longer pass vacuously.
…ntract The published page now says only platform-managed properties are compared (local-stack sections never report), documents the JSON payload's top-level fields, and flags -o env as counts-only.
… decision The issue thread settled this on 2026-08-21: config diff is net-new with no Go-compat contract, so every -o value fails fast with a pointer to --output-format before any resolution or network call. A later bot review comment flipped the command to honoring the legacy machine formats, which contradicted that decision — this restores the rejection (widening the wrapper's per-command enum so table/csv also get the pointed message) and records the net-new exception on Legacy Shell Invariant #6 so it isn't relitigated.
- SIDE_EFFECTS.md no longer opens a line with '> 0', which CommonMark read as a blockquote and oxfmt would have mangled; both formatter failures the range introduced are fixed and the changed set is oxfmt-clean. - ADR 0022's command-surface bullet now records the -o rejection instead of the interim honored-flag direction. - The global -o choice list is single-sourced (LEGACY_GLOBAL_OUTPUT_FORMATS in global-flags.ts): the diff command's outputFormats widening and the rejection test both consume it, so a new value extends coverage instead of silently regressing to the wrapper's pflag message. - Parse failures render cause.path workdir-relative, matching the family's supabase/config.* phrasing; branch-not-found gains the shared ref-typo hint; stale unconfiguredValue comment in the live test updated; the -o telemetry consequence of the enum widening is documented.
Coly010
approved these changes
Sep 2, 2026
…6-add-supabase-config-diff-to-the-cli
…6-add-supabase-config-diff-to-the-cli
Coly010
deleted the
kanadgupta/cli-2156-add-supabase-config-diff-to-the-cli
branch
September 2, 2026 11:36
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.
Implements CLI-2156: a read-only
supabase config diffthat classifies drift betweensupabase/config.tomland the effective configurationGET /v2/projects/{ref}/configreports for a target project or branch. Never writes. Builds on CLI-2155's sparse subtraction/defaults (#6205) and consumes CLI-2230'sProjectConfigconvergence normalizers (#6339) as its comparison operands.What changed
packages/config— the comparison core (ADR 0022)config-diff.ts: a pure classifier producing a typedConfigChangeSet(update/remote_only/local_only, plusmaskedand per-classcounts), reusable byconfig pullwithout the command layer.fromConfigDocument({config, document})locally (raw-presence-masked, canonicalized, secret-omitting) andfromApiProjectConfig(response)remotely. All wire knowledge — renames, boolean inversions, duration/byte-size conversions, the GoTrue key table — lives in the sharedprojectConfigMappingRowsregistry, so the managed surface isisComparableProjectConfigPathby construction: a path with no registry row ([studio], ports, image pins,[realtime]locals,workers) can never be reported.updatevs suppressed/remote_only).remote_onlysuppression baseline: the default config's own convergence projection, falling back to the raw schema default for push-gated containers (network restrictions' allow-all default is exactly the platform's unconfigured state), then to the type's zero value. An untouched project diffs clean.isSecretrows) are "present, unknown": both normalizers omit them, they never classify or count, and locally-declared ones surface viamaskedso a clean diff is visibly a partial claim.additional_redirect_urlsorder is not drift) and string/number, string/boolean scalar tolerance.io.ts/lib/env.ts: value origins now record the resolving env-var name, so a change on anenv()-fed property names the variable.apps/cli— legacy-shell commandlegacy/commands/config/diff/: command + handler + errors +SIDE_EFFECTS.md. Target resolution:--target <branch-name|uuid|ref>(same acceptance aslink; 404 → "runsupabase branches list"),--project-ref, else the linked ref;--target+--project-reftogether is a hard error. When the resolved ref matches a[remotes.*]block'sproject_id, the local operand is the branch's merged effective config (ADR 0018), otherwise the base config — the echoed line always says which.(unset)/(not returned);(from env VAR)annotations; masked note) and--output-format json|stream-json(structured payload withschema_version,target,scope,changes[],masked[],counts). The comparison-scope line lists which response blocks were carried. The Go-compat-o/--outputflag is rejected outright (every value,prettyincluded) with an error pointing at--output-format— per the ticket-thread decision that net-new commands carry no Go parity contract.--exit-codesets exit 1 on drift viaProcessControl.setExitCodeafter the payload is emitted.legacy/shared/legacy-branch-ref.resolver.ts: the branch name/UUID/ref resolver hoisted out of the branches family (cross-family use) with injected error mappers; the branches family keeps a thin binding so its call sites are unchanged.Docs: ADR 0022 (classification + managed surface, incl. the registry consolidation and its relationship to ADR 0019/0020/0021),
go-cli-divergences.mdTS-only command entry (replacing the ticket's stalego-cli-porting-status.mdcriterion), per-commandSIDE_EFFECTS.md.History note for reviewers
The branch was first implemented with a self-contained translation table (a ~900-line port of the Go CLI's
FromRemoteAuthConfig). After #6339 landed the registry-driven normalizers ondevelop— with this command as their named consumer — the merge commit (766182f) broughtdevelopin and the follow-up (cae9c14) deleted the tables and rebuilt the classifier on the registry, per the repo's no-parallel-code-paths policy. ADR 0022's "Considered Alternatives" records both designs.Decisions & assumptions worth reviewing
schema_versionin the payload is the file's$schemaref (falling back to the current schema URL) — CLI-2155 shipped no separate version token.SIDE_EFFECTS.md.-osupport was implemented per the original acceptance criteria, then removed after Colum confirmed on the ticket that parity isn't a goal for net-new commands. The flag now fails fast with a bespoke invalid-input error; the JSON payload always carries explicitnullfor unset sides.masked[]/ a text note rather than silently skipped, and never affect--exit-code.local_onlywhen declared locally, silent otherwise; the scope line calls out missing blocks. (Today's v2 schema requires all six blocks, so this is belt-and-braces for API evolution and permission-trimmed keys.)remote_onlyentries (tracked on CLI-2266).🤖 Generated with Claude Code