[VPEX][12] Report incompatible target flags as E_USAGE#5963
Merged
Conversation
This was referenced Jul 17, 2026
rugpanov
changed the title
Collaborator
Integration test reportCommit: d5c4d6f
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 1 slowest tests (at least 2 minutes):
|
rugpanov
force-pushed
the
dbconnect/12-serverless-v5
branch
from
July 17, 2026 13:27
179ea18 to
846170a
Compare
rugpanov
force-pushed
the
dbconnect/13-usage-error
branch
from
July 17, 2026 13:27
f20628a to
6b4a2a4
Compare
rugpanov
added a commit
that referenced
this pull request
Jul 17, 2026
…ssage codex P2 on #5963: with --output json, error.message came from PipelineError.Msg only ('invalid compute target flags'), while the wrapped validation error is json:"-" — so JSON consumers lost which flags conflicted. Fold the validation detail into Msg (and do not wrap err, since PipelineError.Error() appends a non-nil Err and would duplicate the detail in text output; E_USAGE is the stable contract, not the raw error). Both text and JSON now carry the full actionable message. Co-authored-by: Isaac
anton-107
reviewed
Jul 17, 2026
anton-107
left a comment
anton-107
left a comment
Contributor
There was a problem hiding this comment.
Reviewed as part of the full stack (#5960–#5965). The intent here is right — routing the mutual-exclusion conflict through the phase/JSON contract so --output json emits a structured error instead of a bare pre-RunE Cobra error. Removing the runPipeline early return is safe: ValidateTargetFlags still runs at pipeline preflight (pipeline.go:100), with ResolveTarget (target.go:73) as a defensive second check, so no call path goes unvalidated. Leaving this as a comment (not a block), but two gaps undercut the stated rationale and are worth addressing before this is relied on:
-
[high] The JSON error object drops the actionable detail.
PipelineError.Errisjson:"-"(libs/localenv/result.go:98), so the emitted object is just{"code":"E_USAGE","failurePhase":"preflight","message":"invalid compute target flags"}— it never says which flags conflict. The text path shows the full…: flags --cluster-id and --serverless-version are mutually exclusive; specify at most one(compare goldensflag-conflict/output.txtvsflag-conflict-json/output.txt). So the--output jsonconsumer this PR is written to serve gets strictly less information than the terminal user. This is not E_USAGE-specific: the same loss makes E_RESOLVE's ambiguous-vs-unknown cluster-name cases marshal to byte-identical JSON, and drops the job-ambiguity detail too. If the JSON contract matters, folding the cause intoMsg(or serializingError()) is the fix. -
[medium] E_USAGE is unreachable in JSON mode when auth isn't set up.
setup_local.go:40setsPreRunE = root.MustWorkspaceClient, which Cobra runs beforeRunE(and thus before the preflight where E_USAGE is emitted). A flag conflict needs zero workspace access, yet with no/invalid auth the user gets a plain non-JSON auth error, never the promised{"code":"E_USAGE"}object — precisely in the "haven't logged in yet" case. -
[low] JSON contract untested for the lossy codes. The only
--output jsongoldens cover E_USAGE and E_NO_TARGET — the two codes whose whole message lives inMsg(nothing to lose). No JSON golden exercises E_RESOLVE / E_FETCH / E_ENV_UNSUPPORTED, which are exactly the paths where the message-loss above bites, so a regression there would pass CI. -
[low] Shell completion no longer signals mutual exclusivity. Dropping
MarkFlagsMutuallyExclusivemeans completion no longer hides the sibling target flags after one is set (verified against a built binary). Acceptable given the command is hidden/experimental — flagging for awareness, not as a blocker.
Reviewed with AI assistance (build + unit tests + adversarial verification against the checked-out top of stack).
rugpanov
added a commit
that referenced
this pull request
Jul 20, 2026
Follow-up to #5963 review (anton-107 [high]): PipelineError.Err is json:"-", so the --json error object carried only Msg and dropped the wrapped cause — a JSON consumer got strictly less than the text output. This bit every code that wraps a cause (E_RESOLVE ambiguous-vs-unknown cluster name, E_FETCH, E_ENV_UNSUPPORTED, the job-ambiguity detail), not just E_USAGE. Fix at the serialization layer: PipelineError.MarshalJSON now folds Error() (Msg + cause) into the "message" field, so text and JSON always agree. Revert the E_USAGE-specific message fold (no longer needed) back to wrapping err, which keeps errors.Is/As chaining intact. Add cluster-name-ambiguous-json to lock in the E_RESOLVE JSON detail — previously only E_USAGE and E_NO_TARGET (whose whole message is in Msg) had JSON goldens, so the loss was untested where it bit. Co-authored-by: Isaac
Contributor
Author
|
Addressed. Two substantive gaps, both fixed: [high] JSON dropped the actionable detail — fixed at the serialization layer rather than per-code: [low] JSON contract untested for lossy codes — added [medium] E_USAGE unreachable in JSON when auth unset (PreRunE MustWorkspaceClient runs before the pipeline) — real, but the fix is the same command-bootstrap rewrite as the deferred "serverless doesn't need auth" item; tracking it separately rather than reworking auth wiring in this PR. [low] completion no longer signals mutual exclusivity — acknowledged; leaving as-is per your note (hidden/experimental command). Fixes are on the branch ( |
rugpanov
force-pushed
the
dbconnect/12-serverless-v5
branch
from
July 20, 2026 11:14
846170a to
9d8fc59
Compare
rugpanov
added a commit
that referenced
this pull request
Jul 20, 2026
…ssage codex P2 on #5963: with --output json, error.message came from PipelineError.Msg only ('invalid compute target flags'), while the wrapped validation error is json:"-" — so JSON consumers lost which flags conflicted. Fold the validation detail into Msg (and do not wrap err, since PipelineError.Error() appends a non-nil Err and would duplicate the detail in text output; E_USAGE is the stable contract, not the raw error). Both text and JSON now carry the full actionable message. Co-authored-by: Isaac
rugpanov
added a commit
that referenced
this pull request
Jul 20, 2026
Follow-up to #5963 review (anton-107 [high]): PipelineError.Err is json:"-", so the --json error object carried only Msg and dropped the wrapped cause — a JSON consumer got strictly less than the text output. This bit every code that wraps a cause (E_RESOLVE ambiguous-vs-unknown cluster name, E_FETCH, E_ENV_UNSUPPORTED, the job-ambiguity detail), not just E_USAGE. Fix at the serialization layer: PipelineError.MarshalJSON now folds Error() (Msg + cause) into the "message" field, so text and JSON always agree. Revert the E_USAGE-specific message fold (no longer needed) back to wrapping err, which keeps errors.Is/As chaining intact. Add cluster-name-ambiguous-json to lock in the E_RESOLVE JSON detail — previously only E_USAGE and E_NO_TARGET (whose whole message is in Msg) had JSON goldens, so the loss was untested where it bit. Co-authored-by: Isaac
rugpanov
force-pushed
the
dbconnect/13-usage-error
branch
from
July 20, 2026 12:09
daaa475 to
ee1f606
Compare
rugpanov
enabled auto-merge
July 20, 2026 14:03
Per the [P0] CLI Changes spec, a bad-flags usage error is now surfaced as
E_USAGE at the preflight phase, through the phase/JSON contract, instead
of a bare Cobra mutual-exclusion error printed before RunE.
- Add the E_USAGE error code.
- Validate target flags at the top of the pipeline's preflight; a conflict
fails preflight with E_USAGE (diskMutated=false).
- Drop cmd.MarkFlagsMutuallyExclusive and the redundant early return in
runPipeline so the conflict flows into the pipeline and --output json
emits the structured error{code:"E_USAGE", failurePhase:"preflight"}
object the VS Code extension branches on.
- flag-conflict golden updated; new flag-conflict-json asserts the JSON
error object; pipeline unit test for the E_USAGE path.
Co-authored-by: Isaac
…ssage codex P2 on #5963: with --output json, error.message came from PipelineError.Msg only ('invalid compute target flags'), while the wrapped validation error is json:"-" — so JSON consumers lost which flags conflicted. Fold the validation detail into Msg (and do not wrap err, since PipelineError.Error() appends a non-nil Err and would duplicate the detail in text output; E_USAGE is the stable contract, not the raw error). Both text and JSON now carry the full actionable message. Co-authored-by: Isaac
Follow-up to #5963 review (anton-107 [high]): PipelineError.Err is json:"-", so the --json error object carried only Msg and dropped the wrapped cause — a JSON consumer got strictly less than the text output. This bit every code that wraps a cause (E_RESOLVE ambiguous-vs-unknown cluster name, E_FETCH, E_ENV_UNSUPPORTED, the job-ambiguity detail), not just E_USAGE. Fix at the serialization layer: PipelineError.MarshalJSON now folds Error() (Msg + cause) into the "message" field, so text and JSON always agree. Revert the E_USAGE-specific message fold (no longer needed) back to wrapping err, which keeps errors.Is/As chaining intact. Add cluster-name-ambiguous-json to lock in the E_RESOLVE JSON detail — previously only E_USAGE and E_NO_TARGET (whose whole message is in Msg) had JSON goldens, so the loss was untested where it bit. Co-authored-by: Isaac
rugpanov
force-pushed
the
dbconnect/13-usage-error
branch
from
July 20, 2026 15:51
c0e7dea to
d5c4d6f
Compare
Collaborator
Integration test reportCommit: 34d85ce
25 interesting tests: 12 flaky, 6 FAIL, 4 RECOVERED, 2 SKIP, 1 KNOWN
Top 50 slowest tests (at least 2 minutes):
|
yansonggao-db
pushed a commit
to yansonggao-db/cli
that referenced
this pull request
Jul 21, 2026
…icks#5965) Stacked on databricks#5964 → databricks#5963 → databricks#5962 → databricks#5961 → databricks#5960 → databricks#5959. ## What `--serverless-version` is now documented to take a **bare number** (e.g. `5`) rather than `v5`. - Input `5` is the documented form; `v5`/`V5` are still accepted (tolerant). - Both map to the same env key `serverless/serverless-vN`, so the **environments-repo layout is unchanged** (still `serverless/serverless-v5/…`). ## Changes - Flag help: `e.g. v4` → `e.g. 5`; the `E_ENV_UNSUPPORTED` hint suggests `--serverless-version 5`. - `defaultServerlessVersion` stored in bare form (`"5"`); still resolves to `serverless-v5` via `NormalizeServerless`. - Acceptance scripts pass the bare form; env-key goldens unchanged (`serverless-vN`). - Unit tests cover bare + v-prefixed input and the default constant. ## Testing `go build ./...`, lint (0 issues), deadcode clean, unit + acceptance green. This pull request and its description were written by Isaac.
tanishgupta-db
pushed a commit
to tanishgupta-db/cli
that referenced
this pull request
Jul 21, 2026
## Why PRs touching `libs/localenv/`, `cmd/environments/`, and `acceptance/localenv/` currently fall through to the `*` rule in `.github/OWNERS` and therefore require a global maintainer's approval, even though these areas have dedicated owners. This adds review-turnaround friction on DB Connect / local-environment work (e.g. databricks#5963). ## What Adds an OWNERS section granting per-path ownership of those three directories to `@rugpanov`, `@rclarey`, `@anton-107`, and `@misha-db`, so any of them can satisfy the `maintainer-approval` gate for changes in those areas — matching the existing per-path pattern used for Labs and Pipelines. ## Verification - `node .github/scripts/owners.js validate` passes (all three paths exist in the tree; each rule resolves to 4 owners; only the pre-existing `team:ai-training` warning remains). - `node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js` passes (64/64). > Note: because this edits `.github/OWNERS` (a `*`-owned file), this PR itself still requires one existing global maintainer's approval to merge. This pull request and its description were written by Isaac.
tanishgupta-db
pushed a commit
to tanishgupta-db/cli
that referenced
this pull request
Jul 21, 2026
…ricks#5964) Stacked on databricks#5963 (E_USAGE) → databricks#5962 → databricks#5961 → databricks#5960 → databricks#5959. ## What Reconciles the error codes with the `[P0] CLI Changes` spec table (spec item databricks#10). **No behavior change** — the code already emits the correct set. The spec's table was stale: - omitted `E_NOT_WRITABLE` (preflight) and `E_PYTHON_INSTALL` (provision), and - wrongly listed `E_UV_MISSING` as "reserved / not emitted" when the CLI does emit it at preflight. ## Changes - Annotate each `ErrorCode` constant with the phase that emits it, and document the two spec codes the CLI deliberately never emits: `E_PYTHON_POLICY` (no signal source yet) and `E_AUTH` (handled earlier by `MustWorkspaceClient`). This makes the constant block the authoritative reference the spec mirrors. - Fix a `TargetInfo` doc comment that still said "four precedence sources" — with `--cluster-name` there are now five flag sources (the JSON `source` value is still one of cluster/serverless/job/bundle). ## Note The spec doc's error-code table itself (in the VPEX ERD, a Google Doc) has been updated separately to match — that's outside this repo. ## Testing `go build ./...`, lint (0 issues), deadcode clean, unit + acceptance green (comment-only code change). This pull request and its description were written by Isaac.
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
4 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.
Stacked on #5962 (serverless-v5) → #5961 → #5960 → #5959.
What
Per the
[P0] CLI Changesspec, a bad-flags usage error is now surfaced asE_USAGEat the preflight phase, through the phase/JSON contract — instead of a bare Cobra mutual-exclusion error printed beforeRunE(which produces no command JSON object).Why
The VS Code extension branches on the JSON
error.code. Previously, passing two target flags triggered Cobra'sMarkFlagsMutuallyExclusivebeforeRunE, so--output jsonemitted nothing structured. Now the conflict is caught inside the pipeline and reported like any other phase failure.Changes
E_USAGEerror code.E_USAGE,diskMutated=false.cmd.MarkFlagsMutuallyExclusiveand the redundant early-return inrunPipelineso the conflict flows into the pipeline.flag-conflictgolden updated (now shows the phase table + preflight error); newflag-conflict-jsonassertserror{code:"E_USAGE", failurePhase:"preflight"}; pipeline unit test for the path.Testing
go build ./..., lint (0 issues), deadcode clean, unit + acceptance green.This pull request and its description were written by Isaac.