[VPEX][12] Report incompatible target flags as E_USAGE#5963
Conversation
Waiting for approvalCould not determine reviewers from git history. Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
rugpanov
changed the title
Integration test reportCommit: 27b9d27
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
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
179ea18 to
846170a
Compare
f20628a to
6b4a2a4
Compare
…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
left a comment
anton-107
left a comment
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).
3 participants
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.