Skip to content

feat(cli): add config diff command - #6295

Open
kanadgupta wants to merge 6 commits into
developfrom
kanadgupta/cli-2156-add-supabase-config-diff-to-the-cli
Open

feat(cli): add config diff command#6295
kanadgupta wants to merge 6 commits into
developfrom
kanadgupta/cli-2156-add-supabase-config-diff-to-the-cli

Conversation

@kanadgupta

@kanadguptakanadgupta commented Aug 21, 2026

Copy link
Copy Markdown
Member

Implements CLI-2156: a read-only supabase config diff that classifies drift between supabase/config.toml and the effective configuration GET /v2/projects/{ref}/config reports for a target project or branch. Never writes. Builds on CLI-2155's sparse subtraction/defaults (#6205) and consumes CLI-2230's ProjectConfig convergence normalizers (#6339) as its comparison operands.

What changed

packages/config — the comparison core (ADR 0022)

  • config-diff.ts: a pure classifier producing a typed ConfigChangeSet (update / remote_only / local_only, plus masked and per-class counts), reusable by config pull without the command layer.
  • Both operands are CLI-2230 convergence projections (ADR 0021): fromConfigDocument({config, document}) locally (raw-presence-masked, canonicalized, secret-omitting) and fromApiProjectConfig(response) remotely. All wire knowledge — renames, boolean inversions, duration/byte-size conversions, the GoTrue key table — lives in the shared projectConfigMappingRows registry, so the managed surface is isComparableProjectConfigPathby construction: a path with no registry row ([studio], ports, image pins, [realtime] locals, workers) can never be reported.
  • Classification is driven by the raw document's declared-key set — the one signal a decoded config cannot recover — so "the file wrote the default" and "the file is silent" classify differently (update vs suppressed/remote_only).
  • remote_only suppression 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.
  • Secrets (the registry's isSecret rows) are "present, unknown": both normalizers omit them, they never classify or count, and locally-declared ones surface via masked so a clean diff is visibly a partial claim.
  • Residual equality is meaning-based: multiset array comparison (additional_redirect_urls order 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 an env()-fed property names the variable.

apps/cli — legacy-shell command

  • legacy/commands/config/diff/: command + handler + errors + SIDE_EFFECTS.md. Target resolution: --target <branch-name|uuid|ref> (same acceptance as link; 404 → "run supabase branches list"), --project-ref, else the linked ref; --target + --project-ref together is a hard error. When the resolved ref matches a [remotes.*] block's project_id, the local operand is the branch's merged effective config (ADR 0018), otherwise the base config — the echoed line always says which.
  • Output: text (unset renders (unset) / (not returned); (from env VAR) annotations; masked note) and --output-format json|stream-json (structured payload with schema_version, target, scope, changes[], masked[], counts). The comparison-scope line lists which response blocks were carried. The Go-compat -o/--output flag is rejected outright (every value, pretty included) with an error pointing at --output-format — per the ticket-thread decision that net-new commands carry no Go parity contract. --exit-code sets exit 1 on drift via ProcessControl.setExitCode after 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.md TS-only command entry (replacing the ticket's stale go-cli-porting-status.md criterion), per-command SIDE_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 on develop — with this command as their named consumer — the merge commit (766182f) brought develop in 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

  1. Managed surface = the shared registry (vs schema annotations or response-key walking): single source of truth shared with Studio and the future push mapper; a missing row fails safe (silently unmanaged, never misreported). Alternatives in ADR 0022.
  2. Suppression baseline chain (default projection → raw schema default → zero value) is a judgment call: without it, every unconfigured OAuth provider and the platform's allow-all network restrictions read as drift on untouched projects. The cost is that a remote override that happens to equal a schema default on an undeclared path is not reported.
  3. schema_version in the payload is the file's $schema ref (falling back to the current schema URL) — CLI-2155 shipped no separate version token.
  4. Rendered "local" values are convergence projections (ADR 0021), i.e. what pushing the file would produce hosted — canonicalized durations/byte sizes, push-gated omissions — not necessarily the file's literal spelling. Documented in SIDE_EFFECTS.md.
  5. Legacy -o support 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 explicit null for unset sides.
  6. Masked credentials are transparent: listed in masked[] / a text note rather than silently skipped, and never affect --exit-code.
  7. Partial responses degrade, never error: comparable paths the response doesn't carry are local_only when 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.)
  8. The classifier inherits ADR 0021's limits: unconditionally-mapped fields with no local-silence signal can surface as honest-but-push-unactionable remote_only entries (tracked on CLI-2266).

🤖 Generated with Claude Code

Base automatically changed from kanad-claude/config-default-values-mapping-ced354 to developAugust 24, 2026 14:23
@Coly010
Coly010force-pushed the kanadgupta/cli-2156-add-supabase-config-diff-to-the-cli branch from 8bc72ee to 0181e6cCompareAugust 24, 2026 14:23
kanadguptaand others added 4 commits August 24, 2026 15:03
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
kanadguptaforce-pushed the kanadgupta/cli-2156-add-supabase-config-diff-to-the-cli branch from 0181e6c to 24607dcCompareAugust 24, 2026 20:03
kanadguptaand others added 2 commits August 27, 2026 16:48
…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>
@kanadgupta

Copy link
Copy Markdown
MemberAuthor

Merged develop in (merge commit, no rebase) and consolidated the comparison core onto CLI-2230's registry:

  • Merge commit (766182f): conflict resolution + mechanical adaptation — our ADR renumbered 0019→0022 (develop took 0019–0021), the env-var-name threading re-applied to the relocated CliConfigValueOrigin, CliConfig/entrypoint-split renames, the live test rewritten for the fixture-based live harness, and the v2 test fixture gaining the new required database.major_version.
  • Consolidation (cae9c14): feat(config): add toProjectConfig and the ProjectConfig hosted subset (CLI-2230) #6339 built fromConfigDocument/fromApiProjectConfig explicitly as this diff's operands (ADR 0021), so the branch's own ~900-line translation tables (config-diff.{managed,auth,read}.ts) are deleted and diffProjectConfig now compares the two convergence projections over isComparableProjectConfigPath, with masked secrets from the registry's isSecret rows and remote_only suppression baselined on the default config's projection (falling back to raw schema defaults for push-gated containers, e.g. network restrictions' allow-all). Classification semantics (declared-set-driven update/remote_only/local_only, masked note, env-var naming, --exit-code) are unchanged. ADR 0022 records the consolidation and its relationship to ADR 0019/0020/0021.

@kanadgupta
kanadgupta marked this pull request as ready for review August 27, 2026 22:25
@kanadgupta
kanadgupta requested a review from a team as a code ownerAugust 27, 2026 22:25

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +72 to +76
if (Option.isSome(goOutputFlag)) {
return yield* new LegacyConfigDiffOutputFlagUnsupportedError({
message:
"the -o/--output flag is not supported by config diff; use --output-format json|stream-json instead.",
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Honor the legacy output flag instead of rejecting it

When any global -o/--output value is supplied—including pretty—the handler exits before performing the diff. Legacy handlers are required to honor both output mechanisms, with --output taking priority, so scripted invocations such as config diff -o json unexpectedly fail instead of producing the requested payload. Implement the legacy encodings rather than rejecting the flag.

AGENTS.md reference: apps/cli/AGENTS.md:L300-L300

Useful? React with 👍 / 👎.

if (flags.exitCode && changeSet.changes.length > 0) {
yield* processControl.setExitCode(1);
}
}).pipe(Effect.ensuring(linkedProjectCache.cache(ref)), Effect.ensuring(telemetryState.flush));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Flush telemetry for pre-resolution failures

The finalizers only wrap the inner effect after ref has been resolved, so unsupported-output errors, flag conflicts, missing-link failures, and branch-resolution failures all bypass telemetryState.flush. Those are normal command invocations and failures, and omitting the flush violates the legacy lifecycle invariant and leaves telemetry state incomplete; wrap the full invocation with the telemetry finalizer while keeping the ref-dependent cache finalizer appropriately scoped.

AGENTS.md reference: apps/cli/AGENTS.md:L290-L290

Useful? React with 👍 / 👎.

if (Option.isSome(flags.target) && !LEGACY_BRANCH_PROJECT_REF_PATTERN.test(flags.target.value)) {
const target = flags.target.value;
branch = target;
const parentRef = yield* resolver.resolve(Option.none());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve UUID targets without requiring a linked project

For a UUID --target, resolver.resolve(Option.none()) runs before legacyResolveBranchProjectRef, even though the UUID lookup endpoint does not use a parent ref. Consequently, in an unlinked non-interactive directory, config diff --target <uuid> fails with LegacyProjectNotLinkedError without making the documented /v1/branches/{uuid} request; only branch-name targets should require the parent project ref.

Useful? React with 👍 / 👎.

| `api` | [`../src/next/commands/platform/api.command.ts`](../src/next/commands/platform/api.command.ts) | Low-level Management API client. It supersedes the old generated tree with explicit discovery via `supabase api routes` and execution via `supabase api request <route> [--method <METHOD>]`. |
| `stack` | [`../src/next/cli/root.ts`](../src/next/cli/root.ts) | TS-only local runtime namespace exposing `stack start`, `stack stop`, `stack status`, `stack list`, and `stack update`. Top-level `start`, `stop`, and `status` remain aliases. |
| `branches switch` | [`../src/next/commands/branches/switch/switch.command.ts`](../src/next/commands/branches/switch/switch.command.ts) | No direct Go equivalent. Updates local active-branch state so subsequent commands target the selected branch. |
| `config diff` | [`../src/legacy/commands/config/diff/diff.command.ts`](../src/legacy/commands/config/diff/diff.command.ts) | Read-only drift report between `supabase/config.toml` and `GET /v2/projects/{ref}/config` (CLI-2156). TS-only: the old Go CLI had no config diff. `--target` accepts a branch name/UUID/ref; `--exit-code` exits 1 on drift. Rejects the Go-compat `-o/--output` flag outright — machine output is `--output-format json\|stream-json` only (no Go parity contract for net-new commands, per the CLI-2156 discussion). Comparison core lives in `@supabase/config` (ADR 0022). |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove the entry from the frozen divergence record

This adds config diff to go-cli-divergences.md, but that file is explicitly frozen and new CLI behavior must instead be documented through help text, tests, and SIDE_EFFECTS.md. Remove this row so the historical record does not keep accumulating current feature documentation.

AGENTS.md reference: apps/cli/AGENTS.md:L539-L542

Useful? React with 👍 / 👎.

Comment on lines +68 to +71
// Net-new TS command with no Go parity contract: the Go-compat `-o/--output`
// flag is rejected outright (every value, `pretty` included) rather than
// honored — machine output goes through `--output-format` only (CLI-2156,
// per Colum). Checked first so no target resolution or network call runs.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Describe the output behavior without Go-parity framing

This new comment defines the command's behavior by saying it has no Go parity contract, and the same framing is repeated in the new error and side-effect documentation. New legacy work must describe behavior on its own terms rather than use the removed Go implementation as the compatibility baseline; rephrase this around the supported output flags themselves.

AGENTS.md reference: apps/cli/AGENTS.md:L56-L58

Useful? React with 👍 / 👎.

Comment on lines +126 to +129
const loaded = yield* loadCliConfig(runtimeInfo.cwd, {
projectRef: ref,
goViperCompat: true,
}).pipe(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate the local config before resolving remote branches

When --target is a branch name or UUID, target resolution can make a Management API request before this load runs. Therefore a missing or malformed local config does not abort before network activity as promised by LegacyConfigDiffLoadConfigError and SIDE_EFFECTS.md; for example, config diff --target staging can contact /v1/projects/.../branches/staging before reporting the TOML parse failure. Parse and validate the local document before performing branch lookup, then apply the target-specific remote overlay once the ref is known.

Useful? React with 👍 / 👎.

Comment on lines +95 to +98
ref = yield* legacyResolveBranchProjectRef(target, parentRef, {
mapGetError: mapBranchResolveError,
mapFindError: mapBranchResolveError,
}).pipe(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Show progress while resolving branch targets

For branch-name and UUID targets, legacyResolveBranchProjectRef performs a Management API request without an output.task, so text-mode users receive no progress indication while that network request is pending. Wrap this lookup in a task and fail or clear it on every exit, as is already done for the subsequent project-config fetch.

AGENTS.md reference: apps/cli/AGENTS.md:L425-L427

Useful? React with 👍 / 👎.

@github-actions

Copy link
Copy Markdown
Contributor

Supabase CLI preview

npx --yes https://pkg.pr.new/supabase/cli/supabase@cae9c14a9723bf0df9eddf3f983d0dca848b6b60

Preview package for commit cae9c14.

@Coly010Coly010 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. --exit-code is accidentally required → plain supabase config diff (the help's own first example) errors out. One-line fix; needs a parser-level test.
  2. Order-insensitive array equality false-negatives on api.schemas / api.extra_search_path--exit-code exits 0 on real drift.
  3. The remote_only suppression 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).
  4. False clean: a declared auth.oauth_server.enabled disagreeing 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_only erases the local value it just compared — the output can't answer "what would config push change?".
  • --workdir is silently ignored (config push shares 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 (legacySanitizeInlineName exists for exactly this and is used 14 lines away).
  • --exit-code conflates drift with failure (both exit 1).
  • JSON schema_version is the user's $schema URL, 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

  • DiffProjectConfigOptions asks for local and declared separately — two params that must come from the same load, with nothing enforcing it. Consider accepting the loaded pair and deriving both.
  • counts is derived state computed in three places (config-diff.ts, both formatters + changes.length in the handler); one will drift. Either drop it from the package type or make total part of it and use it everywhere.
  • No docs-site overlay (config push has docs/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_only across label, summary, and JSON. And N 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 "run supabase link" error wins over the friendlier "run supabase init" one.
  • JSON scope lists only present blocks; consumers must re-derive the missing set from a hardcoded list — consider scope: {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 -o rejection, and the remote_only local-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.

),
Flag.optional,
),
exitCode: Flag.boolean("exit-code").pipe(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker:Flag.boolean("exit-code") without Flag.withDefault(false) makes this a required flag, so the command cannot be invoked as documented:

$ supabase config diff
Error: required flag(s) "exit-code" not set

That's the exact invocation in the EXAMPLES block below and in the generated docs spec (which even emits default_value: "false"). The integration suite can't catch this because every test hands a pre-built flags object to the handler and never goes through the parser — please ship a parser-level test with the one-line fix (e.g. an e2e assertion that config diff with no args doesn't emit required flag(s)), or the next boolean flag will regress the same way.

* scalars tolerate string/number and string/boolean representation skew.
*/
export function isEqualConfigValue(a: unknown, b: unknown): boolean {
if (Array.isArray(a) && Array.isArray(b)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker: comparing every array as a multiset false-negatives on order-significant config. api.schemas (PostgREST's default schema is the first entry) and api.extra_search_path (a literal search_path, where order is resolution order) are sequences, not sets. Reproduced: local ["public","extensions"] vs remote db_extra_search_path: "extensions,public"changes: [], so --exit-code exits 0 on a difference that changes runtime behaviour — in a drift detector.

"Is this array a set or a sequence?" is per-field wire semantics, i.e. registry knowledge: suggest a row property (arrayEquality: "set" | "sequence") on projectConfigMappingRows, defaulting to sequence (over-report rather than under-report), with additional_redirect_urls opting into set semantics.

return `j:${JSON.stringify(value)}`;
}

function isZeroValue(value: unknown): boolean {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker: this fallback tests JS zeros, but the operand is already canonicalized by the registry — GoTrue's sessions_timebox: 0 arrives here as the string"0s" and escapes the check. Confirmed live against staging: a near-default project reports auth.sessions.timebox / auth.sessions.inactivity_timeout as [remote only] remote: "0s", plus 13 more noise lines for auth.email.template.*.subject / notification.*.subject (platform-reported defaults with no baseline in the default CLI config; the three storage.* entries look like the same class). On the staging bench project, 15 of the 18 remote_only entries were this noise — an untouched project fails --exit-code, which is exactly the flooding ADR 0022 says the baseline prevents.

Suggest not inferring "unconfigured" from JS zeros at all: put the platform's unconfigured value on the registry row (e.g. unconfiguredValue), and add a registry-driven test enumerating every comparable path whose baseline is undefined, asserting its zero form suppresses.

// (e.g. `db.network_restrictions.allowed_cidrs`'s allow-all default is
// exactly the platform's unconfigured state), then to the type's zero
// value (the platform's report of an unconfigured feature).
const baseline = valueAtPath(defaults, path) ?? valueAtPath(getDefaultCliConfig(), path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (false clean):applyPushUnmanagedOmissions unconditionally drops auth.oauth_server from the local projection, but auth.oauth_server.enabled is a comparable registry path the API reports. Reproduced end-to-end: file declares [auth.oauth_server] enabled = true, remote reports oauth_server_enabled: false → local is silent, remote equals the raw default, suppressed here — and the command prints No config differences found. A declared local value that genuinely disagrees with the remote vanishes with no change entry, no masked note, no signal.

Suggestion (additive): a third bucket alongside maskedunmanaged: ReadonlyArray<string> for declared paths the local projection dropped — surfaced like the masked note ("N declared property(ies) cannot be pushed and were not compared: …"). The classifier already has both inputs it needs (declared + local silence).

Related, same line: the ?? valueAtPath(getDefaultCliConfig(), path) fallback is hard-coded, so the defaults option (which no call site currently passes) only controls one of the three baseline tiers. Either make the option the complete baseline or delete it.

remote: remoteValue,
...(envVariable === undefined ? {} : { envVariable }),
}
: { path, class: "remote_only", local: undefined, remote: remoteValue },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major: on an undeclared path the classifier had the local effective value (the materialized default it just compared) and throws it away. The user sees:

api.max_rows [remote only]
local: (unset)
remote: 250

…when the local effective value is 1000 and a config push would overwrite the remote 250 with it. That's the "someone changed it in the dashboard" case — the primary reason this command exists — and [remote only] reads as "key exists only remotely", which is false for anything with a schema default. It also collapses two states the future config pull consumer needs distinguished (file-silent vs materialized-default-disagrees).

Suggestion: keep local populated and add readonly declared: boolean to ConfigChange (already computed above); render local: 1000 (schema default — not declared in config.toml).

]),
Command.withHandler((flags) =>
legacyConfigDiff(flags).pipe(
withLegacyCommandInstrumentation({ flags, safeFlags: ["project-ref"] }),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safeFlags: ["project-ref"] logs the ref verbatim while config push (same family, same flag) redacts it, and the established safe list in apps/cli/CLAUDE.md doesn't include the config family. Telemetry drift is silent and breaks dashboards — either drop this or add it to config push and extend the documented list in the same change.

"LegacyConfigDiffReadStatusError",
)<StatusErrorArgs> {
get [ErrorActionabilityId](): CliErrorActionabilityDeclaration {
return statusCodeActionability(this.status);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/v2/projects/{ref}/config names a user-selected resource, so a 404 means "wrong project ref" — user-actionable. Without { notFoundIsInvalidInput: true } (which LegacyConfigDiffBranchResolveStatusError above and all 11 push.errors.ts status errors on ref-addressed routes pass), this classifies a 404 as an external-service problem and skews the KPI split.

push-gated omissions), not necessarily the file's literal spelling.
- **Partial responses:** a managed property the response does not carry is `local_only`
when the file declares it and silent otherwise; a missing block is called out on the
scope line rather than treated as an error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This claim (and ADR 0022's "partially-populated responses degrade … instead of an error") doesn't hold: V2GetProjectConfigOutput makes all six blocks — and their keys — required, so a missing block or key fails the typed decode inside the API client before any of this leniency runs. Confirmed live: staging doesn't return storage.database_pool_mode yet, and the command hard-fails on every invocation with

failed to read project config: SchemaError(Missing key
at ["data"]["attributes"]["storage"]["database_pool_mode"])

— i.e. the command is currently broken against staging, and a permission-truncated response (the case the ADR names) surfaces as an opaque SchemaError. Consequences: the scope line's "(not returned: …)" branch is unreachable in production (it prints the constant six-block list on every run), and two diff.format.unit.test.ts cases exercise unreachable states.

Pick one: loosen the contract (make blocks/keys optional, matching auth's leniency — the stated intent) and keep the scope machinery, or delete the scope machinery and correct this doc + ADR 0022. Don't leave the doc asserting behaviour the contract forbids.

| `api` | [`../src/next/commands/platform/api.command.ts`](../src/next/commands/platform/api.command.ts) | Low-level Management API client. It supersedes the old generated tree with explicit discovery via `supabase api routes` and execution via `supabase api request <route> [--method <METHOD>]`. |
| `stack` | [`../src/next/cli/root.ts`](../src/next/cli/root.ts) | TS-only local runtime namespace exposing `stack start`, `stack stop`, `stack status`, `stack list`, and `stack update`. Top-level `start`, `stop`, and `status` remain aliases. |
| `branches switch` | [`../src/next/commands/branches/switch/switch.command.ts`](../src/next/commands/branches/switch/switch.command.ts) | No direct Go equivalent. Updates local active-branch state so subsequent commands target the selected branch. |
| `config diff` | [`../src/legacy/commands/config/diff/diff.command.ts`](../src/legacy/commands/config/diff/diff.command.ts) | Read-only drift report between `supabase/config.toml` and `GET /v2/projects/{ref}/config` (CLI-2156). TS-only: the old Go CLI had no config diff. `--target` accepts a branch name/UUID/ref; `--exit-code` exits 1 on drift. Rejects the Go-compat `-o/--output` flag outright — machine output is `--output-format json\|stream-json` only (no Go parity contract for net-new commands, per the CLI-2156 discussion). Comparison core lives in `@supabase/config` (ADR 0022). |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apps/cli/CLAUDE.md declares this file "a frozen historical record … Do not add new entries — new flags and features are simply new CLI behavior." This row was flagged independently by every review pass. Suggest dropping it (the -o rationale already lives in SIDE_EFFECTS.md and the error text; ADR 0022 is the right home for the decision), which also undoes the table reflow that turned a 1-line change into a 15-line diff. SIDE_EFFECTS.md:9 points here too and should stop.

default_pool_size: 20,
max_client_conn: 100,
},
auth: {},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This auth: {} is the structural blind spot that let both classifier blockers through: v2 attributes.auth is an open record, so an empty object is schema-valid — meaning the entire GoTrue mapping surface (~200 keys; all the duration/inversion/sentinel logic) is never exercised end-to-end, and the fixture docstring's "an empty config.toml diffs clean" claim is only true because auth is empty.

Highest-value test additions:

  1. A realistic all-defaults auth block in this fixture, asserting a fresh config diffs clean (this is where the "0s" suppression bug lives).
  2. The same cleanliness assertion in diff.live.test.ts — its own comment names "the GoTrue-keyed auth record … classifying cleanly" as the one thing mocks can't prove, then asserts only exitCode === 0.
  3. A not.toContain(<secret>) assertion on the masking scenario (it already seeds GITHUB_SECRET=shh and an HMAC-shaped remote value without asserting absence), so the "secrets never leak" claim is pinned against formatter changes.
  4. Telemetry-flush assertions on the -o, flag-conflict, and branch-404 paths (currently they'd fail).

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kanadgupta@Coly010