fix(cli): match Go's functions serve docker-down error, kong reload args, and secret name casing - #5976
Conversation
…rgs, and secret name casing (CLI-1984)
|
@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: bfdda3a730
ℹ️ 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".
| // A down Docker daemon therefore surfaces from the DB inspect below | ||
| // (`assertLocalDbRunning`) as `failed to inspect service: …` with the | ||
| // Docker Desktop install hint as a suggestion (`misc.go:155-166`), never as | ||
| // an upfront `failed to run docker.` failure. |
There was a problem hiding this comment.
When neither docker nor podman is installed or available on PATH, runChildProcess fails during spawning, but assertLocalDbRunning catches that failure and replaces it with an empty stderr. Consequently, legacyIsDockerDaemonUnreachable("") is false and this newly removed precheck turns the previous actionable Docker prerequisite error into only failed to inspect service, without the install suggestion. Preserve the spawn failure or explicitly classify the no-runtime error so first-time users still receive the required guidance.
AGENTS.md reference: apps/cli/AGENTS.md:L246-L256
Useful? React with 👍 / 👎.
| Object.entries(edgeRuntime.secrets ?? {}).flatMap(([name, value]) => | ||
| Redacted.isRedacted(value) ? [[name.toUpperCase(), Redacted.value(value)] as const] : [], | ||
| Redacted.isRedacted(value) && Redacted.value(value).length > 0 | ||
| ? [[name, Redacted.value(value)] as const] |
There was a problem hiding this comment.
When a [edge_runtime.secrets] key contains lowercase characters, this now injects that key verbatim, but the Go config loader explicitly rewrites every key with strings.ToUpper in apps/cli-go/pkg/config/config.go:766-771 before set.ListSecrets reads it. A function that previously received MY_LOWER_SECRET will therefore receive only my_lower_secret in the legacy TS CLI and can fail at runtime; retaining name.toUpperCase() is required for Go parity.
AGENTS.md reference: apps/cli/AGENTS.md:L246-L256
Useful? React with 👍 / 👎.
1 participant
What changed
Three Go-parity fixes for
functions serveinapps/cli/src/shared/functions/serve.ts, plus one minor watcher parity item and review follow-ups.1. Docker-down error and ordering — removed the TS-only upfront docker precheck (
failed to run docker. Docker Desktop is a prerequisite …). Go'srestartEdgeRuntimehas no such precheck (internal/functions/serve/serve.go:107-113): it loads config first, then asserts the local DB container, so a down Docker daemon surfaces asfailed to inspect service: <connection error>with the Docker Desktop install hint on its own stderr line (internal/utils/misc.go:155-166CmdSuggestion, rendered bycmd/root.go:300-303).assertLocalDbRunningnow attaches that hint as an errorsuggestionwhen the daemon is unreachable, and the daemon-unreachable matcher (legacy-docker-suggest.ts) additionally recognizes socket permission-denied errors, which the pinned Docker SDK (v28.5.2,client/request.go:144-152) also classifies as connection failures. Ordering verified against Go: everything Go validates during config load before its DB assert (signing keys,jwt_secret,deno_version—pkg/config/config.go:1110-1172) already runs before the TS DB assert too.2. Kong reload args —
reloadKongnow runs barekong reload, matching Go (serve.go:129). The--nginx-conf /home/kong/custom_nginx.templateargument belongs tostart's Kong bring-up entrypoint (start.go:589-592), which persists the prefix configurationkong reloadreuses — it was never part of Go's serve path, TLS-enabled stacks included.3.
[edge_runtime.secrets]name casing + filtering —toPlainEdgeRuntimeConfignow injects secret names verbatim (previously uppercased:my_secretreached the container asMY_SECRET) and only entries with a resolved non-empty value, mirroring Go'sset.ListSecretsSHA256 > 0gate (internal/secrets/set/set.go:48-52,pkg/config/secret.go:94-107): empty values and unresolvedenv(VAR)literals are skipped.Both shells / shared consumers
shared/functions/serve.tsis shared infrastructure. Fix 3's helper is also used by legacystart's edge-runtime bring-up — the verbatim/filtered semantics are Go-correct there too (Gostartreaches the identicalListSecretspath viaServeFunctions,start.go:1104), and astartintegration assertion now pins it. Fixes 1–2 arefunctions serve-only code paths; other commands' docker-down messages (legacy-docker-run.layer.tsetc.) are untouched. The widened daemon-unreachable matcher benefits all its consumers (serve, db bootstrap seam, image resolve/prepull) in the Go-correct direction. Thenextshell does not consume these paths (functions devruns via@supabase/stack).Minor watcher items (from the issue)
File change detected: <path> (WRITE|CREATE|REMOVE)— instead of always(update); the nodefs.watchlayer classifiesrenameevents via an existence check.RENAME/CHMOD/OR-combined ops are unreachable under node's watcher (documented in-code).SUPABASE_FUNCTIONS_WATCH_LIMITcap/hint (watcher.go:114-142) — TS uses a single recursive watch, not per-directory fsnotify registration, so the cap doesn't fall out naturally; and the debounce is still trailing-500 ms vs Go's fire-first (watcher.go:102-104) — a semantics change judged too risky to bundle here.Known residuals (documented, deliberately not fixed here)
encrypted:secret values (pre-existingpackages/configgap;startdecrypts explicitly, Go decrypts during config load).supabase start is not running.lacks Go's Aqua color on "supabase start" (pre-existing).fs.watchrename→create/delete classification has no direct test (real-watcher tests are flaky); the printed-token mapping is covered via the mocked watcher.Fixes CLI-1984
https://linear.app/supabase/issue/CLI-1984/functions-serve-parity-docker-down-error-kong-reload-args-edge