From c16af41f84423a7936b272ae068b8d5e87dd7fd7 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Sat, 15 Aug 2026 07:11:32 +0200 Subject: [PATCH 1/3] =?UTF-8?q?feat(ingest):=20guided=20mode=20always=20as?= =?UTF-8?q?ks,=20pre-filled=20=E2=80=94=20never=20skips=20a=20question=20(?= =?UTF-8?q?#505)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(ingest): guided mode always asks, pre-filled — never skips a question A user hit "no such file or directory: data" after answering two questions of the guided flow. Not a defect in the check: they had passed `data` as the positional path, and step 3 is guarded by `if a.LocalPath == ""`, so the flow skipped "Where is your data?" and went straight to validating a path that was never asked about. Every step was guarded that way — intent, name, path, task, and the per-task extras. Skipping is defensible for a name or a task: what you passed is still true on the next run. It is not defensible for a PATH, which is the one answer that silently stops being true when data moves. The result was a dead end: the user sitting at a prompt, ready to answer, getting a hard error instead of the question that would have fixed it in two seconds. Rather than special-case the path, the rule is now uniform and simple: In guided mode, every question relevant to the chosen task is asked. A value that arrived on the command line becomes that question's DEFAULT — Enter accepts it — never a reason to skip. In non-interactive mode (--no-input, no TTY, --output-json) nothing is ever asked and flags/positionals are obeyed exactly as before. The task gate survives, one level down: which questions apply still depends on the task chosen at step 4 (self-supervised text has no label and no extras). That gate is about relevance, not about what the user typed. Details worth knowing: - An explicit --task now settles the FAMILY directly instead of skipping the picker. Scoping the list to the sniffed family instead would let the two disagree — a tabular task against a folder that sniffs image — and leave the user's own answer missing from its own default. - pickTask only honours a pre-selection that is actually in the family's list; survey would otherwise render a default the user cannot see. - The review + confirm is now unconditional. It was gated on "did we prompt anything", which can only ever be true now; keeping it would be a condition that reads as a choice while having none. - TaskSet is removed rather than left threaded through unused. Three defaults ("train", "bucket", "time") moved from inline arguments into variables so a supplied value can take their place, which drops them from the zz-all-strings index. Replaced with something stronger: tests that assert the fallbacks behaviourally. The two tests that specified the old rule are inverted rather than deleted — they were the specification, so they now specify the new one. Refs #711 Co-Authored-By: Claude Opus 5 * fix(ingest): guard supplied Select defaults + always ask the label column Guided ingest pre-fills survey.Select prompts with command-line values, but survey aborts when a Default is not one of the Options ("default value ... not found in options"). A typo'd --intent or --label-policy therefore crashed the prompt on a real TTY, while the fake prompter ignored Default so the tests stayed green. Add a shared defaultInOptions guard — the check pickTask already applied — and route intent, label-policy and the label-column pick through it; an unknown supplied value now falls back to the sensible default and the question is still asked. pickTask is unified onto the same helper. Also stop the label-column step from skipping when --label-column was supplied: like every other value under #711 it now pre-fills the header-backed picker instead of bypassing it, so a wrong or mistyped column can be corrected at the prompt the same way a stale path can. Make fakePrompter.Select honour survey's default-in-options contract so the crash class can no longer pass in tests, and add mutation-proven coverage. Fixes two Cursor Bugbot findings on #505. Co-Authored-By: Claude Opus 4.8 * fix(ingest): a task change in guided mode drops the old task's flags Bugbot, and it is the guided flow contradicting its own promise. `pickTask` wrote the chosen task and left every task-scoped value from the ORIGINAL --task on the spec. Start a run as tracebloc data ingest ./d --task time_to_event_prediction --time-column t pick tabular_classification at the prompt, and TimeColumn rides through: no prompt asks about it (it is time_to_event_prediction-only), Review shows it anyway, and the run then dies AFTER the confirm with exitBadInput blaming a flag the user just spent a prompt walking away from. #711's whole claim is that the answers on screen are the run; a value no question asked about and no answer can reach is not one of them. Same for --label-policy, --number-of-keypoints, --target-size and --min-size. The reset is one call after the picker. What it needed was a place to read the scopes from, and there wasn't one — the misapplied-flag guard held them as five inline conditions plus a two-element loop. A second hand-written copy in the guided flow is the failure mode this codebase keeps finding: both copies pass their own tests while disagreeing with each other, and the disagreement surfaces as an error message about a flag that no longer applies. So the scopes move to task_scope.go as one table, and both callers read it: rejectMisappliedTaskValues (unchanged behaviour) and dropOutOfScopeTaskValues (new). Adding a task-scoped flag is now one row, and it cannot be added to the guard while being forgotten in the reset. Each message is written out whole rather than composed from a flag name and a fragment, so zz-all-strings.golden shows a reviewer the exact sentence a user sees. That is the only golden change: "%s is image tasks only…" becomes the two concrete --target-size / --min-size lines. Tests: the four values are gone after a task change and the label column just answered survives; nothing is cleared when the task is UNCHANGED (a blanket wipe passes the first test and silently discards flags the user meant); every row rejects out-of-scope and names its own flag, with the counterexample task taken from push.SupportedCategoryIDs() rather than hand-picked; and for every supported task, clearing satisfies the guard — which is what makes the shared predicate worth sharing. Mutation-proved both ways: dropping the reset call reddens with all five values surviving; clearing unconditionally reddens the unchanged-task test. Co-Authored-By: Claude Opus 5 * fix(ingest): scope the reset to the task CHANGE, not to the picked task Bugbot, on my own previous commit, and it is the more interesting half of the bug. `dropOutOfScopeTaskValues` cleared everything the PICKED task doesn't use — which also cleared a flag that was misapplied on the command line, before any picking happened. So tracebloc data ingest ./d --task tabular_classification --time-column t then Enter on the pre-selected task, silently ignored --time-column and continued, while the identical invocation under --no-input exits 2. Guided mode quietly meaning something different from the flags it echoes is worse than the stale-value bug it replaced: the first version lost an answer the user could see, this one loses one they cannot. "Out of scope for the new task" and "left behind by the change" are different sets, and only the second is the reset's business. A flag nobody walked away from is the guard's to reject. So the reset now takes the task the user ARRIVED with and clears only what was in scope for that and is not in scope for what they chose. A run with no --task supplied is not a change either — there was no task to move away from — so every value stands or falls on the guard. My comment claiming an unchanged task clears nothing was false in exactly this case, which is the kind of comment worth deleting rather than correcting: it described the intent while the code did something else. Tests: the misapplied flag survives the guided flow and is rejected, both with --task supplied and without. The stale-value tests now start from a LEGAL state — every value in scope for the supplied task — because starting from an illegal one tests the guard, not the reset; a second case covers the image family, where the dropped value is a number and two neighbouring flags must survive. TestClearingAlwaysSatisfiesTheGuard is replaced by the stronger property it was reaching for: from any legal starting state, a change to ANY other supported task leaves a state the guard accepts — both sides built from the table, so a scope widened later is exercised without editing the test. Mutation-proved both ways: clearing everything out of scope reddens the two misapplied-flag tests; removing the reset reddens the two stale-value ones. Co-Authored-By: Claude Opus 5 * test(ingest): pin the both-invalid edge Shujaat named on review He asked what happens when the user changes task A -> B and the flag was invalid for both, and offered the weaker rule (`!inScope(to) && !inScope(from)` as the DROP condition) as an option. The stricter answer is already what the code does, and it is the right one: the flag was wrong when they typed it, and no answer they gave walks away from it, so it must be rejected exactly as --no-input rejects it. A re-pick is not blanket permission to drop. `v.inScope(from)` in the clear condition is what makes that true — but nothing tested it, so it was true by accident. Mutation-proved: relaxing to "clear anything out of scope for the picked task" reddens this and nothing else, which is precisely the case that was untested. Co-Authored-By: Claude Opus 5 * fix(ingest): a typo'd --task is not a task the user walked away from Bugbot, and it is right — this is the third distinct way the same reset has swallowed a value, and the worst of them, because it loses TWO. Every `inScope` predicate answers from the registry, so an unknown id lands on the DEFAULT side of each one: `!SelfSupervisedText("tabular_clasification")` is true because the lookup misses, not because that task uses a label column. `dropValuesLeftBehindByATaskChange` treated any non-empty `from` as a real prior task, so tracebloc data ingest ./d --task tabular_clasification --label-column churned then picking masked_language_modeling read --label-column as "in scope before, out of scope now" and cleared it — leaving rejectMisappliedTaskValues nothing to object to. The typo is lost in the same breath, and that half is worth stating: the guided flow runs BEFORE the category gate (data_ingest_local.go:103 vs :165), and the picker has already overwritten Category with a valid id by then, so the gate never sees the typo either. The run proceeds as though the user typed neither flag. Under --no-input the identical command line exits 2 on the unrecognized task. Guided mode quietly meaning something other than the flags it echoes is the exact failure this helper was added to prevent. An unknown `from` is therefore the same case as no --task at all: nothing was walked away from, so clear nothing and let the guard speak. `push.IsKnown`, not `IsCLISupported` — a known-but-unsupported task is still a task the user declared, and the gate has its own message for that. Mutation-proved: dropping the `IsKnown` guard reddens TestGuided_ATypoedTaskIsNotATaskChange with "the reset cleared it on behalf of a task that does not exist", and nothing else. Anchor asserted rather than assumed. gofmt/go vet clean, full package green. Co-Authored-By: Claude Opus 5 * fix(lint): the unknown task in the new test was a real misspelling `misspell` is a golangci-lint gate here, and "tabular_clasification" is a word it knows how to correct — 4 hits across the helper's comment and the test, so my own previous commit went red on a gate the fix had nothing to do with. The test needs a `--task` the registry does not recognize; it does not need a MISSPELLED one. "tabular_classifier" is a plausible wrong name a user would actually type, spelled correctly, and exercises the identical path (`push.IsKnown` false → every `inScope` defaults to true). Renamed to `TestGuided_AnUnrecognizedTaskIsNotATaskChange` so the name describes what is being tested rather than one way of arriving at it, and the comment now says the spelling is deliberate so nobody reintroduces a typo for flavour. Re-proved after the rename: dropping the `IsKnown` guard still reddens it with "the reset cleared it on behalf of a task that does not exist". gofmt clean, `misspell -error ./internal` clean, package green. Co-Authored-By: Claude Opus 5 * docs: point the guided-flow references at cli#509, the real issue The comments and commit trail referenced #711, which does not exist in this repo — I carried the number over from an unrelated client PR, and the later review commits propagated it into three more comments. Filed cli#509 with the actual defect (a supplied path skips its question, so a dataset that moved dead-ends on a path the user was never asked for) and corrected every reference. No behaviour change. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- internal/cli/copy_catalog_test.go | 4 +- internal/cli/data_ingest_cmd.go | 14 +- internal/cli/data_ingest_local.go | 63 +--- internal/cli/interactive.go | 214 ++++++++---- internal/cli/interactive_test.go | 308 ++++++++++++++++-- internal/cli/task_scope.go | 184 +++++++++++ internal/cli/task_scope_test.go | 290 +++++++++++++++++ .../cli/testdata/golden/01-data-ingest.golden | 3 +- .../cli/testdata/golden/zz-all-strings.golden | 6 +- 9 files changed, 928 insertions(+), 158 deletions(-) create mode 100644 internal/cli/task_scope.go create mode 100644 internal/cli/task_scope_test.go diff --git a/internal/cli/copy_catalog_test.go b/internal/cli/copy_catalog_test.go index c4d5959..5826d4e 100644 --- a/internal/cli/copy_catalog_test.go +++ b/internal/cli/copy_catalog_test.go @@ -131,7 +131,7 @@ func TestCopyCatalog(t *testing.T) { p.Hintf("For help: https://docs.tracebloc.io/create-use-case/prepare-dataset") pr := &catalogPrompter{w: &b, answers: answers} a := &runDataIngestArgs{} - if err := runInteractive(p, pr, a, false /*taskSet*/); err != nil { + if err := runInteractive(p, pr, a); err != nil { t.Fatalf("driveIngest(%s): %v", dir, err) } return strings.ReplaceAll(b.String(), dir, shownPath) @@ -211,7 +211,7 @@ func TestCopyCatalog(t *testing.T) { } dataIngestFile := doc( "tb data ingest — stage a dataset into your secure environment", - "What you see when you run `tb data ingest` with no flags: a short intro, a\nfour-step guided setup (intent, name, path, task) then the task-specific\nquestions, and — after you confirm — the run itself. The setup is\ndriven through the real flow for one task in each family (tabular, image, text)\nso the task-specific questions are visible; each core question prints as a\n`Step N of 4 · …` header, the task-specific ones (the label column, and extras\nlike resolution or schema) as their own header, the\nsupporting line beneath it, and the `?` line shows your answer. The run (shown\nonce, for tabular) is the three steps + the final summary as the CLI renders\nthem. Passing flags (--as, --task, a path, …) skips the matching questions. The\nother tasks' extra questions (keypoints, label policy, time column),\nself-supervised text (which skips the label question), and the failure-summary\nwordings are in zz-all-strings.golden. The raw ingestor stream the CLI streams\nthrough (MySQL waits, the 📊 banner, per-validator lines) is the engine's own\nstdout — not CLI copy — so it isn't shown. (`tb ingest` is a hidden deprecated\nalias; `push` is a deprecated alias of the verb.)", + "What you see when you run `tb data ingest` with no flags: a short intro, a\nfour-step guided setup (intent, name, path, task) then the task-specific\nquestions, and — after you confirm — the run itself. The setup is\ndriven through the real flow for one task in each family (tabular, image, text)\nso the task-specific questions are visible; each core question prints as a\n`Step N of 4 · …` header, the task-specific ones (the label column, and extras\nlike resolution or schema) as their own header, the\nsupporting line beneath it, and the `?` line shows your answer. The run (shown\nonce, for tabular) is the three steps + the final summary as the CLI renders\nthem. Values passed as flags (--as, --task, a path, …) pre-fill the matching\nquestions rather than skipping them — guided mode always asks. The\nother tasks' extra questions (keypoints, label policy, time column),\nself-supervised text (which skips the label question), and the failure-summary\nwordings are in zz-all-strings.golden. The raw ingestor stream the CLI streams\nthrough (MySQL waits, the 📊 banner, per-validator lines) is the engine's own\nstdout — not CLI copy — so it isn't shown. (`tb ingest` is a hidden deprecated\nalias; `push` is a deprecated alias of the verb.)", []run{ {"tb data ingest # guided · tabular classification", tabularIngest}, {"tb data ingest # guided · image classification", imageIngest}, diff --git a/internal/cli/data_ingest_cmd.go b/internal/cli/data_ingest_cmd.go index 0f3d854..b1ab3f2 100644 --- a/internal/cli/data_ingest_cmd.go +++ b/internal/cli/data_ingest_cmd.go @@ -187,7 +187,6 @@ Exit codes: // Dropping --task's old image_classification default means an // unset task now drives the picker (TTY) or a clear error // (non-interactive), never a silent image assumption. - taskSet := cmd.Flags().Changed("task") || cmd.Flags().Changed("category") // Record whether --number-of-keypoints was explicitly passed, so // the keypoint set-vs-unset message (#76b) can distinguish an // explicit zero value from an unset flag (both look like the Go @@ -240,7 +239,6 @@ Exit codes: Printer: printer, Interactive: interactive, Prompter: pr, - TaskSet: taskSet, ChangedFlags: changedFlags, OutputJSON: outputJSON, JSONOut: jsonOut, @@ -337,15 +335,13 @@ type runDataIngestArgs struct { // the RunE from the persistent --plain flag (see printerFor). Printer *ui.Printer - // Interactive guided mode (#28). When Interactive is true, - // runDataIngest prompts (via Prompter) for any missing core inputs - // before validation. TaskSet records whether the task was passed - // explicitly (via --task or the hidden --category alias); an unset - // task drives the picker rather than assuming a default. Prompter is - // nil off a TTY / --no-input. + // Interactive guided mode (#28). When Interactive is true, runDataIngest + // walks every question relevant to the chosen task (via Prompter) before + // validation, pre-filling each from whatever arrived on the command line + // (#509). Prompter is nil off a TTY / --no-input, which is what keeps + // scripts flag-only. Interactive bool Prompter prompter - TaskSet bool // ReviewShown records whether the guided flow rendered the pre-confirm // Review (it only does when it actually prompted for something). It gates diff --git a/internal/cli/data_ingest_local.go b/internal/cli/data_ingest_local.go index a28436b..d977849 100644 --- a/internal/cli/data_ingest_local.go +++ b/internal/cli/data_ingest_local.go @@ -100,7 +100,7 @@ func resolveLocalInput(out, errOut io.Writer, a *runDataIngestArgs) (layout *pus // 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 err := runInteractive(a.Printer, a.Prompter, a); err != nil { if errors.Is(err, errInteractiveCancelled) { // cleanCancel prints the shared note and returns the clean exit. return nil, nil, nil, true, cleanCancel(a.Printer, "nothing was ingested.") @@ -201,55 +201,18 @@ func resolveLocalInput(out, errOut io.Writer, a *runDataIngestArgs) (layout *pus 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)} + // Task-scoped flags. 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). + // + // The scopes themselves live in task_scope.go, shared with the guided + // flow's post-picker reset: both need the same answer to "does this task + // use this value?", and two copies of that answer would drift silently. + if err := rejectMisappliedTaskValues(a); err != nil { + return nil, nil, nil, false, &exitError{code: exitBadInput, err: err} } // 3. Walk the local directory FIRST (local "fail fast"), dispatched diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index 82dbc8a..bf3d05c 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -178,17 +178,40 @@ func isInteractiveTTY() bool { return term.IsTerminal(int(os.Stdin.Fd())) && term.IsTerminal(int(os.Stdout.Fd())) } -// runInteractive fills the gaps in a's core ingest fields by prompting, -// data-first (RFC-0002 §12.1): intent → name → path → task → task-specific -// questions → review. It only prompts for what's still missing, so flags -// the user already passed win. taskSet says whether the task was passed -// explicitly (via --task or the hidden --category alias); when it wasn't, -// the family is sniffed from the data the user pointed at (echoed back, or -// asked plainly when ambiguous) and only that family's tasks are offered. +// defaultInOptions returns want when it is one of options, otherwise fallback. +// A survey.Select whose Default is not among its Options aborts on a real +// terminal ("default value … not found in options"), so any command-line value +// used to PRE-FILL a Select must be validated against that Select's options +// first — a typo like --intent training or --label-policy buckets would +// otherwise crash the prompt survey can't draw. When the supplied value isn't a +// real option the question is still ASKED; the prompt just opens on the +// sensible fallback. This is the guard pickTask has always applied, factored out +// so every supplied-default Select shares one implementation. +func defaultInOptions(want string, options []string, fallback string) string { + for _, o := range options { + if o == want { + return want + } + } + return fallback +} + +// runInteractive walks a's core ingest fields by prompting, data-first +// (RFC-0002 §12.1): intent → name → path → task → task-specific questions → +// review. +// +// It asks EVERY question relevant to the chosen task (#509). A value the user +// passed on the command line pre-fills the answer — Enter accepts it — but never +// suppresses the question. Which questions are relevant still depends on the +// task picked at step 4; that is the only gate. +// +// When no task was supplied, the family is sniffed from the data the user +// pointed at (echoed back, or asked plainly when ambiguous) and only that +// family's tasks are offered. When one WAS supplied it selects the family +// directly, so the user's own answer is always among the options. // // Mutates a through the pointer. -func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bool) error { - prompted := false +func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs) error { // The guided flow is a four-step setup: intent → name → path → task. Each // question prints as its own step header (PromptStep), with any supporting @@ -206,37 +229,58 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo // blank sits directly between header and prompt. A result that belongs to an // answer (the sniff echo) attaches to it with no blank. + // Guided mode ASKS. A value that arrived on the command line becomes the + // prompt's DEFAULT — never a reason to skip the question (#509). + // + // It used to skip: each step was wrapped in `if == ""`. That turned a + // path which had merely gone stale into a dead end — the user was sitting at + // a prompt, ready to answer, and instead got "no such file or directory" with + // no way to correct it. Data moves between runs; a name or a task does not + // stop being true, but the flow should not have to reason about which is + // which. So: always ask, pre-filled, Enter accepts. + // + // Only the TASK gate survives, one level down — which questions apply still + // depends on the task chosen at step 4 (self-supervised text has no label and + // no extras). That gate is about relevance, not about what the user typed. + // + // Non-interactive is untouched: runInteractive is only reached on a TTY + // without --no-input/--output-json, so scripts still take flags silently. + // Step 1 — intent: what this data is for. - if a.Spec.Intent == "" { + { + opts := []string{"train", "test"} + // A supplied --intent pre-fills the prompt, but only when it names a real + // option: a typo would otherwise crash survey.Select on a TTY (see + // defaultInOptions). Unknown value → fall back to "train" and still ask. + def := defaultInOptions(a.Spec.Intent, opts, "train") p.PromptStep(1, 4, "Do you want to ingest training or test data?") p.Newline() ans, err := pr.Select("Do you want to ingest training or test data?", "which split this data is", - []string{"train", "test"}, "train") + opts, def) if err != nil { return err } a.Spec.Intent = ans - prompted = true } - // Step 2 — name. No auto-fill; the character rules surface only if the - // name is rejected (see ValidateTableName), so the prompt stays clean. - if a.Spec.Table == "" { + // Step 2 — name. The character rules surface only if the name is rejected + // (see ValidateTableName), so the prompt stays clean. + { p.PromptStep(2, 4, "Please name the dataset.") p.Newline() ans, err := pr.Input("Please name the dataset.", - "letters, digits, and underscores — no hyphens or spaces, use _; start with a letter or underscore e.g. churn_train", "", + "letters, digits, and underscores — no hyphens or spaces, use _; start with a letter or underscore e.g. churn_train", + a.Spec.Table, push.ValidateTableName) if err != nil { return err } a.Spec.Table = ans - prompted = true } // Step 3 — path. Show what "file or folder" means per modality, then // detect the family from the layout and echo it back. - if a.LocalPath == "" { + { p.PromptStep(3, 4, "Where is your data?") p.Newline() exTab, exImg, exTxt := datasetPathExamples(runtime.GOOS) @@ -245,7 +289,8 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo p.Infof("Images a folder with labels.csv + images/ e.g. %s", exImg) p.Infof("Text a folder with labels.csv + texts/ e.g. %s", exTxt) p.Newline() - ans, err := pr.Input("Where is your data?", fmt.Sprintf("e.g. %s or %s", exTab, exImg), "", validateDatasetPath) + ans, err := pr.Input("Where is your data?", fmt.Sprintf("e.g. %s or %s", exTab, exImg), + a.LocalPath, validateDatasetPath) if err != nil { return err } @@ -259,7 +304,6 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo // otherwise become part of the path (#386). validateDatasetPath applies // the same canonicalization, so the re-prompt guard stays consistent. a.LocalPath = dequotePath(ans) - prompted = true } // Expand a leading ~ now so the family sniff + label-header preview read // the real path; runDataIngest's own expandHome then no-ops. @@ -275,33 +319,53 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo return err } - // (d) task — family-scoped. An explicit --task wins and skips both the - // sniff and the picker (§5.1). Otherwise the family is sniffed from the - // layout (and echoed), or asked plainly when the layout is ambiguous, - // and then only that family's tasks are offered. - if !taskSet { - fam, err := resolveFamily(p, pr, a.LocalPath) - if err != nil { - return err + // (d) task — family-scoped, and always asked (#509). + // + // An explicit --task no longer skips the picker; it selects the family and + // becomes the pre-selected option. Taking the family from the SUPPLIED task + // rather than the sniff matters: the two can disagree (a tabular task passed + // against a folder that sniffs image), and scoping the list to the sniffed + // family would leave the user's own answer missing from its own default. + // With no task supplied, the family is sniffed from the layout (and echoed), + // or asked plainly when the layout is ambiguous — unchanged. + { + var fam push.Family + if spec, ok := push.Lookup(a.Spec.Category); ok { + fam = spec.Family + } else { + f, err := resolveFamily(p, pr, a.LocalPath) + if err != nil { + return err + } + fam = f } - id, err := pickTask(p, pr, fam) + supplied := a.Spec.Category + id, err := pickTask(p, pr, fam, supplied) if err != nil { return err } a.Spec.Category = id - prompted = true + // What the user just walked away from goes with it. A run started as + // `--task time_to_event_prediction --time-column t` that picks + // tabular_classification here would otherwise carry TimeColumn through: + // no prompt asks about it (it is time_to_event_prediction-only), Review + // shows it anyway, and the run dies AFTER the confirm blaming a flag the + // user just walked away from. Scoped to the CHANGE, so a flag that was + // misapplied on the command line survives to be rejected. + dropValuesLeftBehindByATaskChange(a, supplied) } // (e) task-specific questions, including the label column. - cp, err := promptCategorySpecific(p, pr, a) - if err != nil { + if _, err := promptCategorySpecific(p, pr, a); err != nil { return err } - prompted = prompted || cp - // (f) review + single confirm. Only when we actually prompted something - // — an ingest fully specified by flags (on a TTY) isn't nagged. - if prompted { + // (f) review + single confirm — unconditional now (#509). It used to be + // gated on "did we actually prompt anything", so an ingest fully specified + // by flags wasn't nagged. Guided mode always prompts now, so that gate can + // only ever be true; keeping it would be a condition that reads as a choice + // while having none. + { renderReview(p, a) a.ReviewShown = true // No header here: Confirm keeps its own label ("Proceed with the @@ -354,7 +418,7 @@ func resolveFamily(p *ui.Printer, pr prompter, path string) (push.Family, error) // task_id", split into Available now and (greyed) Not yet in the CLI — // and asks the user to pick one of the available ones. It never shows the // flat 15-item wall: only this family's tasks appear (§7). -func pickTask(p *ui.Printer, pr prompter, fam push.Family) (string, error) { +func pickTask(p *ui.Printer, pr prompter, fam push.Family, want string) (string, error) { var available, pending []push.CategorySpec for _, s := range push.CategoriesByFamily(fam) { if s.CLISupported { @@ -406,7 +470,12 @@ func pickTask(p *ui.Printer, pr prompter, fam push.Family) (string, error) { for i, s := range available { opts[i] = s.ID } - ans, err := pr.Select("Which task?", "pick the task this data is for", opts, opts[0]) + // def pre-selects a task the user already named (#509), honoured only when it + // is actually in this family's list (defaultInOptions) — survey would + // otherwise render a default the user cannot see, and an Enter on it would be + // unexplainable. + def := defaultInOptions(want, opts, opts[0]) + ans, err := pr.Select("Which task?", "pick the task this data is for", opts, def) if err != nil { return "", err } @@ -419,10 +488,11 @@ func pickTask(p *ui.Printer, pr prompter, fam push.Family) (string, error) { } // promptCategorySpecific prompts for the inputs a particular task needs -// beyond the core fields, filling only the gaps. The label column comes -// first (it's the one question every non-self-supervised task shares), -// then the family-specific extras. Returns whether it prompted anything -// (so the caller knows to show the confirm). +// beyond the core fields. Like the core steps (#509), each question is always +// asked with any supplied value pre-filled as the default — never skipped. The +// label column comes first (the one question every non-self-supervised task +// shares), then the family-specific extras. Returns whether it prompted anything +// (retained for the caller; the confirm is unconditional now). func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (bool, error) { cat := a.Spec.Category prompted := false @@ -437,7 +507,12 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b // (data-ingestors#340) that free-typing "Label" against a "label" header // would cause. Wording is per-task: a class to sort into vs a numeric value // to predict (§8). - if !push.SelfSupervisedText(cat) && a.Spec.LabelColumn == "" { + // + // A supplied --label-column pre-fills the pick (like every other value under + // #509) — it no longer SKIPS the question, so a wrong or mistyped column can + // be corrected here the same way a stale path can. Only self-supervised text + // (no label at all) still bypasses it. + if !push.SelfSupervisedText(cat) { question := "Which column holds the label?" desc := "The answer the model learns to produce — for classification, the class. e.g. diagnosis, churned" if push.IsRegressionClass(cat) { @@ -448,7 +523,7 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b p.Newline() p.Hintf("%s", desc) p.Newline() - ans, err := promptLabelColumn(pr, cat, a.LocalPath, question) + ans, err := promptLabelColumn(pr, cat, a.LocalPath, question, a.Spec.LabelColumn) if err != nil { return prompted, err } @@ -461,13 +536,17 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b // depends on the task. switch { case push.IsImage(cat): - if cat == "keypoint_detection" && a.Spec.NumberOfKeypoints <= 0 { + if cat == "keypoint_detection" { p.Section("How many keypoints per sample?") p.Newline() p.Hintf("The number of landmark points each sample is annotated with — dataset-specific. e.g. 17 for COCO human pose") p.Newline() + kpDef := "" + if a.Spec.NumberOfKeypoints > 0 { + kpDef = strconv.Itoa(a.Spec.NumberOfKeypoints) + } ans, err := pr.Input("How many keypoints per sample?", - "e.g. 17 for COCO pose", "", validatePositiveInt) + "e.g. 17 for COCO pose", kpDef, validatePositiveInt) if err != nil { return prompted, err } @@ -475,13 +554,13 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b a.Spec.NumberOfKeypoints = n prompted = true } - if a.TargetSizeFlag == "" { + { p.Section("Image resolution") p.Newline() p.Hintf("The size your images already are, as WxH — tracebloc checks every image matches and never resizes. Press Enter to read it from your first image. e.g. 224x224") p.Newline() ans, err := pr.Input("Image resolution", - "the size your images already are; tracebloc checks it, it never resizes", "", + "the size your images already are; tracebloc checks it, it never resizes", a.TargetSizeFlag, validateOptionalTargetSize) if err != nil { return prompted, err @@ -490,38 +569,46 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b prompted = true } case push.IsTabular(cat): - if a.SchemaFlag == "" { + { p.Section("Column types") p.Newline() p.Hintf("We infer each column's type from your CSV. Press Enter to accept, or type overrides like age:INT,price:FLOAT.") p.Newline() - ans, err := pr.Input("Column types", "e.g. age:INT,price:FLOAT", "", validateOptionalSchema) + ans, err := pr.Input("Column types", "e.g. age:INT,price:FLOAT", a.SchemaFlag, validateOptionalSchema) if err != nil { return prompted, err } a.SchemaFlag = strings.TrimSpace(ans) prompted = true } - if push.IsRegressionClass(cat) && a.Spec.LabelPolicy == "" { + if push.IsRegressionClass(cat) { p.Section("Label policy") p.Newline() p.Hintf("Regression targets are continuous. 'bucket' groups them into ranges before they leave the cluster; 'passthrough' keeps raw values.") p.Newline() + opts := []string{"bucket", "passthrough"} + // A supplied --label-policy pre-fills only when valid; a typo like + // "buckets" would otherwise abort the Select on a TTY (defaultInOptions). + lpDef := defaultInOptions(a.Spec.LabelPolicy, opts, "bucket") ans, err := pr.Select("Label policy", "bucket bins the target before it leaves the cluster", - []string{"bucket", "passthrough"}, "bucket") + opts, lpDef) if err != nil { return prompted, err } a.Spec.LabelPolicy = ans prompted = true } - if cat == "time_to_event_prediction" && a.Spec.TimeColumn == "" { + if cat == "time_to_event_prediction" { p.Section("Time column") p.Newline() p.Hintf("The column holding the duration / time-to-event. e.g. time, tenure_days") p.Newline() - ans, err := pr.Input("Time column", "the duration/time column name", "time", nil) + tcDef := a.Spec.TimeColumn + if tcDef == "" { + tcDef = "time" + } + ans, err := pr.Input("Time column", "the duration/time column name", tcDef, nil) if err != nil { return prompted, err } @@ -533,17 +620,24 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b } // promptLabelColumn asks for the label/target column. When the CSV header -// can be read, it offers those columns as an exact-match SELECT (defaulting -// to a column literally named "label" if present); otherwise — the header -// isn't readable yet — it falls back to free text so the flow never stalls. -func promptLabelColumn(pr prompter, category, root, question string) (string, error) { +// can be read, it offers those columns as an exact-match SELECT — pre-selecting +// a supplied --label-column when it names a real header, else a column literally +// named "label" if present; otherwise — the header isn't readable yet — it falls +// back to free text pre-filled with the supplied value so the flow never stalls. +// +// supplied is guarded through defaultInOptions before it reaches survey.Select: +// a mistyped --label-column that is not one of the real headers would otherwise +// abort the prompt on a TTY, the same default-not-in-options crash guarded +// everywhere else in the guided flow. +func promptLabelColumn(pr prompter, category, root, question, supplied string) (string, error) { headers, err := push.PreviewLabelHeaders(category, root) if err == nil && len(headers) > 0 { ans, serr := pr.Select(question, - "pick the label/target column from your CSV header", headers, defaultLabelChoice(headers)) + "pick the label/target column from your CSV header", headers, + defaultInOptions(supplied, headers, defaultLabelChoice(headers))) return strings.TrimSpace(ans), serr } - ans, ierr := pr.Input(question, "the label/target column name", "", nil) + ans, ierr := pr.Input(question, "the label/target column name", supplied, nil) return strings.TrimSpace(ans), ierr } diff --git a/internal/cli/interactive_test.go b/internal/cli/interactive_test.go index 548fc66..56a5ff2 100644 --- a/internal/cli/interactive_test.go +++ b/internal/cli/interactive_test.go @@ -3,8 +3,10 @@ package cli import ( "bytes" "errors" + "fmt" "os" "path/filepath" + "slices" "strings" "testing" @@ -41,7 +43,17 @@ func (f *fakePrompter) Input(label, _ /*help*/, def string, validate func(string return ans, nil } -func (f *fakePrompter) Select(label, _ /*help*/ string, _ []string, def string) (string, error) { +func (f *fakePrompter) Select(label, _ /*help*/ string, options []string, def string) (string, error) { + // Honour survey.Select's real contract: a non-empty Default that is not one + // of the Options aborts the prompt on a terminal ("default value … not found + // in options"). The original double ignored Options entirely, so a Select + // whose pre-filled default came from a mistyped flag stayed green here while + // crashing on a real TTY (PR #505). Validating the default in the double is + // what connects these tests to that failure — an unguarded supplied default + // now reddens instead of passing. + if def != "" && !slices.Contains(options, def) { + return "", fmt.Errorf("default value %q not found in options %v", def, options) + } return f.answer(label, def), nil } @@ -110,7 +122,7 @@ func TestRunInteractive_PromptOrder(t *testing.T) { "Which column holds the label?": "churned", }} a := &runDataIngestArgs{} - if err := runInteractive(discardPrinter(), f, a, false /*taskSet*/); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } @@ -146,7 +158,7 @@ func TestRunInteractive_PathPromptCopyIsFileOrFolder(t *testing.T) { "Which column holds the label?": "churned", }} a := &runDataIngestArgs{} - if err := runInteractive(discardPrinter(), f, a, false /*taskSet*/); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } found := false @@ -174,7 +186,7 @@ func TestRunInteractive_SniffEchoesFamily(t *testing.T) { a := &runDataIngestArgs{LocalPath: dir, Spec: push.SpecArgs{Intent: "train"}} var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - if err := runInteractive(p, f, a, false); err != nil { + if err := runInteractive(p, f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if !strings.Contains(buf.String(), "Found a CSV table") { @@ -201,7 +213,7 @@ func TestRunInteractive_SniffIsHintNotLock(t *testing.T) { "Which column holds the label?": "label", }} a := &runDataIngestArgs{LocalPath: empty, Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if !contains(f.asked, "What kind of data is this?") { @@ -263,9 +275,10 @@ func TestResolveFamily_SurfacesMiscasedHint(t *testing.T) { } } -// TestRunInteractive_ExplicitTaskSkipsSniff: an explicit --task wins — no -// sniff echo, no family question, no task picker. -func TestRunInteractive_ExplicitTaskSkipsSniff(t *testing.T) { +// TestRunInteractive_ExplicitTaskStillAsks: an explicit --task no longer skips +// the picker (#509) — it PRE-SELECTS. It does still settle the family directly, +// so the sniff is unnecessary and the user's own task is among the options. +func TestRunInteractive_ExplicitTaskStillAsks(t *testing.T) { dir := tabularDir(t) f := &fakePrompter{answers: map[string]string{ "Please name the dataset.": "t", @@ -277,16 +290,26 @@ func TestRunInteractive_ExplicitTaskSkipsSniff(t *testing.T) { } var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - if err := runInteractive(p, f, a, true /*taskSet*/); err != nil { + if err := runInteractive(p, f, a); err != nil { t.Fatalf("runInteractive: %v", err) } + if !slices.Contains(f.asked, "Which task?") { + t.Errorf("the task question must still be asked; asked: %v", f.asked) + } + // The family came from the supplied task, so the layout sniff is not needed + // and its echo must not appear. for _, l := range f.asked { - if l == "Which task?" || l == "What kind of data is this?" { - t.Errorf("explicit --task must skip the picker/sniff; asked %q", l) + if l == "What kind of data is this?" { + t.Errorf("a supplied task settles the family; must not ask %q", l) } } if strings.Contains(buf.String(), "Found a CSV table") { - t.Errorf("explicit --task must not echo a sniff") + t.Errorf("a supplied task must not echo a sniff") + } + // Unanswered by the fake, so it returns the prompt's default — proving the + // supplied task was pre-selected rather than discarded. + if a.Spec.Category != "tabular_classification" { + t.Errorf("Category = %q, want the supplied task pre-selected", a.Spec.Category) } } @@ -301,7 +324,7 @@ func TestPickTask_FamilyScoped(t *testing.T) { f := &fakePrompter{answers: map[string]string{"Which task?": "text_classification"}} var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - id, err := pickTask(p, f, push.FamilyText) + id, err := pickTask(p, f, push.FamilyText, "") if err != nil { t.Fatalf("pickTask: %v", err) } @@ -341,7 +364,7 @@ func TestPickTask_ImageAllAvailable(t *testing.T) { f := &fakePrompter{answers: map[string]string{"Which task?": "image_classification"}} var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - if _, err := pickTask(p, f, push.FamilyImage); err != nil { + if _, err := pickTask(p, f, push.FamilyImage, ""); err != nil { t.Fatalf("pickTask: %v", err) } out := buf.String() @@ -367,7 +390,7 @@ func TestPickTask_TabularGloss(t *testing.T) { f := &fakePrompter{answers: map[string]string{"Which task?": "time_to_event_prediction"}} var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - id, err := pickTask(p, f, push.FamilyTabular) + id, err := pickTask(p, f, push.FamilyTabular, "") if err != nil { t.Fatalf("pickTask: %v", err) } @@ -391,7 +414,7 @@ func TestRunInteractive_LabelSelectFromHeaders(t *testing.T) { "Which column holds the label?": "income", }} a := &runDataIngestArgs{LocalPath: dir, Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if a.Spec.LabelColumn != "income" { @@ -410,7 +433,7 @@ func TestRunInteractive_RegressionLabelWording(t *testing.T) { LocalPath: dir, Spec: push.SpecArgs{Category: "tabular_regression", Table: "t", Intent: "train"}, } - if err := runInteractive(discardPrinter(), f, a, true /*taskSet*/); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if !contains(f.asked, "Which column holds the value to predict?") { @@ -436,7 +459,7 @@ func TestRunInteractive_LabelFreeTextFallback(t *testing.T) { LocalPath: empty, Spec: push.SpecArgs{Category: "image_classification", Table: "t", Intent: "train"}, } - if err := runInteractive(discardPrinter(), f, a, true); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if a.Spec.LabelColumn != "my_label" { @@ -453,7 +476,7 @@ func TestRunInteractive_MLMSkipsLabel(t *testing.T) { "Which task?": "masked_language_modeling", }} a := &runDataIngestArgs{LocalPath: dir, Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } for _, l := range f.asked { @@ -466,9 +489,15 @@ func TestRunInteractive_MLMSkipsLabel(t *testing.T) { } } -// TestRunInteractive_SkipsProvidedValues: flags already set (and an -// explicit --task) mean nothing is prompted. -func TestRunInteractive_SkipsProvidedValues(t *testing.T) { +// TestRunInteractive_AsksEvenWhenFullySpecified is the inverse of the rule this +// replaced (#509). Guided mode used to prompt for nothing when every value +// arrived on the command line — which is what turned a path that had merely gone +// stale into a dead end, with the user sitting at a prompt that never came. +// +// Every core question must now be asked, and every supplied value must survive +// as the default: the fake answers nothing, so each field keeping its original +// value proves the pre-fill rather than a re-entered answer. +func TestRunInteractive_AsksEvenWhenFullySpecified(t *testing.T) { dir := textDirLayout(t) f := &fakePrompter{answers: map[string]string{}} a := &runDataIngestArgs{ @@ -477,11 +506,226 @@ func TestRunInteractive_SkipsProvidedValues(t *testing.T) { Category: "text_classification", Table: "t", Intent: "train", LabelColumn: "label", }, } - if err := runInteractive(discardPrinter(), f, a, true /*taskSet*/); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + for _, want := range []string{ + "Do you want to ingest training or test data?", + "Please name the dataset.", + "Where is your data?", + "Which task?", + } { + if !slices.Contains(f.asked, want) { + t.Errorf("guided mode must ask %q even when it was supplied; asked: %v", want, f.asked) + } + } + if a.Spec.Intent != "train" || a.Spec.Table != "t" || + a.Spec.Category != "text_classification" || a.LocalPath != dir { + t.Errorf("supplied values must survive as prompt defaults, got intent=%q table=%q category=%q path=%q", + a.Spec.Intent, a.Spec.Table, a.Spec.Category, a.LocalPath) + } +} + +// With nothing supplied, each question must still fall back to the value it +// always defaulted to. This pins behaviourally what the string-index backstop +// used to pin textually: moving those defaults out of inline arguments and into +// variables (so a supplied value can take their place) removed "train", +// "bucket" and "time" from zz-all-strings.golden. +func TestRunInteractive_UnsuppliedDefaultsUnchanged(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "time_to_event_prediction", + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{LocalPath: dir} + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.Intent != "train" { + t.Errorf("Intent = %q, want the unchanged \"train\" fallback", a.Spec.Intent) + } + if a.Spec.TimeColumn != "time" { + t.Errorf("TimeColumn = %q, want the unchanged \"time\" fallback", a.Spec.TimeColumn) + } +} + +func TestRunInteractive_LabelPolicyDefaultUnchanged(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "tabular_regression", + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{LocalPath: dir} + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.LabelPolicy != "bucket" { + t.Errorf("LabelPolicy = %q, want the unchanged \"bucket\" fallback", a.Spec.LabelPolicy) + } +} + +// A supplied value must be offered back as the DEFAULT, not silently replaced by +// the flow's own fallback — the specific regression that would make "always ask" +// feel like "always retype". +func TestRunInteractive_SuppliedValuesArePrefilled(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "tabular_regression", Table: "prefilled_name", Intent: "test", + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + // "test" must survive: the Select's own fallback is "train". + if a.Spec.Intent != "test" { + t.Errorf("Intent = %q, want the supplied \"test\" pre-selected (flow default is \"train\")", a.Spec.Intent) + } + if a.Spec.Table != "prefilled_name" { + t.Errorf("Table = %q, want the supplied name pre-filled", a.Spec.Table) + } + if a.LocalPath != dir { + t.Errorf("LocalPath = %q, want the supplied path pre-filled", a.LocalPath) + } +} + +// TestDefaultInOptions pins the guard that keeps a command-line value from +// crashing a survey.Select: a valid value passes through, an empty or unknown +// one drops to the fallback. Reverting the helper body to `return want` reddens +// the empty, typo and case-mismatch rows. +func TestDefaultInOptions(t *testing.T) { + opts := []string{"train", "test"} + cases := []struct { + name, supplied, fallback, want string + }{ + {"valid-supplied-passes-through", "test", "train", "test"}, + {"first-option-supplied", "train", "train", "train"}, + {"empty-uses-fallback", "", "train", "train"}, + {"unknown-typo-uses-fallback", "training", "train", "train"}, + {"case-mismatch-uses-fallback", "Train", "train", "train"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := defaultInOptions(tc.supplied, opts, tc.fallback); got != tc.want { + t.Errorf("defaultInOptions(%q, %v, %q) = %q, want %q", + tc.supplied, opts, tc.fallback, got, tc.want) + } + }) + } +} + +// TestRunInteractive_InvalidSuppliedSelectDefaultFallsBack: a mistyped --intent +// or --label-policy must not crash the guided prompt. survey.Select aborts when +// its Default is not one of the Options; guided mode pre-fills those Selects with +// the command-line value, so a typo like --intent training would open a prompt +// survey refuses to draw (#505, High). The value is guarded (defaultInOptions): +// an unknown supplied value drops back to the flow's own default, and the +// question is still ASKED. +// +// Mutation-proof: the strict fake Select errors on a default that isn't in its +// options, so reverting either guard makes runInteractive return that error and +// this test fails. +func TestRunInteractive_InvalidSuppliedSelectDefaultFallsBack(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Which column holds the value to predict?": "income", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "tabular_regression", + Table: "reg_train", + Intent: "training", // typo — not train/test + LabelPolicy: "buckets", // typo — not bucket/passthrough + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("a mistyped supplied Select default must not crash the prompt: %v", err) + } + // Typo'd values were dropped to the flow's own valid defaults (the fake + // returns the prompt default when nothing is scripted for that question). + if a.Spec.Intent != "train" { + t.Errorf("Intent = %q, want the \"train\" fallback after a typo'd --intent", a.Spec.Intent) + } + if a.Spec.LabelPolicy != "bucket" { + t.Errorf("LabelPolicy = %q, want the \"bucket\" fallback after a typo'd --label-policy", a.Spec.LabelPolicy) + } +} + +// TestRunInteractive_SuppliedLabelColumnStillAsks: a supplied --label-column no +// longer SKIPS the label question (#505, Medium) — it PRE-FILLS it, exactly like +// a stale path. The old gate (LabelColumn == "") left a user who mistyped the +// column with no way to correct it at the prompt; now the header-backed picker +// still opens, so re-answering wins. +// +// Mutation-proof: restoring the `&& a.Spec.LabelColumn == ""` gate skips the +// question — f.asked loses it AND the re-answer no longer takes — so both +// assertions redden. +func TestRunInteractive_SuppliedLabelColumnStillAsks(t *testing.T) { + dir := tabularDir(t) // header: age,income,churned + f := &fakePrompter{answers: map[string]string{ + // Re-answer the label with a different real column: only reachable if the + // question was actually asked rather than skipped by the supplied value. + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "tabular_classification", Table: "t", Intent: "train", + LabelColumn: "income", // supplied — must pre-fill, not skip + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } - if len(f.asked) != 0 { - t.Errorf("expected no prompts, but asked: %v", f.asked) + if !contains(f.asked, "Which column holds the label?") { + t.Errorf("a supplied --label-column must still ASK the label question; asked=%v", f.asked) + } + if a.Spec.LabelColumn != "churned" { + t.Errorf("LabelColumn = %q, want the re-answered \"churned\" (prompt was live, not skipped)", a.Spec.LabelColumn) + } +} + +// TestPromptLabelColumn_SuppliedDefaultPrefillsAndGuards drives the label picker +// directly: a supplied column that names a real header is pre-selected, while an +// empty or mistyped one falls back to the header-derived default without crashing +// the Select. It is the same defaultInOptions guard used for --intent, applied to +// the column the label question now pre-fills (#505). +// +// Mutation-proof: passing `supplied` straight to pr.Select (dropping the guard) +// makes the "mistyped" row error under the strict fake; not threading the value +// at all makes the "valid-supplied" row return the header default instead. +func TestPromptLabelColumn_SuppliedDefaultPrefillsAndGuards(t *testing.T) { + dir := tabularDir(t) // header: age,income,churned (no column named "label") + const cat = "tabular_classification" + const q = "Which column holds the label?" + + cases := []struct { + name, supplied, want string + }{ + // Nothing scripted → the strict fake returns the Select's default, so the + // asserted value IS whatever default promptLabelColumn passed in. + {"valid-supplied-is-preselected", "income", "income"}, + {"empty-supplied-uses-header-default", "", "age"}, // defaultLabelChoice → first header + {"mistyped-supplied-falls-back", "incom", "age"}, // guarded, no crash + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := &fakePrompter{answers: map[string]string{}} + got, err := promptLabelColumn(f, cat, dir, q, tc.supplied) + if err != nil { + t.Fatalf("promptLabelColumn(supplied=%q) errored (default not guarded into options?): %v", tc.supplied, err) + } + if got != tc.want { + t.Errorf("promptLabelColumn(supplied=%q) = %q, want %q", tc.supplied, got, tc.want) + } + }) } } @@ -497,7 +741,7 @@ func TestRunInteractive_Keypoint(t *testing.T) { LocalPath: dir, Spec: push.SpecArgs{Category: "keypoint_detection", Table: "kp_train", Intent: "train"}, } - if err := runInteractive(discardPrinter(), f, a, true); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if a.Spec.NumberOfKeypoints != 17 { @@ -520,7 +764,7 @@ func TestRunInteractive_TabularRegression(t *testing.T) { LocalPath: dir, Spec: push.SpecArgs{Category: "tabular_regression", Table: "reg_train", Intent: "train"}, } - if err := runInteractive(discardPrinter(), f, a, true); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if a.Spec.LabelPolicy != "passthrough" { @@ -544,7 +788,7 @@ func TestRunInteractive_Cancel(t *testing.T) { confirm: &no, } a := &runDataIngestArgs{LocalPath: dir, Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); !errors.Is(err, errInteractiveCancelled) { + if err := runInteractive(discardPrinter(), f, a); !errors.Is(err, errInteractiveCancelled) { t.Fatalf("err = %v, want errInteractiveCancelled", err) } } @@ -554,7 +798,7 @@ func TestRunInteractive_Cancel(t *testing.T) { func TestRunInteractive_RejectsBadName(t *testing.T) { f := &fakePrompter{answers: map[string]string{"Please name the dataset.": "../bad"}} a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, true); err == nil { + if err := runInteractive(discardPrinter(), f, a); err == nil { t.Fatal("expected an error for an invalid name, got nil") } } @@ -568,7 +812,7 @@ func TestRunInteractive_RejectsEmptyPath(t *testing.T) { "Where is your data?": " ", }} a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); err == nil { + if err := runInteractive(discardPrinter(), f, a); err == nil { t.Fatal("expected an error for an empty dataset path, got nil") } } @@ -586,7 +830,7 @@ func TestRunInteractive_TrimsPath(t *testing.T) { "Which column holds the label?": "churned", }} a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} - if err := runInteractive(discardPrinter(), f, a, false); err != nil { + if err := runInteractive(discardPrinter(), f, a); err != nil { t.Fatalf("runInteractive: %v", err) } if a.LocalPath != dir { @@ -612,7 +856,7 @@ func TestRunInteractive_ShowsExampleHints(t *testing.T) { a := &runDataIngestArgs{Spec: push.SpecArgs{Intent: "train"}} var buf bytes.Buffer p := ui.New(&buf, ui.WithColor(false)) - if err := runInteractive(p, f, a, false); err != nil { + if err := runInteractive(p, f, a); err != nil { t.Fatalf("runInteractive: %v", err) } for _, want := range []string{"~/data/patients.csv", "age:INT"} { diff --git a/internal/cli/task_scope.go b/internal/cli/task_scope.go new file mode 100644 index 0000000..ccf16a5 --- /dev/null +++ b/internal/cli/task_scope.go @@ -0,0 +1,184 @@ +package cli + +import ( + "errors" + "fmt" + + "github.com/tracebloc/cli/internal/push" +) + +// taskScopedValue is one value that only some tasks use — the flags each read +// inside a single category branch, so a value passed against a task that +// doesn't consume it would otherwise be silently dropped. +// +// The scope predicate lives HERE, once, because two callers need the same +// answer to "does this task use this value?": +// +// - the misapplied-flag guard in runDataIngestLocal, which rejects a value +// the chosen task cannot use, and +// - dropOutOfScopeTaskValues, which the guided flow runs after the task +// picker so a value the user is no longer choosing doesn't survive. +// +// Written as two copies of each predicate, they would agree today and drift on +// the next task added to a family — and drift here is invisible: both copies +// keep passing their own tests while disagreeing with each other. +type taskScopedValue struct { + // flag names the value as the user typed it, for the message. + flag string + // inScope answers whether this task consumes the value. + inScope func(category string) bool + // isSet reports whether the value is present at all. + isSet func(*runDataIngestArgs) bool + // set puts a representative value there. Only tests call it, and they call + // it to build states from the table rather than by hand — a hand-built + // fixture is one more copy of the scopes to drift. + set func(*runDataIngestArgs) + // clear returns the value to "not supplied". + clear func(*runDataIngestArgs) + // message is the whole rejection sentence, written out rather than + // composed, so the copy catalog (zz-all-strings.golden) shows a reviewer + // the exact string a user sees instead of a fragment. A test pins that each + // one names its own flag. + message func(category string) string +} + +// taskScopedValues is the whole set. Order fixes the order of rejection when +// more than one is misapplied, so the message a user sees is deterministic. +var taskScopedValues = []taskScopedValue{ + { + flag: "--target-size", + inScope: push.IsImage, + isSet: func(a *runDataIngestArgs) bool { return a.TargetSizeFlag != "" }, + clear: func(a *runDataIngestArgs) { a.TargetSizeFlag = "" }, + set: func(a *runDataIngestArgs) { a.TargetSizeFlag = "224x224" }, + message: func(cat string) string { + return fmt.Sprintf("--target-size is image tasks only; it doesn't apply to task %q", cat) + }, + }, + { + flag: "--min-size", + inScope: push.IsImage, + isSet: func(a *runDataIngestArgs) bool { return a.MinSizeFlag != "" }, + clear: func(a *runDataIngestArgs) { a.MinSizeFlag = "" }, + set: func(a *runDataIngestArgs) { a.MinSizeFlag = "32x32" }, + message: func(cat string) string { + return fmt.Sprintf("--min-size is image tasks only; it doesn't apply to task %q", cat) + }, + }, + { + flag: "--schema", + inScope: push.IsTabular, + isSet: func(a *runDataIngestArgs) bool { return a.SchemaFlag != "" }, + clear: func(a *runDataIngestArgs) { a.SchemaFlag = "" }, + set: func(a *runDataIngestArgs) { a.SchemaFlag = "age:INT" }, + message: func(cat string) string { + return fmt.Sprintf("--schema is tabular/time-series tasks only; it doesn't apply to task %q", cat) + }, + }, + { + flag: "--label-policy", + inScope: push.IsRegressionClass, + isSet: func(a *runDataIngestArgs) bool { return a.Spec.LabelPolicy != "" }, + clear: func(a *runDataIngestArgs) { a.Spec.LabelPolicy = "" }, + set: func(a *runDataIngestArgs) { a.Spec.LabelPolicy = "bucket" }, + message: func(cat string) string { + return fmt.Sprintf("--label-policy is regression-class tasks only (tabular_regression, "+ + "time_series_forecasting, time_to_event_prediction); it doesn't apply to task %q", cat) + }, + }, + { + flag: "--time-column", + inScope: func(cat string) bool { return cat == "time_to_event_prediction" }, + isSet: func(a *runDataIngestArgs) bool { return a.Spec.TimeColumn != "" }, + clear: func(a *runDataIngestArgs) { a.Spec.TimeColumn = "" }, + set: func(a *runDataIngestArgs) { a.Spec.TimeColumn = "t" }, + message: func(cat string) string { + return fmt.Sprintf("--time-column is time_to_event_prediction only; it doesn't apply to task %q", cat) + }, + }, + { + flag: "--number-of-keypoints", + inScope: func(cat string) bool { return cat == "keypoint_detection" }, + isSet: func(a *runDataIngestArgs) bool { return a.Spec.NumberOfKeypoints != 0 }, + clear: func(a *runDataIngestArgs) { a.Spec.NumberOfKeypoints = 0 }, + set: func(a *runDataIngestArgs) { a.Spec.NumberOfKeypoints = 17 }, + message: func(cat string) string { + return fmt.Sprintf("--number-of-keypoints is keypoint_detection only; it doesn't apply to task %q", cat) + }, + }, + { + // The one inverted scope: every task uses a label column EXCEPT + // self-supervised text, which trains on the text itself. buildText drops + // the value, so accepting it silently discarded the user's answer and + // the review echoed a column that never shipped. + flag: "--label-column", + inScope: func(cat string) bool { return !push.SelfSupervisedText(cat) }, + isSet: func(a *runDataIngestArgs) bool { return a.Spec.LabelColumn != "" }, + clear: func(a *runDataIngestArgs) { a.Spec.LabelColumn = "" }, + set: func(a *runDataIngestArgs) { a.Spec.LabelColumn = "label" }, + message: func(cat string) string { + return fmt.Sprintf("--label-column doesn't apply to task %q — it trains on the text itself, with no label column", cat) + }, + }, +} + +// rejectMisappliedTaskValues returns the first value present that the chosen +// task cannot use. Flag-only runs reach this with whatever the user typed; +// guided runs reach it after dropOutOfScopeTaskValues has already removed +// anything the user re-chose away from, so it can only fire on a real mistake. +func rejectMisappliedTaskValues(a *runDataIngestArgs) error { + for _, v := range taskScopedValues { + if v.isSet(a) && !v.inScope(a.Spec.Category) { + return errors.New(v.message(a.Spec.Category)) + } + } + return nil +} + +// dropValuesLeftBehindByATaskChange clears the task-scoped values that the +// task the user just CHOSE does not use, but the task they arrived with did. +// +// The guided flow calls this immediately after the picker. Without it, a run +// started as `--task time_to_event_prediction --time-column t` that picks +// tabular_classification keeps TimeColumn: the prompt for it never appears (it +// is time_to_event_prediction-only), Review shows it anyway, and the run dies +// AFTER the confirm blaming a flag the user just spent a prompt walking away +// from. Guided mode's promise is that the answers on screen are the run; a +// value no question asked about and no answer can reach is not one of them. +// +// It is scoped to what the CHANGE left behind — `v.inScope(from)` — and not to +// everything out of scope, because those are different sets and clearing the +// wrong one swallows a real mistake. `--task tabular_classification +// --time-column t` is misapplied on the command line: no task change walks away +// from it, so nothing here may clear it and rejectMisappliedTaskValues must +// still fire. Otherwise pressing Enter on the pre-selected task would silently +// drop the flag while the identical invocation under --no-input exits 2 — +// guided mode quietly meaning something different from the flags it echoes +// (Bugbot). +// +// A run with no --task supplied is not a change either: the user never declared +// a task to move away from, so every value stands or falls on the guard. +// +// Nor is a --task the registry does not recognize. A name nobody registered is +// not a task anyone walked away from, and treating it as one clears values on +// its behalf: every +// `inScope` predicate answers from the registry, so an unknown id lands on the +// default side of each one — `!SelfSupervisedText("tabular_classifier")` is +// true because the lookup misses, not because the task uses a label column. So +// `--task tabular_classifier --label-column x`, picking a self-supervised +// text task, silently dropped --label-column, and the guided flow runs BEFORE +// the category gate (data_ingest_local.go:103 vs :165), so the typo itself was +// never reported either — the picker had already overwritten it with a valid id. +// Two values lost, no message, where --no-input exits 2 on the same command. +// An unknown `from` is therefore the no-task case: clear nothing, and let +// rejectMisappliedTaskValues speak (Bugbot). +func dropValuesLeftBehindByATaskChange(a *runDataIngestArgs, from string) { + if from == "" || !push.IsKnown(from) || from == a.Spec.Category { + return + } + for _, v := range taskScopedValues { + if v.inScope(from) && !v.inScope(a.Spec.Category) { + v.clear(a) + } + } +} diff --git a/internal/cli/task_scope_test.go b/internal/cli/task_scope_test.go new file mode 100644 index 0000000..81fc78e --- /dev/null +++ b/internal/cli/task_scope_test.go @@ -0,0 +1,290 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/tracebloc/cli/internal/push" +) + +// A run started with task-scoped flags that then picks a DIFFERENT task at the +// guided prompt must not carry the old task's values through. Before the reset, +// `--task time_to_event_prediction --time-column t` + picking +// tabular_classification kept TimeColumn on the spec: no prompt asks about it +// (it is time_to_event_prediction-only), Review showed it anyway, and the run +// then died AFTER the confirm blaming a flag the user had just walked away from. +func TestGuided_TaskScopedValuesDoNotSurviveATaskChange(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "tabular_classification", + "Which column holds the label?": "churned", + }} + // A LEGAL starting state: every value here is in scope for the supplied + // task. Starting from an illegal one would test the wrong thing — a flag + // misapplied from the outset is the guard's job, not the reset's. + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "time_to_event_prediction", + TimeColumn: "t", + LabelPolicy: "bucket", + LabelColumn: "churned", + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.TimeColumn != "" { + t.Errorf("TimeColumn = %q survived the change to tabular_classification", a.Spec.TimeColumn) + } + if a.Spec.LabelPolicy != "" { + t.Errorf("LabelPolicy = %q survived the change", a.Spec.LabelPolicy) + } + // Not a blanket wipe: the label column IS in scope for the new task, and + // the answer just given must stand. + if a.Spec.LabelColumn != "churned" { + t.Errorf("LabelColumn = %q, want the answer just given", a.Spec.LabelColumn) + } + if err := rejectMisappliedTaskValues(a); err != nil { + t.Errorf("the state left behind is one the guard rejects: %v", err) + } +} + +// The same within the image family, where the value dropped is a number rather +// than a string and two neighbouring flags must survive. +func TestGuided_KeypointsGoWhenTheTaskStopsUsingThem(t *testing.T) { + dir := imageDirLayout(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "image_classification", + "Which column holds the label?": "label", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + TargetSizeFlag: "224x224", + MinSizeFlag: "32x32", + Spec: push.SpecArgs{ + Category: "keypoint_detection", + NumberOfKeypoints: 17, + LabelColumn: "label", + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.NumberOfKeypoints != 0 { + t.Errorf("NumberOfKeypoints = %d survived the change to image_classification", a.Spec.NumberOfKeypoints) + } + if a.TargetSizeFlag == "" || a.MinSizeFlag == "" { + t.Errorf("image flags were wiped though both tasks are image tasks: target=%q min=%q", + a.TargetSizeFlag, a.MinSizeFlag) + } +} + +// The reset must be a no-op when the picked task is the supplied one — the +// tests above cannot tell "cleared what the change left behind" from "cleared +// everything", and clearing everything would silently discard flags the user +// meant and the prompts pre-fill from. +func TestGuided_TaskScopedValuesSurviveWhenTheTaskIsUnchanged(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "time_to_event_prediction", + "Which column holds the value to predict?": "days", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "time_to_event_prediction", + TimeColumn: "tenure_days", + LabelPolicy: "passthrough", + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.TimeColumn != "tenure_days" { + t.Errorf("TimeColumn = %q, want the supplied value to survive as the prompt default", a.Spec.TimeColumn) + } + if a.Spec.LabelPolicy != "passthrough" { + t.Errorf("LabelPolicy = %q, want the supplied value to survive", a.Spec.LabelPolicy) + } +} + +// Every row's rejection must still fire for a task outside its scope, and must +// name its own flag. Without the name check a row could carry another row's +// message — the table makes that a copy-paste away, and the resulting error +// would send the user to a flag they never passed. +func TestEveryTaskScopedValueRejectsOutOfScopeAndNamesItsFlag(t *testing.T) { + for _, v := range taskScopedValues { + t.Run(v.flag, func(t *testing.T) { + // Find a task this value does NOT apply to, from the real task list + // rather than a hand-picked one, so a scope widened later still + // finds its counterexample or fails loudly here. + var out string + for _, cat := range push.SupportedCategoryIDs() { + if !v.inScope(cat) { + out = cat + break + } + } + if out == "" { + t.Fatalf("%s applies to every supported task — it is not task-scoped", v.flag) + } + msg := v.message(out) + if !strings.Contains(msg, v.flag) { + t.Errorf("message %q does not name its own flag %s", msg, v.flag) + } + if !strings.Contains(msg, out) { + t.Errorf("message %q does not name the task it was rejected for", msg) + } + }) + } +} + +// A flag misapplied on the COMMAND LINE must still be rejected in guided mode. +// The first version of the reset cleared everything out of scope for the picked +// task, which also cleared a flag that was wrong from the start — so pressing +// Enter on the pre-selected task silently dropped it while the identical +// invocation under --no-input exited 2. Guided mode quietly meaning something +// different from the flags it echoes is worse than the bug it replaced. +func TestGuided_AMisappliedFlagIsStillRejectedWhenTheTaskIsKept(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "tabular_classification", + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{Category: "tabular_classification", TimeColumn: "t"}, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.TimeColumn != "t" { + t.Fatalf("TimeColumn = %q — the reset swallowed a flag no task change walked away from", a.Spec.TimeColumn) + } + if err := rejectMisappliedTaskValues(a); err == nil { + t.Error("the misapplied --time-column was accepted; --no-input would exit 2 on the same command line") + } +} + +// Same, with no --task supplied at all: the user never declared a task to move +// away from, so nothing was walked away from and the guard still speaks. +func TestGuided_AMisappliedFlagIsStillRejectedWhenNoTaskWasSupplied(t *testing.T) { + dir := tabularDir(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "tabular_classification", + "Which column holds the label?": "churned", + }} + a := &runDataIngestArgs{LocalPath: dir, Spec: push.SpecArgs{TimeColumn: "t"}} + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if err := rejectMisappliedTaskValues(a); err == nil { + t.Error("a --time-column with no --task was silently dropped rather than rejected") + } +} + +// An UNRECOGNIZED --task is not a task the user walked away from, and this is the +// that made the reset silently eat two values at once. Every `inScope` predicate +// answers from the registry, so an unknown id lands on the DEFAULT side of each: +// `!SelfSupervisedText("tabular_classifier")` is true because the lookup +// misses. Pick a self-supervised text task and --label-column then reads as +// "in scope before, out of scope now" and gets cleared — so +// rejectMisappliedTaskValues finds nothing to complain about. The typo is gone +// too: the guided flow runs before the category gate +// (data_ingest_local.go:103 vs :165) and the picker has already overwritten it +// with a valid id, so the run proceeds as though the user typed neither flag, +// where --no-input exits 2 on the same command line. +func TestGuided_AnUnrecognizedTaskIsNotATaskChange(t *testing.T) { + dir := textDirLayout(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "mlm_train", + "Which task?": "masked_language_modeling", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + // A plausible wrong name (not tabular_classification): unknown to the + // registry, so SelfSupervisedText() misses and every inScope() defaults + // to true. Spelled correctly on purpose — `misspell` is a lint gate here. + Spec: push.SpecArgs{Category: "tabular_classifier", LabelColumn: "churned"}, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.LabelColumn != "churned" { + t.Fatalf("LabelColumn = %q — the reset cleared it on behalf of a task that does not exist", a.Spec.LabelColumn) + } + if err := rejectMisappliedTaskValues(a); err == nil { + t.Error("a --label-column misapplied alongside an unrecognized --task was accepted; --no-input would exit 2") + } +} + +// The edge Shujaat named on review: the user changes A -> B and the flag was +// invalid for BOTH. It is tempting to treat a re-pick as blanket permission to +// drop, but the flag was already wrong when they typed it and no answer they +// gave walks away from it — so it must still be rejected, exactly as it would +// be under --no-input. `v.inScope(from)` is what makes that true; a reset +// keyed only on the picked task would swallow it. +func TestGuided_AFlagInvalidForBothTasksIsStillRejected(t *testing.T) { + dir := imageDirLayout(t) + f := &fakePrompter{answers: map[string]string{ + "Please name the dataset.": "t", + "Which task?": "image_classification", + "Which column holds the label?": "label", + }} + a := &runDataIngestArgs{ + LocalPath: dir, + Spec: push.SpecArgs{ + Category: "keypoint_detection", + LabelColumn: "label", + // --time-column applies to neither keypoint_detection nor + // image_classification. + TimeColumn: "t", + }, + } + if err := runInteractive(discardPrinter(), f, a); err != nil { + t.Fatalf("runInteractive: %v", err) + } + if a.Spec.TimeColumn != "t" { + t.Fatalf("TimeColumn = %q — a re-pick is not permission to drop a flag that was always wrong", a.Spec.TimeColumn) + } + if err := rejectMisappliedTaskValues(a); err == nil { + t.Error("the misapplied --time-column was accepted after a task change") + } +} + +// The property the shared predicate buys: from ANY legal starting state — the +// values in scope for the task the user arrived with — a change to ANY other +// task leaves a state the guard accepts. Both sides are built from the table, +// so a scope widened later is exercised here without editing this test. +func TestAnyLegalStateSurvivesAnyTaskChange(t *testing.T) { + cats := push.SupportedCategoryIDs() + for _, from := range cats { + for _, to := range cats { + if from == to { + continue + } + t.Run(from+"->"+to, func(t *testing.T) { + a := &runDataIngestArgs{Spec: push.SpecArgs{Category: from}} + for _, v := range taskScopedValues { + if v.inScope(from) { + v.set(a) + } + } + if err := rejectMisappliedTaskValues(a); err != nil { + t.Fatalf("the starting state is not legal, so this proves nothing: %v", err) + } + a.Spec.Category = to + dropValuesLeftBehindByATaskChange(a, from) + if err := rejectMisappliedTaskValues(a); err != nil { + t.Errorf("after the change the guard rejects: %v", err) + } + }) + } + } +} diff --git a/internal/cli/testdata/golden/01-data-ingest.golden b/internal/cli/testdata/golden/01-data-ingest.golden index 678512a..15f6427 100644 --- a/internal/cli/testdata/golden/01-data-ingest.golden +++ b/internal/cli/testdata/golden/01-data-ingest.golden @@ -9,7 +9,8 @@ so the task-specific questions are visible; each core question prints as a like resolution or schema) as their own header, the supporting line beneath it, and the `?` line shows your answer. The run (shown once, for tabular) is the three steps + the final summary as the CLI renders -them. Passing flags (--as, --task, a path, …) skips the matching questions. The +them. Values passed as flags (--as, --task, a path, …) pre-fill the matching +questions rather than skipping them — guided mode always asks. The other tasks' extra questions (keypoints, label policy, time column), self-supervised text (which skips the label question), and the failure-summary wordings are in zz-all-strings.golden. The raw ingestor stream the CLI streams diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 368f832..3dec5c0 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -44,7 +44,6 @@ screen. %s/%d are runtime placeholders. "%s has a header but no data rows (0 ingestable records). Add at least one data row and re-run." "%s is empty — add a header and at least one data row, then re-run" "%s is empty — no header row" -"%s is image tasks only; it doesn't apply to task %q" "%s of %s GiB" "%s of %s cores" "%s requires CLIENT_WRITE permission" @@ -78,10 +77,12 @@ screen. %s/%d are runtime placeholders. "-%02d" "--%s has no effect without --seal" "--label-column doesn't apply to task %q — it trains on the text itself, with no label column" +"--min-size is image tasks only; it doesn't apply to task %q" "--number-of-keypoints is keypoint_detection only; it doesn't apply to task %q" "--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)." "--schema is empty; expected col:TYPE,col:TYPE,..." "--schema is tabular/time-series tasks only; it doesn't apply to task %q" +"--target-size is image tasks only; it doesn't apply to task %q" "--time-column is time_to_event_prediction only; it doesn't apply to task %q" "--timeout has no effect without --wait or --seal" "--wait and --seal are separate modes — run them one at a time" @@ -369,7 +370,6 @@ screen. %s/%d are runtime placeholders. "backend" "backend %s — requesting a device code …" "backfilling the cluster anchor onto the existing client: %w" -"bucket" "bucket bins the target before it leaves the cluster" "building SPDY transport: %w" "building rest config from kubeconfig: %w" @@ -606,7 +606,6 @@ screen. %s/%d are runtime placeholders. "the sign-in code expired — re-run `tracebloc login`" "the size your images already are; tracebloc checks it, it never resizes" "this machine has %s, but you asked for %s." -"time" "time column" "token saved to ~/.tracebloc (0600)" "total records" @@ -621,7 +620,6 @@ screen. %s/%d are runtime placeholders. "tracebloc's downloaded images" "tracebloc-doctor-%s.txt" "tracebloc-stage-%s-%s" -"train" "unavailable" "unknown command %q for %q" "upgrade didn't complete (%w). You can run the installer directly:\n %s" From 26cdd1cc13d4750180a7b436c3a0a9014b9d8f52 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Sat, 15 Aug 2026 09:45:53 +0200 Subject: [PATCH 2/3] chore(release): bump VERSION to 0.10.8 for next cycle (#512) --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 2d993c4..1a46c7f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.7 +0.10.8 From 5ad2a0e57075aa775de06a407348716746b5789d Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Sat, 15 Aug 2026 10:45:13 +0200 Subject: [PATCH 3/3] fix(interactive): resolve --label-column case-insensitively before pre-fill (#513) defaultInOptions matches exactly, so a --label-column differing from the CSV header only in case failed the match and fell through to defaultLabelChoice -- the FIRST column when nothing is named "label". Enter then accepted that wrong column silently. Until #505 a supplied --label-column skipped the prompt entirely and its spelling was kept verbatim, so it never had to agree with the header. Now that the question is always asked, the mismatch became reachable. canonicalHeader resolves the supplied value to the header's own spelling before the guard. Resolving there rather than loosening defaultInOptions keeps the case-insensitivity where the options are user data -- defaultInOptions also guards --intent and --label-policy against fixed vocabularies, which should stay exact. A value matching nothing is returned unchanged, so a genuine typo is still caught rather than case-folded into a hit. Mutation-proved: reverting the canonicalHeader call reddens both new rows with "Income" -> "age" and "CHURNED" -> "age", the defect verbatim. Found by Bugbot on release-train promotion PR cli#511. Co-authored-by: Claude Opus 5 --- internal/cli/interactive.go | 35 +++++++++++++++++++++++++++----- internal/cli/interactive_test.go | 12 ++++++++++- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index bf3d05c..27d1a50 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -625,22 +625,47 @@ func promptCategorySpecific(p *ui.Printer, pr prompter, a *runDataIngestArgs) (b // named "label" if present; otherwise — the header isn't readable yet — it falls // back to free text pre-filled with the supplied value so the flow never stalls. // -// supplied is guarded through defaultInOptions before it reaches survey.Select: -// a mistyped --label-column that is not one of the real headers would otherwise -// abort the prompt on a TTY, the same default-not-in-options crash guarded -// everywhere else in the guided flow. +// supplied is resolved to the header's own spelling (canonicalHeader) and then +// guarded through defaultInOptions before it reaches survey.Select: a mistyped +// --label-column that is not one of the real headers would otherwise abort the +// prompt on a TTY, the same default-not-in-options crash guarded everywhere +// else in the guided flow. func promptLabelColumn(pr prompter, category, root, question, supplied string) (string, error) { headers, err := push.PreviewLabelHeaders(category, root) if err == nil && len(headers) > 0 { ans, serr := pr.Select(question, "pick the label/target column from your CSV header", headers, - defaultInOptions(supplied, headers, defaultLabelChoice(headers))) + defaultInOptions(canonicalHeader(supplied, headers), headers, defaultLabelChoice(headers))) return strings.TrimSpace(ans), serr } ans, ierr := pr.Input(question, "the label/target column name", supplied, nil) return strings.TrimSpace(ans), ierr } +// canonicalHeader returns the header that matches want case-insensitively, so a +// --label-column differing from the CSV only in case pre-selects the real column +// instead of silently falling back to the header default. +// +// Why this is needed at all: defaultInOptions matches EXACTLY, and until #505 a +// supplied --label-column skipped the prompt entirely, so its spelling was kept +// verbatim and never had to agree with the header. Now that the question is +// always asked, an unresolved value falls through to defaultLabelChoice — the +// FIRST column when nothing is named "label" — and Enter accepts that wrong +// column. Resolving here (rather than loosening defaultInOptions, which also +// guards --intent and --label-policy against fixed vocabularies) keeps the +// case-insensitivity where the options are user data. +// +// Returns want unchanged when nothing matches, leaving defaultInOptions to apply +// the fallback: a genuine typo must still be caught, not case-folded into a hit. +func canonicalHeader(want string, headers []string) string { + for _, h := range headers { + if strings.EqualFold(h, want) { + return h + } + } + return want +} + // defaultLabelChoice pre-highlights a column literally named "label" // (case-insensitive) when one exists, else the first column — a sensible // starting point for the SELECT. diff --git a/internal/cli/interactive_test.go b/internal/cli/interactive_test.go index 56a5ff2..e96a259 100644 --- a/internal/cli/interactive_test.go +++ b/internal/cli/interactive_test.go @@ -700,7 +700,9 @@ func TestRunInteractive_SuppliedLabelColumnStillAsks(t *testing.T) { // // Mutation-proof: passing `supplied` straight to pr.Select (dropping the guard) // makes the "mistyped" row error under the strict fake; not threading the value -// at all makes the "valid-supplied" row return the header default instead. +// at all makes the "valid-supplied" row return the header default instead; +// dropping canonicalHeader (resolving case) reddens the case-mismatch rows, +// which resolve to "age" — the first column — exactly as the defect did. func TestPromptLabelColumn_SuppliedDefaultPrefillsAndGuards(t *testing.T) { dir := tabularDir(t) // header: age,income,churned (no column named "label") const cat = "tabular_classification" @@ -714,6 +716,14 @@ func TestPromptLabelColumn_SuppliedDefaultPrefillsAndGuards(t *testing.T) { {"valid-supplied-is-preselected", "income", "income"}, {"empty-supplied-uses-header-default", "", "age"}, // defaultLabelChoice → first header {"mistyped-supplied-falls-back", "incom", "age"}, // guarded, no crash + + // Case-only differences must bind the HEADER's spelling, not fall back. + // Before the canonicalHeader resolve these landed on "age": the exact + // match failed, and with no column named "label" defaultLabelChoice + // returns the first column — which Enter then silently accepts as the + // label. That is the whole defect, so these rows are the regression. + {"case-mismatch-supplied-resolves", "Income", "income"}, + {"case-mismatch-uppercase-resolves", "CHURNED", "churned"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) {