feat(cli): port supabase db lint and db advisors to native TypeScript#5579
Merged
Coly010 merged 42 commits intoJun 17, 2026
Merged
Conversation
Replace the Go-proxy handlers for `db lint` and `db advisors` with native Effect implementations in the legacy shell (CLI-1314). - lint: runs plpgsql_check per user schema inside an always-rolled-back transaction; ListUserSchemas when --schema omitted. - advisors: local lints.sql query (rolled back) and a --linked path that fetches security/performance advisors via raw HTTP with a tolerant parse (closed-enum-safe, mirroring Go's permissive string structs). - Shared, hoisted helpers: legacy-go-json.ts (byte-faithful Go encoding/json encoder — struct-order keys, HTML escaping, omitempty, trailing newline) and legacy-fail-on.ts (--level/--fail-on ordinal machinery). - advisors runtime layer uses the lazy platform-API factory so the auth-free --local path never resolves an access token. - text mode is byte-exact with Go (pretty JSON to stdout, diagnostics to stderr); json/stream-json emit additive envelopes; --fail-on exits non-zero. Unit + integration tests for both commands and the shared helpers; SIDE_EFFECTS docs rewritten; go-cli-porting-status flips both rows to ported.
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@a2d2ce4fc479baaab948132bd454500635aa65e4Preview package for commit |
![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: 0a5c0e8bca
ℹ️ 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".
…h Go driver divergence (ci: e2e shard 2/3)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f5ae04715
ℹ️ 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".
Resolves five P2 parity findings from the Codex review of the native db lint / db advisors port, each grounded in the Go CLI: - db lint --schema is a Cobra StringSliceVar in Go (cmd/db.go:506), so --schema public,private must lint both schemas. Flag.atLeast(0) only accumulates repeated flags, so hoist gen/types' comma-split helper to legacy/shared/legacy-schema-flags.ts (legacyNormalizeSchemaFlags) and apply it in lint (review threads lint.handler.ts:58). - db lint --linked / db advisors --linked must resolve the project ref via Go's non-prompting flags.LoadProjectRef (project_ref.go:54-76, db_url.go:88, cmd/db.go:362), not the prompting ParseProjectRef. Add a non-prompting, hard-failing loadProjectRef to LegacyProjectRefResolver and use it on both linked paths (advisors handler + the shared db-config resolver's linked branch, which also fixes db dump/push/pull/ reset/query). Previously a TTY run with a token but no linked-project file would open a project picker instead of failing fast. - db lint --linked must write the linked-project cache like Go's root PersistentPostRun ensureProjectGroupsCached (root.go:176,214-235): wire the project-ref + linked-project-cache layers into lint.layers.ts (mirroring advisors) and cache the ref on success/failure, gated to the linked branch so --local/--db-url stay cache-free (Go no-ops on empty ProjectRef). - Prefix all bare exported helpers in the port with Legacy/legacy per apps/cli/AGENTS.md (lint.format.ts, advisors.format.ts, advisors.linked.ts). Verified: apps/cli check:all (types/lint/fmt/knip), full in-process unit+integration suite, and a bundled-binary smoke test of db lint --local (layer constructs, no missing-service panic) and db lint --linked (fails fast with "Cannot find project ref").
Four more P2 parity findings from the Codex review, each grounded in Go: - Telemetry exit code: Go records the real process exit code (cmd/root.go:177 -> exitCode(err) = 1). lint/advisors set ProcessControl's exit code in json/stream-json mode after a --fail-on trigger without failing the Effect (to keep the machine payload on stdout intact), so withLegacyCommandInstrumentation logged exit_code 0. Read ProcessControl.getExitCode and report 1 when the process exit code is non-zero — one fix covering both commands. - db advisors --linked token gate: Go's PreRunE calls utils.LoadAccessTokenFS, which validates the token (env/keyring/file) against the sbp_ pattern and fails with ErrInvalidToken before any API call (access_token.go:24-33). The gate used resolveLegacyAccessToken, which skips env validation and collapses validation failures to None. Switch the gate to the validating LegacyCredentials.getAccessToken and map a malformed token to a new LegacyDbAdvisorsInvalidTokenError (resolveLegacyAccessToken stays as-is for the raw-HTTP header path the sso commands rely on). - Advisors API response decode: Go decodes the 200 body via json.Unmarshal into a typed struct, so a wrong-typed container (lints/categories/metadata/ fkey_columns) or non-object entry is an UnmarshalTypeError -> non-zero failure. The tolerant parser coerced those to "No issues found". Make apiResponseToLegacyAdvisorLints throw on structural mismatches (surfaced as the same "failed to fetch … advisors" error) while keeping the deliberate string-enum tolerance for advisor names/types the API can add. - plpgsql_check decode: Go's json.Unmarshal into lint.Result rejects a top-level non-object, a non-array issues, or a non-object entry. parseLegacyLintResult coerced those to an empty result; make it throw (handler maps to LegacyDbLintMalformedJsonError), preserving Go's tolerance for null/missing/ unknown fields. Adding ProcessControl to withLegacyCommandInstrumentation's requirements meant the four full-command-wiring integration tests (telemetry/services/gen-types/ gen-signing-key) needed the production processControlLayer in their root layer. Verified: apps/cli check:all, full unit (1220) + integration (1190) suites, and a bundled-binary smoke test of db lint --local (instrumentation path, no missing-service panic) and db advisors --linked with a malformed token.
Go wraps every Management API response in identityTransport, which fires OnGotrueID -> StitchLogin on the X-Gotrue-Id header to persist the session's authenticated identity (apps/cli-go/internal/utils/api.go:128, identity_transport.go:12-20, cmd/root.go:146-154). The TS port only ran that stitch inside the typed LegacyPlatformApi client's transport; db advisors --linked deliberately uses a raw HttpClient (for enum tolerance) and omits that client, so a session whose first API call is db advisors --linked silently skipped identity stitching — invisible telemetry drift. Hoist the stitch logic out of legacy-platform-api.layer.ts into a shared legacy/shared/legacy-identity-stitch.ts (a factory the typed client keeps using directly, plus a LegacyIdentityStitch service/layer for the raw path), and run the stitch on each raw advisor response. One stitcher is shared across the security + performance calls so it fires at most once per session, matching Go's NeedsIdentityStitch gate. Scoped to advisors; the other raw-HTTP Management API paths (config push, postgres-config) have the same gap and are tracked as a follow-up. Verified: apps/cli check:all, full unit (1220) + integration (1191) suites (platform-api stitch behaviour unchanged), and a bundled-binary smoke test of db advisors --linked (runtime constructs with the new layer, no missing-service panic).
Go's root PersistentPreRunE runs flags.ParseDatabaseConfig for every db
subcommand — including db advisors — before the command's RunE
(apps/cli-go/cmd/root.go:118). On --linked that takes the linked branch
(internal/utils/flags/db_url.go:87-97 -> NewDbConfigWithPassword): it TCP-probes
the direct DB host, mints a temporary login role ("Initialising login role..."),
or falls back to the pooler / fails with the "IPv6 is not supported" error —
before RunLinked, which then ignores the resolved config and only calls the
advisors Management API (internal/db/advisors/advisors.go:79-100).
The native handler skipped LegacyDbConfigResolver entirely on the linked path,
so db advisors --linked could exit 0 (or emit advisors) where Go fails on the
DB-config resolution. Run resolver.resolve({linked}) in runLinked — discarding
the result, in the PreRun-equivalent position before the token gate — to
reproduce those side effects and the early-failure ordering.
Verified: apps/cli check:all, full unit (1220) + integration (1193) suites, and
a bundled-binary smoke test of db advisors --linked (now fails with the IPv6
error from DB-config resolution before any advisors API call, matching Go).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6b1f177a9
ℹ️ 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".
Go's changedFlagValues treats every utils.EnumFlag as telemetry-safe and records the actual value (cmd/root_analytics.go:88-116). The advisors/lint choice flags (--type, --level, --fail-on) were landing as "<redacted>" because the legacy instrumentation only passes booleans and safeFlags verbatim. Whitelist them via safeFlags so `db advisors --type security --fail-on error` records those values, matching Go's telemetry dimensions. --db-url stays redacted (may carry secrets); lint's --schema stays redacted ([]string slice flag, not an EnumFlag). git add apps/cli/src/legacy/commands/db/advisors/advisors.format.ts apps/cli/src/legacy/commands/db/advisors/advisors.format.unit.test.ts
…iew)
The malformed-response handling rejected wrong-typed containers but still
coerced scalar mismatches: `{"lints":[{"name":123}]}` became name "123". Go
decodes the 200 body via json.Unmarshal into the typed V1ProjectAdvisorsResponse,
which returns an UnmarshalTypeError for a non-string string/`type X string` field
or a non-string []string element, so fetch{Security,Performance}Advisors fail
non-zero (pkg/api/types.gen.go:4825-4860). Add requireApiString/
requireApiStringArray (absent/null -> zero value; present non-string -> throw)
and tighten projectApiMetadata to reject present-non-string subfields and
non-number fkey_columns elements, while keeping the deliberate any-string
(unknown-enum) tolerance. The local rows.Scan path (scanLegacyAdvisorLintRow)
stays lenient, matching Go's rows.Scan.
Go's db --linked family resolves DB config via NewDbConfigWithPassword (internal/utils/flags/db_url.go), which on a reachable host with SUPABASE_DB_PASSWORD returns a working config WITHOUT an access token — the token is only needed to mint a temporary login role on the no-password path. The shared LegacyDbConfigResolver linked branch wrapped resolveLinked in the eager legacyManagementApiRuntimeLayer, which resolves the access token at layer-construction time and fails with "Access token not provided" even when resolveLinked returns the password-only config early. That broke password-only linked CI jobs (lint/dump/push/pull/reset, and advisors) where Go would connect. Switch the linked branch to a lazy stack (legacyPlatformApiFactoryLayer + project-ref resolver) and force LegacyPlatformApiFactory.make only inside initLoginRole/listAndUnban (the temp-role mint / unban paths). A password-only or unreachable-host resolution now never resolves a token. The compile-time ambient-coverage guard is retyped to the lazy stack's requirements. Verified: apps/cli check:all (incl. the ambient coverage guard), full unit (1222) + integration (1193) suites, and a bundled-binary smoke — db lint --linked / db advisors --linked with no token + no ref now fail with "Cannot find project ref" (lazy stack builds, token never resolved) instead of "Access token not provided" (eager). The password-only success path needs a reachable DB, covered by the cli-e2e parity harness.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4db598830b
ℹ️ 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".
… (ci: knip) The lazy-linked-resolver change (4db5988) made `legacy-db-config.layer.ts`'s `--linked` branch use `LegacyPlatformApiFactory` instead of the eager `legacyManagementApiRuntimeLayer`, removing the only consumer of the exported `LegacyManagementApiRuntimeRequirements` type. knip flagged it as an unused export (the "Check code quality" CI job; my local check:all missed it via a stale nx cache hit on knip:check). Remove the dead export and trim its JSDoc; `LegacyManagementApiRuntimeError` stays — it's still consumed by `legacy-db-config.service.ts`. Verified: nx code-quality targets (types/lint/fmt/knip) run with --skip-nx-cache.
Go decodes the plpgsql_check payload via json.Unmarshal into lint.Result
(internal/db/lint/lint.go:149-152), whose lint.Issue / lint.Statement /
lint.Query string fields reject a non-string JSON value (UnmarshalTypeError ->
"failed to marshal json" -> non-zero exit). The parser still coerced scalars via
String(...), so a malformed extension payload (e.g. {"level":123} or a non-object
statement) was emitted as a valid lint result. Replace asString with a strict
requireLintString (absent/null -> "" zero value; present non-string -> throw) and
reject a present-non-object statement/query, mirroring the advisors parser. Absent/
null/unknown fields stay tolerated (Go has no DisallowUnknownFields).
Go's generated parser only decodes the advisors 200 body when the Content-Type
header contains "json" (pkg/api/client.gen.go); otherwise JSON200 stays nil and
fetch{Security,Performance}Advisors return the status-200 error (internal/db/
advisors/advisors.go:167-169,178-180). The raw-HTTP path parsed the body
regardless, so a proxy/header regression returning JSON text was treated as a
valid advisor result. Gate the 200 parse on the response Content-Type containing
"json" and otherwise fail via the existing status error ("unexpected ... advisors
status 200: <body>"), matching Go. (Also includes the runLinked cache reorder +
content-type integration test.)
…(review) Go's Execute runs ensureProjectGroupsCached after the command on the error path too (cmd/root.go:176, before the err panic at :185), and ParseDatabaseConfig's linked branch loads the project ref (db_url.go:88) before the host probe (:95) — so Go writes supabase/.temp/linked-project.json even when the DB-config resolve fails (e.g. the IPv6 error). The advisors runLinked installed the cache finalizer only after resolve succeeded, so linked failure runs lost the project/org telemetry grouping. Load the ref first (non-prompting; ErrNotLinked -> nothing to cache, matching Go's empty flags.ProjectRef) and wrap the resolve + token gate + fetch in Effect.ensuring(linkedProjectCache.cache(ref)). db lint already had this ordering (ref loaded before the resolve-containing body), so only advisors changed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 055114f098
ℹ️ 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".
Go's initLoginRole resolves the token via utils.GetSupabase() -> LoadAccessTokenFS (internal/utils/api.go:121-123), which exits with the RAW missing/invalid-token message; the "failed to initialise login role: %w" wrap (db_url.go:206-208) only covers the createLoginRole HTTP call. The lazy-factory change wrapped factory.make (token resolution) failures with that login-role prefix, so db lint --linked without SUPABASE_DB_PASSWORD and no token showed a misleading login-role/network error. Drop the factory.make mapper in initLoginRole/listAndUnban so auth/config errors propagate raw (they're already in the resolve service's LegacyManagementApiRuntimeError channel; widen the resolveLinked/waitForTempRole annotations accordingly), and delete the now-unused mapper/helper. loginRoleErrorMapper stays on createLoginRole. git add apps/cli/src/legacy/commands/db/lint/lint.format.ts apps/cli/src/legacy/commands/db/lint/lint.format.unit.test.ts
…(review)
Go decodes the plpgsql_check payload into lint.Result (Function is a string)
BEFORE overriding r.Function with <schema>.<name> (internal/db/lint/lint.go:150-154),
so a present non-string `function` (e.g. {"function":123}) fails json.Unmarshal
with "failed to marshal json". parseLegacyLintResult never inspected the payload's
function field, so it returned a clean result. Validate record["function"] via
requireLintString (present non-string -> throw; absent/null/string -> ok) before
discarding it for the override.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37ee756c0d
ℹ️ 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".
…eview)
Go's ensureProjectGroupsCached GETs /v1/projects/{ref} through GetSupabase()'s
identityTransport (cmd/root.go:226, internal/utils/api.go:128-134,
identity_transport.go:12-20), so the X-Gotrue-Id on that response stitches the
session identity. For a password-only `db lint`/`db advisors --linked` run the
DB config resolves without hitting LegacyPlatformApi, so the linked-project cache
GET can be the ONLY Management API response — and our raw cache GET dropped its
X-Gotrue-Id, leaving telemetry anonymous. Tap makeLegacyIdentityStitcher on the
cache response (before the status gate, matching Go's transport). Additive and
idempotent (the stitcher self-gates), so commands that also hit LegacyPlatformApi
are unaffected. The new Analytics/TelemetryRuntime deps are ambient in the
runtimes that provide the cache layer; the one test that builds the layer
directly (bootstrap workdir-cache) now provides them.
Verified: apps/cli code-quality (--skip-nx-cache), full unit (1224) + integration
(1195) suites incl. a new cache-layer integration test asserting the X-Gotrue-Id
stitch, and a bundled-binary smoke (advisors --linked runtime constructs the cache
layer, no missing-service panic).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6ac3e9139
ℹ️ 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".
Go's GetSupabase() calls LoadAccessTokenFS, which validates the token — including SUPABASE_ACCESS_TOKEN from env — against the sbp_ pattern and fails with ErrInvalidToken before any API call (internal/utils/api.go:121, access_token.go:24-41). legacyMakePlatformApi's resolveAccessToken returned the env token unvalidated (only the LegacyCredentials.getAccessToken fallthrough validated), so a malformed SUPABASE_ACCESS_TOKEN was sent to the API and produced an unexpected 401-ish error instead of Go's clean invalid-token error — for every typed-API command (the linked db role-mint path that surfaced it included). Validate the env token in resolveAccessToken. Hoist the shared sbp_ pattern + invalid-token message + validator into legacy/auth/legacy-access-token.ts (reused by legacy-credentials.layer.ts, which had the only copy) per the hoist-before-you-duplicate rule. The error type was already in the builder's union (the credentials path declares it), so no signature change. Verified: apps/cli code-quality (--skip-nx-cache), full unit (1225) + integration (1195) suites incl. a new malformed-env-token test, and a bundled-binary smoke (projects list with a bad SUPABASE_ACCESS_TOKEN fails with "Invalid access token format" before any network call).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f960ea1551
ℹ️ 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".
…s to match Go (review) Go installs a single sync.Once in the root command context (cmd/root.go:145-154) and wires the one OnGotrueID closure into every transport's identityTransport (internal/utils/identity_transport.go:17-19), so StitchLogin's analytics.Alias + SaveState (internal/telemetry/service.go:137-142) run at most once per command across ALL Management API responses. The TS port had each transport build its own makeLegacyIdentityStitcher with a separate stitchAttempted flag (typed client, raw advisor GETs, linked-project cache), so a command touching several transports — e.g. a no-password db advisors --linked that mints a temp role via the typed client and then issues raw advisor GETs — emitted duplicate alias events and duplicate telemetry.json writes (not idempotent at the analytics layer). Make LegacyIdentityStitch the single owner of stitchAttempted: the typed client and the linked-project cache consume the service instead of building their own stitcher, and every command runtime provides legacyIdentityStitchLayer by the same reference (memoised -> one instance per command) to the platform-API factory, the cache, and the db-config resolver. The db-config resolver snapshots that instance into its lazy linked stack's ambient layer so the temp-role mint shares the guard too.
…times (ci: e2e) eb9a7b4 made legacyDbConfigLayer eagerly require LegacyIdentityStitch but missed the inspect and test runtimes, which build their dbConfig without providing legacyIdentityStitchLayer. Every inspect invocation (and test db --linked) panicked at layer construction with "Service not found: supabase/legacy/IdentityStitch", failing the e2e shards. Provide the layer to both, matching the lint/advisors db-config wiring.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f09472cee
ℹ️ 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".
inspect db leaves and inspect report derived connType from parsed boolean flag values, so --local=false fell through to the linked branch and --linked --local=false skipped the mutual-exclusion error. Go keys both off pflag Changed (db_url.go:46-63; MarkFlagsMutuallyExclusive on inspectCmd, cmd/inspect.go:258-263). Route both through the shared resolveLegacyDbTargetFlags(argv) helper, matching db lint/advisors/test.
legacyTestDbRuntimeLayer only Layer.provide-d the stitcher to its child dbConfig, so withLegacyCommandInstrumentation's serviceOption returned None and test db --linked's cli_command_executed was mis-attributed to the device id. Merge legacyIdentityStitchLayer at top level, mirroring the lint/advisors/inspect runtimes, with a layer-exposure regression test.
resolveGroups attached cli_command_executed to the organization group keyed by organization_slug, but the linked-project-cache groupIdentify publishes org metadata under organization_id, so the identify never applied to the events. Go keys both by OrganizationID and omits the org group when the id is empty (telemetry/project.go:99-103). Key events by organization_id and omit the empty org group; add resolveGroups tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 469228447c
ℹ️ 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".
Go's --schema is a Cobra StringSliceVarP; pflag's StringSlice.Set parses
each value via encoding/csv (readAsCSV), so a quoted value like
"tenant,one" stays one schema while public,private splits into two. The
TS port used plain split(","), breaking quoted commas. Replace with a
minimal CSV single-record parser matching encoding/csv defaults.
pflag accepts --flag value (space form) for non-boolean flags, so in db lint --schema --linked the --linked token is --schema's value, not a changed target flag, and db advisors --db-url --local makes --local the db-url value. The raw-argv scan treated those value tokens as changed selectors, mis-routing or raising spurious mutual-exclusion errors. Make the scan value-aware: skip the token after a bare value-consuming flag (db-url/schema/level/fail-on/type + globals), matching Go's parsed flag set.
When plpgsql_check returns an issues array containing null, Go's encoding/json decodes it to a zero-value lint.Issue (later dropped by the level filter) while preserving valid siblings. The TS guard treated null as malformed and aborted the whole db lint run. Let null normalize to the zero-value issue; still reject scalar/array entries.
The gen types test asserted trimmed/whitespace-collapsed --schema output, but Go's pflag StringSlice (encoding/csv readAsCSV) preserves whitespace verbatim and gen types/db lint never trim downstream. Align the expectation with Go's actual output now that legacyNormalizeSchemaFlags parses values as CSV without trimming.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5ee4f7c51
ℹ️ 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".
Go's encoding/json decodes a null element in the advisors lints array to a zero-value lint (dropped by the level/type filter) and a null element inside categories to the zero string, preserving valid siblings. The TS port treated either as malformed and failed the whole linked advisors call. Normalize null lints to the zero-value lint and null category elements to "", still rejecting scalar/array entries.
Two gaps in the argv target-flag scan: inspect report's string --output-dir flag was missing from the value-consuming set, so --output-dir --local wrongly treated --local as a changed selector; and the scan stopped at a bare -- before honoring a pending value skip, but pflag lets a string flag consume -- as its value (db lint --schema -- --linked marks linked changed). Add output-dir to the value set and consume a pending value token before the end-of-options break.
Go's --schema is a pflag StringSlice that parses each value via encoding/csv at parse time, so a malformed value (unterminated quote, bytes after a closing quote, or a bare quote) fails the command before it runs. The TS port parsed CSV in the handler and was too lenient, silently rewriting the schema list. Move parsing to parse time via Flag.mapTryCatch(legacyParseSchemaFlags) on db lint and gen types so a malformed value fails with exit 1 like Go; handlers consume the already-split, validated list. Drop legacyNormalizeSchemaFlags.
avallete
approved these changes
Jun 16, 2026
Member
There was a problem hiding this comment.
nitpick
Trying things out locally when there is a connection error we get this error message:
➜ local-supabase-test git:(feat/functions-deploy) ✗ npx --yes https://pkg.pr.new/supabase/cli/supabase@abcef7605bc8b7c10cc3d90cc72d2ecf7c605a06 db lint
Connecting to local database...
failed to connect to postgres: effect/sql/SqlError: PgClient: Failed to connect
Try rerunning the command with --debug to troubleshoot the error.
➜ local-supabase-test git:(feat/functions-deploy) ✗ npx --yes https://pkg.pr.new/supabase/cli/supabase@abcef7605bc8b7c10cc3d90cc72d2ecf7c605a06 db advisors
Connecting to local database...
failed to connect to postgres: effect/sql/SqlError: PgClient: Failed to connect
Try rerunning the command with --debug to troubleshoot the error.
VS with the go version:
➜ local-supabase-test git:(feat/functions-deploy) ✗ /apps/cli/dist/supabase-go db lint
Connecting to local database...
<go stack trace error>
failed to connect to postgres: failed to connect to `host=127.0.0.1 user=postgres database=postgres`: dial error (dial tcp 127.0.0.1:54322: connect: connection refused)
Make sure your local IP is allowed in Network Restrictions and Network Bans.
https://supabase.com/dashboard/project/_/database/settings
Thow things that we might wanna improve:
- The go error give a bit more debug details (host / user / reason)
- It wrap such error with the "advise" of checking the network bans
Might wanna have a similar output for error but that can be addressed in a follow-up I think.
Contributor
Author
Yep, I like it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abcef7605b
ℹ️ 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".
metadata.fkey_columns maps to Go's *[]float32; encoding/json decodes a null array element to the zero value 0, not an error. The TS decoder threw on a null element and failed the whole linked advisors call. Normalize null/undefined elements to 0 (numbers unchanged), still rejecting non-number non-null elements.
extractChangedFlagNames recorded every flag-looking argv token, so db lint --schema --linked reported a spurious 'linked' flag in cli_command_executed telemetry — but Go's pflag consumes --linked as --schema's value, so changedFlags() records only schema. Share the value-consuming flag sets (single source of truth in legacy-db-target-flags.ts) and skip the token after a bare value-taking flag in the telemetry scan, matching Go's parsed flag set.
The e2e parity normalizer ran the duration regex before the JWT regex, so a randomly-generated JWT signature containing a duration-shaped substring (e.g. "-60s-" bounded by base64url chars) had <DURATION> injected mid-token, corrupting redaction and producing a spurious stdout diff on gen bearer-jwt (flaky e2e shard 3/3). Move JWT masking before duration normalization, matching the JWK-key-bytes ordering. Test harness only; CLI output is unaffected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b0cffbbfa
ℹ️ 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".
extractChangedFlagNames kept scanning past a bare --, so test db -- --linked recorded a phantom linked flag in cli_command_executed — but Go's pflag stops parsing flags at --, making --linked a positional arg that changedFlags() never sees. Break on a bare -- (after honoring a pending value skip), mirroring resolveLegacyDbTargetFlags's end-of-options handling.
There was a problem hiding this comment.
💡 Codex Review
Keep retrying when unban callbacks fail
When a linked command falls back to the pooler temp-role path and the first few connection attempts fail, any error from listing/removing network bans on attempt 3+ now aborts the whole retry loop at yield* unban. In Go, the NewErrorCallback callback errors are only printed and the backoff still sleeps and retries, so a transient Management API failure in the unban helper can make db lint/advisors --linked fail here even though the temp role would have become usable on a later retry.
ℹ️ 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".
On the linked pooler temp-role path, the network-ban unban step (run from the 3rd connect failure onward) was yielded unguarded, so a transient Management API failure in listAllNetworkBans/deleteNetworkBans aborted the whole backoff loop and failed db lint/advisors --linked. Go runs the unban inside backoff.RetryNotify's notify callback (utils.NewErrorCallback), whose error is printed and swallowed (retry.go:27-29) and can never abort the retry. Catch the unban error, print it to stderr (Go's logger is os.Stderr from the 3rd failure), and keep retrying.
Coly010
deleted the
columferry/cli-1314-port-supabase-db-lint-supabase-db-advisors
branch
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2d2ce4fc4
ℹ️ 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
+129
to
+130
| const canonical = aliases[short]; | ||
| if (canonical !== undefined) used.add(canonical); |
There was a problem hiding this comment.
When a native legacy command is invoked with the global shorthand, e.g. db lint -o json or db advisors -o yaml, Go's changedFlags() visits the root persistent output flag under its canonical name (pflag's documented shorthand forms include non-boolean -n 1234 / -n1234). This scanner only resolves single-dash flags through the per-command aliases map, and those maps only add command-local shorthands such as s, so cli_command_executed.flags omits the changed output flag for shorthand invocations even though the long --output form is seen. Add the global o -> output alias/value handling centrally so machine-output usage is not undercounted.
Useful? React with 👍 / 👎.
pull Bot
pushed a commit
to oogalieboogalie/cli
that referenced
this pull request
Jun 17, 2026
…entityStitch service (supabase#5607) ## Problem The supabase#5366 gate stopped the ephemeral-env `$identify` spike, but at the cost of attribution: in CI, Docker, and `npx supabase`, `cli_*` events stay orphaned on throwaway device IDs and never link to the authenticated user. GROWTH-891 (supabase#5559) fixes that with a hybrid stitch+stamp model. While supabase#5559 was in review, supabase#5579 (db lint/advisors port) landed on develop and independently extracted the legacy identity stitch into a shared `LegacyIdentityStitch` service — one per-command `stitchAttempted` guard so the advisor transports alias at most once. That's the architecture we want, but it's a port of the pre-891 behavior: it only stamps when it aliases (persistent, first login), so it doesn't restore CI/Docker/npx attribution; it sets `stitchAttempted` after the file-read yield; and it reads the `runtime.distinctId` field that 891 replaced with a mutable identity slot. Merging supabase#5559 on top as-is would silently drop the attribution feature and reintroduce the race. This PR reconciles the two: keep supabase#5579's shared-service architecture, fold the hybrid stitch+stamp behavior into it. Supersedes supabase#5559. ## Changes - **The shared `LegacyIdentityStitch` now stamps everywhere.** On the first authenticated response the user UUID is stamped into `runtime.identity` in every runtime, so captures in CI/Docker/npx carry the real user. The `$create_alias` (pre-login history merge) and the `telemetry.json` write still only happen on a persistent machine. - **Hardening preserved:** `stitchAttempted` is set before the first yield (no double-stitch race); when an identity already exists we stamp without aliasing (never merge two person graphs); alias fires at most once across all transports sharing the service. - **`stitchedDistinctId()` returns `runtime.identity.current()`** so the post-run `cli_command_executed` is attributed to the real user in every runtime, including steady state. - `legacy-analytics.layer.ts` resolves `distinctId` from the identity slot while keeping develop's already-keyed `groups` map. - Stitch behavior tests live in `legacy-identity-stitch.integration.test.ts` (CI-stamp-no-alias, stale-identity-stamp-no-alias, concurrent-alias-once); the platform-api layer test keeps develop's service-mocked wiring. A few command test runtimes still using the removed `distinctId` field were updated to `makeTelemetryIdentity`. - Brings the Go + next-TS 891 changes (logout identity reset + device-id rotation, the redundant `$identify` removal, ADR 0013) along through the merge. ## Testing Typecheck clean, full unit suite (1318) green, and the affected integration suites (identity-stitch, platform-api, login, logout, advisors, lint, services, gen/types, issue, linked-project-cache) pass under bun. Also ran an independent Codex review focused on the spike-regression risk — it confirmed no alias in ephemeral runtimes, alias-at-most-once across transports, the pre-yield race guard, and the no-cross-graph-merge invariant, with no findings. GROWTH-891 --------- Co-authored-by: Julien Goux <hi@jgoux.dev>
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.
Native (Phase 1+) TypeScript port of
supabase db lintandsupabase db advisorsinto the legacy shell, replacing the Go-proxy handlers (CLI-1314).What changed
db lint— runsplpgsql_checkper user schema inside an always-rolled-back transaction (matching Go's "lint has no side effects" contract, including the in-transactionCREATE EXTENSION). Resolves schemas viaListUserSchemaswhen--schemais omitted.db advisors— two backends:--local/--db-url: runs the embeddedlints.sqlquery in a rolled-back transaction and filters by--typecategory.--linked: fetches the security/performance advisor endpoints via raw HTTP with a tolerant parse, mirroring Go's permissivetype X stringstructs — the generated client's closedname/metadata.typeliterals would reject values the API can add.Shared, hoisted helpers (
legacy/shared/):legacy-go-json.ts— a byte-faithful reproduction of Go'sencoding/jsonindented encoder (struct-order keys, HTML escaping of<>&,omitempty, trailing newline).JSON.stringifygets the escaping and key order wrong, so a dedicated encoder is required for stdout parity.legacy-fail-on.ts— the--level/--fail-onordinal machinery shared by both commands (lint's prefix matcher vs advisors' exact case-insensitive matcher).Reviewer-relevant context
textis byte-exact with Go (pretty 2-space JSON array to stdout, diagnostics likeConnecting to … database.../Linting schema:/No schema errors found/No issues foundto stderr).json/stream-jsonadd a standardoutput.successenvelope — additive, since Go has no machine output.--fail-onforces a non-zero exit in every mode (viaProcessControl.setExitCodein machine modes so the payload isn't clobbered).advisors.layers.tsdeliberately does not uselegacyManagementApiRuntimeLayer(which eagerly resolves an access token at build and would break the auth-free--localpath). It uses the lazylegacyPlatformApiFactoryLayer, whosemakeis only forced by an interactive project-ref prompt that advisors never triggers. Verified against the bundled binary:db advisors --localis auth-free,--linkedsurfaces a proper not-logged-in error rather than a layer panic.db lintdoes not write the linked-project cache (noLegacyLinkedProjectCachedependency);db advisors --linkeddoes, on success and failure, matching Go'sPersistentPostRun.SUPABASE_API_URLis intentionally not honored (Go parity; API URL comes fromSUPABASE_PROFILE).cli_command_executedevent; no flags are marked telemetry-safe (matches Go).apps/cli/docs/go-cli-porting-status.mdflips only thedb lintanddb advisorsrows toported.CLOSES CLI-1314