feat(cli): port supabase db lint and db advisors to native TypeScript#5579
feat(cli): port supabase db lint and db advisors to native TypeScript#5579Coly010 wants to merge 21 commits into
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.
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@e89d47f3cbace7b082ebc9d19bd88748c20b2a58Preview 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".
| const response = yield* httpClient | ||
| .execute(request) | ||
| .pipe(Effect.mapError((cause) => endpoint.network(describeHttpError(cause)))); |
There was a problem hiding this comment.
When db advisors --linked --dns-resolver https runs in an environment where native DNS for the Management API host fails, Go's GetSupabase() installs withFallbackDNS on the HTTP transport before these advisor requests (apps/cli-go/internal/utils/api.go:100-104, with behavior covered in api_test.go). This raw HttpClient path never receives the selected resolver, so the advisor GETs still use the default/native resolver and can fail in exactly the scenario the flag is meant to support. Thread the resolver into this HTTP transport or reuse a DoH-capable Management API client for these requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 pr-autopilot — the parity gap is real, but I'm escalating this as a shared-infrastructure follow-up rather than fixing it in the advisors handler.
Confirmed: Go installs the DoH fallback resolver on the Management API transport when --dns-resolver https is set (withFallbackDNS mutating http.DefaultTransport.DialContext, internal/utils/api.go:100-101,125-127), and the advisor GETs inherit it via utils.GetSupabase() (internal/db/advisors/advisors.go:163,174). The TS port currently honors --dns-resolver https only on the Postgres connection path (legacy-db-connection.sql-pg.layer.ts, via legacyResolveHostsOverHttps); no HTTP path installs DoH — not this raw HttpClient and not the typed LegacyPlatformApi client (both run over the default FetchHttpClient).
Why a follow-up, not an advisors-scoped fix:
- It's a codebase-wide gap shared by every Management API call in the legacy shell (config push, postgres-config, linked-project cache, the whole platform-api surface). A per-command patch here would be an inconsistent divergence rather than parity; the right fix is one DoH-capable HTTP transport applied uniformly to
legacyHttpClientLayer+ theLegacyPlatformApitransport. - There's no reusable HTTP DoH transport: the pg-path helper works by swapping the dial-target IP while preserving the TLS servername at the socket level, but
fetch/FetchHttpClientresolves DNS internally and has no equivalent hook — honoring the resolver requires a custom undici/Bun dispatcher with its own TLS-servername handling (with real CWE-20/350 risk if gotten wrong). - It's not verifiable in CI/sandbox: the path only activates when native DNS for the API host fails AND a DoH server answers, which isn't reproducible here — so a fix would land essentially untestable inside this PR.
Recommend tracking it as a dedicated shared-transport task; the advisors path (and every other Management API call) inherits the fix once that lands. Happy to take it on separately, or fold it in here if you'd prefer — your call. Leaving this thread open.
…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".
| Layer.provide(linkedCliConfig), | ||
| Layer.provide(legacyDebugLoggerLayer), | ||
| ); | ||
| const linkedPlatformApiFactory = legacyPlatformApiFactoryLayer.pipe( |
There was a problem hiding this comment.
When db lint/advisors --linked --dns-resolver https has to mint a login role or clear network bans, this newly lazy Management API stack still builds legacyPlatformApiFactoryLayer on the default fetch transport and never receives the selected resolver. I checked apps/cli-go/internal/utils/api.go:121-130 and internal/utils/flags/db_url.go:202-244: Go's GetSupabase() wraps these createLoginRole/ban requests with withFallbackDNS, so users who select DoH because native DNS for the Management API host fails can still proceed; this port can fail before the DB connection is attempted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 pr-autopilot — this is the same DoH gap already escalated on advisors.linked.ts:67 (a few comments up the page), just at a different call site. The login-role mint / network-ban requests here, like the advisor GETs, run over the default FetchHttpClient and never receive --dns-resolver https. Confirmed against the same Go evidence (internal/utils/api.go:121-130 withFallbackDNS, internal/utils/flags/db_url.go:202-244 for the createLoginRole/ban path).
Both resolve via the one fix: a DoH-capable HTTP transport applied uniformly to legacyHttpClientLayer + the LegacyPlatformApi transport — there's no reusable HTTP DoH transport today (the pg-path helper swaps the dial-target IP at the socket level, which fetch has no hook for), and it can't be exercised in CI (needs failing native DNS + a live DoH server). So I'm tracking this under the same dedicated shared-transport follow-up rather than patching individual call sites; every Management API call (advisors GETs, login-role, bans) inherits the fix once that lands. Leaving this open alongside the advisors DoH thread for your call.
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: 049414f4f8
ℹ️ 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".
| device_id: stringField(prior, "device_id") ?? runtime.deviceId, | ||
| session_id: stringField(prior, "session_id") ?? runtime.sessionId, | ||
| session_last_active: new Date().toISOString(), | ||
| distinct_id: gotrueId, |
There was a problem hiding this comment.
When the first Management API response in a session carries X-Gotrue-Id, this stitcher only writes the new distinct_id to telemetry.json; it never updates the in-memory identity that legacyAnalyticsLayer uses later for cli_command_executed (context.distinct_id ?? runtime.distinctId ?? runtime.deviceId). Fresh evidence after the stitching fixes is that the capture path still reads the immutable TelemetryRuntime, so the command that performed the stitch is sent under the device id, whereas Go's StitchLogin mutates the telemetry service state before the post-run capture and records that same command under the gotrue id.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 pr-autopilot — confirmed real, but escalating the implementation for a design call rather than guessing.
The parity gap is exactly as you describe. In Go the post-run capture and the stitch share one live *telemetry.Service: StitchLogin mutates s.state.DistinctID (internal/telemetry/service.go:141) and the cli_command_executed capture in Execute() (cmd/root.go:177) sends under s.distinctID(), which returns the just-stitched id first (service.go:203-207). Our stitcher only writes telemetry.json + alias; the capture resolves identity via context.distinct_id ?? runtime.distinctId ?? runtime.deviceId (legacy-analytics.layer.ts:190) and TelemetryRuntime is immutable, loaded once before the command runs — so the stitching command's own event is attributed to the device id, not the gotrue id.
Why I'm not just patching it here:
- The capture honors
context.distinct_idfirst, so the natural fix is to thread the stitched id into the command's analytics context after the handler runs. But the stitched state lives in the per-commandLegacyIdentityStitchservice, whilelegacyAnalyticsLayeris provided at program scope in the sharedrun.ts— a root-provided holder can't be shared with the per-command stitch without a module singleton (poor test isolation) or aContext.Referencewith careful scoping. - The bug is general (any command whose first Management API response carries
X-Gotrue-Id— e.g.projects listvia the eager runtime — mis-attributes that command), so a complete fix means exposing the stitched id on every command runtime and extending the stitch service shape, not just lint/advisors. - Telemetry identity has no e2e/integration safety net, so a wrong cross-scope fix would ship silently — the exact drift class we want to avoid.
Proposed approach for your sign-off: extend LegacyIdentityStitch with a stitchedDistinctId getter (same closed-over cell the stitcher already writes), expose the service on every legacy command runtime, and have the shared instrumentation read it (via Effect.serviceOption, defaulting to none) after self completes and pass it as context.distinct_id to the cli_command_executed capture. That reproduces Go's one-live-service attribution without touching shared run.ts. Does that shape work, or would you prefer a different home for the stitched-identity state?
| if (flags.linked) setFlags.push("linked"); | ||
| if (flags.local) setFlags.push("local"); |
There was a problem hiding this comment.
When users pass Cobra-accepted boolean forms such as db lint --linked=false or --local=false --linked, Go still treats those flags as explicitly set because ParseDatabaseConfig and MarkFlagsMutuallyExclusive key off pflag Changed, not the boolean value. This truthiness check drops explicitly-false flags, so --linked=false falls through to local and --local=false --linked avoids the mutual-exclusion error; use raw argv/CLI changed metadata instead. The advisors handler has the same value-based target check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 pr-autopilot — you're right about the parity gap, but escalating because the correct fix is bigger than these two handlers.
Confirmed against Go: ParseDatabaseConfig selects the connection by pflag Changed first (internal/utils/flags/db_url.go:49-63), and MarkFlagsMutuallyExclusive("db-url","linked","local") (cmd/db.go:505 for lint, :535 for advisors) is enforced by cobra pre-RunE off Changed too — not the boolean value. So Go routes --linked=false to the linked branch and errors on --local=false --linked, both of which our value-based truthiness checks (lint.handler.ts:112-113, advisors.handler.ts:225-226) get wrong. And it's reachable: the Effect CLI parser accepts both --linked=false and --no-linked, so this isn't theoretical.
Why not inline here: Effect hands the handler only the resolved boolean with no "changed"/presence metadata, so a faithful fix has to read raw argv via the CliArgs service and reconstruct presence for --linked / --linked=false / --no-linked. To make --local=false --linked error and --linked=false actually hit the linked branch, that presence logic also has to replace the truthiness branch in the shared resolver LegacyDbConfigResolver (legacy-db-config.layer.ts:411,456), which the whole db family (reset, dump, push, pull, query, test, inspect, …) shares as it's ported. That's a deliberate family-wide change with its own test matrix, not something to fold into this lint/advisors PR.
Proposal: track this as a follow-up that introduces a shared legacy-db-target-flags helper (argv-presence → setFlags + connType following db_url.go's Changed-first order), wired through the resolver and every db-family handler at once, with integration coverage for --linked=false, --no-linked, and --local=false --linked. Want it split out that way?
…miss to match Go (review) Go's ensureProjectGroupsCached (cmd/root.go:214-235) delegates to CacheProjectAndIdentifyGroups (internal/telemetry/project.go:66-88), which on a cache miss writes linked-project.json AND sends GroupIdentify for the organization and project before the post-run cli_command_executed capture. The ported cache writer only wrote the file, so the first db lint/advisors --linked run before a supabase link never published the org/project group metadata Go sends. Fire the same two gated groupIdentify calls (same payload shape as the link handler) inside cache(), reusing the existing fetched ref/name/org fields; gated on the same cache-miss as the write.
…y to match Go (review)
Go's changedFlags() (cmd/root_analytics.go:53-76) builds the telemetry flags map
from pflag's Visit, which reports the canonical flag name (e.g. schema) whether
the user typed --schema or the registered shorthand -s (cmd/db.go:506). The TS
instrumentation discovered changed flags only from --long argv tokens, so
`db lint -s public` recorded no schema flag at all. Teach extractChangedFlagNames
to resolve single-dash shorthands (-s, -s=value, -svalue) through an optional
alias map, and pass { s: "schema" } from the lint command.
1 participant
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