fix(cli): port db reset --experimental remote schema-files path to native TS (CLI-1958) - #6062
Merged
Coly010 merged 52 commits intoAug 10, 2026
Conversation
…tive TS (CLI-1958) Ports Go's apply.MigrateAndSeed EXPERIMENTAL declarative branch (apps/cli-go/internal/migration/apply/apply.go:19,51-68) for db reset's remote (--linked / remote --db-url) path, replacing the last Go-binary delegation on that command. A versionless --experimental / SUPABASE_EXPERIMENTAL reset with pg-delta not enabled now applies [db.migrations].schema_paths files directly (legacyApplySchemaFiles) instead of replaying timestamped migrations, faithfully reproducing two undocumented Go quirks: an empty schema_paths default silently applies nothing, and a partial glob failure is swallowed once at least one pattern matches. Hoists the Glob.SQLFiles traversal (legacySqlFilesGlob) out of the seed pipeline into shared/ so both [db.seed].sql_paths and the new [db.migrations].schema_paths resolve through one port of Go's glob semantics. Exposes schema_paths from the db-config TOML reader with the same env-override/remote-block-merge handling as the sibling seed field. Removes the remaining LegacyGoProxy delegation from db reset's handler and runtime layer now that both the remote and local paths are fully native (the local path's own schema-files branch still runs behind the existing db __db-bootstrap seam, out of scope here).
Whitespace-only fix following the db reset (CLI-1958) note update — oxfmt recomputes column widths across the whole markdown table.
…ort (CLI-1958) Fixes the items three reviewers (go-parity, engineer, architect) converged on: - Correct legacyMigrateAndSeed's stale docstring: the EXPERIMENTAL schema-files branch is reachable from start's fresh-volume setup (version: ""), not just migration down, and is deliberately deferred to CLI-2040, not unreachable. - Route legacy-seed.ts's resolveSeedFiles through the shared legacySqlFilesGlob instead of a third hand-rolled glob copy, fixing a silent directory-expansion gap on the migration down/start seed path. Remove legacy-seed-ops.ts's now- redundant legacyGlobSeedFiles/LegacyGlobResult pass-through shim. - Add Go's GlobOption surface (skipEmptyGlobs/errorOnAllSkipped) to legacySqlFilesGlob for db diff's upcoming declarative path; this issue's own callers pass no options, so behavior is unchanged. - Fix legacy-sql-files-glob.ts's toSlash to only convert on win32, matching Go's filepath.ToSlash (a no-op on non-Windows) — otherwise routing legacy-seed.ts's backslash-escape patterns through the shared glob would corrupt them. - Add reset.integration.test.ts coverage for schema_paths declaration order across multiple patterns and directory-entry expansion, plus toml-read tests for SUPABASE_DB_MIGRATIONS_SCHEMA_PATHS's env-override, string, non-string- filter, and remote-suppression branches. - Correct reset.layers.ts's docstring: the local reset path still reaches LegacyGoProxy through the bootstrap seam (CLI-1955's scope), only the remote path is fully native.
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…8-port-db-reset-experimental-remote-schema-files-path-natively # Conflicts: # apps/cli/docs/go-cli-porting-status.md
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@fe3c34fe1b2547c50e6e4c300fb38d48a442836fPreview 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: 8ff4485289
ℹ️ 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".
… (CLI-1958)
`legacySqlFilesGlob`/`legacyWalkSqlFiles` diverged from Go's
`config.Glob.SQLFiles` (`apps/cli-go/pkg/config/config.go:123-211`) in two
ways, both confirmed empirically against a built `apps/cli-go` probe:
- Directory expansion re-`stat`ed each child, following symlinks. Go's
`fs.WalkDir` types children from their parent's `ReadDir` entry
(Lstat-based), so a symlinked `.sql` file or subdirectory below a matched
schema/seed directory is never included or recursed into
(`io/fs/walk.go:114-115`). Detect this with `readLink` (succeeds only for
symlinks) before falling back to `stat`.
- An empty pattern (e.g. `schema_paths = [""]`) resolved via
`path.join(workdir, "")` to the workdir itself and reported a match. Go's
`Lstat("")` fails, so `fs.Glob`/`afero.Glob` always report no match for an
empty pattern. Short-circuit on `pattern.length === 0`.
Review: PR #6062 (chatgpt-codex-connector), threads on
legacy-sql-files-glob.ts:92 and :40.
…I-1958)
`[db.migrations].schema_paths` and `[db.seed].sql_paths` decode through Go's
`v.UnmarshalExact` (`apps/cli-go/pkg/config/config.go:749-756`), whose
decoder config never overrides `WeaklyTypedInput`, so viper's
`defaultDecoderConfig` default of `true` stands. mapstructure's
`decodeString` therefore weakly converts a non-string scalar array element
(bool to "1"/"0", a number to its decimal string) instead of erroring or
dropping it. The TS reader filtered non-string entries out instead, silently
dropping schemas.
Verified empirically against a built `apps/cli-go` probe: `schema_paths =
[42, true, "schemas/*.sql"]` resolves to `supabase/{42,1,schemas/*.sql}`.
Review: PR #6062 (chatgpt-codex-connector), thread on
legacy-db-config.toml-read.ts:1887.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7020d74f0f
ℹ️ 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".
…stion to exec-phase (CLI-1958)
Two Go-parity gaps in the experimental `db reset` remote schema-files path,
both verified empirically against apps/cli-go:
- legacySqlFilesGlob/legacyWalkSqlFiles silently treated a directory-read
failure during walk as an empty match. Go's fs.WalkDir propagates a ReadDir
error and walkMatchedDir wraps it as "failed to walk matched directory: ...",
which applySchemaFiles only discards when at least one OTHER file was still
found (declared non-empty); with nothing else matched, Go aborts before
applying anything. The walk failure now surfaces as a warning via
Effect.result/Result.isFailure, so the existing files.length === 0 gate
correctly turns it fatal instead of reporting silent success after schemas
are already dropped.
- legacyApplySchemaFiles attached Go's CmdSuggestion ("See schema file: ...")
to every legacyExecSqlFile failure, but Go's applySchemaFiles only sets
CmdSuggestion after ExecBatch (statement execution) fails -- a
NewMigrationFromFile (file-read) failure returns before CmdSuggestion is
ever touched. execMigrationBatch's mapError callback now carries a
"read"/"exec" phase tag so the suggestion is attached only on exec-phase
failures.
Extracted the ad hoc errMessage helper (legacy-migration-apply.ts) into
shared legacy-error-message.ts so legacy-sql-files-glob.ts can reuse it
without a circular import.
…ies (CLI-1958)
Go's config.Glob decode (UnmarshalExact, config.go:749-756) weakly coerces a
bool/number array element but hits mapstructure's UnconvertibleTypeError for
a non-scalar one (nested array/table), which aborts the ENTIRE config load
with "failed to parse config: decoding failed due to the following
error(s): ...". Verified empirically against apps/cli-go:
`schema_paths = [[]]` / `[{path = "x.sql"}]` both fail config.Load with that
exact message before any schema is dropped; multiple bad entries are
aggregated in one message.
legacyWeakCoerceGlobEntry previously filtered these elements out silently,
which on an experimental remote reset could drop remote schemas and then
apply zero files while reporting success. legacyReadDbToml now fails the
whole config load with the byte-matching mapstructure-style message instead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fa1508080
ℹ️ 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".
…1958) Go's mapstructure decode is weakly typed on the whole `[]string` field, not just on array elements: a top-level scalar (e.g. `schema_paths = 42`) is wrapped into a synthetic single-element array and decoded through the same per-element rules as a real array entry, and a non-empty table fails with the same unconvertible-type error an array element would. The native reader only applied that weak coercion to array elements, so a scalar fell through to the empty/default fallback instead of resolving (and potentially warning) like Go does. Verified empirically against apps/cli-go.
…1958) Two Go-parity gaps in the shared SQL-file globber: - A matched path that fails to stat (a broken symlink, or a file that disappears between the glob and the stat) was falling back to treating it as a regular file. Go's Glob.SQLFiles records a "failed to stat matched file" warning and skips the path instead; the fallback here let a later read of the nonexistent path turn a warned-but-otherwise-successful reset into a hard apply error. - Splitting an absolute pattern whose meta character is in the first path component (e.g. `/*.sql`, `/tmp*/*.sql`) collapsed the root directory to `""`, which the globber treats as "use the workdir". Go's real runtime glob path (afero.IOFS.Glob -> afero.Glob) explicitly special-cases a bare `/` and keeps it, so the pattern resolves against the filesystem root, not cwd. Verified empirically against apps/cli-go.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efbc9a070b
ℹ️ 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".
…review: CLI-1958)
Go's fs.WalkDir builds each child path via path.Join, which runs path.Clean
and collapses a doubled `/`. The native walkMatchedDir port instead
string-concatenated `${rel}/${name}`, so a literal schema_paths/sql_paths
directory entry ending in `/` (e.g. "/tmp/schemas/") produced
"/tmp/schemas//a.sql" instead of Go's "/tmp/schemas/a.sql" for every child.
Verified empirically: a scratch apps/cli-go probe calling
config.Glob{"<dir>/"}.SQLFiles(...) against a real trailing-slash directory
returns the single-slash path.
…IDE_EFFECTS (review: CLI-1958) The env var table omitted SUPABASE_DB_MIGRATIONS_SCHEMA_PATHS even though this change makes the native remote --experimental schema-files branch resolve [db.migrations].schema_paths through legacy-db-config.toml-read.ts's generic AutomaticEnv-override reader (LEGACY_ENV_OVERRIDABLE_KEYS), which this PR newly added for that key. No dedicated CLI flag exists for schema_paths, so the env var is the only non-config-file override surface.
Contributor
Author
|
pr-autopilot: heads up, this PR now shows Not auto-resolving this — it needs a manual rebase/merge to reconcile the two sides' table edits rather than a scripted fix. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dabdb6ade
ℹ️ 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".
…(review: CLI-1958) Go's fs.WalkDir joins child paths with path.Join, which cleans a bare `.` root away entirely. schema_paths/sql_paths entries like [".."] can resolve to exactly "." via config's own path.Join(SupabaseDirPath, ..), so joinRelChild must special-case rel === "." to match Go's foo.sql instead of ./foo.sql -- otherwise the seed_files.path hash key diverges between Go and native tooling, causing seeds to needlessly re-run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2502f3544f
ℹ️ 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".
…LI-1958) joinRelChild string-concatenated rel/name instead of running Go's path.Clean-equivalent lexical cleaning, so a directory configured with a cleanable segment (e.g. "/tmp/x/../schemas") produced a literal ".."-containing path instead of matching Go's fs.WalkDir output. Verified empirically that Node's path.join matches Go's path.Join byte-for-byte across dot-root, trailing-slash, and embedded "."/".." cases, so delegate to the injected Path service instead of special-casing more segment shapes.
…iew: CLI-1958) Go's fs.WalkDir types each child from the DirEntry its parent ReadDir already returned and never re-Stats through it, so a .sql file removed between ReadDir and the walk callback's own visit stays in Go's declared file list; only the later, real file-open fails loudly. This port's follow-up fs.stat call opens a race window Go doesn't have, and on failure silently classified the child as Unknown and dropped it with no warning - an experimental reset whose only schema file hit this race would "succeed" having applied nothing. Verified empirically with a scratch filepath.WalkDir probe that deletes a sibling .sql file between ReadDir and that file's own visit: Go still reports it IsRegular from the cached DirEntry, keeps it declared, and the later os.Open fails with "no such file or directory" - never a silent drop. Match that outcome: on a stat failure, best-effort include a '.sql'- named child anyway and let the real downstream read surface the failure.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e8393d526
ℹ️ 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".
…1958) legacyDbResetRuntimeLayer omitted LegacyPgDeltaSslProbe, LegacyEdgeRuntimeScript, and LegacyDockerRun, unlike legacyDbPushRuntimeLayer. When pg-delta caching is enabled, the post-reset catalog cache reaches those services via legacyExportCatalogPgDelta; missing them is an untyped missing-service defect the handler's Effect.catch cannot recover from, crashing the process after the remote database has already been reset instead of writing the catalog or emitting Go's best-effort warning. Compose the same three layers db push uses. Added a regression test that builds the real legacyDbResetRuntimeLayer (not a mocked service) and asserts both services are actually exposed.
…egers (review: CLI-1958) parseGoBaseZeroInt rejected underscore digit separators outright (e.g. "1_048_576"), silently falling back to the 256KiB default even though Go's strconv.ParseInt(s, 0, 64) accepts them per its integer-literal grammar. A statement between the two limits would apply in TS but Go would already have failed with "bufio.Scanner: token too long". Implemented Go's exact underscore-placement grammar, verified empirically against a real Go strconv.ParseInt(s, 0, 64): a single underscore may sit immediately after a base prefix (0x/0o/0b, or the bare leading "0" of legacy octal) or between two digits — never doubled, never leading a plain decimal literal, never trailing.
…efore (review: CLI-1958) Both db reset and db push captured the snapshot's Clock.currentTimeMillis before calling legacyTryCacheMigrationsCatalog, i.e. before the hash and the pg-delta export (a network round-trip) resolved. Go's real WriteMigrationCatalogSnapshot reads time.Now().UTC() internally, after TryCacheMigrationsCatalog has already resolved hash and snapshot, immediately before the write. The early capture could make a concurrent cache write from another process sort in the wrong order during catalog resolution/retention. Moved the clock read inside legacyTryCacheMigrationsCatalog itself, right before the write, fixing both callers at their shared root. Added a regression test that proves the timestamp reflects a real time gap the mocked export takes before resolving.
…view: CLI-1958) SIDE_EFFECTS.md labeled the migrations-catalog cache write as remote-path only. Go's start.SetupLocalDatabase (called by the local reset's PG15 recreate branch, behind this port's db __db-bootstrap seam) also calls pgcache.TryCacheMigrationsCatalog after MigrateAndSeed, with a "local" prefix — inherited automatically since the local path delegates to the real Go binary rather than being reimplemented in TS. The PG<=14 branch never calls it at all. Documented both paths and the PG14/PG15 split.
…Dir order (review: CLI-1958) legacyWalkSqlFiles iterated a directory's readDirectory() result in raw filesystem enumeration order. Go's fs.WalkDir visits entries in lexical byte order (os.ReadDir's own "sorted by filename" contract), so when a matched directory has multiple problematic children (e.g. two unreadable subdirectories), Go deterministically fails on the lexically-first one. This port's unsorted iteration could pick a different one depending on filesystem enumeration order, surfacing a different fatal/WARN message than Go. Sorted directory entries with the existing UTF-8 byte-order comparator before recursing. Added a regression test using a fake FileSystem that deliberately returns entries in reverse order to prove the fix, independent of what the real OS happens to return.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89acd6c011
ℹ️ 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".
…log export (review: CLI-1958) db push scopes legacyApplyProjectEnv(projectEnv) around its whole run so a SUPABASE_INTERNAL_IMAGE_REGISTRY/PGDELTA_NPM_REGISTRY set only in supabase/.env reaches the pg-delta edge-runtime helpers, which read process.env directly. db reset loaded the same projectEnv but never applied it, so the same override was silently ignored for its post-reset migrations-catalog export, falling back to the default registries.
…r text (review: CLI-1958)
Two follow-ups on the SUPABASE_SCANNER_BUFFER_SIZE parsing work:
- Reject a magnitude outside Go's signed int64 range (e.g.
"9223372036854775808", one over math.MaxInt64) the same way
cast.ToInt does: strconv.ParseInt(s, 0, 0) returns a range error, and
cast.ToInt discards ANY parseFn error and returns exactly 0 (falls back to
the 256KiB default), not the huge value Number.parseInt would silently
round to. Verified against the pinned spf13/cast@v1.10.0
(cast.ToInt("9223372036854775808") -> 0).
- Track the last RAW scanned token unconditionally, matching Go's
`token = scanner.Text()` (runs on every successful Scan(), before the
len(trim) > 0 append gate). A statement that trims to empty right before
an oversized one (e.g. a lone ";") must still show in the "After
statement N: ..." error text, not a blank token.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ac66092de
ℹ️ 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".
… treating as empty (review: CLI-1958)
smol-toml parses every TOML datetime variant to a TomlDate (a Date
subclass) that stores its value internally, not as an enumerable own
property, so it satisfies the same zero-enumerable-key test this reader
used to detect an empty inline table (`schema_paths = {}`). A bare
datetime therefore silently resolved to an empty pattern list instead of
failing config load.
Verified empirically against the real apps/cli-go config.Load: Go's
mapstructure decoder reports a bare datetime as unconvertible and aborts
the whole load, with a distinct Go type per TOML datetime variant
(time.Time for offset date-time, toml.LocalDateTime/LocalDate/LocalTime
for the three zone-less "local" variants). Exclude TomlDate from the
empty-table special case and teach legacyGoUnconvertibleType to name the
correct per-variant Go type, matching Go's error text exactly whether the
datetime is a top-level scalar or an array element.
…8-port-db-reset-experimental-remote-schema-files-path-natively # Conflicts: # apps/cli/docs/go-cli-porting-status.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d7e1f8ab1
ℹ️ 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".
…8-port-db-reset-experimental-remote-schema-files-path-natively # Conflicts: # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/commands/db/reset/reset.handler.ts # apps/cli/src/legacy/shared/legacy-db-config.toml-read.ts # apps/cli/src/legacy/shared/legacy-migrate-and-seed.ts # apps/cli/src/legacy/shared/legacy-pgdelta.cache.unit.test.ts # apps/cli/src/legacy/shared/legacy-seed.ts
- legacy-sql-split.ts: match Go's unicode.IsDigit (decimal digits only,
\p{Nd}) for dollar-quote tag/identifier characters instead of \p{N}
(all Unicode numbers), which wrongly accepted non-decimal digit runes
like superscript-2 and could split schema SQL differently than Go.
- legacy-seed-ops.ts / legacy-seed.ts: enforce SUPABASE_SCANNER_BUFFER_SIZE
on seed file parsing via the shared checkScannerBufferSize, matching
Go's SeedFile.ExecBatchWithCache, which parses through the same
parseFile every other file type does.
- legacy-pgdelta.cache.ts: listJsonEntries now only swallows a genuinely
missing catalog directory and propagates every other read failure
(e.g. permission denied), matching Go's ReadDir usage after
ensureTempDir — previously every failure was treated as empty,
silently defeating catalog retention and cache resolution.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d1eb7a2b4
ℹ️ 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".
avallete
approved these changes
Aug 10, 2026
avallete
left a comment
avallete
left a comment
Member
There was a problem hiding this comment.
Verified at head 9df1d7b: the two blocking parity bugs (Kong --nginx-conf, PG14 toml.schemaPaths) are fixed and now pinned by tests — confirmed the new integration test fails on the reverted code. None of the items below are merge-blockers; batching them here so they don't get lost. Note several of these were raised by Codex in earlier rounds and their threads were marked resolved without a corresponding commit — worth checking why the autopilot closes threads it hasn't acted on.
Behavior / UX
- Reset-failure suggestions are dropped by the declarative wrap.
LegacyDeclarativeApplyErrorcarries onlymessage(declarative.errors.ts:80-82), so the Kong-reload recovery hint, health-timeout architecture hints, etc. never reach the renderer fromdeclarative.smart-target.ts:181-186andsync.handler.ts:403-405. Go preservesCmdSuggestion; the old subprocess printed it on inherited stderr. Add an optionalsuggestionto the error (or propagate the reset failure unwrapped, matching Go'sreturn resetErr) — and drop the extradatabase reset failed:prefix in sync's recovery path, which currently printsDatabase reset also failed: database reset failed: …where Go prints the error once. - Ctrl-C during sync's recovery reset becomes a fake error.
sync.handler.ts:397-398extracts only fail reasons from the exit cause, so an interrupt (or defect) surfaces asdatabase reset failed: unknown errorand the command continues writing debug bundles instead of propagating cancellation. Capture typed failures only; let interrupts/defects propagate (smart-target'sEffect.mapErroralready behaves correctly). - JSON
--output-formaterror codes changed. The tag renames (LegacyStartNetworkCreateError→LegacyNetworkCreateErroretc. incontainer-lifecycle.ts:65-90,LegacyStartDbSetupError→LegacyDbSetupErrorindb-setup.ts:148,LegacyDbResetNotRunningError→LegacyResetLocalDbNotRunningErrorinreset-local-database.ts:69) are surfaced verbatim as the JSONcodevianormalizeCliError. Keep the class renames but retain the original tag strings so machine consumers don't break. - Bucket seeding skips where Go seeds.
reset-local-database.ts:224'slegacySeedBucketsRunreloads config through the narrower.env/.env.localset, soenv(VAR)backed by.env.<SUPABASE_ENV>triggers the warn-and-skip path where Go (env alreadyos.Setenv'd) seeded successfully. The divergence is documented in-code, but the fully resolved config is already in scope — thread it through instead of tolerating the gap.
Code health
-
collectText(legacy-container-cli.ts:131) andrunContainerCliExpectSuccess(:174) are bare exports fromsrc/legacy/— rename with thelegacyprefix per the workspace rule. -
legacyBuildLocalDbContainerInputs(local-container-inputs.ts:97) is only called by the reset path;db startstill carries its byte-identical ~130-line inline copy (start.handler.ts:916-1125) while the module header claims both callers share it. Either finish the hoist or fix the header — as-is the two copies can silently drift. - PG14
DROP/CREATE DATABASEfailures (recreate-local-database.ts:300-306, same for the disconnect trio at:231-239) surface only the bare driver error. Go routes these throughExecBatch's formatter (At statement: <i>+ failing SQL,pkg/migration/file.go:124-146); the TS port of that formatter already exists inlegacy-migration-apply.tsand just isn't used here. - Dead test knob:
storageUnhealthy(reset.integration.test.ts:361) is declared and wired into the route but never used by any test — exercise it (storage-timeout-fails-reset at integration level) or delete it.
Docs
-
db/reset/SIDE_EFFECTS.md:67,103-104still documents the pre-fix barekong reload; the argv now includes--nginx-conf /home/kong/custom_nginx.templateand the flag is load-bearing (#6059). The env-var table is also missingSUPABASE_DB_MIGRATIONS_SCHEMA_PATHS, now genuinely effective on the experimental local reset. - The new modules cite
reset.goline numbers from before this PR's own 32-line Go deletion (e.g.recreate-local-database.tsheader'sreset.go:146-174,await-storage-ready.tsciting lines this PR removes) — all off by ~32 once merged; refresh the parity-oracle citations. -
start/SIDE_EFFECTS.md:59still sayslegacyStartVolumeExists(renamed tolegacyVolumeExistseverywhere else).
…ental-remote-schema-files-path-natively Resolves conflicts from develop's CLI-1955/CLI-2062 local-reset native port (legacyResetLocalDatabase, hoisted db-bootstrap primitives) by layering this branch's remote schema-files native port on top: the local target already went fully native on develop; this branch now makes the remote --experimental schema-files path native too, closing the last Go delegation on db reset. Also addresses avallete's review (PR #6062, review 4895183734): - LegacyDeclarativeApplyError now carries the wrapped failure's suggestion (e.g. a Kong-reload recovery hint) instead of dropping it; sync's recovery-reset path no longer double-prefixes the reset failure's message and now propagates a Ctrl-C/defect instead of synthesizing a fake "unknown error". - db reset's local bucket seeding now threads the already-resolved project config into legacySeedBucketsRun (resolvedConfig), closing the narrower-env-file-set gap that could wrongly warn-and-skip buckets Go would have seeded. - collectText/runContainerCliExpectSuccess renamed with the mandatory legacy prefix. - local-container-inputs.ts's header now accurately says only db reset consumes the hoisted prelude today (db start still has its own inline copy) instead of claiming both callers already share it. - PG14's DROP/CREATE DATABASE statements now get Go's ExecBatch error context (At statement: N + caret-marked SQL) via a newly exported legacyFormatExecBatchError, instead of a bare driver error. - Removed the dead storageUnhealthy test knob (the behaviour it would exercise is already covered precisely by await-storage-ready.unit.test.ts's fake-clock tests). - Refreshed ~20 stale Go reset.go line-number citations across recreate-local-database.ts/await-storage-ready.ts/restart-services.ts, and the legacyStartVolumeExists -> legacyVolumeExists rename in start/SIDE_EFFECTS.md. Not addressed (left as-is, with rationale): - The LegacyStartNetworkCreateError/LegacyStartDbSetupError/ LegacyDbResetNotRunningError tag renames predate this PR (already shipped in develop as CLI-1955, PR #6026) and are out of scope here. - Finishing db start's hoist onto legacyBuildLocalDbContainerInputs is tracked as a follow-up rather than folded into this db-reset change, given how much db-start-specific nuance sits in its current inline copy.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e55868cee
ℹ️ 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".
…ental-remote-schema-files-path-natively A second develop commit (CLI-1956, shadow database provisioning) landed concurrently with the first merge, reintroducing conflicts. Resolves them the same way: takes develop's newer/hoisted shared primitives (legacySqlFilesGlob, LegacyPgDeltaContext.projectEnv, schema_paths resolution in legacy-db-config.toml-read.ts) over this branch's now-superseded inline duplicates, and fixes the fallout: - reset.handler.ts's pgDeltaCtx now supplies the newly-required projectEnv field (toml.projectEnv). - shadow-database.ts's import updated to the legacyCollectText rename. - legacy-db-config.toml-read.ts's schemaPathPatterns (the new raw, pre-join form CLI-1956 added for db diff/db pull's shadow provisioning) now sourced from the same schemaPathsResolved.patterns this branch's own schema_paths resolution already computes. - Corrected local-container-inputs.ts's header: CLI-1956 concurrently finished the exact db-start hoist flagged as a follow-up in the first merge commit (db start/db diff/db pull all call legacyBuildLocalDbContainerInputs now), so that follow-up note no longer applies. All 7362 tests pass; types/lint/fmt/knip clean.
Contributor
Author
|
Thanks for the thorough review — went through every item below, accept/reject, all accepted items fixed directly in this PR (nothing deferred). Pushed as two follow-up merge commits ( Behavior / UX
Code health
Docs
Also re: "the autopilot closes threads it hasn't acted on" — noted, not something I can fix retroactively here, but flagging it back since you raised it: if that's still happening on future rounds it's worth someone looking at the autopilot's thread-resolution logic directly. All checks green: types/lint/fmt/knip clean, 7362 tests passing. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe3c34fe1b
ℹ️ 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".
Coly010
deleted the
columferry/cli-1958-port-db-reset-experimental-remote-schema-files-path-natively
branch
August 10, 2026 15:23
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.
What changed
Ports
db reset's--experimentalremote schema-files path from a Go-binary delegation to native TypeScript, removing the lastLegacyGoProxydependency fromdb reset's remote branch.Trigger (matches Go's
apply.MigrateAndSeedthree-conjunct gate exactly):--experimental/SUPABASE_EXPERIMENTALset, no explicit--version/--last, and[experimental.pgdelta].enabledunset. Body: globs[db.migrations].schema_pathsand execs each matched file with no history tracking (no version row, noRESET ALLbetween files) — reproducing two undocumented Go quirks byte-for-byte:schema_paths = []makes this a silent no-op (schemas/seeds still drop and reseed, nothing gets applied).New shared primitives:
legacyApplySchemaFiles(legacy-migration-apply.ts) and a hoistedlegacy-sql-files-glob.ts(replacing three separate hand-rolled copies of Go'sGlob.SQLFilestraversal that had existed acrossdb push's seed path, this new schema-files path, andstart/migration down's seed path — the third one had already silently diverged, missing directory-entry expansion; that's fixed too).This path connects directly (no shadow database involved) — confirmed via go-parity-auditor, addressing the issue's own note about overlapping with CLI-1956's shadow-provisioning work (a separate, still-in-progress issue in another PR): there turned out to be no actual dependency.
Known, deliberately-deferred gap (tracked separately)
legacyMigrateAndSeed(shared bymigration downand nativesupabase start's fresh-volume setup) does not yet implement this same schema-files branch, sosupabase start --experimentalon a fresh volume doesn't reproduce Go's behavior. This is pre-existing (not introduced here) and out of scope fordb reset— filed as CLI-2040 with the same go-parity-auditor findings, and the relevant docstring here now points at it instead of asserting (falsely) that the gap doesn't exist.Review notes
Reviewed independently by go-parity-auditor, engineer-reviewer, and architect-reviewer — all three converged on the same two follow-ups (now fixed): a stale "unreachable" docstring papering over the CLI-2040 gap, and an incomplete hoist that left a third, silently-diverging copy of the shared glob logic in
db push/start/migration down's seed path. Consolidating that hoist also surfaced and fixed a real latent bug in the shared glob's Windows-path handling (toSlashwas applying backslash-to-slash conversion unconditionally instead of Windows-only, which would have corrupted backslash-escaped glob patterns on non-Windows once rerouted). Added test coverage for schema-file application order, directory-entry expansion, and the newSUPABASE_DB_MIGRATIONS_SCHEMA_PATHSenv-override branches.Fixes CLI-1958