fix(cli): hedge config's 404 message, align push load errors - #6490
Merged
Coly010 merged 2 commits intoSep 7, 2026
Merged
Conversation
…s load-error text with diff/pull
A 404 on config push's GET /v2/projects/{ref}/config read could also mean
the v2 endpoint isn't served by the configured API host (older self-hosted
Management API, a proxy, a SUPABASE_PROFILE pointing elsewhere), not just a
wrong project ref — the shared message now names the resolved API host and
hedges accordingly for diff/pull/push alike.
Also hoists diff/pull's shared local-config-load helper (parse error naming
the actual file, duplicate-remote message, missing-file pointing at
`supabase init`) into config.load.ts and switches push to it, replacing its
own hardcoded `config.toml`-only messages.
avallete
approved these changes
Sep 7, 2026
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🤖 AI Review
Both independent reviews completed. The loader refactor appears functionally sound, but the PR leaves six confirmed quality issues: an unsanitized configurable API URL in terminal output, incomplete push coverage and side-effect documentation, an unconditional default-host hedge, stale error-class comments, and one inaccurate handler comment. The reported path-prefix bug is refuted because the loader cannot supply the sibling path needed to trigger it.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/commands/config/push/push.handler.ts:254 |
test-coverage |
claude | The push tests exercise malformed and missing configuration failures but do not verify the newly changed message behavior, including naming a malformed config.json and directing a missing-file user to supabase init. |
| 🟡 MINOR | apps/cli/src/commands/config/config.read-status.ts:41 |
output-sanitization |
claude+codex | The 404 message embeds the configurable API URL verbatim, allowing control characters in a custom profile URL to corrupt terminal output. |
| 🟡 MINOR | apps/cli/src/commands/config/push/SIDE_EFFECTS.md:19 |
documentation |
claude | The push side-effects document still describes a TOML-only configuration load and does not document the missing-config failure introduced by the shared loader. |
| 🟡 MINOR | apps/cli/src/commands/config/push/push.errors.ts:199 |
documentation |
claude | The read-status error comments still say a 404 unambiguously means a wrong project ref, contradicting the new endpoint-availability diagnostic. |
| ⚪ NIT | apps/cli/src/commands/config/config.read-status.ts:41 |
user-experience |
claude | The endpoint-availability hedge is emitted even for the standard production API host, where it distracts from the primary wrong-ref diagnosis. |
| ⚪ NIT | apps/cli/src/commands/config/push/push.handler.ts:250 |
documentation |
claude | The handler comment incorrectly says legacyLoadLocalConfig raises CliConfigParseError even though that helper catches and converts it. |
Findings outside the diff
- 🟡 MINOR
apps/cli/src/commands/config/push/SIDE_EFFECTS.md:19— The push side-effects document still describes a TOML-only configuration load and does not document the missing-config failure introduced by the shared loader. - 🟡 MINOR
apps/cli/src/commands/config/push/push.errors.ts:199— The read-status error comments still say a 404 unambiguously means a wrong project ref, contradicting the new endpoint-availability diagnostic.
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/commands/config/config.load.ts:10(correctness): The textual prefix check could misrender a sibling path whose name starts with workdir, and the helper lacks a direct unit test.
Refuted: The malformed sibling-path case cannot arise from the actual call graph. packages/config/src/paths.ts:24-44 constructs configuration paths beneath a searched root, and lines 70-96 search only the supplied directory and its ancestors. Descendant paths have the intended prefix; ancestor paths fail startsWith and remain unchanged. The helper's comment also limits its contract to loader-produced paths.
Stats
Claude findings: 7 · Codex findings: 1 · Confirmed: 6 · Refuted: 1 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
…rror text Addresses review feedback on PR #6490 (CLI-2321): - Sanitize apiHost through legacySanitizeInlineName before interpolating it into the 404 message, matching how ref is already handled — apiHost can trace back to a SUPABASE_PROFILE YAML file whose api_url isn't stripped of control characters. - Add push integration test coverage for the new load-error message shapes: a malformed config.json names config.json (not config.toml), and a missing config file asserts the full "Run `supabase init` to create one." message. - Fix a doc comment that misattributed CliConfigParseError to legacyLoadLocalConfig instead of the underlying loadCliConfig call it wraps.
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@3a4fa06398698337a48f2ce6c7e9637a9f12cd63Preview package for commit |
Coly010
deleted the
columferry/cli-2321-config-family-hedge-the-shared-404-read-status-message-and
branch
September 7, 2026 10:19
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
config push's 404 read-status message (shared withconfig diff/config pull, since all three readGET /v2/projects/{ref}/config) now names the resolved API host and hedges that this v2 endpoint may not be served there, instead of only suggesting a wrong project ref.config diff/config pull's shared local-config-load helper (parse-error file naming, duplicate-remote message, missing-file pointing atsupabase init) into a newconfig.load.ts, and switchedconfig pushto use it — replacing its own hardcodedconfig.toml-only messages.Why
Two DX follow-ups from the CLI-2313 review, deferred because each changes an established, test-pinned string: CLI-2321.
User-facing text changes
config push/config diff/config pull's 404 message changed fromProject <ref> not found. Check the project ref, or run \supabase projects list`...toCould not read configuration for project (404). Check the project ref with `supabase projects list`; if the ref is correct, this Supabase API endpoint may not be available at .`config push's local-config-load failure messages now matchconfig diff/config pull's established shape (correct file name on parse failure,Run \supabase init` to create one.` on a missing file).