From 6b4a2a4835022ac535d221e3bee33134ded7f472 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 17 Jul 2026 14:53:53 +0200 Subject: [PATCH 1/2] Report incompatible target flags as E_USAGE 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 --- .../localenv/flag-conflict-json/out.test.toml | 3 ++ .../localenv/flag-conflict-json/output.txt | 42 +++++++++++++++++++ acceptance/localenv/flag-conflict-json/script | 1 + .../localenv/flag-conflict-json/test.toml | 1 + acceptance/localenv/flag-conflict/output.txt | 8 +++- cmd/environments/sync.go | 14 +++---- libs/localenv/pipeline.go | 11 ++++- libs/localenv/pipeline_test.go | 19 +++++++++ libs/localenv/result.go | 1 + 9 files changed, 91 insertions(+), 9 deletions(-) create mode 100644 acceptance/localenv/flag-conflict-json/out.test.toml create mode 100644 acceptance/localenv/flag-conflict-json/output.txt create mode 100644 acceptance/localenv/flag-conflict-json/script create mode 100644 acceptance/localenv/flag-conflict-json/test.toml diff --git a/acceptance/localenv/flag-conflict-json/out.test.toml b/acceptance/localenv/flag-conflict-json/out.test.toml new file mode 100644 index 00000000000..d6187dcb046 --- /dev/null +++ b/acceptance/localenv/flag-conflict-json/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/acceptance/localenv/flag-conflict-json/output.txt b/acceptance/localenv/flag-conflict-json/output.txt new file mode 100644 index 00000000000..64378420c77 --- /dev/null +++ b/acceptance/localenv/flag-conflict-json/output.txt @@ -0,0 +1,42 @@ +{ + "schemaVersion": 1, + "command": "environments setup-local", + "ok": false, + "mode": "default", + "dryRun": false, + "greenfield": false, + "phases": [ + { + "phase": "preflight", + "status": "error" + }, + { + "phase": "resolve", + "status": "pending" + }, + { + "phase": "fetch", + "status": "pending" + }, + { + "phase": "merge", + "status": "pending" + }, + { + "phase": "provision", + "status": "pending" + }, + { + "phase": "validate", + "status": "pending" + } + ], + "warnings": [], + "error": { + "code": "E_USAGE", + "failurePhase": "preflight", + "message": "invalid compute target flags", + "diskMutated": false + }, + "durationMs": 0 +} diff --git a/acceptance/localenv/flag-conflict-json/script b/acceptance/localenv/flag-conflict-json/script new file mode 100644 index 00000000000..a41f74e8de6 --- /dev/null +++ b/acceptance/localenv/flag-conflict-json/script @@ -0,0 +1 @@ +musterr $CLI environments setup-local --cluster-id abc --serverless-version v4 --output json diff --git a/acceptance/localenv/flag-conflict-json/test.toml b/acceptance/localenv/flag-conflict-json/test.toml new file mode 100644 index 00000000000..c63fe3fe108 --- /dev/null +++ b/acceptance/localenv/flag-conflict-json/test.toml @@ -0,0 +1 @@ +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/acceptance/localenv/flag-conflict/output.txt b/acceptance/localenv/flag-conflict/output.txt index a6379fb10cd..85a4bd285e0 100644 --- a/acceptance/localenv/flag-conflict/output.txt +++ b/acceptance/localenv/flag-conflict/output.txt @@ -1 +1,7 @@ -Error: if any flags in the group [cluster-id cluster-name serverless-version job-id] are set none of the others can be; [cluster-id serverless-version] were all set +preflight error invalid compute target flags: flags --cluster-id and --serverless-version are mutually exclusive; specify at most one +resolve pending +fetch pending +merge pending +provision pending +validate pending +For more detail, re-run with --debug, or --output json to share a structured report. diff --git a/cmd/environments/sync.go b/cmd/environments/sync.go index cbe97da43d3..fa2d450bccb 100644 --- a/cmd/environments/sync.go +++ b/cmd/environments/sync.go @@ -56,7 +56,10 @@ func addTargetFlags(cmd *cobra.Command) { cmd.Flags().String("constraint-source-url", "", "URL for the constraint source (overrides "+envConstraintSource+")") // Hide constraint-source-url from casual --help output; it is a power-user escape hatch. _ = cmd.Flags().MarkHidden("constraint-source-url") - cmd.MarkFlagsMutuallyExclusive("cluster-id", "cluster-name", "serverless-version", "job-id") + // The mutual exclusivity of the target flags is enforced in the pipeline's + // preflight (as E_USAGE) rather than via cmd.MarkFlagsMutuallyExclusive, so + // the conflict is reported through the phase/JSON contract the --output json + // consumer relies on, instead of a bare pre-RunE Cobra error. } // runPipeline builds and runs the setup-local Pipeline. @@ -77,12 +80,9 @@ func runPipeline(cmd *cobra.Command) error { Serverless: serverless, Job: job, } - // ValidateTargetFlags is kept despite MarkFlagsMutuallyExclusive above: - // it also validates the library path (no Cobra equivalent) and guards - // non-Cobra call paths such as tests that invoke runPipeline directly. - if err := libslocalenv.ValidateTargetFlags(targetFlags); err != nil { - return err - } + // Flag validation (including mutual exclusivity) happens in the pipeline's + // preflight, so a conflict is reported as E_USAGE through the phase/JSON + // contract rather than as a bare error here. mode := libslocalenv.ModeDefault if constraintsOnly { diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index c6f6f547d32..1c0df251ba7 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -90,7 +90,16 @@ func (p *Pipeline) Run(ctx context.Context) (*Result, error) { // run drives the phases and returns the first phase error. Result bookkeeping // (phase status, error object) is handled by fail / markOK. func (p *Pipeline) run(ctx context.Context) error { - // Phase: preflight — manager detection, writability, package-manager availability. + // Phase: preflight — flag validation, manager detection, writability, + // package-manager availability. + // + // Incompatible target flags are a usage error (E_USAGE), reported at preflight + // before any other work so the failure flows through the phase/JSON reporting + // (a plain Cobra mutual-exclusion error would print no command JSON object, + // which the --output json consumer needs). + if err := ValidateTargetFlags(p.Flags); err != nil { + return p.fail(PhasePreflight, false, NewError(ErrUsage, err, "invalid compute target flags")) + } // P0 supports only uv; any other detected manager is a clean, non-blaming exit. if m := detectManager(p.ProjectDir); m != managerUv { return p.fail(PhasePreflight, false, NewError(ErrManagerUnsupported, nil, "%s", managerGuidance(m))) diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index 1823bcdcc90..5c55ecbca2e 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -78,6 +78,25 @@ func newTestServer(t *testing.T) *httptest.Server { })) } +func TestPipelineRejectsConflictingTargetFlagsAtPreflight(t *testing.T) { + // Incompatible target flags are a usage error surfaced as E_USAGE at + // preflight, before any manager/writability/fetch work. + dir := writeProject(t) + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, CacheDir: t.TempDir(), + Flags: TargetFlags{Cluster: "abc", Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, ErrUsage, pe.Code) + assert.Equal(t, PhasePreflight, pe.FailurePhase) + assert.False(t, pe.DiskMutated) + require.NotNil(t, res.Error) + assert.Equal(t, ErrUsage, res.Error.Code) +} + func TestPipelineCheckMutatesNothing(t *testing.T) { dir := writeProject(t) before, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) diff --git a/libs/localenv/result.go b/libs/localenv/result.go index f8d4919ad7c..9e0ef43e5db 100644 --- a/libs/localenv/result.go +++ b/libs/localenv/result.go @@ -64,6 +64,7 @@ const ( type ErrorCode string const ( + ErrUsage ErrorCode = "E_USAGE" ErrNoTarget ErrorCode = "E_NO_TARGET" ErrManagerUnsupported ErrorCode = "E_MANAGER_UNSUPPORTED" ErrUvMissing ErrorCode = "E_UV_MISSING" From 27b9d27291cde8f70184b0896a128e4c76356f8b Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 17 Jul 2026 15:44:07 +0200 Subject: [PATCH 2/2] Address review: include conflicting flag names in the E_USAGE JSON message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- acceptance/localenv/flag-conflict-json/output.txt | 2 +- libs/localenv/pipeline.go | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/acceptance/localenv/flag-conflict-json/output.txt b/acceptance/localenv/flag-conflict-json/output.txt index 64378420c77..84840872e3f 100644 --- a/acceptance/localenv/flag-conflict-json/output.txt +++ b/acceptance/localenv/flag-conflict-json/output.txt @@ -35,7 +35,7 @@ "error": { "code": "E_USAGE", "failurePhase": "preflight", - "message": "invalid compute target flags", + "message": "invalid compute target flags: flags --cluster-id and --serverless-version are mutually exclusive; specify at most one", "diskMutated": false }, "durationMs": 0 diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 1c0df251ba7..5f36c6caa75 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -98,7 +98,13 @@ func (p *Pipeline) run(ctx context.Context) error { // (a plain Cobra mutual-exclusion error would print no command JSON object, // which the --output json consumer needs). if err := ValidateTargetFlags(p.Flags); err != nil { - return p.fail(PhasePreflight, false, NewError(ErrUsage, err, "invalid compute target flags")) + // Put the validation detail in Msg (which is serialized) and do not wrap + // err: PipelineError.Error() appends a non-nil Err, which would duplicate + // the detail in text output, and E_USAGE — not the raw error — is the + // stable contract callers match on. Without this the --output json + // consumer would see only "invalid compute target flags" (Err is + // json:"-") and lose which flags conflict. + return p.fail(PhasePreflight, false, NewError(ErrUsage, nil, "invalid compute target flags: %s", err)) } // P0 supports only uv; any other detected manager is a clean, non-blaming exit. if m := detectManager(p.ProjectDir); m != managerUv {