fix(cli): default functions download --use-docker to true (Go parity)#5793
Open
Coly010 wants to merge 4 commits into
Open
fix(cli): default functions download --use-docker to true (Go parity)#5793Coly010 wants to merge 4 commits into
Coly010 wants to merge 4 commits into
Conversation
## Current Behavior
Go defaults `--use-docker` to `true` on `functions download`
(`cmd/functions.go:181`); the TS port omitted the default, so it
resolved to `false`. This flipped the default download path
(native HTTP vs. Docker-proxied) whenever no flags were passed, and
once fixed naively, the mutex check between `--use-api`/`--use-docker`/
`--legacy-bundle` (truthiness-based) would start misfiring on the new
default-true value, and `--use-api` alone would keep routing to Docker
instead of overriding it the way Go's `if useApi { useDocker = false }`
does.
## Expected Behavior
- `useDocker` now defaults to `true` via `Flag.withDefault(true)`,
matching Go.
- The mutex check (`validateDownloadFlags`) now scans raw argv for
explicit flag presence (`hasExplicitLongFlag`, hoisted out of
`deploy.ts` into the shared `functions.shared.ts` family root, since
it's now used by two commands) instead of checking parsed boolean
truthiness — mirroring Go's `pflag.Changed`-based
`MarkFlagsMutuallyExclusive`.
- `downloadFunctions` gates the Docker/legacy-bundle proxy branch on
`!explicitUseApi`, mirroring Go's `useApi` override.
- The Go-proxy arg builder (`makeGoProxyDownloadArgs`) now forwards at
most one of `--use-docker`/`--legacy-bundle` — previously it could
forward both once `useDocker` defaults true, which the Go binary's
own mutex check would then reject.
- Slug validation now runs unconditionally, before the proxy-dispatch
branch, so a malformed/traversal-shaped function name is rejected
before ever reaching the Go proxy's argv.
- `rawArgs` is threaded through `DownloadFunctionsDependencies` from
the `Stdio.Stdio` service in both the legacy and next shell handlers
(the shared `downloadFunctions` helper is used by both); `next`'s own
`--use-docker` default is intentionally left unchanged, this is a
legacy/Go-parity-only fix.
## Known follow-ups (not fixed here)
- Go's own server-side unbundle extractor (`saveFile` in
`apps/cli-go/internal/functions/download/download.go`) lacks the
path-containment checks the native TS downloader already has —
pre-existing Go CLI behavior, now reachable by default rather than
via opt-in flags. Tracked as CLI-1891.
- The Go-proxy path doesn't route through `execCapture` for
`--output-format json|stream-json`, so machine output can be
corrupted by inherited subprocess stdout (pre-existing, same as
`deploy`'s already-shipped Docker default).
- `hasExplicitLongFlag` detects flag token presence, not resolved
value, so `--use-docker=false` isn't treated as disabling Docker.
Pre-existing pattern shared with `deploy.ts`.
Fixes CLI-1862
Contributor
Author
|
@codex review |
![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: 03e7474878
ℹ️ 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".
Four issues Codex raised against the --use-docker default-to-true change,
each verified against apps/cli-go/ and fixed:
- Gate the Docker/legacy-bundle proxy branch on the parsed `flags.useApi`
value instead of raw-argv presence, matching Go's value-based
`if useApi { useDocker = false }` (cmd/functions.go:51-53) — an explicit
`--use-api=false` was being treated like `--use-api`, wrongly skipping
the proxy path Go would still take.
- Capture/discard the Go proxy's stdout via `execCapture` in machine-output
mode instead of inheriting it, and emit the `Output` envelope from
`downloadFunctions` itself, so `--output-format json|stream-json` no
longer loses its structured payload now that the proxy branch is the
default path (CLI-1546).
- Run the delegated Go call with `SUPABASE_TELEMETRY_DISABLED=1` (mirrors
`db pull`/`db diff`'s delegated-call pattern) so the child's own
`cli_command_executed` doesn't double-count on top of this command's own
telemetry now that the proxy branch is the default.
- Drop the `--legacy-bundle`-alone e2e case: it delegates to Go's
`RunLegacy`, which installs Deno before any network call, so every e2e
run would trigger a real, uncached download from GitHub. Equivalent
coverage already exists in download.integration.test.ts via a mocked
proxy.
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: 2e2496191e
ℹ️ 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".
Two more issues from Codex's re-review of 2e24961, both fixed; a third was investigated and found to be a false positive (documented in the reply rather than "fixed"): - List remote function slugs before delegating to the Go proxy in machine-output mode, not after. The Go child never reports which slugs it downloaded, so this list only exists to build the JSON payload; doing it first mirrors Go's own `downloadAll` (which also lists before looping) and means a transient listing failure is reported before any download side effect, instead of masking an already-successful delegated download with an unrelated post-hoc listing failure. - Drop the "--use-api alone" e2e case: now that the mutex fix routes --use-api to the native downloader correctly, this case no longer fails during argument validation — it proceeds to a real Management API call with a fake token/ref. Equivalent (better) coverage already exists in download.integration.test.ts via a mocked platform API. - Investigated the claimed "missing Stdio service" regression in hidden-flag.unit.test.ts: confirmed via the actual test run (and a temporary debug trace) that it passes and does not crash — `withEnv()`'s `BunServices.layer` already satisfies the `Stdio.Stdio` requirement. No code change; explained in the review reply instead.
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: 5df8d953f2
ℹ️ 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".
Codex re-review of 5df8d95 found a real bug: in the default Docker/proxy path with --output-format json|stream-json and no function name, an empty project would still delegate to the Go proxy (an unnecessary Docker/child round-trip) and then report success as "Downloaded Edge Function source." with an empty slug list — inconsistent with the native path just below, which correctly short-circuits empty projects as "No functions found." without delegating anywhere. Added the same short-circuit to the proxy branch, right after resolving the slug list and before calling proxyDownload, in both the legacy and next handlers.
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: 78e37acb06
ℹ️ 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".
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@78e37acb0610ccb299777c7b0b12b8a8a434eb91Preview package for commit |
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
1 participant
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.
Current Behavior
Go defaults
--use-dockertotrueonfunctions download(cmd/functions.go:181); the TS port omitted the default, so it resolved tofalse. This flipped the default download path (native HTTP vs. Docker-proxied) whenever no flags were passed, and once fixed naively, the mutex check between--use-api/--use-docker/--legacy-bundle(truthiness-based) would start misfiring on the new default-true value, and--use-apialone would keep routing to Docker instead of overriding it the way Go'sif useApi { useDocker = false }does.Expected Behavior
useDockernow defaults totrueviaFlag.withDefault(true), matching Go.validateDownloadFlags) now scans raw argv for explicit flag presence (hasExplicitLongFlag, hoisted out ofdeploy.tsinto the sharedfunctions.shared.tsfamily root, since it's now used by two commands) instead of checking parsed boolean truthiness — mirroring Go'spflag.Changed-basedMarkFlagsMutuallyExclusive.downloadFunctionsgates the Docker/legacy-bundle proxy branch on!flags.useApi, mirroring Go's value-basedif useApi { useDocker = false }override (cmd/functions.go:51-53) — not presence, so--use-api=falsestill proxies to Docker like an unpassed--use-apiwould.makeGoProxyDownloadArgs) now forwards at most one of--use-docker/--legacy-bundle— previously it could forward both onceuseDockerdefaults true, which the Go binary's own mutex check would then reject.SUPABASE_TELEMETRY_DISABLED=1(mirrorsdb pull/db diff's delegated-call pattern), and in--output-format json|stream-jsonits stdout is captured/discarded viaexecCaptureinstead of inherited, withdownloadFunctionsemitting theOutputenvelope itself — the Docker/legacy-bundle proxy branch is now safe as the default path for both telemetry accuracy and machine-output correctness (CLI-1546), not just opt-in flags.rawArgsis threaded throughDownloadFunctionsDependenciesfrom theStdio.Stdioservice in both the legacy and next shell handlers (the shareddownloadFunctionshelper is used by both);next's own--use-dockerdefault is intentionally left unchanged — this is a legacy/Go-parity-only fix.Known follow-ups (not fixed here)
saveFileinapps/cli-go/internal/functions/download/download.go) lacks the path-containment checks the native TS downloader already has — pre-existing Go CLI behavior, now reachable by default rather than via opt-in flags. Tracked as CLI-1891.hasExplicitLongFlagdetects flag token presence, not resolved value, so--use-docker=falseisn't treated as disabling Docker in the mutex check. Pre-existing pattern shared withdeploy.ts.Fixes CLI-1862