release-train: develop -> staging - #511
Merged
Merged
Conversation
…stion (#505) * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c16af41. Configure here.
Contributor
Author
|
bugbot run |
tracebloc-release-train
Bot
added
the
gate-nudge
4 tasks
tracebloc-release-train
Bot
removed
the
gate-nudge
…e-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 <noreply@anthropic.com>
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5ad2a0e. Configure here.
tracebloc-release-train
Bot
added
gate-nudge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated promotion by the release train (RFC-0008 D14). Head is the train-managed
release-train/to-stagingbranch (a mirror ofdevelop), so it never collides with a human PR. Merged only when the fr-gate is green.Note
Medium Risk
Changes default TTY behavior for a primary CLI workflow (data ingest); non-interactive paths are unchanged but guided UX and task-change flag handling are nuanced and regression-prone without the new tests.
Overview
Bumps VERSION to
0.10.8as part of the develop → staging release train.Guided
tb data ingest(#509) no longer skips steps when flags are already set: intent, name, path, task, and task-specific fields are always asked, with CLI values used only as defaults (Enter keeps them).--taskstill drives the family and pre-selects the picker instead of bypassing it. Review/confirm runs every time on a TTY;--no-input/--output-jsonbehavior is unchanged.Adds
defaultInOptionsandcanonicalHeaderso mistyped--intent,--label-policy, or--label-columnvalues cannot crashsurvey.Selecton a real terminal (#505).Introduces
task_scope.goto centralize task-scoped flag rules:rejectMisappliedTaskValuesreplaces inline checks indata_ingest_local.go, anddropValuesLeftBehindByATaskChangeclears flags that belonged to a previous task only after the user actually changes task in the picker (misapplied flags on the command line are still rejected, including unknown--tasknames).Copy catalog / golden strings updated to describe pre-fill vs skip; broad new tests cover the interactive and task-scope behavior.
Reviewed by Cursor Bugbot for commit 5ad2a0e. Bugbot is set up for automated code reviews on this repo. Configure here.