feat(cli): port db dump, query, and schema declarative to native TypeScript#5586
feat(cli): port db dump, query, and schema declarative to native TypeScript#5586Coly010 wants to merge 136 commits into
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@04dfb9557e4ab31d289e488a7e6402f2656ea310Preview 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
cli/apps/cli/src/legacy/commands/db/dump/dump.command.ts
Lines 66 to 69 in 8299115
Split comma-separated string-slice flags
Go registers this as StringSliceVarP, so --schema public,storage becomes two schemas before the dump env joins them with |; here it remains a single value (public,storage), so pg_dump receives a schema pattern that does not match the requested schemas. The same parser shape is used for the new declarative schema filters, so comma-separated schema lists regress from the Go CLI.
ℹ️ 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".
|
🤖 pr-autopilot — correction + fix: the My earlier framing here ("environmental flake, not a code defect") was wrong — correcting the record. Why it failed only on this PR: this PR flips What actually differed (verified): Go streams Docker image-pull progress to stderr via the Docker API + Fix (pushed, |
Reproduce cobra's MarkFlagsMutuallyExclusive ValidateFlagGroups errors, byte-for-byte, before any side effects run (cobra validates flag groups before PreRunE/RunE): - db query: --db-url/--linked/--local (apps/cli-go/cmd/db.go:526). Previously the handler silently branched on --linked, risking a query against a different database than intended. - db schema declarative generate: --db-url/--linked/--local (apps/cli-go/cmd/db_schema_declarative.go:499). Previously an arbitrary precedence ternary picked --local and ignored the others. - db schema declarative sync: --apply/--no-apply (apps/cli-go/cmd/db_schema_declarative.go:490). Previously --no-apply won silently in the apply-decision helper. Matches the existing inline pattern in inspect db / test db / db dump. review: PR #5586 threads (query/generate/sync mutually-exclusive targets)
The success PostRun line (Go's fmt.Println → stdout, apps/cli-go/cmd/db_schema_declarative.go:93) was written via output.raw unconditionally, so under --output-format json/stream-json it corrupted the machine payload on stdout. Gate it on output.format: keep the raw line on stdout in text mode (Go parity), emit a structured output.success in json/stream-json modes (CLI-1546: machine stdout is payload-only). Mirrors the backups/list gating pattern. review: PR #5586 thread (avoid raw stdout in JSON output mode)
Go's ToPostgresURL appends every pgconn RuntimeParams entry to the query string (apps/cli-go/internal/utils/connect.go:30-33). The TS builder dropped them, emitting only connect_timeout, so the Supavisor pooler tenant-routing `options=reference=<ref>` never reached pg-delta and declarative generate could connect to the wrong tenant on pooler fallback. Append `options` using a Go url.QueryEscape-faithful encoder (space -> +). review: PR #5586 thread (preserve pooler startup options in pg-delta URLs)
…ails When applying the generated migration fails, the user accepts reset, and `db reset --local` also fails, Go returns resetErr — the failure that blocked recovery — not the original apply error (apps/cli-go/cmd/db_schema_declarative.go:414-423). The TS port returned the apply error, hiding the reset failure. Return a reset error and include its detail in the "Database reset also failed: …" line. review: PR #5586 thread (return the reset failure after reset also fails)
Two Go-parity fixes for the declarative pg-delta edge-runtime container: - --network-id: Go's DockerStart overrides the network mode (host included) with --network-id when set (apps/cli-go/internal/utils/docker.go:267-271). The layer hardcoded host networking, so declarative runs couldn't reach the local stack on custom networks. Resolve LegacyNetworkIdFlag and pass a named network, mirroring db dump / gen types / test db. - deno_version: Go switches the edge-runtime image to the deno1 tag when [edge_runtime].deno_version = 1 (apps/cli-go/pkg/config/config.go:999-1008). The layer hardcoded the default (2). Surface deno_version from legacyReadDbToml and feed it to the already-parity-correct image resolver. Unit tests added. review: PR #5586 threads (honor --network-id; read edge_runtime.deno_version)
Go derives the local DB host from utils.Config.Hostname / GetHostname() (SUPABASE_SERVICES_HOSTNAME -> tcp DOCKER_HOST -> 127.0.0.1, apps/cli-go/internal/utils/misc.go:298-312), not a hardcoded loopback. The declarative generate and sync handlers hardcoded 127.0.0.1, so the edge-runtime container connected to the wrong host in dev-container / remote-Docker setups. Reuse the already-ported legacyGetHostname() (same resolver gen types / the db-config resolver use). review: PR #5586 thread (use the configured local database hostname)
…Script Replace the Go-proxy stubs for `db dump`, `db query`, and `db schema declarative generate`/`sync` with native Effect handlers in the legacy shell, plus the shared infrastructure they require (raw Postgres connection layer with COPY/queryRaw, Docker run-capture, pg-delta SSL + URL helpers, edge-runtime script layer, declarative orchestration/cache/debug-bundle, and the `__catalog` Go seam they delegate to). Output-flag parity: - `db query` honors `-o json|table|csv` (Go's command-local enum). The shared global `LegacyOutputFlag` cannot vary its choice per command (the Effect CLI builds one tree-wide global registry), so its choice is the union of every command's `--output` values and each command re-validates against its own Go enum via `withLegacyCommandInstrumentation`'s `outputFormats`, rejecting out-of-enum values with Go's byte-exact pflag message before the handler runs. - Resource commands keep rejecting `table`/`csv`; only `db query` accepts them. Connection errors: establishing the shared raw client now raises `LegacyDbConnectError` (surfaced verbatim by `copyToCsv`/`queryRaw`) instead of a misleading copy/exec error.
Mirror Go's ensureProjectGroupsCached PersistentPostRun
(apps/cli-go/cmd/root.go:176,214-234): on the --linked path the handler
now issues GET /v1/projects/{ref} and writes supabase/.temp/linked-project.json
after the query runs (success or failure), via LegacyLinkedProjectCache.
The cache layer no-ops when the file exists, the token is missing, or the
GET is non-200, so an auth-failing query still fires the GET but writes
nothing. --local / --db-url never resolve a ref and so never trigger it.
Fixes the failing e2e parity tests 'db query --linked SELECT 1' and
'[NON_AUTH]' (ci: Run end-to-end tests).
Reproduce cobra's MarkFlagsMutuallyExclusive ValidateFlagGroups errors, byte-for-byte, before any side effects run (cobra validates flag groups before PreRunE/RunE): - db query: --db-url/--linked/--local (apps/cli-go/cmd/db.go:526). Previously the handler silently branched on --linked, risking a query against a different database than intended. - db schema declarative generate: --db-url/--linked/--local (apps/cli-go/cmd/db_schema_declarative.go:499). Previously an arbitrary precedence ternary picked --local and ignored the others. - db schema declarative sync: --apply/--no-apply (apps/cli-go/cmd/db_schema_declarative.go:490). Previously --no-apply won silently in the apply-decision helper. Matches the existing inline pattern in inspect db / test db / db dump. review: PR #5586 threads (query/generate/sync mutually-exclusive targets)
The success PostRun line (Go's fmt.Println → stdout, apps/cli-go/cmd/db_schema_declarative.go:93) was written via output.raw unconditionally, so under --output-format json/stream-json it corrupted the machine payload on stdout. Gate it on output.format: keep the raw line on stdout in text mode (Go parity), emit a structured output.success in json/stream-json modes (CLI-1546: machine stdout is payload-only). Mirrors the backups/list gating pattern. review: PR #5586 thread (avoid raw stdout in JSON output mode)
Go's ToPostgresURL appends every pgconn RuntimeParams entry to the query string (apps/cli-go/internal/utils/connect.go:30-33). The TS builder dropped them, emitting only connect_timeout, so the Supavisor pooler tenant-routing `options=reference=<ref>` never reached pg-delta and declarative generate could connect to the wrong tenant on pooler fallback. Append `options` using a Go url.QueryEscape-faithful encoder (space -> +). review: PR #5586 thread (preserve pooler startup options in pg-delta URLs)
…ails When applying the generated migration fails, the user accepts reset, and `db reset --local` also fails, Go returns resetErr — the failure that blocked recovery — not the original apply error (apps/cli-go/cmd/db_schema_declarative.go:414-423). The TS port returned the apply error, hiding the reset failure. Return a reset error and include its detail in the "Database reset also failed: …" line. review: PR #5586 thread (return the reset failure after reset also fails)
Two Go-parity fixes for the declarative pg-delta edge-runtime container: - --network-id: Go's DockerStart overrides the network mode (host included) with --network-id when set (apps/cli-go/internal/utils/docker.go:267-271). The layer hardcoded host networking, so declarative runs couldn't reach the local stack on custom networks. Resolve LegacyNetworkIdFlag and pass a named network, mirroring db dump / gen types / test db. - deno_version: Go switches the edge-runtime image to the deno1 tag when [edge_runtime].deno_version = 1 (apps/cli-go/pkg/config/config.go:999-1008). The layer hardcoded the default (2). Surface deno_version from legacyReadDbToml and feed it to the already-parity-correct image resolver. Unit tests added. review: PR #5586 threads (honor --network-id; read edge_runtime.deno_version)
Go derives the local DB host from utils.Config.Hostname / GetHostname() (SUPABASE_SERVICES_HOSTNAME -> tcp DOCKER_HOST -> 127.0.0.1, apps/cli-go/internal/utils/misc.go:298-312), not a hardcoded loopback. The declarative generate and sync handlers hardcoded 127.0.0.1, so the edge-runtime container connected to the wrong host in dev-container / remote-Docker setups. Reuse the already-ported legacyGetHostname() (same resolver gen types / the db-config resolver use). review: PR #5586 thread (use the configured local database hostname)
Coly010
force-pushed
the
cli/port-db-dump-query-schema-commands
branch
from
65bf5ff to
e86f08c
Compare
The db dump --local parity test diverged because Go streams Docker image-pull progress to stderr via the Docker API + jsonmessage (apps/cli-go/internal/utils/docker.go:206-214), while the native ts-legacy LegacyDockerRun shells out to docker run (different auto-pull format). Pull progress is non-deterministic (layer IDs/order/timing) and only appears on a cache miss; Go's own dump tests mock Docker and never assert on it. Strip both pull formats (API jsonmessage + docker-run CLI) from the shared normalize() so a cold image pull no longer produces false parity failures. The CLI output itself already matches Go byte-for-byte (the schemas line, pg_dump error, 'error running container: exit N', and the --debug suggestion all have exact Go equivalents) — only the pull-progress noise differed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cba59d44cd
ℹ️ 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 confirmOverwrite goes through Console.PromptYesNo, which returns true immediately when the global YES flag is set (apps/cli-go/internal/utils/console.go:70-73). The TS handler always prompted, so --yes runs errored in non-interactive/JSON mode and blocked in a TTY. Gate the overwrite prompt on the yes flag. review: PR #5586 thread (honor --yes for overwrite prompts)
…ations Go's applyMigrationToLocal connects with utils.Config.Hostname (apps/cli-go/cmd/db_schema_declarative.go:463); the apply path still hardcoded 127.0.0.1 even after the diff/generate URLs were switched to legacyGetHostname(). Under SUPABASE_SERVICES_HOSTNAME / tcp DOCKER_HOST, --apply wrote the migration then applied it to the wrong host. Use legacyGetHostname() for the apply too. review: PR #5586 thread (use configured hostname when applying sync migrations)
The hidden supabase-go __catalog seam provisions the shadow DB via DockerStart, which reads --network-id from viper (apps/cli-go/internal/utils/docker.go:267-271). The seam argv omitted it, so on a custom network the shadow/catalog containers landed on the default network while pg-delta containers used --network-id. Forward --network-id on the seam argv when set, like LegacyGoProxy does. review: PR #5586 thread (forward --network-id into the catalog seam)
Two Go-parity fixes for the native db dump: - --schema/--exclude are cobra StringSlice in Go (apps/cli-go/cmd/db.go:432,444), which comma-splits each value before building the pg_dump env. The Effect CLI flags don't split, so --schema public,auth emitted one pattern; split on comma. - Go chdir's into the workdir before opening --file (cmd/root.go:104), so a relative --file resolves against the workdir. Resolve flags.file against cliConfig.workdir for the open, write, and the reported absolute path. review: PR #5586 threads (split dump comma-list flags; resolve dump output files from the workdir)
Go chdir's into the workdir in PersistentPreRunE before ResolveSQL reads --file (apps/cli-go/cmd/root.go:104), so a relative --file resolves against the workdir, not the original process cwd. Resolve flags.file against cliConfig.workdir. review: PR #5586 thread (resolve db query files from the workdir)
Go's db query --linked PreRun calls flags.LoadProjectRef (load-or-fail, no prompt) and errors with ErrNotLinked when the workdir isn't linked (apps/cli-go/cmd/db.go). The TS handler used the prompting resolve(), which on an interactive TTY would prompt for a project and run the SQL against it. Use the non-prompting resolveOptional, failing with the not-linked error and validating the ref like Go's AssertProjectRefIsValid. review: PR #5586 thread (reject unlinked --linked query instead of prompting)
…ve generate Go's smart-mode 'Custom database URL' branch parses the entry with pgconn.ParseConfig and feeds pg-delta a normalized ToPostgresURL (apps/cli-go/cmd/db_schema_declarative.go:283-287). The TS branch only empty-checked and returned the raw string, so malformed URLs were passed to pg-delta and valid libpq DSNs were not normalized. Parse with parseLegacyConnectionString (project-env layered, like the --db-url path), fail with the redacted 'failed to parse connection string' error, and return legacyToPostgresURL on success. review: PR #5586 thread (validate smart custom database URLs before pg-delta)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e37dba771
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Reject db.port=0 during config load
When db.port = 0 or SUPABASE_DB_PORT=0 is set, this helper returns 0 and the caller only checks for undefined, so native local query/dump/declarative paths proceed with port 0. Go decodes the value but then config.Validate rejects c.Db.Port == 0 with Missing required field in config: db.port (apps/cli-go/pkg/config/config.go:866-868), so this should fail before any local connection or Docker work is attempted.
Treat sslrootcert=system as system roots
For a non-local --db-url or PG/service setting with sslrootcert=system, pgconn's TLS config treats system as a sentinel and loads the OS certificate pool instead of opening a file. This path passes the literal string to readFileSync, so native direct connections fail with failed to read sslrootcert system unless a file named system happens to exist; handle the sentinel before reading a CA bundle path.
ℹ️ 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 fresh = new Pg.Client(winningRawConfig); | ||
| yield* Effect.tryPromise({ | ||
| try: () => fresh.connect(), |
There was a problem hiding this comment.
For local/--db-url db query, connect() has already opened and keeps the probed PgClient connection alive, then queryRaw opens this additional pg.Client for the actual query. On databases or roles with a one-connection limit this native path can fail or hang even though Go's RunLocal uses the single pgx connection for the query and the agent RLS advisory; the raw query path should not hold an unused first backend while opening a second one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Escalating to a maintainer — real parity gap, but no safe minimal fix.
The diagnosis is correct: on the local / --db-url db query path, runLocal only ever uses queryRaw (query.handler.ts), so the probed primary PgClient from connect() sits idle while queryRaw opens a second physical pg.Client (legacy-db-connection.sql-pg.layer.ts) for the actual query + RLS advisory. Go's RunLocal uses a single pgx connection for both (internal/db/query/query.go:27-79), so we hold two backends where Go holds one — which can fail/hang on a one-connection-limit role.
I'm not patching it under this PR because there's no safe, contained fix:
queryRawcan't reuse the probed client — it needs node-postgres features@effect/sql-pgdoesn't expose (positionalrowMode, extended-mode multi-statement rejection, raw command tag).- The probe can't be dropped — it drives the HA-fallback / DoH / TLS chain and selects the
winningRawConfigthe raw client reuses. - Closing the primary mid-scope would break consumers that interleave
exec/query/extensionExists/waitForTempRole/copyToCsvwithqueryRaw.
A proper fix likely means giving the shared connection layer a notion of a "raw-only" session (probe + own a single pg.Client, no separate primary) — a change to the layer's contract that touches every db command. That's a maintainer design call, not something to reshape unattended in this port PR. Flagging for a follow-up; the extra idle backend is otherwise a known, low-severity deviation.
| case "keep-comments": | ||
| return Option.isSome(flags.keepComments); | ||
| case "schema": | ||
| return flags.schema.length > 0; |
There was a problem hiding this comment.
When a script passes db dump --schema '' --role-only, pflag still marks schema as changed, so Cobra's MarkFlagsMutuallyExclusive("schema", "role-only") rejects the invocation before running. Because the CSV parser turns the empty value into an empty array, this length check treats schema as unset and lets a role dump run where Go would stop at flag validation; preserve schema flag presence separately from its parsed values for this group check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferring — Go's behaviour is clear, but the clean fix doesn't fit the current flag architecture, and the input is pathological.
You're right that Go treats --schema '' --role-only as a conflict: --schema is a StringSliceVarP (cmd/db.go:444) whose Changed is set on any value including empty, and MarkFlagsMutuallyExclusive("schema","role-only") (:445) keys off Changed. Our mutex check uses flags.schema.length > 0, and the CSV parser collapses --schema '' to [], so presence is lost.
The problem is recovering presence cleanly. Unlike the other flags in this group (modelled as Option, so --x=false keeps presence), --schema is a repeatable Flag.string(...).atLeast(0) whose parsed value (string[]) genuinely can't distinguish "absent" from "present-empty". The only ways to fix it are:
- Read raw
process.argvin the handler to recover pflagChanged— but that's architecturally wrong (handlers are driven by parsed flags, not argv), untestable in the handler harness (tests pass synthetic flags; argv is the test runner's), and fragile against the runner's own args. - Rework the schema flag modelling to carry presence alongside the parsed list — a broader change to the parse-at-flag-level approach used by
gen types/db lint/ declarative / dump.
Given --schema '' is a pathological input (an explicitly-empty schema selector) and both fixes are disproportionate, I'm leaving this open for a maintainer call rather than introducing argv coupling. If you'd like option 2, it'd be a deliberate flag-modelling change across the schema-flag commands.
…tch Go PreRun order (review: #PRRT_kwDOErm0O86KSSSE)
There was a problem hiding this comment.
💡 Codex Review
Load linked overrides before validating DB config
When resolving a linked target, this unconditional base legacyReadDbToml validates the unmerged supabase/config.toml before the linked branch has a chance to apply [remotes.<ref>]. I checked the Go path (apps/cli-go/internal/utils/flags/db_url.go calls LoadProjectRef before LoadConfig, and apps/cli-go/pkg/config/config.go merges the matching remote block before c.load validation), so a linked workspace where the base config has e.g. an invalid db.major_version or edge_runtime.deno_version but the matching remote override supplies a valid value succeeds in Go and fails here before db dump --linked / linked query pre-run can run.
ℹ️ 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".
…view: #PRRT_kwDOErm0O86KTBIu)
…tch Go confirmOverwrite (review: #PRRT_kwDOErm0O86KTBIp)
…ch Go PostRun (review: #PRRT_kwDOErm0O86KTBIs)
…ostRun (review: #PRRT_kwDOErm0O86KTBIy)
There was a problem hiding this comment.
💡 Codex Review
cli/apps/cli/src/legacy/shared/legacy-db-config.layer.ts
Lines 453 to 456 in 552172e
Do not inject local config passwords into db-url dumps
When --db-url points at the local stack without a password, e.g. postgresql://postgres@127.0.0.1:54322/postgres, this shared resolver replaces the empty password with [db].password for every consumer. Go only fills that default inside ConnectLocalPostgres; db dump and declarative generate pass flags.DbConfig to pg_dump/ToPostgresURL directly, so they leave the password empty unless the URL/env/pgpass supplied one. With this override native dump/generate can succeed against a target Go would reject, so the local-password fill needs to be limited to live connection paths such as query/lint.
ℹ️ 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 unlinked, to match Go LoadProjectRef (review: #PRRT_kwDOErm0O86KTxWV)
… (review: #PRRT_kwDOErm0O86KTxWP)
…PostRun (review: #PRRT_kwDOErm0O86KTxWR)
…stRun (review: #PRRT_kwDOErm0O86KTxWS)
…Go PostRun (review: #PRRT_kwDOErm0O86KTxWJ)
…E_EFFECTS (ci: code quality)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 842dfd6170
ℹ️ 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".
| // Validate the `-o` enum first, before instrumentation runs the handler, so a | ||
| // rejected flag fails without emitting a `cli_command_executed` event — Go | ||
| // rejects it at parse time, before telemetry. | ||
| Effect.andThen(validateLegacyOutputFormat(allowed), instrument(self)); |
There was a problem hiding this comment.
Because this per-command enum check runs inside the handler effect, it only executes after the command's Command.provide(...) layer has been acquired. Now that the global parser accepts table/csv, an eager Management API command such as projects list can build legacyManagementApiRuntimeLayer and resolve the access token before this validation runs; in a logged-out shell, supabase projects list -o table will fail with the missing-token error instead of Go's parse-time invalid argument "table"..., and logged-in runs still perform auth/keyring work before rejecting the flag. The output enum rejection needs to happen before command runtime acquisition, or the parser needs a pre-runtime per-command rejection path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — this is a real parity gap, not just theoretical. In Go the global -o/--output is an EnumFlag allowing only [env|pretty|json|toml|yaml] (cmd/root.go:332, internal/utils/enum.go:20-26), and pflag rejects an out-of-set value during flag parsing, before PersistentPreRunE/auth runs (cmd/root.go:93,159). So supabase projects list -o table fails at parse time with invalid argument "table" for "-o, --output" flag: ... and never touches auth/keyring.
Here the union global flag (shared/legacy/global-flags.ts, widened to include table/csv for db query) pushes per-command rejection into validateLegacyOutputFormat inside the handler effect (legacy-command-instrumentation.ts), which runs after Command.provide(...) builds the runtime. For eager Management API commands (projects list), legacyPlatformApiLayer resolves+validates the access token at layer-build time, so logged-out runs fail with the missing-token error and logged-in runs do keyring/auth work before the flag is rejected. (db query avoids this because it uses the lazy resolver runtime.)
I don't think there's a safe drive-by fix, so I'm flagging this for a maintainer decision rather than forcing it. The Effect CLI builds one tree-wide global-flag registry, so we can't restore Go's per-command parse-time Flag.choice (the constraint documented at global-flags.ts:3-11). The two viable fixes both have non-trivial blast radius:
- Lift the enum check outside
Command.providefor the ~72 commands that usewithLegacyCommandInstrumentation(split validation out of the instrumentation wrapper). Most parity-faithful, since the check only needsLegacyOutputFlag(a root global) — but it changes the command-wiring contract broadly. - Make the eager Management API runtime lazy (like
legacyLinkedDbResolverRuntimeLayer) so no token resolves at build — but that shifts auth-failure ordering/timing for every eager command.
Both interact with telemetry ordering and the CLI-1546 quiet-layer wiring, so I'd rather not pick one inline. @author — which approach do you prefer? I'd lean toward (1). Leaving this open for your call.
…Token to match Go LoadAccessTokenFS (review: #PRRT_kwDOErm0O86KUyNX)
There was a problem hiding this comment.
💡 Codex Review
Defer linked config validation until after remote merge
When a linked DB command has a [remotes.<ref>] block that overrides a base config value which would otherwise be invalid, this unmerged read fails before the ref-specific read below can apply the override. Go's linked ParseDatabaseConfig loads the project ref first and then LoadConfig merges [remotes.<ref>] before decoding/validating, so a base db.major_version = 12 corrected to 17 in the matching remote block succeeds there but db dump/query --linked fails here before reaching the merged validation.
ℹ️ 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 status: the 3 red checks are infra flakes, not this PR. 🟡 The failing checks —
All unit, integration, type, lint, fmt and knip checks pass, and every non-e2e CI check is green. I re-ran the failed jobs once; they hit the same sustained registry throttling, so I'm not re-running blindly again. This needs an infra/maintainer action rather than a code change here — e.g. authenticating the e2e Docker pulls, routing the I'll leave the reruns to a maintainer to avoid burning the rate-limit budget. Happy to re-run once the registry throttling clears. |
…match Go flags.Visit (review: #PRRT_kwDOErm0O86KVYMQ #PRRT_kwDOErm0O86KVYMS)
…ses (ci: code quality)
…surface, matching Go (review: #PRRT_kwDOErm0O86KVYMH)
… to match config.Load (review: #PRRT_kwDOErm0O86KVYMN)
…ump to match Go (review: #PRRT_kwDOErm0O86KVYMW)
|
CI status: the The The green spec's Crucially, unlike the earlier This is independent of this PR — the diff here is entirely TypeScript under |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8df2b0fa7
ℹ️ 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".
…to match Go (review: #PRRT_kwDOErm0O86KWEZP #PRRT_kwDOErm0O86KWEZT)
…to match Go (review: #PRRT_kwDOErm0O86KWEZQ #PRRT_kwDOErm0O86KWEZX)
…s surface, matching Go (review: #PRRT_kwDOErm0O86KWEZk)
…o match Go (review: #PRRT_kwDOErm0O86KWEZa)
…unctionSlug (review: #PRRT_kwDOErm0O86KWEZf)
1 participant
What changed
Replaces the Go-proxy stubs for
db dump,db query, anddb schema declarative generate/syncwith native Effect handlers in the legacy shell, along with the shared infrastructure they need:legacy-db-connection.sql-pg.layer.ts): rawpgclient for the COPY protocol and full-metadataqueryRaw(command tag via thecommandCompleteprotocol message), reusing the winning dial target so TLS/fallback/DoH parity holds.__catalogGo seam (apps/cli-go/...) the TS port delegates to for shadow-database provisioning.Why / reviewer context
Strict Go parity is the contract for the legacy shell. Behaviors that look improvable but match Go are intentional and documented in each
SIDE_EFFECTS.md(e.g.db dump --dry-runprints the resolvedPGPASSWORDin cleartext like Go'snoExec;db query --linkednon-2xx maps to a uniformunexpected statusmessage; failed declarativesync --applyleaves the migration file on disk).-o/--outputparity. Go registers--outputper command (db query→json|table|csv; resource commands →env|pretty|json|toml|yaml). The Effect CLI hoists global flags into a single tree-wide registry, so a command cannot redeclare anoutputglobal to vary its enum. The sharedLegacyOutputFlagchoice is therefore the union of all commands' values, and each command re-validates against its own Go enum inwithLegacyCommandInstrumentation(outputFormats), rejecting out-of-enum values with Go's byte-exact pflag message (invalid argument "x" for "-o, --output" flag: must be one of [ … ]) before the handler runs and before any telemetry event fires. The validation reads the flag viaEffect.serviceOption, so it adds no requirement to the wrapper. Net result:db query -o csv/tableworks; resource commands still rejecttable/csvexactly as Go does. This change is fully legacy-scoped —next/uses its own--output-formatflag and is untouched.Connection error typing. Establishing the shared raw client now raises
LegacyDbConnectError(surfaced verbatim by bothcopyToCsvandqueryRaw) rather than a misleading "failed to copy output" / "failed to execute query".Follow-ups (tracked, not in scope)
db dump --linked(the macOS/Docker IPv6 edge case) is not yet ported — noted indump/SIDE_EFFECTS.md.runCapturebuffers dump output in memory rather than streaming to--file.CLOSES CLI-1315