Uh oh!
There was an error while loading. Please reload this page.
fix(data ingest): #197 review follow-ups — picker crash + README/help/label - #199
Merged
Conversation
- interactive picker no longer passes an empty default to survey.Select after --task's default was dropped (crashed "omit --task to pick interactively" on a TTY); falls back to the first task. - rename the guided name prompt "Destination table name" -> "Dataset name" to match --name / the Review + summary labels (finish the #180 rename). - --name flag help now states the leading letter/underscore rule the tightened validator enforces. - README quickstart: --split train -> --intent train (--split was removed; --intent stayed canonical). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
saadqbal
reviewed
Jul 9, 2026
saadqbal
left a comment
Collaborator
There was a problem hiding this comment.
Fix does what it says — picker crash, README flag, label + help rename all check out. A few loose ends from the same rename; pushing the hint + def cleanups on top, leaving the test/drift notes for you.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Two review follow-ups on #199: - The PromptHint above the "Dataset name" prompt still described "the table created on the cluster (and its folder on the shared storage). Letters, digits, underscores only" — it leaked the k8s/MySQL ceremony the rename was removing and stated the old rule (implying a leading digit is allowed, which ValidateTableName rejects). Reworded to plain language matching the flag help + validator. - The picker's default was seeded from a.Spec.Category, which is always "" in the !taskSet branch, so the len-guarded fallback always fired. Seed promptCategories[0] directly — the list is a fixed non-empty package var backed by the hardcoded category registry. go build ./..., go vet, gofmt -l clean; go test ./internal/cli/... ./internal/push/... green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The #197 fix seeds the task picker with promptCategories[0] when the task is unset, since survey.Select rejects an empty default. No test drove taskSet=false with an empty category, so a reseed to "" would have kept the suite green while crashing a real terminal. This case omits the "Task" answer so the fake returns the seeded default, then asserts the category landed on promptCategories[0] — verified to fail if the seed regresses to "". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#180b (#198) landed the data-first flow inversion + family-scoped task picker on develop, which supersedes most of this branch: - The flat Select("Task", ...) picker — and its empty-default crash fix (#197 finding 1/2) — is gone, replaced by resolveFamily + pickTask. Resolved interactive.go to develop's new flow. - The "Destination table name" → "Dataset name" rename is superseded by develop's "What should we call this dataset?" prompt. Resolved the test conflicts to develop's rewritten new-flow suite (my old-flow tests, incl. the seed test, are obsolete). Kept the still-relevant copy fixes: - README quickstart --split → --intent (auto-merged). - --name flag help states the leading-letter rule (auto-merged), and I folded the same wording into develop's interactive name prompt so the prompt and the flag help agree (finding 4 drift, otherwise reintroduced by the merge). gofmt -l clean; go build ./..., go vet, go test ./internal/cli/... ./internal/push/... green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
saadqbal
approved these changes
Jul 10, 2026
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Follow-up fixes for the four CONFIRMED findings from the
/code-reviewof #180a. #197 was merged before that review pass finished, so these landed ondevelopuncorrected — this PR fixes them forward.Findings fixed
--task'simage_classificationdefault (#180a) left the guided picker passing an empty string assurvey.Select's default, which errorsdefault value "" not found in options— so "omit--taskto pick interactively" (the headline flow) crashed on a real terminal. Now falls back to the first task. (Also superseded by the #180b family-scoped picker, but live ondevelopuntil that lands.)--split train— that flag was removed (--intentstayed canonical), so the documented command failed withunknown flag: --split. Changed to--intent train.name— renamed to "Dataset name" to finish the #180a rename.--nameflag help said "(letters, digits, underscore)" but the tightened validator also forbids a leading digit — help now states the "start with a letter or underscore" rule the validator enforces.Test plan
go build ./...,go test ./internal/cli/... ./internal/push/...(green),gofmt -l internal/(clean),go vetclean. Updated theinteractive_test.gofake-prompter keys to the renamed "Dataset name" prompt.🤖 Generated with Claude Code
Note
Low Risk
Documentation and user-facing strings only; no runtime or validation logic changes in this diff.
Overview
Docs and copy fixes so the quickstart and guided ingest flow match the current CLI.
The README Customer experience example now uses
--intent traininstead of the removed--splitflag, so copy-paste installs no longer hitunknown flag: --split.--namehelp and the interactive dataset-name hints now state that names must start with a letter or underscore, then use letters, digits, and underscores — matchingValidateTableName(leading digits were already rejected but help text implied they were allowed).Reviewed by Cursor Bugbot for commit 237e87d. Bugbot is set up for automated code reviews on this repo. Configure here.