diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 52cb48e3..c28c0dd2 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -153,6 +153,12 @@ jobs: go install golang.org/x/tools/cmd/deadcode@v0.48.0 deadcode ./cmd/tracebloc + - name: File budget (line-count ratchet) + # Per-file line ceilings; they only ratchet DOWN — raising one is a + # deliberate, reviewed edit to scripts/file-budget.sh. Keeps the next + # 1500-line data.go from growing quietly (backend#1106 WS-B). + run: ./scripts/file-budget.sh + govulncheck: timeout-minutes: 10 name: govulncheck diff --git a/Makefile b/Makefile index 5e948b1c..a6080ea8 100644 --- a/Makefile +++ b/Makefile @@ -23,7 +23,7 @@ GOVULNCHECK_VERSION ?= v1.1.4 # ---- top-level targets ------------------------------------------- .PHONY: ci -ci: vet test lint fmt-check schema-check vulncheck deadcode +ci: vet test lint fmt-check schema-check vulncheck file-budget deadcode @echo "==> ci: all green" .PHONY: build @@ -155,6 +155,14 @@ schema-check: schema-sync: ./scripts/sync-schema.sh +# file-budget: per-file line ceilings (ratchet down only — raising one is a +# reviewed edit to scripts/file-budget.sh). Keeps the next 1500-line data.go +# from growing quietly (backend#1106 WS-B). Also enforced by build.yml's +# lint job, keeping the "make ci green => CI green" invariant. +.PHONY: file-budget +file-budget: + ./scripts/file-budget.sh + # ---- cleanup ----------------------------------------------------- .PHONY: clean diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 272a7ff9..f9f38703 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -104,6 +104,33 @@ If you have a `~/.bash_profile` that doesn't source `~/.profile` or `~/.bashrc`, the PATH entry may be skipped. Either add the `export PATH=…` line to the file your login shell actually reads, or use the `/usr/local/bin` approach above. +## Exit codes + +Every command exits `0` on success. Non-zero codes are a scripting contract — +they're stable, and each command's `--help` documents the subset it can +produce (`tracebloc data ingest --help` has the fullest list). This is the +cross-command view; the names in the last column are the constants in +`internal/cli/exitcodes.go`, so grepping a name finds every site that +produces that code. + +| Code | Meaning | Produced by | Constant | +|------|---------|-------------|----------| +| `0` | Success — includes `--dry-run` completing, a guided run you cancelled cleanly, and `doctor` passing with warnings only | all commands | `exitOK` | +| `1` | Generic failure with no more specific bucket (also any error without an explicit code) | `login`, `client …`, `delete`, mistyped commands | `exitFailure` | +| `2` | Your input didn't validate: schema validation failed (spec synthesized from flags, or your YAML), an unsupported/unknown `--task`, a task-scoped flag applied to the wrong task, an invalid dataset name, or a resource size that doesn't fit the machine | `data ingest`, `data validate`, `data delete`, `resources set` | `exitBadInput` | +| `2` | One or more checks failed | `doctor` | `exitChecksFailed` | +| `3` | Local environment problem: kubeconfig couldn't be loaded, the dataset path is missing or unreadable, the local layout is wrong, a YAML file didn't parse, or a prompt was needed but the run is non-interactive (`--no-input` / `--output-json` / no TTY) | `data ingest`, `data validate`, `data list`, `data delete`, `doctor`, `resources`, `resources set` | `exitLocalEnv` | +| `4` | Cluster reachable but no tracebloc client found in the namespace — or its shared storage / dataset list is missing, so the target can't be confirmed | `data ingest`, `data list`, `data delete`, `cluster info`, `resources`, `resources set` | `exitNoWorkspace` | +| `5` | Auth: the ingestor SA token couldn't be obtained, or jobs-manager rejected it (401/403) | `data ingest`, `cluster info` | `exitAuth` | +| `5` | No dataset by that name on this client (nothing to delete) | `data delete` | `exitNoSuchDataset` | +| `6` | Destination table already exists — re-run with `--overwrite` to replace it, or pick a different `--name` | `data ingest` | `exitTableExists` | +| `7` | Pre-flight succeeded but staging the files failed (Pod creation, image pull, exec stream, or remote tar error) | `data ingest` | `exitStagingFailed` | +| `7` | Removing an existing table + its files failed partway (see the error for the recovery command) | `data delete`, `data ingest --overwrite` | `exitTeardownFailed` | +| `7` | The cluster couldn't be queried for its datasets | `data list` | `exitQueryFailed` | +| `8` | jobs-manager rejected the submitted run (a non-auth 4xx/5xx), or the port-forward to it couldn't be set up | `data ingest` | `exitSubmitFailed` | +| `9` | The ingestion Job exited non-zero, completed with row-level failures the summary panel reports, or its outcome couldn't be determined / followed | `data ingest` | `exitIngestFailed` | +| `130` | You hit Ctrl-C at an interactive prompt (128+SIGINT) | interactive prompts | `exitInterrupted` | + ## Still stuck? Open an issue at [github.com/tracebloc/cli/issues](https://github.com/tracebloc/cli/issues) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index c8c65cd3..6982161c 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -53,7 +53,7 @@ var ( func runLogin(ctx context.Context, p *ui.Printer, envFlag string) error { cfg, err := config.Load() if err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } env := api.ResolveEnv(envFlag) // login PICKS the session env and persists it (cfg.CurrentEnv below), so a @@ -61,7 +61,7 @@ func runLogin(ctx context.Context, p *ui.Printer, envFlag string) error { // unknown→prod fallback would otherwise route `--env staging` / `CLIENT_ENV=prd` // to production and store it as the active env for every later command. if !api.IsKnownEnv(env) { - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "unknown backend environment %q — valid values are dev, stg, prod (default). "+ "Check --env / $CLIENT_ENV", env)} } @@ -72,11 +72,11 @@ func runLogin(ctx context.Context, p *ui.Printer, envFlag string) error { if err != nil { var ae *api.APIError if errors.As(err, &ae) && ae.StatusCode == http.StatusNotFound { - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "this backend (%s) doesn't support browser login yet — the device-grant "+ "endpoints land in backend#835: %w", env, err)} } - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } p.Section("Sign in to tracebloc") @@ -117,7 +117,7 @@ func runLogin(ctx context.Context, p *ui.Printer, envFlag string) error { prof.FirstName = id.FirstName } if err := cfg.Save(); err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } if prof.Email != "" { p.Successf("Signed in as %s.", prof.Email) @@ -149,11 +149,11 @@ func pollForToken(ctx context.Context, p *ui.Printer, client *api.Client, dc *ap for { if !deadline.IsZero() && time.Now().After(deadline) { - return "", &exitError{code: 1, err: errors.New("login timed out — re-run `tracebloc login`")} + return "", &exitError{code: exitFailure, err: errors.New("login timed out — re-run `tracebloc login`")} } select { case <-ctx.Done(): - return "", &exitError{code: 130} // Ctrl-C: exit quietly (no "Error: context canceled") + return "", &exitError{code: exitInterrupted} // Ctrl-C: exit quietly (no "Error: context canceled") case <-pollAfter(time.Duration(interval) * time.Second): } @@ -168,11 +168,11 @@ func pollForToken(ctx context.Context, p *ui.Printer, client *api.Client, dc *ap // interval by 5 seconds for this and all subsequent polls. interval += 5 case errors.Is(err, api.ErrExpiredToken): - return "", &exitError{code: 1, err: errors.New("the sign-in code expired — re-run `tracebloc login`")} + return "", &exitError{code: exitFailure, err: errors.New("the sign-in code expired — re-run `tracebloc login`")} case errors.Is(err, api.ErrAccessDenied): - return "", &exitError{code: 1, err: errors.New("sign-in was denied in the browser")} + return "", &exitError{code: exitFailure, err: errors.New("sign-in was denied in the browser")} default: - return "", &exitError{code: 1, err: err} + return "", &exitError{code: exitFailure, err: err} } } } @@ -188,7 +188,7 @@ func newLogoutCmd() *cobra.Command { p := printerFor(cmd) cfg, err := config.Load() if err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } if !cfg.SignedIn() { p.Hintf("Already signed out.") @@ -212,7 +212,7 @@ func newLogoutCmd() *cobra.Command { // would bleed into the next sign-in on this env. *prof = config.Profile{} if err := cfg.Save(); err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } // Then revoke the token server-side so a copied/leaked credential stops @@ -259,7 +259,7 @@ func newAuthStatusCmd() *cobra.Command { } cfg, err := config.Load() if err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } p := printerFor(cmd) if !cfg.SignedIn() { @@ -309,7 +309,7 @@ func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { if p.Verbose() { p.Hintf("Not signed in. Run `tracebloc login`.") } - return &exitError{code: 1} + return &exitError{code: exitFailure} } target := api.ResolveEnv(envFlag) if !cfg.SignedIn() || cfg.CurrentEnv != target { @@ -320,7 +320,7 @@ func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { p.Hintf("Not signed in. Run `tracebloc login`.") } } - return &exitError{code: 1} + return &exitError{code: exitFailure} } // Signed in AND CurrentEnv == target: probe it. authedClient() builds the client // for sessionEnv (== CurrentEnv == target) with the stored token — reuse it and @@ -330,7 +330,7 @@ func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { if p.Verbose() { p.Hintf("Not signed in. Run `tracebloc login`.") } - return &exitError{code: 1} + return &exitError{code: exitFailure} } if _, err := client.WhoAmI(ctx); err != nil { // A 426 means the CLI is too old, not that the session is invalid — surface @@ -338,7 +338,7 @@ func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { // instead of the "re-login" advice, which wouldn't help. var ue *api.UpgradeRequiredError if errors.As(err, &ue) { - return &exitError{code: 1, err: ue} + return &exitError{code: exitFailure, err: ue} } if p.Verbose() { // Only a 401/403 is genuinely a rejected token (where re-login helps); a @@ -351,7 +351,7 @@ func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { p.Hintf("Couldn't verify your session with the backend (%v).", err) } } - return &exitError{code: 1} + return &exitError{code: exitFailure} } if p.Verbose() { if email := cfg.Current().Email; email != "" { diff --git a/internal/cli/client.go b/internal/cli/client.go index 494774bc..8a449194 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -181,7 +181,7 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien client, cfg, err := authedClient() if err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } ilog.Logf("authenticated; provisioning against the signed-in account") @@ -233,9 +233,9 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien // upgrade signal verbatim instead of framing it as a transient outage. var ue *api.UpgradeRequiredError if errors.As(listErr, &ue) { - return &exitError{code: 1, err: ue} + return &exitError{code: exitFailure, err: ue} } - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "couldn't reach the backend to choose a unique client name (%v) — retry, "+ "or pass --name explicitly", listErr)} } @@ -305,7 +305,7 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien } namespace, derr := slug.Derive(name, existing, "client-"+randHex(4)) if derr != nil { - return &exitError{code: 1, err: derr} + return &exitError{code: exitFailure, err: derr} } if pr != nil && !opts.yes { @@ -333,12 +333,12 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien // (would leak a credential) from an idempotent adopt (safe). Fail closed, // but name the real cause: a retry once the backend is reachable will // adopt an existing client without any flag. - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "couldn't read the account's client list to tell whether this cluster is new "+ "or already registered (%v) — retry when the backend is reachable (a re-run "+ "adopts an existing client), or pass --yes/--credential-file to provision now", listErr)} } - return &exitError{code: 1, err: errors.New( + return &exitError{code: exitFailure, err: errors.New( "refusing to provision non-interactively without confirmation — pass --yes to " + "confirm, and --credential-file to write the credential to a file instead of stdout")} } @@ -366,10 +366,10 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien // A 409 on POST /edge-device/ is a cross-account cluster_conflict // (R6) or a same-account cluster_in_use; conflictMessage picks the // right guidance (and names the owner when the backend supplies it). - return &exitError{code: 1, err: errors.New(conflictMessage(ae))} + return &exitError{code: exitFailure, err: errors.New(conflictMessage(ae))} } } - return &exitError{code: 1, err: cerr} + return &exitError{code: exitFailure, err: cerr} } } @@ -399,7 +399,7 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien "TB_NAMESPACE=" + pc.Namespace, "TRACEBLOC_CLIENT_ADOPTED=1", }); werr != nil { - return &exitError{code: 1, err: werr} + return &exitError{code: exitFailure, err: werr} } p.Hintf("Wrote client id + namespace to %s (no new credential — the existing one stands).", opts.credentialFile) } @@ -424,7 +424,7 @@ func runClientCreate(ctx context.Context, p *ui.Printer, pr prompter, opts clien }); werr != nil { // The credential is the only copy — a write failure must be fatal, not a // silent drop (the installer would have nothing to connect with). - return &exitError{code: 1, err: werr} + return &exitError{code: exitFailure, err: werr} } p.Hintf("Credential written to %s (mode 0600, not shown). This machine is set to enroll as client %d.", opts.credentialFile, pc.ID) } else { @@ -523,7 +523,7 @@ func adoptLiveInClusterClient( // create — that mint stamps no anchor, so it can't orphan one. if clusterID != "" { ilog.Logf("in-cluster client discovery failed on a reachable cluster (failing closed): %v", err) - return nil, false, &exitError{code: 1, err: fmt.Errorf( + return nil, false, &exitError{code: exitFailure, err: fmt.Errorf( "couldn't check whether a tracebloc client is already running on this cluster (%w) — "+ "provisioning now could mint a duplicate that never deploys and locks the cluster to it. "+ "Re-run (if this was transient); if it persists, ensure your kubeconfig/context can list "+ @@ -541,7 +541,7 @@ func adoptLiveInClusterClient( // listed we can't verify ownership, so fail closed (re-run) rather than mint a // duplicate (orphan) or adopt across accounts. if listErr != nil { - return nil, false, &exitError{code: 1, err: fmt.Errorf( + return nil, false, &exitError{code: exitFailure, err: fmt.Errorf( "a tracebloc client is already running on this cluster, but listing your account to verify ownership failed (%w) — re-run once tracebloc is reachable, or resolve manually", listErr)} } @@ -559,7 +559,7 @@ func adoptLiveInClusterClient( // Live here, but not in the signed-in account — adopting it would be a silent // cross-account takeover. Refuse (mirrors the create 409, R6). ilog.Logf("live client %s not in this account — refusing cross-account adopt", live.ClientID) - return nil, false, &exitError{code: 1, err: errors.New(crossAccountConflictMsg)} + return nil, false, &exitError{code: exitFailure, err: errors.New(crossAccountConflictMsg)} } switch { @@ -584,11 +584,11 @@ func adoptLiveInClusterClient( // naming the owner) or a same-account live sibling (cluster_in_use). // Fix #2's same-account reclaim means this no longer fires for a stale // same-account holder — that path now succeeds (200). - return nil, false, &exitError{code: 1, err: errors.New(conflictMessage(ae))} + return nil, false, &exitError{code: exitFailure, err: errors.New(conflictMessage(ae))} case errors.As(perr, &ae) && ae.StatusCode == http.StatusForbidden: return nil, false, askAnAdmin(ctx, p, apiClient, "provision a client", "provisioning") } - return nil, false, &exitError{code: 1, err: fmt.Errorf("backfilling the cluster anchor onto the existing client: %w", perr)} + return nil, false, &exitError{code: exitFailure, err: fmt.Errorf("backfilling the cluster anchor onto the existing client: %w", perr)} } ilog.Logf("backfilled cluster_id onto client id=%d", owner.ID) owner = patched @@ -596,7 +596,7 @@ func adoptLiveInClusterClient( // The live client is anchored to a DIFFERENT cluster than the one we're // pointed at — the kubeconfig and the in-cluster client disagree. Don't // re-anchor (write-once); surface it rather than guess. - return nil, false, &exitError{code: 1, err: fmt.Errorf( + return nil, false, &exitError{code: exitFailure, err: fmt.Errorf( "the client running in this namespace is anchored to a different cluster (%s) than --kubeconfig/--context points at (%s) — check you're targeting the right cluster", owner.ClusterID, clusterID)} } @@ -703,17 +703,17 @@ func askAnAdmin(ctx context.Context, p *ui.Printer, client *api.Client, action, p.Field(label, a.Email) } } - return &exitError{code: 1, err: fmt.Errorf("%s requires CLIENT_WRITE permission", capability)} + return &exitError{code: exitFailure, err: fmt.Errorf("%s requires CLIENT_WRITE permission", capability)} } func runClientList(ctx context.Context, p *ui.Printer) error { client, cfg, err := authedClient() if err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } clients, err := client.ListClients(ctx) if err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } if len(clients) == 0 { p.Hintf("No clients yet. Run `tracebloc client create`.") @@ -750,7 +750,7 @@ timeout elapses (non-zero), to confirm the client connected after setup.`, // --timeout only governs the --wait poll; accepting it alone would be a // silent no-op, so reject it rather than mislead. if cmd.Flags().Changed("timeout") && !wait { - return &exitError{code: 1, err: errors.New("--timeout has no effect without --wait")} + return &exitError{code: exitFailure, err: errors.New("--timeout has no effect without --wait")} } return runClientStatus(cmd.Context(), printerFor(cmd), wait, timeout) }, @@ -768,11 +768,11 @@ const clientStatusPollInterval = 3 * time.Second func runClientStatus(ctx context.Context, p *ui.Printer, wait bool, timeout time.Duration) error { client, cfg, err := authedClient() if err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } active := cfg.Current().ActiveClientID if active == "" { - return &exitError{code: 1, err: errors.New( + return &exitError{code: exitFailure, err: errors.New( "no active client on this machine — run `tracebloc client create` (or re-run the installer) first")} } @@ -780,10 +780,10 @@ func runClientStatus(ctx context.Context, p *ui.Printer, wait bool, timeout time if !wait { st, found, lerr := lookupClientStatus(ctx, client, active) if lerr != nil { - return &exitError{code: 1, err: lerr} + return &exitError{code: exitFailure, err: lerr} } if !found { - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "active client %s isn't in your account — run `tracebloc client create` "+ "(or re-run the installer) to provision this machine", active)} } @@ -807,19 +807,19 @@ func runClientStatus(ctx context.Context, p *ui.Printer, wait bool, timeout time switch { case errors.As(lerr, &ue): // 426 (CLI too old) won't recover by waiting — surface the upgrade signal. - return &exitError{code: 1, err: lerr} + return &exitError{code: exitFailure, err: lerr} case errors.As(lerr, &apiErr) && (apiErr.StatusCode == http.StatusUnauthorized || apiErr.StatusCode == http.StatusForbidden): // A revoked/expired/forbidden token (401/403) won't recover by waiting — // the client itself may be online. Fail fast, point at sign-in. Note 429 // and 5xx stay transient (below): those DO recover on retry. - return &exitError{code: 1, err: errors.New( + return &exitError{code: exitFailure, err: errors.New( "tracebloc rejected your credentials while waiting — run `tracebloc login`, then retry")} case lerr != nil: lastErr = lerr // transient (5xx / 429 / network) — keep waiting, remember why case !found: // The active client isn't in the account (deleted / wrong account) — no // amount of waiting surfaces it. Fail fast, matching the one-shot path. - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "active client %s isn't in your account — run `tracebloc client create` "+ "(or re-run the installer) to provision this machine", active)} case st == clientStatusOnline: @@ -839,15 +839,15 @@ func runClientStatus(ctx context.Context, p *ui.Printer, wait bool, timeout time if remaining <= 0 { switch { case lastErr != nil: - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "timed out after %s waiting for tracebloc to report this client online; "+ "the last status check failed: %v", timeout, lastErr)} case lastState >= 0: - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "timed out after %s waiting for tracebloc to report this client online (last state: %s). "+ "Run `tracebloc doctor` to diagnose, or re-run the installer.", timeout, clientStateLabel(lastState))} default: - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "timed out after %s before tracebloc could confirm this client — retry, "+ "or run `tracebloc doctor`.", timeout)} } @@ -858,7 +858,7 @@ func runClientStatus(ctx context.Context, p *ui.Printer, wait bool, timeout time } select { case <-ctx.Done(): - return &exitError{code: 130} // Ctrl-C: exit quietly (no "Error: context canceled") + return &exitError{code: exitInterrupted} // Ctrl-C: exit quietly (no "Error: context canceled") case <-pollAfter(wait): } } @@ -1016,7 +1016,7 @@ func mapClientErr(err error) error { if errors.Is(err, errInteractiveCancelled) { return nil } - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } // randHex returns nbytes of crypto-random data hex-encoded. diff --git a/internal/cli/cluster.go b/internal/cli/cluster.go index 673ebfb1..dc3847a5 100644 --- a/internal/cli/cluster.go +++ b/internal/cli/cluster.go @@ -140,12 +140,12 @@ func runClusterInfo( // Kubeconfig errors are exit-code-3 territory (file/parse // problem, same conceptual class as `ingest validate`'s // unreadable-input). - return &exitError{code: 3, err: fmt.Errorf("loading kubeconfig: %w", err)} + return &exitError{code: exitLocalEnv, err: fmt.Errorf("loading kubeconfig: %w", err)} } cs, err := newClientsetFn(resolved) if err != nil { - return &exitError{code: 3, err: err} + return &exitError{code: exitLocalEnv, err: err} } p.Section("Kubeconfig") @@ -163,9 +163,9 @@ func runClusterInfo( // installed on this cluster". A binding miss gets the §7.3 // "runs elsewhere" explanation, same as the data commands. if errors.Is(err, cluster.ErrNoParentRelease) { - return binding.explain(&exitError{code: 4, err: &noParentReleaseError{err}}) + return binding.explain(&exitError{code: exitNoWorkspace, err: &noParentReleaseError{err}}) } - return &exitError{code: 4, err: err} + return &exitError{code: exitNoWorkspace, err: err} } resolved.Namespace = nsUsed // Printed after discovery so it reflects the namespace the scan actually @@ -197,7 +197,7 @@ func runClusterInfo( // 5 = "release found but no usable token." Distinct from // 4 (no release) so customers can RBAC-debug separately // from install issues. - return &exitError{code: 5, err: err} + return &exitError{code: exitAuth, err: err} } hash := sha256.Sum256([]byte(tok.Token)) diff --git a/internal/cli/clustertarget.go b/internal/cli/clustertarget.go index 2471a5cf..2de5862e 100644 --- a/internal/cli/clustertarget.go +++ b/internal/cli/clustertarget.go @@ -65,11 +65,11 @@ type clusterTarget struct { func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOptions, b activeClientBinding, needPVC bool) (*clusterTarget, error) { resolved, err := loadClusterFn(opts) if err != nil { - return nil, &exitError{code: 3, err: fmt.Errorf("loading kubeconfig: %w", err)} + return nil, &exitError{code: exitLocalEnv, err: fmt.Errorf("loading kubeconfig: %w", err)} } cs, err := newClientsetFn(resolved) if err != nil { - return nil, &exitError{code: 3, err: err} + return nil, &exitError{code: exitLocalEnv, err: err} } // The cluster-wide fallback scan only engages when the target namespace is // the kubeconfig's default — i.e. nobody chose it: not the user (explicit @@ -82,9 +82,9 @@ func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.Kubec // "runs elsewhere" rewrite; an API/RBAC list failure or an // ambiguous multiple-release match keeps its own message. if errors.Is(err, cluster.ErrNoParentRelease) { - return nil, &exitError{code: 4, err: &noParentReleaseError{err}} + return nil, &exitError{code: exitNoWorkspace, err: &noParentReleaseError{err}} } - return nil, &exitError{code: 4, err: err} + return nil, &exitError{code: exitNoWorkspace, err: err} } // The scan may have retargeted discovery to the namespace that actually // hosts the client; everything downstream (PVC discovery, dataset listing, @@ -94,7 +94,7 @@ func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.Kubec if needPVC { pvc, err := cluster.DiscoverSharedPVC(ctx, cs, resolved.Namespace) if err != nil { - return nil, &exitError{code: 4, err: err} + return nil, &exitError{code: exitNoWorkspace, err: err} } t.PVC = pvc } @@ -202,7 +202,7 @@ func (b activeClientBinding) explain(err error) error { if handle == "" { handle = b.namespace } - return &exitError{code: 4, err: fmt.Errorf( + return &exitError{code: exitNoWorkspace, err: fmt.Errorf( "active client %q runs on another machine — namespace %q isn't on the cluster your kubeconfig points at; "+ "run this command there, or override with --namespace/--context", handle, b.namespace)} diff --git a/internal/cli/data.go b/internal/cli/data.go index cb14b3a5..4a2e36a8 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -5,15 +5,10 @@ import ( "errors" "fmt" "io" - "path/filepath" - "strings" "github.com/spf13/cobra" - "gopkg.in/yaml.v3" - "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/push" - "github.com/tracebloc/cli/internal/schema" ) // newDataCmd wires the `tracebloc data` subtree. The dominant @@ -114,7 +109,7 @@ func runDataIngest(ctx context.Context, out, errOut io.Writer, a runDataIngestAr jsonEmitted := false defer func() { if a.OutputJSON && err != nil && !jsonEmitted { - code := 1 + code := exitFailure var ee *exitError if errors.As(err, &ee) { code = ee.Code() @@ -123,478 +118,30 @@ func runDataIngest(ctx context.Context, out, errOut io.Writer, a runDataIngestAr } }() - // Intro header: brand + a plain-English explainer of what an ingest - // does, so a first-time user understands it before any prompts. - // Routed through a.Printer, so --output-json keeps it on stderr and - // --plain/non-TTY degrade cleanly. (#31) - // --overwrite + a reused --idempotency-key is a data-loss trap: the - // teardown removes the existing data, then jobs-manager treats the - // duplicate key as a REPLAY and attaches to the previous run instead of - // ingesting anything — old data gone, new data never loaded, exit 0 from - // the old Job's status. Refuse the combination outright. - if a.Overwrite && a.IdempotencyKey != "" { - return &exitError{code: 2, err: errors.New( - "--overwrite can't be combined with --idempotency-key: a reused key makes the cluster replay the previous run instead of ingesting the new data — after --overwrite's removal that would report success while loading nothing. Drop one of the two (a fresh per-run key is the default).")} - } - - a.Printer.Banner("tracebloc", "data ingest") - a.Printer.Para(strings.TrimSpace(` -This ingests a dataset so models can train on it. Your files never leave your -own infrastructure — tracebloc copies them into your workspace's storage, -checks them, and loads them into a table your training runs read from. Other -collaborators can train against that table without ever seeing the raw files.`)) - a.Printer.Hintf("Learn more: https://docs.tracebloc.io") - - // 0. Guided mode: prompt for any missing core inputs before - // validation. Flags already provided win; non-TTY / --no-input - // leaves Prompter nil and skips straight to the flag-only path. - if a.Interactive && a.Prompter != nil { - if err := runInteractive(a.Printer, a.Prompter, &a, a.TaskSet); err != nil { - if errors.Is(err, errInteractiveCancelled) { - a.Printer.Infof("Cancelled — nothing was ingested.") - return nil - } - // A typed exitError from a guided step (e.g. the path-existence - // guard, which runInteractive runs before the family sniff) - // already carries its own code + clean message — surface it as-is - // rather than burying it under "interactive setup:". - var ee *exitError - if errors.As(err, &ee) { - return err - } - return &exitError{code: 3, err: fmt.Errorf("interactive setup: %w", err)} - } - } - // --intent defaults to train. Applied after the interactive block so - // the guided flow still asks "training or test?" (it prompts on an - // empty value); a non-interactive run that omits --intent gets train - // without erroring (RFC-0002 §5). The wire field stays "intent". - if a.Spec.Intent == "" { - a.Spec.Intent = "train" - } - if a.LocalPath == "" { - return &exitError{code: 3, err: errors.New( - "local dataset path is required — pass it as an argument, or run " + - "on a terminal without --no-input for guided prompts")} - } - // Expand a leading ~ ourselves. The shell expands an unquoted ~ on - // the command line, but a path typed at the interactive prompt (or - // a quoted/literal ~ arg) reaches us unexpanded — and filepath.Abs - // would just prepend the CWD, yielding ".../cwd/~/...". Mirrors - // cluster.expandPath; done here so it covers both entry points - // before any push.Discover* call. (#37) - a.LocalPath = expandHome(a.LocalPath) - - // 0b. Path existence FIRST — before any spec / schema / family - // validation. A typo'd path should fail on the path with a plain - // "no such file or directory", not surface later as a confusing - // downstream error (e.g. the task gate asking which task the - // non-existent data is for). runInteractive runs this same guard - // before its family sniff / label preview, so the invariant holds on - // the guided route too; this re-check covers the flag-only path and - // is cheap (one stat). The family walk below stats again for its - // layout-specific diagnostics; this is only about ordering the first - // failure a customer sees. (#181) - if err := statDatasetPath(a.LocalPath); err != nil { - return err - } - - // 1. Validate the table name BEFORE anything else. It's both - // the MySQL identifier and the /data/shared// PVC - // subdirectory — an unsanitized traversal name (../../etc) - // would escape that subtree once the stage Pod writes to - // it. The embedded schema only checks minLength on `table`, - // so this CLI-side guard is the real fix. SpecArgs.Build() - // below calls StagedPrefix, which panics on an unsafe name — - // so this check MUST come first. - if err := push.ValidateTableName(a.Spec.Table); err != nil { - return &exitError{code: 2, err: err} - } - - // 2. Category gate. Runs BEFORE schema validation so an - // unsupported category gets a clear, actionable CLI message - // rather than the schema's terse enum / missing-property error. - // Every schema task category is CLI-supported now; this gate stays as - // defensive routing so a future known-but-not-yet-wired category gets a - // clear per-category message, and a typo'd category gets the supported - // list rather than the schema's raw enum dump. - switch { - case a.Spec.Category == "": - // No task chosen. In guided mode the picker already filled this; - // reaching here means a non-interactive run (or --no-input / - // --output-json) that omitted --task. Give a clear, actionable - // error instead of silently assuming images (the old default). - return &exitError{code: 2, err: fmt.Errorf( - "which task is this data for? pass --task — one of: %s. "+ - "(On a terminal without --no-input, tracebloc asks you to pick.)", - push.SupportedCategoriesList())} - case push.IsCLISupported(a.Spec.Category): - // supported - case push.IsKnown(a.Spec.Category): - // A recognized category the CLI doesn't implement yet. None today — every - // schema category is wired — but kept as defensive routing so a future - // known-but-unsupported category gets the registry's per-category reason, - // not a misleading "unrecognized category". Supported categories were - // already caught above, so IsKnown here means known-but-unsupported. - spec, _ := push.Lookup(a.Spec.Category) - reason := "" - if spec.UnsupportedNote != "" { - reason = " (" + spec.UnsupportedNote + ")" - } - return &exitError{code: 2, err: fmt.Errorf( - "task %q isn't supported by the CLI yet%s. Supported tasks: %s.", - a.Spec.Category, reason, push.SupportedCategoriesList())} - default: - return &exitError{code: 2, err: fmt.Errorf( - "task %q isn't a recognized task. Supported tasks: %s.", - a.Spec.Category, push.SupportedCategoriesList())} - } - - // Image-only flags. --target-size / --min-size describe image - // resolution, so they're meaningless on a tabular / text task. - // Reject them explicitly here: without this guard they'd be parsed - // only inside the image branch below, so on a non-image task the - // value — even a malformed one — was silently dropped with no error. - if !push.IsImage(a.Spec.Category) { - for _, f := range []struct{ name, val string }{ - {"--target-size", a.TargetSizeFlag}, - {"--min-size", a.MinSizeFlag}, - } { - if f.val != "" { - return &exitError{code: 2, err: fmt.Errorf( - "%s is image tasks only; it doesn't apply to task %q", - f.name, a.Spec.Category)} - } - } - } - - // Task-scoped flags. Like --target-size/--min-size above, each of these is - // read only inside the one category branch that consumes it, so passing one - // on a task that doesn't use it silently dropped the value — and the user's - // intent — with no error, even though the help text says each is scoped. - // Reject a misapplied flag explicitly so it fails fast instead of being - // ignored (the scope mirrors spec.go's build gates exactly). - if a.SchemaFlag != "" && !push.IsTabular(a.Spec.Category) { - return &exitError{code: 2, err: fmt.Errorf( - "--schema is tabular/time-series tasks only; it doesn't apply to task %q", a.Spec.Category)} - } - if a.Spec.LabelPolicy != "" && !push.IsRegressionClass(a.Spec.Category) { - return &exitError{code: 2, err: fmt.Errorf( - "--label-policy is regression-class tasks only (tabular_regression, "+ - "time_series_forecasting, time_to_event_prediction); it doesn't apply to task %q", - a.Spec.Category)} - } - if a.Spec.TimeColumn != "" && a.Spec.Category != "time_to_event_prediction" { - return &exitError{code: 2, err: fmt.Errorf( - "--time-column is time_to_event_prediction only; it doesn't apply to task %q", a.Spec.Category)} - } - if a.Spec.NumberOfKeypoints != 0 && a.Spec.Category != "keypoint_detection" { - return &exitError{code: 2, err: fmt.Errorf( - "--number-of-keypoints is keypoint_detection only; it doesn't apply to task %q", a.Spec.Category)} - } - // --label-column is meaningless for self-supervised text (the label is the - // text itself); buildText drops it, so accepting it silently discarded the - // user's value and the review echoed a column that never shipped. - if a.Spec.LabelColumn != "" && push.SelfSupervisedText(a.Spec.Category) { - return &exitError{code: 2, err: fmt.Errorf( - "--label-column doesn't apply to task %q — it trains on the text itself, with no label column", - a.Spec.Category)} - } - - // 3. Walk the local directory FIRST (local "fail fast"), dispatched - // by category family. Image categories expect labels.csv + - // images/; tabular / time-series categories expect a single - // data CSV. The walk also yields what the per-category - // resolution below needs (the image list for target-size, the - // CSV for schema inference). - // err is the function's named return (see the --output-json defer - // at the top), so it's not redeclared here. The walk can take a moment - // on a large tree, so it gets a spinner — no blocking wait stays silent. - var layout *push.LocalLayout - walkSpin := a.Printer.Spinner("Reading your files", "") - switch { - case push.IsTabular(a.Spec.Category): - layout, err = push.DiscoverTabular(a.LocalPath) - case push.IsText(a.Spec.Category): - layout, err = push.DiscoverText(a.Spec.Category, a.LocalPath) - case a.Spec.Category == "object_detection": - layout, err = push.DiscoverObjectDetection(a.LocalPath) - case a.Spec.Category == "semantic_segmentation": - layout, err = push.DiscoverSemanticSegmentation(a.LocalPath) - default: - // image_classification + keypoint_detection: labels.csv + images/. - layout, err = push.Discover(a.LocalPath) - } - walkSpin.Stop() + // Steps 0–4 + the P3 preflight — everything local — live in + // resolveLocalInput (cli#283). It mutates a.Spec in place, so the defer + // above and the cluster steps below see the resolved spec; err feeds the + // named return, so the defer fires on its failures too. + layout, spec, specBytes, cancelled, err := resolveLocalInput(out, errOut, &a) if err != nil { - return &exitError{code: 3, err: err} - } - - a.Printer.Step(1, 3, "Check your data") - a.Printer.Hintf("Reading your files locally first — nothing has touched your workspace yet — so a layout or settings problem shows up right away.") - - // 3a. Per-category spec resolution from the local data, so the - // synthesized spec carries the right fields before validation. - switch { - case push.IsTabular(a.Spec.Category): - // P3 (cli#71): a BOM'd tabular CSV is doomed in-cluster AND would - // corrupt InferSchema's own header read below — reject before - // either. The rest of the content preflight runs after the spec - // schema validation (mirroring the in-cluster order). - if perr := push.CheckTabularBOM(layout.LabelsCSV); perr != nil { - return &exitError{code: 3, err: perr} - } - if perr := push.CheckHasDataRows(layout.LabelsCSV); perr != nil { - return &exitError{code: 3, err: perr} - } - - // Column schema. An explicit --schema wins (raw flag, or the - // optional override the interactive prompt captures into SchemaFlag). - // Otherwise infer the types here — mirroring the ingestor's own rules - // (di#349) — and EMIT the result explicitly (below, via a.Spec.Schema - // → spec.schema), so the ingestor uses the CLI's answer regardless of - // its own version. Inference runs on both a no-schema non-interactive - // run and an interactive run where the user left the schema prompt - // blank; the risky cases below are surfaced as warnings. - if a.SchemaFlag != "" { - sch, perr := push.ParseSchema(a.SchemaFlag) - if perr != nil { - return &exitError{code: 2, err: perr} - } - a.Spec.Schema = sch - } else { - res, ierr := push.InferSchema(layout.LabelsCSV) - if ierr != nil { - return &exitError{code: 3, err: fmt.Errorf("inferring schema from CSV: %w", ierr)} - } - a.Spec.Schema = res.Schema - _, _ = fmt.Fprintf(out, - "Inferred schema for %d column(s) from %s (override with --schema).\n", - len(res.Schema), filepath.Base(layout.LabelsCSV)) - if len(res.Skipped) > 0 { - _, _ = fmt.Fprintf(out, - " (skipped framework-managed column(s): %s)\n", strings.Join(res.Skipped, ", ")) - } - if len(res.Empty) > 0 { - _, _ = fmt.Fprintf(out, - " (warning: %d column(s) had no values in the sample and were typed VARCHAR(1): %s)\n", - len(res.Empty), strings.Join(res.Empty, ", ")) - } - if len(res.IDLike) > 0 { - _, _ = fmt.Fprintf(out, - " (warning: %d column(s) look like identifiers (all-unique integers): %s — "+ - "if any is a zero-padded code, pass --schema to type it VARCHAR)\n", - len(res.IDLike), strings.Join(res.IDLike, ", ")) - } - } - case push.IsImage(a.Spec.Category): - // keypoint_detection needs --number-of-keypoints (dataset- - // specific, no default). Catch it here with an actionable - // message rather than letting the ingestor fail mid-run. Split - // UNSET from SET-BUT-INVALID (#76b): a Go 0 could mean either, so - // key on whether the flag was passed (ChangedFlags). Unset → the - // "requires" nudge; set to a non-positive value → name the bad - // value so the user sees exactly what was rejected. - if a.Spec.Category == "keypoint_detection" && a.Spec.NumberOfKeypoints <= 0 { - if a.ChangedFlags["number-of-keypoints"] { - return &exitError{code: 2, err: fmt.Errorf( - "--number-of-keypoints must be a positive integer (got %d); "+ - "it's the number of keypoints per sample (e.g. 17 for COCO pose)", - a.Spec.NumberOfKeypoints)} - } - return &exitError{code: 2, err: errors.New( - "keypoint_detection requires --number-of-keypoints (e.g. " + - "--number-of-keypoints 17); it's dataset-specific and has no default")} - } - // Image target resolution: the ingestor's image_classification - // default is 512x512 and it VALIDATES (it does not resize), so - // a mismatch hard-fails. Honour an explicit --target-size; - // otherwise auto-detect from the first image so the common - // "all my images are NxN" case just works. - if a.TargetSizeFlag != "" { - w, h, perr := push.ParseTargetSize(a.TargetSizeFlag) - if perr != nil { - return &exitError{code: 2, err: perr} - } - a.Spec.TargetSize = []int{w, h} - } else if len(layout.Images) > 0 { - if w, h, derr := push.DetectImageSize(layout.Images[0]); derr == nil { - a.Spec.TargetSize = []int{w, h} - _, _ = fmt.Fprintf(out, - "Auto-detected image target size %dx%d from %s (override with --target-size).\n", - w, h, filepath.Base(layout.Images[0])) - } else { - _, _ = fmt.Fprintf(errOut, - "Note: couldn't auto-detect image size (%v); using the ingestor "+ - "default. Pass --target-size WxH if ingestion reports a "+ - "resolution mismatch.\n", derr) - } - } - // Minimum-size floor override (#348): plumb an explicit --min-size to - // spec.file_options.min_size. When unset, no spec field is emitted, so - // the ingestor applies its own default (none on the deployed - // v0.5.7/v0.6.0; 32x32 on develop post-#348) — and the local preview - // applies NO floor either (PreflightDataset only previews the floor - // when --min-size is set, so it never rejects an ingest the live - // cluster accepts). The below-floor reject is previewed in - // runLocalPreflight (ValidateImages). - if a.MinSizeFlag != "" { - w, h, perr := push.ParseMinSize(a.MinSizeFlag) - if perr != nil { - return &exitError{code: 2, err: perr} - } - a.Spec.MinSize = []int{w, h} - } - // Extension: every image must share one type, and the spec tells - // the cluster which one to validate against (file_options.extension). - // Without this the ingestor checked its .jpeg convention default and - // rejected .jpg/.png datasets AFTER the full upload (cli#68). - ext, exterr := push.DetectExtension(layout.Images) - if exterr != nil { - return &exitError{code: 3, err: exterr} - } - a.Spec.Extension = ext - default: - // Text family: no extra per-category resolution. The supervised text - // tasks (text_classification, token_classification, - // sentence_pair_classification) carry a label straight from - // --label-column; the self-supervised ones (masked/causal language - // modeling, seq2seq, embeddings) need neither a label nor a schema. - // buildText emits the label for exactly the supervised set, keyed on - // the registry's SelfSupervised flag (not a hardcoded id). + return err } - - // 3b. Friendly missing-label pre-check (#214). Tabular / time-series tasks - // AND semantic_segmentation carry a required label column (the ingest - // schema's allOf requires `label` for them). With no --label-column the - // synthesized spec's `label` is an empty string, which trips the schema's - // label oneOf and the raw validation below dumps an opaque "got object, - // want string" / "minLength" pair. semseg is especially prone to this — - // its per-image label reads as vestigial beside the pixel masks, so the - // flag is easy to forget. Intercept ONLY that specific missing case here — - // a label present-but-not-in-the-CSV still flows to runLocalPreflight's - // CheckLabelColumn, and every other schema error still reaches the dump — - // and name the flag to fix instead. - if (push.IsTabular(a.Spec.Category) || a.Spec.Category == "semantic_segmentation") && a.Spec.LabelColumn == "" { - msg := "this task needs a label column, but --label-column wasn't set — " + - "pass --label-column with the name of the target column in your data CSV" - if cols := sortedKeys(a.Spec.Schema); len(cols) > 0 { - msg += " (columns: " + strings.Join(cols, ", ") + ")" - } - return &exitError{code: 2, err: errors.New(msg)} + if cancelled { + return nil } - // 4. Synthesize the spec from flags + validate against schema. - // Catches "bad category", "missing intent" etc. BEFORE we - // touch the cluster. The error formatter is the same one - // ingest validate uses, so a customer who YAML'd manually - // first sees identical wording. - spec := a.Spec.Build() - specBytes, err := yaml.Marshal(spec) + // Steps 5–8a — cluster discovery + the destination-table guard — live in + // connectIngestTarget (cli#283). It may set a.Overwrite (the folded + // interactive replace decision); the teardown below keys on that. + target, existingTable, cancelled, err := connectIngestTarget(ctx, &a) if err != nil { - return &exitError{code: 3, err: fmt.Errorf("marshaling synthesized spec: %w", err)} - } - v, err := schema.NewV1Validator() - if err != nil { - return &exitError{code: 3, err: fmt.Errorf("loading embedded schema: %w", err)} - } - _, errs, parseErr := v.ValidateYAML(specBytes) - if parseErr != nil { - // "Parse" failing on a spec we marshaled ourselves is a - // programming error, not a customer error — surface it - // with the bytes so we can diagnose. Exit 3 (the - // "internal" bucket) matches the marshal-failure branch - // above. - return &exitError{code: 3, err: fmt.Errorf("internal: re-parsing synthesized spec: %w\n%s", parseErr, specBytes)} - } - if len(errs) > 0 { - // Use the SAME formatter `ingest validate` uses, so the - // experience is identical whether the customer authored - // YAML by hand or via flags. Diagnostics go to stderr - // (matching ingest validate) so a downstream pipe of - // stdout (e.g. piping the summary to jq once that's a - // JSON output mode) isn't polluted by error text. Exit 2 - // is reserved for schema violations across the CLI. - _, _ = fmt.Fprintf(errOut, "synthesized spec failed schema validation (%d issue%s):\n", - len(errs), plural(len(errs))) - _, _ = fmt.Fprintln(errOut, schema.FormatErrors(errs)) - return &exitError{code: 2, err: errors.New("synthesized spec failed schema validation; check the flag values above")} - } - - // P3 content preflight (backend#828, cli#69/#71/#72/#73): preview the - // ingestor's own validators locally — AFTER the spec schema validation, - // mirroring the in-cluster order (jsonschema first, then validators), - // and BEFORE any cluster work. Each check names the rule it previews; - // parity is pinned by internal/push/parity_golden_test.go. - if perr := runLocalPreflight(a, layout, errOut); perr != nil { - return perr + return err } - - printLocalSummary(a.Printer, layout, spec) - - // 5. Cluster discovery — same kubeconfig path as `cluster info`. - // Errors mirror that command's exit-code contract (3 for - // kubeconfig, 4 for missing release) so behaviour is - // consistent across pre-flight commands. - // Connecting to the workspace + discovering its shared storage is - // Kubernetes plumbing (release / PVC / jobs-manager) the happy path keeps - // quiet — it's no longer a numbered step (RFC-0002 §6), and --verbose adds - // the release/PVC detail below. But the discovery itself is several blocking - // apiserver round-trips (kubeconfig load, release + PVC discovery, then the - // destination-exists check), so it still needs a visible status line — no - // silent wait on the happy path (RFC-0002 "progress on every wait"). - // A plain line, not a spinner: discoverRelease can print its own - // namespace-fallback note mid-call, and a spinner's \r redraw would clobber - // it. ALL the logic below (discovery + the exit-6 destination guard) is - // unchanged; only the presentation moved. - a.Printer.Infof("Connecting to your workspace…") - // 6. PVC discovery (needPVC) confirms the chart's shared-data PVC is - // Bound before we waste time provisioning a Pod that can't mount it. - opts := cluster.KubeconfigOptions{Path: a.Kubeconfig, Context: a.Context, Namespace: a.Namespace} - binding := bindActiveClientNamespace(&opts) - target, err := resolveClusterTarget(ctx, a.Printer, opts, binding, true) - if err != nil { - return binding.explain(err) + if cancelled { + return nil } resolved, cs, release, pvc := target.Resolved, target.Clientset, target.Release, target.PVC - // release.IngestorSAName is discovered from the ingestionAuthz ConfigMap by - // DiscoverParentRelease (#7) and flows into the stage/teardown pods + the - // jobs-manager token mint below — no --ingestor-sa override. - - // 7. Under --verbose, show what we found on the cluster; the happy path - // keeps this Kubernetes detail hidden (printClusterSummary is a no-op - // without --verbose). - printClusterSummary(a.Printer, release, pvc) - - // 8a. Destination guard (cli#70, P4-lite): re-ingesting an existing - // table used to stage EVERYTHING and then fail the in-cluster Job - // on the ingestor's duplicate check — a full upload burned to learn - // the table exists. One cheap read heads that off. The check fails - // open (dim note) — the ingestor still refuses duplicates, so a - // broken check can't cause silent data loss. - existingTable, checkNote := destTableExists(ctx, cs, resolved, a.Spec.Table) - if checkNote != "" { - a.Printer.Hintf("%s", checkNote) - } tableExists := existingTable != "" - if tableExists && !a.Overwrite { - // Folded decision (RFC-0002): in interactive mode a pre-existing table - // is a question, not a wall. Prompt to replace it; a "no" cancels - // cleanly (exit 0). Non-interactive (or --output-json / --no-input) - // still hard-fails exit 6 — a script must opt in with --overwrite. - proceed, aerr := existingTableAction(&a, existingTable) - if aerr != nil { - return aerr - } - if !proceed { - a.Printer.Infof("Cancelled — %q was left as-is; nothing was ingested.", existingTable) - return nil - } - a.Overwrite = true - } - if tableExists && a.Overwrite { - a.Printer.Warnf("Table %q already exists — replacing it (table + files).", existingTable) - } // 8. Dry-run stop. Acknowledged success, plus a reminder of the // live-only steps (stage + ingest) the customer just skipped. @@ -632,7 +179,7 @@ collaborators can train against that table without ever seeing the raw files.`)) // partial failure can leave files the DB-backed guard can no // longer see — a plain re-run would upload everything and then // hit them in-cluster. data delete first is the real recovery. - return &exitError{code: 7, err: fmt.Errorf( + return &exitError{code: exitTeardownFailed, err: fmt.Errorf( "replacing table %q failed partway — its removal may be incomplete, and a plain re-run "+ "would hit the leftovers after uploading everything. Run `tracebloc data delete %s` "+ "first, then re-run this ingest. Nothing new was staged. (%w)", @@ -678,7 +225,7 @@ collaborators can train against that table without ever seeing the raw files.`)) Out: out, }) if stageErr != nil { - return &exitError{code: 7, err: stageErr} + return &exitError{code: exitStagingFailed, err: stageErr} } // 10–12. The ingestion-run tail: mint token → port-forward → submit → diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index 890e852d..0a5b5f61 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -154,14 +154,14 @@ undone — re-ingesting the data is the only way back.`) // through to ValidateTableName's "set --name" text (that flag belongs to // `data ingest`, not here). ExactArgs(1) still accepts an explicit "". if a.Table == "" { - return &exitError{code: 2, err: errors.New( + return &exitError{code: exitBadInput, err: errors.New( "dataset name is required — pass it as an argument: tracebloc data delete ")} } // 1. Validate the name before we build any PVC path from it // (push.PlanTeardown panics on an unsafe name by design). if err := push.ValidateTableName(a.Table); err != nil { - return &exitError{code: 2, err: fmt.Errorf("invalid table name %q: %w", a.Table, err)} + return &exitError{code: exitBadInput, err: fmt.Errorf("invalid table name %q: %w", a.Table, err)} } // 2. Resolve cluster + clientset (kubeconfig errors = exit 3), then @@ -225,7 +225,7 @@ undone — re-ingesting the data is the only way back.`) // a decline still keeps the stdout-always-JSON contract.) if !a.Yes { if a.Prompter == nil { - return &exitError{code: 3, err: errors.New( + return &exitError{code: exitLocalEnv, err: errors.New( "refusing to delete without confirmation: pass --yes or run on a terminal")} } p.PromptHint("This drops the table and removes the files listed above — there's no undo. Pass --yes next time to skip this prompt.") @@ -239,7 +239,7 @@ undone — re-ingesting the data is the only way back.`) } return nil } - return &exitError{code: 3, err: err} + return &exitError{code: exitLocalEnv, err: err} } if !ok { p.Infof("Cancelled — nothing was deleted.") @@ -271,12 +271,12 @@ undone — re-ingesting the data is the only way back.`) // so re-running is safe; if it keeps failing, remove the leftover // staging dirs on the node directly. if res.DroppedTable { - return &exitError{code: 7, err: fmt.Errorf( + return &exitError{code: exitTeardownFailed, err: fmt.Errorf( "teardown incomplete — the table %s.%s was dropped, but removing its files failed: %w; "+ "re-run `tracebloc data delete %s`, or delete the leftover staging dirs on the node", plan.Database, plan.Table, err, matched)} } - return &exitError{code: 7, err: fmt.Errorf("teardown failed: %w", err)} + return &exitError{code: exitTeardownFailed, err: fmt.Errorf("teardown failed: %w", err)} } p.Newline() @@ -366,7 +366,7 @@ func writeDataDeleteErrorJSON(w io.Writer, e error, code int) { func resolveDeleteTarget(ctx context.Context, cs kubernetes.Interface, resolved *cluster.ResolvedConfig, requested string) (string, error) { names, err := listDatasetsFn(ctx, cs, resolved.RestConfig, resolved.Namespace) if err != nil { - return "", &exitError{code: 4, err: fmt.Errorf( + return "", &exitError{code: exitNoWorkspace, err: fmt.Errorf( "can't confirm %q exists on this client — refusing to delete without "+ "confirming the target first: %w", requested, err)} } @@ -375,7 +375,7 @@ func resolveDeleteTarget(ctx context.Context, cs kubernetes.Interface, resolved return n, nil } } - return "", &exitError{code: 5, err: fmt.Errorf( + return "", &exitError{code: exitNoSuchDataset, err: fmt.Errorf( "no dataset named %q on this client%s", requested, availableHint(names))} } diff --git a/internal/cli/data_ingest_cluster.go b/internal/cli/data_ingest_cluster.go index f7718dfe..2984b219 100644 --- a/internal/cli/data_ingest_cluster.go +++ b/internal/cli/data_ingest_cluster.go @@ -19,6 +19,86 @@ import ( "github.com/tracebloc/cli/internal/ui" ) +// connectIngestTarget is the cluster half of `data ingest`'s pre-flight: +// resolve the kubeconfig and discover the parent release + shared PVC +// (steps 5–7), then run the destination-table guard (8a). Extracted +// verbatim from runDataIngest (cli#283) — step order, output, and exit +// codes unchanged. +// +// The folded replace decision can set a.Overwrite (interactive "replace +// it?" answered yes); the caller's teardown step keys on that, exactly as +// before. cancelled=true with a nil err is the user declining the replace +// prompt: the caller exits 0, nothing ingested. existingTable is the +// EXISTING table's exact spelling ("" when absent) — any teardown must act +// on it, not on the flag's casing. +func connectIngestTarget(ctx context.Context, a *runDataIngestArgs) (target *clusterTarget, existingTable string, cancelled bool, err error) { + // 5. Cluster discovery — same kubeconfig path as `cluster info`. + // Errors mirror that command's exit-code contract (3 for + // kubeconfig, 4 for missing release) so behaviour is + // consistent across pre-flight commands. + // Connecting to the workspace + discovering its shared storage is + // Kubernetes plumbing (release / PVC / jobs-manager) the happy path keeps + // quiet — it's no longer a numbered step (RFC-0002 §6), and --verbose adds + // the release/PVC detail below. But the discovery itself is several blocking + // apiserver round-trips (kubeconfig load, release + PVC discovery, then the + // destination-exists check), so it still needs a visible status line — no + // silent wait on the happy path (RFC-0002 "progress on every wait"). + // A plain line, not a spinner: discoverRelease can print its own + // namespace-fallback note mid-call, and a spinner's \r redraw would clobber + // it. ALL the logic below (discovery + the exit-6 destination guard) is + // unchanged; only the presentation moved. + a.Printer.Infof("Connecting to your workspace…") + // 6. PVC discovery (needPVC) confirms the chart's shared-data PVC is + // Bound before we waste time provisioning a Pod that can't mount it. + opts := cluster.KubeconfigOptions{Path: a.Kubeconfig, Context: a.Context, Namespace: a.Namespace} + binding := bindActiveClientNamespace(&opts) + target, err = resolveClusterTarget(ctx, a.Printer, opts, binding, true) + if err != nil { + return nil, "", false, binding.explain(err) + } + resolved, cs, release, pvc := target.Resolved, target.Clientset, target.Release, target.PVC + // release.IngestorSAName is discovered from the ingestionAuthz ConfigMap by + // DiscoverParentRelease (#7) and flows into the stage/teardown pods + the + // jobs-manager token mint below — no --ingestor-sa override. + + // 7. Under --verbose, show what we found on the cluster; the happy path + // keeps this Kubernetes detail hidden (printClusterSummary is a no-op + // without --verbose). + printClusterSummary(a.Printer, release, pvc) + + // 8a. Destination guard (cli#70, P4-lite): re-ingesting an existing + // table used to stage EVERYTHING and then fail the in-cluster Job + // on the ingestor's duplicate check — a full upload burned to learn + // the table exists. One cheap read heads that off. The check fails + // open (dim note) — the ingestor still refuses duplicates, so a + // broken check can't cause silent data loss. + existingTable, checkNote := destTableExists(ctx, cs, resolved, a.Spec.Table) + if checkNote != "" { + a.Printer.Hintf("%s", checkNote) + } + tableExists := existingTable != "" + if tableExists && !a.Overwrite { + // Folded decision (RFC-0002): in interactive mode a pre-existing table + // is a question, not a wall. Prompt to replace it; a "no" cancels + // cleanly (exit 0). Non-interactive (or --output-json / --no-input) + // still hard-fails exit 6 — a script must opt in with --overwrite. + proceed, aerr := existingTableAction(a, existingTable) + if aerr != nil { + return nil, "", false, aerr + } + if !proceed { + a.Printer.Infof("Cancelled — %q was left as-is; nothing was ingested.", existingTable) + return nil, "", true, nil + } + a.Overwrite = true + } + if tableExists && a.Overwrite { + a.Printer.Warnf("Table %q already exists — replacing it (table + files).", existingTable) + } + + return target, existingTable, false, nil +} + // runIngestionRun is the money path's outcome tail. It mints the ingestor // token, port-forwards to jobs-manager, POSTs the run, classifies the result // into a status + process exit code (kept in lockstep by classifyPushOutcome), @@ -54,7 +134,7 @@ func runIngestionRun(ctx context.Context, out io.Writer, a runDataIngestArgs, ta tok, err := mintIngestorTokenFn(ctx, cs, resolved.Namespace, release.IngestorSAName, 3600, nil) if err != nil { - return false, &exitError{code: 5, err: err} + return false, &exitError{code: exitAuth, err: err} } // 11. Open a port-forward to a Pod backing the jobs-manager @@ -74,7 +154,7 @@ func runIngestionRun(ctx context.Context, out io.Writer, a runDataIngestArgs, ta resolved.Namespace, release.JobsManagerServiceName, release.JobsManagerPort) connectSpin.Stop() if err != nil { - return false, &exitError{code: 8, err: fmt.Errorf("setting up jobs-manager port-forward: %w", err)} + return false, &exitError{code: exitSubmitFailed, err: fmt.Errorf("setting up jobs-manager port-forward: %w", err)} } defer pf.Close() @@ -263,11 +343,11 @@ func existingTableAction(a *runDataIngestArgs, existingTable string) (proceed bo if errors.Is(perr, errInteractiveCancelled) { return false, nil } - return false, &exitError{code: 3, err: fmt.Errorf("overwrite prompt: %w", perr)} + return false, &exitError{code: exitLocalEnv, err: fmt.Errorf("overwrite prompt: %w", perr)} } return ok, nil } - return false, &exitError{code: 6, err: fmt.Errorf( + return false, &exitError{code: exitTableExists, err: fmt.Errorf( "table %q already exists in this workspace. Re-ingesting the same table doesn't merge or replace — "+ "the run would fail after uploading everything. Re-run with --overwrite to replace it, "+ "or pick a different --name. (`tracebloc data delete %s` also removes it.)", diff --git a/internal/cli/data_ingest_local.go b/internal/cli/data_ingest_local.go index 7a382ac3..4cdc1d72 100644 --- a/internal/cli/data_ingest_local.go +++ b/internal/cli/data_ingest_local.go @@ -1,7 +1,9 @@ -// The local, cluster-free half of `data ingest`: path expansion + the +// The local, cluster-free half of `data ingest`: resolveLocalInput (the +// whole pre-cluster pipeline — prompts, validation, layout walk, spec +// synthesis + schema validation), path expansion + the // path-existence-first guard, the local dataset summary, and the // preflight that previews the ingestor's validators on the local data. -// Moved verbatim from data.go (cli#282) — behavior unchanged. +// Moved verbatim from data.go (cli#282, cli#283) — behavior unchanged. package cli import ( @@ -9,10 +11,15 @@ import ( "fmt" "io" "os" + "path/filepath" "sort" + "strings" + + "gopkg.in/yaml.v3" "github.com/tracebloc/cli/internal/pathutil" "github.com/tracebloc/cli/internal/push" + "github.com/tracebloc/cli/internal/schema" "github.com/tracebloc/cli/internal/ui" ) @@ -46,15 +53,441 @@ func expandHome(path string) string { func statDatasetPath(path string) error { if _, serr := os.Stat(path); serr != nil { if errors.Is(serr, os.ErrNotExist) { - return &exitError{code: 3, err: fmt.Errorf( + return &exitError{code: exitLocalEnv, err: fmt.Errorf( "no such file or directory: %q — check the path to your dataset", path)} } - return &exitError{code: 3, err: fmt.Errorf( + return &exitError{code: exitLocalEnv, err: fmt.Errorf( "can't read %q: %w", path, serr)} } return nil } +// resolveLocalInput is everything `data ingest` does BEFORE touching the +// cluster: the --overwrite/--idempotency-key guard, the intro banner, the +// guided prompts, path expansion + the existence-first check, table-name / +// category / misapplied-flag validation, the local layout walk, per-category +// spec resolution, spec synthesis + schema validation, the P3 content +// preflight, and the local summary. Extracted verbatim from runDataIngest +// (cli#283) — step order, output, and exit codes unchanged. +// +// It mutates a in place (a.LocalPath's ~-expansion; a.Spec's intent default +// and resolved schema / target size / min size / extension), exactly as the +// inline code mutated its copy — so runDataIngest's --output-json error +// defer (which stays next to the named return it reads) and the cluster +// steps that follow see the resolved spec. cancelled=true with a nil err is +// the guided flow's clean Ctrl-C: the caller exits 0, nothing ingested. +func resolveLocalInput(out, errOut io.Writer, a *runDataIngestArgs) (layout *push.LocalLayout, spec map[string]any, specBytes []byte, cancelled bool, err error) { + // Intro header: brand + a plain-English explainer of what an ingest + // does, so a first-time user understands it before any prompts. + // Routed through a.Printer, so --output-json keeps it on stderr and + // --plain/non-TTY degrade cleanly. (#31) + // --overwrite + a reused --idempotency-key is a data-loss trap: the + // teardown removes the existing data, then jobs-manager treats the + // duplicate key as a REPLAY and attaches to the previous run instead of + // ingesting anything — old data gone, new data never loaded, exit 0 from + // the old Job's status. Refuse the combination outright. + if a.Overwrite && a.IdempotencyKey != "" { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: errors.New( + "--overwrite can't be combined with --idempotency-key: a reused key makes the cluster replay the previous run instead of ingesting the new data — after --overwrite's removal that would report success while loading nothing. Drop one of the two (a fresh per-run key is the default).")} + } + + a.Printer.Banner("tracebloc", "data ingest") + a.Printer.Para(strings.TrimSpace(` +This ingests a dataset so models can train on it. Your files never leave your +own infrastructure — tracebloc copies them into your workspace's storage, +checks them, and loads them into a table your training runs read from. Other +collaborators can train against that table without ever seeing the raw files.`)) + a.Printer.Hintf("Learn more: https://docs.tracebloc.io") + + // 0. Guided mode: prompt for any missing core inputs before + // validation. Flags already provided win; non-TTY / --no-input + // leaves Prompter nil and skips straight to the flag-only path. + if a.Interactive && a.Prompter != nil { + if err := runInteractive(a.Printer, a.Prompter, a, a.TaskSet); err != nil { + if errors.Is(err, errInteractiveCancelled) { + a.Printer.Infof("Cancelled — nothing was ingested.") + return nil, nil, nil, true, nil + } + // A typed exitError from a guided step (e.g. the path-existence + // guard, which runInteractive runs before the family sniff) + // already carries its own code + clean message — surface it as-is + // rather than burying it under "interactive setup:". + var ee *exitError + if errors.As(err, &ee) { + return nil, nil, nil, false, err + } + return nil, nil, nil, false, &exitError{code: exitLocalEnv, err: fmt.Errorf("interactive setup: %w", err)} + } + } + // --intent defaults to train. Applied after the interactive block so + // the guided flow still asks "training or test?" (it prompts on an + // empty value); a non-interactive run that omits --intent gets train + // without erroring (RFC-0002 §5). The wire field stays "intent". + if a.Spec.Intent == "" { + a.Spec.Intent = "train" + } + if a.LocalPath == "" { + return nil, nil, nil, false, &exitError{code: exitLocalEnv, err: errors.New( + "local dataset path is required — pass it as an argument, or run " + + "on a terminal without --no-input for guided prompts")} + } + // Expand a leading ~ ourselves. The shell expands an unquoted ~ on + // the command line, but a path typed at the interactive prompt (or + // a quoted/literal ~ arg) reaches us unexpanded — and filepath.Abs + // would just prepend the CWD, yielding ".../cwd/~/...". Mirrors + // cluster.expandPath; done here so it covers both entry points + // before any push.Discover* call. (#37) + a.LocalPath = expandHome(a.LocalPath) + + // 0b. Path existence FIRST — before any spec / schema / family + // validation. A typo'd path should fail on the path with a plain + // "no such file or directory", not surface later as a confusing + // downstream error (e.g. the task gate asking which task the + // non-existent data is for). runInteractive runs this same guard + // before its family sniff / label preview, so the invariant holds on + // the guided route too; this re-check covers the flag-only path and + // is cheap (one stat). The family walk below stats again for its + // layout-specific diagnostics; this is only about ordering the first + // failure a customer sees. (#181) + if err := statDatasetPath(a.LocalPath); err != nil { + return nil, nil, nil, false, err + } + + // 1. Validate the table name BEFORE anything else. It's both + // the MySQL identifier and the /data/shared/
/ PVC + // subdirectory — an unsanitized traversal name (../../etc) + // would escape that subtree once the stage Pod writes to + // it. The embedded schema only checks minLength on `table`, + // so this CLI-side guard is the real fix. SpecArgs.Build() + // below calls StagedPrefix, which panics on an unsafe name — + // so this check MUST come first. + if err := push.ValidateTableName(a.Spec.Table); err != nil { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: err} + } + + // 2. Category gate. Runs BEFORE schema validation so an + // unsupported category gets a clear, actionable CLI message + // rather than the schema's terse enum / missing-property error. + // Every schema task category is CLI-supported now; this gate stays as + // defensive routing so a future known-but-not-yet-wired category gets a + // clear per-category message, and a typo'd category gets the supported + // list rather than the schema's raw enum dump. + switch { + case a.Spec.Category == "": + // No task chosen. In guided mode the picker already filled this; + // reaching here means a non-interactive run (or --no-input / + // --output-json) that omitted --task. Give a clear, actionable + // error instead of silently assuming images (the old default). + return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( + "which task is this data for? pass --task — one of: %s. "+ + "(On a terminal without --no-input, tracebloc asks you to pick.)", + push.SupportedCategoriesList())} + case push.IsCLISupported(a.Spec.Category): + // supported + case push.IsKnown(a.Spec.Category): + // A recognized category the CLI doesn't implement yet. None today — every + // schema category is wired — but kept as defensive routing so a future + // known-but-unsupported category gets the registry's per-category reason, + // not a misleading "unrecognized category". Supported categories were + // already caught above, so IsKnown here means known-but-unsupported. + spec, _ := push.Lookup(a.Spec.Category) + reason := "" + if spec.UnsupportedNote != "" { + reason = " (" + spec.UnsupportedNote + ")" + } + return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( + "task %q isn't supported by the CLI yet%s. Supported tasks: %s.", + a.Spec.Category, reason, push.SupportedCategoriesList())} + default: + return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( + "task %q isn't a recognized task. Supported tasks: %s.", + a.Spec.Category, push.SupportedCategoriesList())} + } + + // Image-only flags. --target-size / --min-size describe image + // resolution, so they're meaningless on a tabular / text task. + // Reject them explicitly here: without this guard they'd be parsed + // only inside the image branch below, so on a non-image task the + // value — even a malformed one — was silently dropped with no error. + if !push.IsImage(a.Spec.Category) { + for _, f := range []struct{ name, val string }{ + {"--target-size", a.TargetSizeFlag}, + {"--min-size", a.MinSizeFlag}, + } { + if f.val != "" { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( + "%s is image tasks only; it doesn't apply to task %q", + f.name, a.Spec.Category)} + } + } + } + + // Task-scoped flags. Like --target-size/--min-size above, each of these is + // read only inside the one category branch that consumes it, so passing one + // on a task that doesn't use it silently dropped the value — and the user's + // intent — with no error, even though the help text says each is scoped. + // Reject a misapplied flag explicitly so it fails fast instead of being + // ignored (the scope mirrors spec.go's build gates exactly). + if a.SchemaFlag != "" && !push.IsTabular(a.Spec.Category) { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( + "--schema is tabular/time-series tasks only; it doesn't apply to task %q", a.Spec.Category)} + } + if a.Spec.LabelPolicy != "" && !push.IsRegressionClass(a.Spec.Category) { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( + "--label-policy is regression-class tasks only (tabular_regression, "+ + "time_series_forecasting, time_to_event_prediction); it doesn't apply to task %q", + a.Spec.Category)} + } + if a.Spec.TimeColumn != "" && a.Spec.Category != "time_to_event_prediction" { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( + "--time-column is time_to_event_prediction only; it doesn't apply to task %q", a.Spec.Category)} + } + if a.Spec.NumberOfKeypoints != 0 && a.Spec.Category != "keypoint_detection" { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( + "--number-of-keypoints is keypoint_detection only; it doesn't apply to task %q", a.Spec.Category)} + } + // --label-column is meaningless for self-supervised text (the label is the + // text itself); buildText drops it, so accepting it silently discarded the + // user's value and the review echoed a column that never shipped. + if a.Spec.LabelColumn != "" && push.SelfSupervisedText(a.Spec.Category) { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( + "--label-column doesn't apply to task %q — it trains on the text itself, with no label column", + a.Spec.Category)} + } + + // 3. Walk the local directory FIRST (local "fail fast"), dispatched + // by category family. Image categories expect labels.csv + + // images/; tabular / time-series categories expect a single + // data CSV. The walk also yields what the per-category + // resolution below needs (the image list for target-size, the + // CSV for schema inference). + // err and layout are this function's named returns, so neither is + // redeclared here. The walk can take a moment on a large tree, so it + // gets a spinner — no blocking wait stays silent. + walkSpin := a.Printer.Spinner("Reading your files", "") + switch { + case push.IsTabular(a.Spec.Category): + layout, err = push.DiscoverTabular(a.LocalPath) + case push.IsText(a.Spec.Category): + layout, err = push.DiscoverText(a.Spec.Category, a.LocalPath) + case a.Spec.Category == "object_detection": + layout, err = push.DiscoverObjectDetection(a.LocalPath) + case a.Spec.Category == "semantic_segmentation": + layout, err = push.DiscoverSemanticSegmentation(a.LocalPath) + default: + // image_classification + keypoint_detection: labels.csv + images/. + layout, err = push.Discover(a.LocalPath) + } + walkSpin.Stop() + if err != nil { + return nil, nil, nil, false, &exitError{code: exitLocalEnv, err: err} + } + + a.Printer.Step(1, 3, "Check your data") + a.Printer.Hintf("Reading your files locally first — nothing has touched your workspace yet — so a layout or settings problem shows up right away.") + + // 3a. Per-category spec resolution from the local data, so the + // synthesized spec carries the right fields before validation. + switch { + case push.IsTabular(a.Spec.Category): + // P3 (cli#71): a BOM'd tabular CSV is doomed in-cluster AND would + // corrupt InferSchema's own header read below — reject before + // either. The rest of the content preflight runs after the spec + // schema validation (mirroring the in-cluster order). + if perr := push.CheckTabularBOM(layout.LabelsCSV); perr != nil { + return nil, nil, nil, false, &exitError{code: exitLocalEnv, err: perr} + } + if perr := push.CheckHasDataRows(layout.LabelsCSV); perr != nil { + return nil, nil, nil, false, &exitError{code: exitLocalEnv, err: perr} + } + + // Column schema. An explicit --schema wins (raw flag, or the + // optional override the interactive prompt captures into SchemaFlag). + // Otherwise infer the types here — mirroring the ingestor's own rules + // (di#349) — and EMIT the result explicitly (below, via a.Spec.Schema + // → spec.schema), so the ingestor uses the CLI's answer regardless of + // its own version. Inference runs on both a no-schema non-interactive + // run and an interactive run where the user left the schema prompt + // blank; the risky cases below are surfaced as warnings. + if a.SchemaFlag != "" { + sch, perr := push.ParseSchema(a.SchemaFlag) + if perr != nil { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: perr} + } + a.Spec.Schema = sch + } else { + res, ierr := push.InferSchema(layout.LabelsCSV) + if ierr != nil { + return nil, nil, nil, false, &exitError{code: exitLocalEnv, err: fmt.Errorf("inferring schema from CSV: %w", ierr)} + } + a.Spec.Schema = res.Schema + _, _ = fmt.Fprintf(out, + "Inferred schema for %d column(s) from %s (override with --schema).\n", + len(res.Schema), filepath.Base(layout.LabelsCSV)) + if len(res.Skipped) > 0 { + _, _ = fmt.Fprintf(out, + " (skipped framework-managed column(s): %s)\n", strings.Join(res.Skipped, ", ")) + } + if len(res.Empty) > 0 { + _, _ = fmt.Fprintf(out, + " (warning: %d column(s) had no values in the sample and were typed VARCHAR(1): %s)\n", + len(res.Empty), strings.Join(res.Empty, ", ")) + } + if len(res.IDLike) > 0 { + _, _ = fmt.Fprintf(out, + " (warning: %d column(s) look like identifiers (all-unique integers): %s — "+ + "if any is a zero-padded code, pass --schema to type it VARCHAR)\n", + len(res.IDLike), strings.Join(res.IDLike, ", ")) + } + } + case push.IsImage(a.Spec.Category): + // keypoint_detection needs --number-of-keypoints (dataset- + // specific, no default). Catch it here with an actionable + // message rather than letting the ingestor fail mid-run. Split + // UNSET from SET-BUT-INVALID (#76b): a Go 0 could mean either, so + // key on whether the flag was passed (ChangedFlags). Unset → the + // "requires" nudge; set to a non-positive value → name the bad + // value so the user sees exactly what was rejected. + if a.Spec.Category == "keypoint_detection" && a.Spec.NumberOfKeypoints <= 0 { + if a.ChangedFlags["number-of-keypoints"] { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: fmt.Errorf( + "--number-of-keypoints must be a positive integer (got %d); "+ + "it's the number of keypoints per sample (e.g. 17 for COCO pose)", + a.Spec.NumberOfKeypoints)} + } + return nil, nil, nil, false, &exitError{code: exitBadInput, err: errors.New( + "keypoint_detection requires --number-of-keypoints (e.g. " + + "--number-of-keypoints 17); it's dataset-specific and has no default")} + } + // Image target resolution: the ingestor's image_classification + // default is 512x512 and it VALIDATES (it does not resize), so + // a mismatch hard-fails. Honour an explicit --target-size; + // otherwise auto-detect from the first image so the common + // "all my images are NxN" case just works. + if a.TargetSizeFlag != "" { + w, h, perr := push.ParseTargetSize(a.TargetSizeFlag) + if perr != nil { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: perr} + } + a.Spec.TargetSize = []int{w, h} + } else if len(layout.Images) > 0 { + if w, h, derr := push.DetectImageSize(layout.Images[0]); derr == nil { + a.Spec.TargetSize = []int{w, h} + _, _ = fmt.Fprintf(out, + "Auto-detected image target size %dx%d from %s (override with --target-size).\n", + w, h, filepath.Base(layout.Images[0])) + } else { + _, _ = fmt.Fprintf(errOut, + "Note: couldn't auto-detect image size (%v); using the ingestor "+ + "default. Pass --target-size WxH if ingestion reports a "+ + "resolution mismatch.\n", derr) + } + } + // Minimum-size floor override (#348): plumb an explicit --min-size to + // spec.file_options.min_size. When unset, no spec field is emitted, so + // the ingestor applies its own default (none on the deployed + // v0.5.7/v0.6.0; 32x32 on develop post-#348) — and the local preview + // applies NO floor either (PreflightDataset only previews the floor + // when --min-size is set, so it never rejects an ingest the live + // cluster accepts). The below-floor reject is previewed in + // runLocalPreflight (ValidateImages). + if a.MinSizeFlag != "" { + w, h, perr := push.ParseMinSize(a.MinSizeFlag) + if perr != nil { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: perr} + } + a.Spec.MinSize = []int{w, h} + } + // Extension: every image must share one type, and the spec tells + // the cluster which one to validate against (file_options.extension). + // Without this the ingestor checked its .jpeg convention default and + // rejected .jpg/.png datasets AFTER the full upload (cli#68). + ext, exterr := push.DetectExtension(layout.Images) + if exterr != nil { + return nil, nil, nil, false, &exitError{code: exitLocalEnv, err: exterr} + } + a.Spec.Extension = ext + default: + // Text family: no extra per-category resolution. The supervised text + // tasks (text_classification, token_classification, + // sentence_pair_classification) carry a label straight from + // --label-column; the self-supervised ones (masked/causal language + // modeling, seq2seq, embeddings) need neither a label nor a schema. + // buildText emits the label for exactly the supervised set, keyed on + // the registry's SelfSupervised flag (not a hardcoded id). + } + + // 3b. Friendly missing-label pre-check (#214). Tabular / time-series tasks + // AND semantic_segmentation carry a required label column (the ingest + // schema's allOf requires `label` for them). With no --label-column the + // synthesized spec's `label` is an empty string, which trips the schema's + // label oneOf and the raw validation below dumps an opaque "got object, + // want string" / "minLength" pair. semseg is especially prone to this — + // its per-image label reads as vestigial beside the pixel masks, so the + // flag is easy to forget. Intercept ONLY that specific missing case here — + // a label present-but-not-in-the-CSV still flows to runLocalPreflight's + // CheckLabelColumn, and every other schema error still reaches the dump — + // and name the flag to fix instead. + if (push.IsTabular(a.Spec.Category) || a.Spec.Category == "semantic_segmentation") && a.Spec.LabelColumn == "" { + msg := "this task needs a label column, but --label-column wasn't set — " + + "pass --label-column with the name of the target column in your data CSV" + if cols := sortedKeys(a.Spec.Schema); len(cols) > 0 { + msg += " (columns: " + strings.Join(cols, ", ") + ")" + } + return nil, nil, nil, false, &exitError{code: exitBadInput, err: errors.New(msg)} + } + + // 4. Synthesize the spec from flags + validate against schema. + // Catches "bad category", "missing intent" etc. BEFORE we + // touch the cluster. The error formatter is the same one + // ingest validate uses, so a customer who YAML'd manually + // first sees identical wording. + spec = a.Spec.Build() + specBytes, err = yaml.Marshal(spec) + if err != nil { + return nil, nil, nil, false, &exitError{code: exitLocalEnv, err: fmt.Errorf("marshaling synthesized spec: %w", err)} + } + v, err := schema.NewV1Validator() + if err != nil { + return nil, nil, nil, false, &exitError{code: exitLocalEnv, err: fmt.Errorf("loading embedded schema: %w", err)} + } + _, errs, parseErr := v.ValidateYAML(specBytes) + if parseErr != nil { + // "Parse" failing on a spec we marshaled ourselves is a + // programming error, not a customer error — surface it + // with the bytes so we can diagnose. Exit 3 (the + // "internal" bucket) matches the marshal-failure branch + // above. + return nil, nil, nil, false, &exitError{code: exitLocalEnv, err: fmt.Errorf("internal: re-parsing synthesized spec: %w\n%s", parseErr, specBytes)} + } + if len(errs) > 0 { + // Use the SAME formatter `ingest validate` uses, so the + // experience is identical whether the customer authored + // YAML by hand or via flags. Diagnostics go to stderr + // (matching ingest validate) so a downstream pipe of + // stdout (e.g. piping the summary to jq once that's a + // JSON output mode) isn't polluted by error text. Exit 2 + // is reserved for schema violations across the CLI. + _, _ = fmt.Fprintf(errOut, "synthesized spec failed schema validation (%d issue%s):\n", + len(errs), plural(len(errs))) + _, _ = fmt.Fprintln(errOut, schema.FormatErrors(errs)) + return nil, nil, nil, false, &exitError{code: exitBadInput, err: errors.New("synthesized spec failed schema validation; check the flag values above")} + } + + // P3 content preflight (backend#828, cli#69/#71/#72/#73): preview the + // ingestor's own validators locally — AFTER the spec schema validation, + // mirroring the in-cluster order (jsonschema first, then validators), + // and BEFORE any cluster work. Each check names the rule it previews; + // parity is pinned by internal/push/parity_golden_test.go. + if perr := runLocalPreflight(*a, layout, errOut); perr != nil { + return nil, nil, nil, false, perr + } + + printLocalSummary(a.Printer, layout, spec) + + return layout, spec, specBytes, false, nil +} + // printLocalSummary shows what the CLI found on disk plus the ingest // settings it assembled — the detail under step 1 ("Check your data"). // Mirrors `cluster info`'s section/Field layout. @@ -121,9 +554,9 @@ func runLocalPreflight(a runDataIngestArgs, layout *push.LocalLayout, errOut io. if problem == nil { return nil } - code := 3 + code := exitLocalEnv if problem.BadFlag { - code = 2 + code = exitBadInput } return &exitError{code: code, err: problem.Err} } diff --git a/internal/cli/data_ingest_output.go b/internal/cli/data_ingest_output.go index 03c0a234..6898f10d 100644 --- a/internal/cli/data_ingest_output.go +++ b/internal/cli/data_ingest_output.go @@ -25,14 +25,14 @@ func classifyPushOutcome(res *submit.Result, err error) (string, *exitError) { if err != nil { switch { case submit.IsAuthError(err): - return "auth_error", &exitError{code: 5, err: err} + return "auth_error", &exitError{code: exitAuth, err: err} case submit.IsWatchError(err): // jobs-manager accepted the run; the cluster is doing the // work, the CLI just couldn't follow along — ingest-side // (exit 9), not submit-side (8). - return "watch_error", &exitError{code: 9, err: err} + return "watch_error", &exitError{code: exitIngestFailed, err: err} default: - return "submit_error", &exitError{code: 8, err: err} + return "submit_error", &exitError{code: exitSubmitFailed, err: err} } } // --detach (no watch) or SIGINT-mid-watch: success; cluster runs on. @@ -41,16 +41,16 @@ func classifyPushOutcome(res *submit.Result, err error) (string, *exitError) { } switch res.Watch.Outcome { case submit.JobOutcomeFailed: - return "failed", &exitError{code: 9, err: errors.New("ingestion Job exited non-zero — see logs above")} + return "failed", &exitError{code: exitIngestFailed, err: errors.New("ingestion Job exited non-zero — see logs above")} case submit.JobOutcomeUnknown: - return "unknown", &exitError{code: 9, err: errors.New( + return "unknown", &exitError{code: exitIngestFailed, err: errors.New( "ingestion Job's final status couldn't be determined within the watch window — " + "check `kubectl get job -n " + res.Submit.Namespace + " " + res.Submit.JobName + "` for the outcome")} case submit.JobOutcomeSucceeded: // Job exited 0, but rows can still have failed — exit 9, and the // JSON status must say so, NOT "succeeded". (Bugbot #38.) if res.Watch.Summary != nil && res.Watch.Summary.HasFailures() { - return "completed_with_failures", &exitError{code: 9, err: errors.New( + return "completed_with_failures", &exitError{code: exitIngestFailed, err: errors.New( "ingestion Job completed but the summary reports failures — see panel above")} } return "succeeded", nil diff --git a/internal/cli/data_list.go b/internal/cli/data_list.go index 549db436..7376c505 100644 --- a/internal/cli/data_list.go +++ b/internal/cli/data_list.go @@ -117,7 +117,7 @@ func runDataList(ctx context.Context, a runDataListArgs) (err error) { tables, err := listDatasetsFn(ctx, cs, resolved.RestConfig, resolved.Namespace) if err != nil { - return &exitError{code: 7, err: err} + return &exitError{code: exitQueryFailed, err: err} } if a.OutputJSON { diff --git a/internal/cli/delete.go b/internal/cli/delete.go index d6cc1001..1e112f98 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -102,16 +102,16 @@ and are erased. Not undoable.`, func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) error { client, cfg, err := authedClient() if err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } prof := cfg.Current() if prof.ActiveClientID == "" { - return &exitError{code: 1, err: errors.New( + return &exitError{code: exitFailure, err: errors.New( "no active client on this machine — nothing to offboard")} } id, cerr := strconv.Atoi(prof.ActiveClientID) if cerr != nil { - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "stored active client id %q is not numeric: %w", prof.ActiveClientID, cerr)} } name := prof.ActiveClientName @@ -141,7 +141,7 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // than imply the guard was merely skipped. var ue *api.UpgradeRequiredError if errors.As(lerr, &ue) { - return &exitError{code: 1, err: lerr} + return &exitError{code: exitFailure, err: lerr} } // A 401/403 means the signed-in credential is revoked/expired — it won't // recover by continuing either, and every later step (the revoke included) @@ -150,7 +150,7 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // poll loop's auth handling in client.go. var ae *api.APIError if errors.As(lerr, &ae) && (ae.StatusCode == http.StatusUnauthorized || ae.StatusCode == http.StatusForbidden) { - return &exitError{code: 1, err: errors.New( + return &exitError{code: exitFailure, err: errors.New( "tracebloc rejected your credentials — run `tracebloc login`, then retry `tracebloc delete`")} } // Other errors (5xx/429/network) are transient: warn and continue, since @@ -162,7 +162,7 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // then continue (the revoke below will 403/404 if it isn't really ours). p.Hintf("This client isn't in the signed-in account's client list — continuing; if that's unexpected, check you're logged into the right account/env.") } else if st == clientStatusOnline { - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "client %q is still online (tracebloc reports it running) — stop its training jobs first, "+ "or pass --force to offboard anyway", name)} } @@ -173,7 +173,7 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // for automation. if !o.yes { if pr == nil { - return &exitError{code: 1, err: errors.New( + return &exitError{code: exitFailure, err: errors.New( "refusing to offboard without confirmation: pass --yes, or run on a terminal to type the client name")} } p.Newline() @@ -202,7 +202,7 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // Mirrors the pre-offboard guard above (which treats 426 as terminal). var ue *api.UpgradeRequiredError if errors.As(rerr, &ue) { - return &exitError{code: 1, err: rerr} + return &exitError{code: exitFailure, err: rerr} } var ae *api.APIError if errors.As(rerr, &ae) { @@ -216,7 +216,7 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er // the online-guard above is skipped, so DON'T silently tear the machine // down while a live credential remains — fail fast and point at sign-in, // mirroring the pre-offboard guard's 401 handling. - return &exitError{code: 1, err: errors.New( + return &exitError{code: exitFailure, err: errors.New( "tracebloc rejected your credentials — run `tracebloc login`, then retry `tracebloc delete`")} } } diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index c56da047..4504fa09 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -159,7 +159,7 @@ func runClusterDoctor( p.Hintf("For deeper triage, send tracebloc a support bundle: ./install-k8s.sh --diagnose") // Silent (err == nil): the per-check lines above already explained it, // so main() shouldn't print a redundant "Error:" line. - return &exitError{code: 2, err: nil} + return &exitError{code: exitChecksFailed, err: nil} case doctor.StatusWarn: p.Warnf("Completed with warnings — review the ⚠ items above.") return nil @@ -252,7 +252,7 @@ func worseStatus(a, b doctor.Status) doctor.Status { // token isn't masked behind a kubeconfig-only exit code (Bugbot). func kubeconfigExitCode(authStatus doctor.Status) int { if authStatus == doctor.StatusFail { - return 2 + return exitChecksFailed } - return 3 + return exitLocalEnv } diff --git a/internal/cli/exit.go b/internal/cli/exit.go index bdbe10aa..2b6dd2aa 100644 --- a/internal/cli/exit.go +++ b/internal/cli/exit.go @@ -9,13 +9,13 @@ package cli // handlers go through the constructor. func ExitCodeFromError(err error) int { if err == nil { - return 0 + return exitOK } var ee *exitError if asExitError(err, &ee) { return ee.code } - return 1 + return exitFailure } // IsSilentError reports whether a handler-returned error wants diff --git a/internal/cli/exitcodes.go b/internal/cli/exitcodes.go new file mode 100644 index 00000000..0ac39fd1 --- /dev/null +++ b/internal/cli/exitcodes.go @@ -0,0 +1,89 @@ +// Named constants for every exit code the CLI produces. exit.go owns the +// extraction (ExitCodeFromError); this file owns the naming, so a reviewer +// reading `&exitError{code: exitTableExists, …}` never has to hold the +// number table in their head. +package cli + +// Exit codes are the CLI's scripting contract: customers branch on them. +// docs/troubleshooting.md carries the cross-command table; `tracebloc data +// ingest --help` documents the fullest per-command list. Every non-test +// &exitError construction site names its code with one of these constants. +// The numeric values are FROZEN — changing one breaks customer scripts; add +// a new code instead of repurposing an old one. +// +// A few numbers carry more than one per-command meaning (they grew +// per-command before this file centralized the names). Those get one +// constant per MEANING sharing the value, so each construction site stays +// honest and the docs table maps number → per-command meaning. +const ( + // exitOK: success. Includes --dry-run completing, a guided run the + // user cancelled cleanly, and doctor passing with warnings only. + exitOK = 0 + + // exitFailure: generic failure with no more specific bucket (cobra + // usage errors; auth / client / delete command failures). Also what + // ExitCodeFromError maps any non-exitError error to. + exitFailure = 1 + + // exitBadInput: the input didn't validate — a schema violation in a + // spec (synthesized from flags or authored as YAML), an unsupported or + // unknown --task, a misapplied task-scoped flag, an invalid table + // name, or a resource size that doesn't fit this machine. + exitBadInput = 2 + + // exitChecksFailed: doctor only — one or more checks failed. Shares 2 + // with exitBadInput (both are "the CLI examined it; it isn't right"). + exitChecksFailed = 2 + + // exitLocalEnv: the local environment refused — kubeconfig couldn't be + // loaded, the dataset path is missing/unreadable, the local layout is + // wrong, a YAML file didn't parse, or a prompt was needed off a + // terminal (--no-input / --output-json). + exitLocalEnv = 3 + + // exitNoWorkspace: the cluster is reachable but no tracebloc client + // (parent release) was found in the namespace — or its shared storage + // or dataset list is missing, so the target can't be confirmed. + exitNoWorkspace = 4 + + // exitAuth: an ingestor SA token couldn't be minted, or jobs-manager + // rejected it (401/403). + exitAuth = 5 + + // exitNoSuchDataset: data delete only — no dataset by that name on + // this client (nothing to delete). Shares 5 with exitAuth; the + // per-command meanings predate this file and are frozen with it. + exitNoSuchDataset = 5 + + // exitTableExists: the destination table already exists — re-run with + // --overwrite to replace it, or pick a different --name. + exitTableExists = 6 + + // The exit-7 trio: an in-cluster operation failed midway. One value, + // three meanings, named per site: + // + // exitStagingFailed: pre-flight succeeded but staging the files into + // the workspace failed (Pod creation, image pull, exec stream, or + // remote tar error). + exitStagingFailed = 7 + // exitTeardownFailed: removing an existing table + its files failed + // partway (data delete, or the teardown data ingest --overwrite runs). + exitTeardownFailed = 7 + // exitQueryFailed: data list only — the cluster couldn't be queried + // for its datasets. + exitQueryFailed = 7 + + // exitSubmitFailed: jobs-manager rejected the submitted run (a + // non-auth 4xx/5xx), or the port-forward to it couldn't be set up. + exitSubmitFailed = 8 + + // exitIngestFailed: the ingestion Job exited non-zero, completed with + // row-level failures the summary panel reports, or its outcome + // couldn't be determined / followed within the watch window. + exitIngestFailed = 9 + + // exitInterrupted: the user hit Ctrl-C at an interactive prompt + // (128+SIGINT, the shell convention). Emitted silent (err == nil) so + // main() prints no "Error:" line on the way out. + exitInterrupted = 130 +) diff --git a/internal/cli/ingest.go b/internal/cli/ingest.go index b40bb58b..748b827f 100644 --- a/internal/cli/ingest.go +++ b/internal/cli/ingest.go @@ -81,7 +81,7 @@ func runIngestValidate(cmd *cobra.Command, args []string) error { // exit-coded error so cobra propagates the right code via // the main()-side os.Exit mapping (in a follow-up commit // we'll wire main.go to inspect for these). - return &exitError{code: 3, err: fmt.Errorf("reading %s: %w", path, err)} + return &exitError{code: exitLocalEnv, err: fmt.Errorf("reading %s: %w", path, err)} } v, err := schema.NewV1Validator() @@ -90,12 +90,12 @@ func runIngestValidate(cmd *cobra.Command, args []string) error { // customer-side — we bundle the schema, so this only fires // if the build is broken. Treat as exit-code-2 so CI can // distinguish from a customer file problem. - return &exitError{code: 2, err: fmt.Errorf("loading embedded schema: %w", err)} + return &exitError{code: exitBadInput, err: fmt.Errorf("loading embedded schema: %w", err)} } doc, violations, parseErr := v.ValidateYAML(body) if parseErr != nil { - return &exitError{code: 3, err: fmt.Errorf("%s: %w", path, parseErr)} + return &exitError{code: exitLocalEnv, err: fmt.Errorf("%s: %w", path, parseErr)} } // The jsonschema types every `schema` value as a bare string, so a bogus @@ -126,7 +126,7 @@ func runIngestValidate(cmd *cobra.Command, args []string) error { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "%s: schema validation failed (%d issue%s)\n", path, len(violations), plural(len(violations))) _, _ = fmt.Fprintln(cmd.ErrOrStderr(), schema.FormatErrors(violations)) - return &exitError{code: 2, err: nil} // err==nil so cobra doesn't print "Error: ..." on top + return &exitError{code: exitBadInput, err: nil} // err==nil so cobra doesn't print "Error: ..." on top } // schemaTypeViolations previews the ingestor's accepted-SQL-type check over a diff --git a/internal/cli/resources_set.go b/internal/cli/resources_set.go index 6e41be92..5772888a 100644 --- a/internal/cli/resources_set.go +++ b/internal/cli/resources_set.go @@ -179,11 +179,11 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * // sum across nodes — mirrors `cluster doctor`'s node-fit. nodes, nerr := target.Clientset.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) if nerr != nil { - return &exitError{code: 3, err: fmt.Errorf("couldn't read this machine's capacity: %w", nerr)} + return &exitError{code: exitLocalEnv, err: fmt.Errorf("couldn't read this machine's capacity: %w", nerr)} } node, ok := resources.LargestReadyNode(nodes.Items) if !ok { - return &exitError{code: 3, err: fmt.Errorf("no Ready node on this machine to size a training run against")} + return &exitError{code: exitLocalEnv, err: fmt.Errorf("no Ready node on this machine to size a training run against")} } machineGPUName, machineGPUCount, machineHasGPU := resources.MachineGPU(node) @@ -263,7 +263,7 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * // wizard paths; --dry-run mutates nothing so it never needs confirming. if !req.yes && !req.dryRun { if pr == nil { - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "refusing to change the ceiling without confirmation: pass --yes, or run on a terminal")} } p.Newline() @@ -403,7 +403,7 @@ func runResourcesWizard(p *ui.Printer, pr prompter, node resources.Machine, curr // (e.g. "1–0"): every answer is rejected and the wizard can't complete // except by interrupting. Fail honestly instead (Bugbot #241). if maxCores < 1 || maxGiB < 2 { - return resources.Training{}, &exitError{code: 2, err: fmt.Errorf( + return resources.Training{}, &exitError{code: exitBadInput, err: fmt.Errorf( "this machine is too small to choose an amount — after tracebloc's ~1 core and 3 GiB "+ "overhead it can offer a training run at most %d core(s) and %d GiB. Free up "+ "resources or use a larger machine.", maxCores, maxGiB)} @@ -510,7 +510,7 @@ func persistCeiling(ctx context.Context, p *ui.Printer, target *clusterTarget, o // override uses a local chart (no remote pull, no version to pin), so it's // exempt — mirroring the same exemption in helm.Upgrade. if chartPathOverride() == "" && strings.TrimSpace(target.Release.ChartVersion) == "" { - return &exitError{code: 1, err: fmt.Errorf( + return &exitError{code: exitFailure, err: fmt.Errorf( "couldn't determine the installed client chart version (the release is missing " + "its helm.sh/chart version label), so the upgrade can't be pinned to it. Refusing " + "to change resources with an unpinned upgrade — it would pull the latest chart and " + @@ -531,7 +531,7 @@ func persistCeiling(ctx context.Context, p *ui.Printer, target *clusterTarget, o } plan, err := helm.Upgrade(ctx, params) if err != nil { - return &exitError{code: 1, err: err} + return &exitError{code: exitFailure, err: err} } if dryRun { @@ -568,7 +568,7 @@ func persistCeiling(ctx context.Context, p *ui.Printer, target *clusterTarget, o // err is non-nil so it isn't silent). Every user-facing "that won't work" path // funnels through here so the exit code stays consistent. func validationError(msg string) error { - return &exitError{code: 2, err: fmt.Errorf("%s", msg)} + return &exitError{code: exitBadInput, err: fmt.Errorf("%s", msg)} } // perRunSize renders a ceiling the way the user reads it: "4 CPU · 16 GiB · 1 GPU". diff --git a/internal/cli/root.go b/internal/cli/root.go index c2c4f418..7954cc35 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -168,7 +168,7 @@ func runGroup(cmd *cobra.Command, args []string) error { } } msg += fmt.Sprintf("\n\nRun '%s --help' for the available commands.", cmd.CommandPath()) - return &exitError{code: 1, err: errors.New(msg)} + return &exitError{code: exitFailure, err: errors.New(msg)} } // printerFor builds a ui.Printer for a command's stdout, honoring the diff --git a/scripts/file-budget.sh b/scripts/file-budget.sh new file mode 100755 index 00000000..290979c4 --- /dev/null +++ b/scripts/file-budget.sh @@ -0,0 +1,59 @@ +#!/usr/bin/env bash +# Fail if a budgeted file grows past its line ceiling. +# +# data.go reached 1539 lines before the WS-B split (backend#1106); a +# 1000+-line file is where the next one quietly grows. This gate makes the +# growth loud at PR time instead of in the next audit. It's a shell ratchet +# rather than a golangci funlen/lll rule per the #6 standalone-tools +# decision (same reasoning as scripts/coverage-floor.sh, whose shape this +# clones). +# +# The ceilings are a RATCHET: set just above the current numbers, and only +# ever lowered as files shrink — never silently raised. Raising one must be +# a deliberate, reviewed edit to this checked-in file (with a reason), not +# a side effect of a big PR. Current (develop, post cli#282/#283 split): +# preflight.go ~1655, client.go ~1027, home.go ~849, data.go ~240. +# +# Usage: scripts/file-budget.sh (run from the repo root) +# +# Portable to bash 3.2 (macOS default): no associative arrays. +set -euo pipefail + +# "path:max_lines" entries. Keep ceilings integers. +BUDGETS=" +internal/push/preflight.go:1700 +internal/cli/data.go:500 +internal/cli/client.go:1050 +internal/cli/home.go:850 +" + +status=0 +for entry in $BUDGETS; do + path="${entry%%:*}" + max="${entry##*:}" + # A malformed entry (no ":max", or a non-integer ceiling) must fail + # loudly, not slip through as a silent no-op for that file — same guard, + # same reason as coverage-floor.sh. + if [ "$path" = "$entry" ] || ! printf '%s' "$max" | grep -qE '^[0-9]+$'; then + echo "::error::malformed BUDGETS entry '$entry' (want 'path:INT') — fix scripts/file-budget.sh" >&2 + status=1 + continue + fi + if [ ! -f "$path" ]; then + # A budgeted file that vanished (moved/renamed) means this list is + # stale — update it in the same PR, as a reviewed diff, so the budget + # follows the file instead of evaporating. + echo "::error::budgeted file '$path' not found — update scripts/file-budget.sh in the same PR" >&2 + status=1 + continue + fi + lines="$(wc -l < "$path" | tr -d '[:space:]')" + if [ "$lines" -gt "$max" ]; then + echo "::error::$path is $lines lines, over its $max-line budget — split it (cli#282 is the pattern), or (with a reason) raise the ceiling in scripts/file-budget.sh" >&2 + status=1 + else + echo "ok: $path $lines <= $max" + fi +done + +exit "$status"