From e8cf38b4dce0e6fd8890fe6b88923b2f94533777 Mon Sep 17 00:00:00 2001 From: Divya Date: Fri, 7 Aug 2026 15:24:53 +0530 Subject: [PATCH] fix(#386): strip surrounding quotes from ingest path prompt + clarify dataset-name rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part 1: the interactive "Where is your data?" prompt is read literally, not shell-parsed, so a pasted quoted path (dragging a folder into a terminal auto-quotes it; users habitually quote paths with spaces) had the quote chars become part of the path and failed to resolve. Add stripSurroundingQuotes / dequotePath and apply them to the interactive answer before expandHome / statDatasetPath, and inside validateDatasetPath so the re-prompt guard is consistent. Conservative: strips at most one matching outer pair of the same quote char, leaves inner/mismatched quotes untouched, and only touches the guided answer — never the flag/positional path (the shell de-quotes those). Part 2: spell out "no hyphens or spaces — use _" in the name-prompt hint and on the first line of the ValidateTableName error. Wording only; nothing changes about what names are accepted or rejected. Regenerated zz-all-strings.golden for the hint change. Added table-driven tests for the quote helpers and a wording-pin test for the error. Closes #386 Co-Authored-By: Claude Opus 4.8 --- internal/cli/interactive.go | 53 ++++++++++++--- internal/cli/interactive_test.go | 68 +++++++++++++++++++ .../cli/testdata/golden/zz-all-strings.golden | 2 +- internal/push/spec.go | 3 +- internal/push/spec_test.go | 17 +++++ 5 files changed, 132 insertions(+), 11 deletions(-) diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go index c388483a..aff4fe8f 100644 --- a/internal/cli/interactive.go +++ b/internal/cli/interactive.go @@ -201,7 +201,7 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo p.PromptStep(2, 4, "Please name the dataset.") p.Newline() ans, err := pr.Input("Please name the dataset.", - "letters, digits, and underscores; 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", "", push.ValidateTableName) if err != nil { return err @@ -224,12 +224,16 @@ func runInteractive(p *ui.Printer, pr prompter, a *runDataIngestArgs, taskSet bo if err != nil { return err } - // Trim before storing: validateDatasetPath only trims to check for - // emptiness, so a pasted " ~/data" (stray leading/trailing space) - // would otherwise survive here and defeat expandHome (first char - // isn't '~') — filepath.Abs then prepends cwd and the sniff / label - // preview read a path that doesn't exist. - a.LocalPath = strings.TrimSpace(ans) + // Canonicalize before storing: dequotePath trims stray leading/ + // trailing space (a pasted " ~/data" would otherwise defeat expandHome, + // whose first char isn't '~', and filepath.Abs would prepend cwd so the + // sniff / label preview read a path that doesn't exist) AND strips one + // matching pair of surrounding quotes. Dragging a folder into a terminal + // auto-quotes it, and users habitually quote paths with spaces; this + // prompt is read literally, not shell-parsed, so the quotes would + // 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 @@ -576,14 +580,45 @@ func renderReview(p *ui.Printer, a *runDataIngestArgs) { // validateDatasetPath rejects an empty / whitespace-only answer. Without // it, a bare Enter at the path prompt yields "" — and SniffFamily(Abs("")) // would sniff the current working directory before any empty-path guard -// runs, silently ingesting whatever happens to sit in the cwd. +// runs, silently ingesting whatever happens to sit in the cwd. It validates +// the canonicalized value (dequotePath) so an answer that is nothing but an +// empty pair of quotes is rejected at the prompt rather than surviving to +// statDatasetPath with a messy error (#386). func validateDatasetPath(s string) error { - if strings.TrimSpace(s) == "" { + if dequotePath(s) == "" { return fmt.Errorf("a dataset path is required") } return nil } +// dequotePath canonicalizes an interactive path answer: it trims surrounding +// whitespace, then strips one matching pair of surrounding quotes (via +// stripSurroundingQuotes), then trims again in case the quotes wrapped padding. +// It is applied ONLY to the guided prompt answer, never to the flag/positional +// path — the shell already de-quotes those. +func dequotePath(s string) string { + return strings.TrimSpace(stripSurroundingQuotes(strings.TrimSpace(s))) +} + +// stripSurroundingQuotes removes at most one matching pair of surrounding +// single or double quotes from s, and only when the first and last rune are +// the SAME quote char (' or ') and s has at least two runes. Inner quotes are +// left untouched, mismatched quotes ('…" ) are left as-is, and a path whose +// real name contains a quote loses only that single matched outer pair. This +// exists because the interactive path prompt is read literally, not +// shell-parsed, so a pasted quoted path would otherwise fail to resolve (#386). +func stripSurroundingQuotes(s string) string { + r := []rune(s) + if len(r) < 2 { + return s + } + first, last := r[0], r[len(r)-1] + if (first == '\'' || first == '"') && first == last { + return string(r[1 : len(r)-1]) + } + return s +} + // validatePositiveInt accepts a string that parses to an int > 0. func validatePositiveInt(s string) error { if n, err := strconv.Atoi(strings.TrimSpace(s)); err != nil || n <= 0 { diff --git a/internal/cli/interactive_test.go b/internal/cli/interactive_test.go index 65b26f42..548fc66f 100644 --- a/internal/cli/interactive_test.go +++ b/internal/cli/interactive_test.go @@ -644,3 +644,71 @@ func orderedSubsequence(got, want []string) bool { } return i == len(want) } + +// TestStripSurroundingQuotes / TestDequotePath / TestValidateDatasetPathDequote +// pin the interactive path-prompt quote handling (#386): a pasted quoted path +// must resolve identically to the bare path, an unquoted path with spaces must +// survive untouched, and a name that really contains a quote must not be +// corrupted beyond the single matched outer pair. +func TestStripSurroundingQuotes(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"single-quoted", "'/a/b'", "/a/b"}, + {"double-quoted", `"/a/b"`, "/a/b"}, + {"bare", "/a/b", "/a/b"}, + {"bare-with-spaces", "/a/b c/train", "/a/b c/train"}, + {"quoted-with-spaces", "'/home/me/my data/train'", "/home/me/my data/train"}, + {"name-contains-a-quote-unquoted", "/a/b's", "/a/b's"}, + {"name-contains-a-quote-quoted-strips-one-pair", "'/a/b's'", "/a/b's"}, + {"inner-quotes-left-untouched", `/a/"b"/c`, `/a/"b"/c`}, + {"mismatched-quotes-left-untouched", `'/a/b"`, `'/a/b"`}, + {"single-char-not-stripped", "'", "'"}, + {"empty", "", ""}, + {"empty-single-quotes", "''", ""}, + {"empty-double-quotes", `""`, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := stripSurroundingQuotes(tc.in); got != tc.want { + t.Errorf("stripSurroundingQuotes(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestDequotePath(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"quoted-with-surrounding-space", " '/a/b' ", "/a/b"}, + {"double-quoted", `"/a/b"`, "/a/b"}, + {"bare-with-interior-spaces", " /a/b c ", "/a/b c"}, + {"quoted-path-with-spaces", "'/home/me/my data/train'", "/home/me/my data/train"}, + {"only-quotes", "''", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := dequotePath(tc.in); got != tc.want { + t.Errorf("dequotePath(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestValidateDatasetPathRejectsEmptyQuotes(t *testing.T) { + for _, in := range []string{"", " ", "''", `""`, " '' "} { + if err := validateDatasetPath(in); err == nil { + t.Errorf("validateDatasetPath(%q) = nil, want a required-path error", in) + } + } + for _, in := range []string{"'/a/b'", "/a/b", "/a/b c"} { + if err := validateDatasetPath(in); err != nil { + t.Errorf("validateDatasetPath(%q) = %v, want nil", in, err) + } + } +} diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 46dd8d40..e3cb006c 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -460,7 +460,7 @@ screen. %s/%d are runtime placeholders. "label column" "label policy" "labels.csv" -"letters, digits, and underscores; 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" "listing Pods for service %s/%s: %w" "listing chart-managed deployments in namespace %s: %w" "listing client deployments to check for an existing client: %w" diff --git a/internal/push/spec.go b/internal/push/spec.go index a6c862d1..b993fcf3 100644 --- a/internal/push/spec.go +++ b/internal/push/spec.go @@ -111,7 +111,8 @@ func ValidateTableName(table string) error { } if !tableNamePattern.MatchString(table) { return fmt.Errorf( - "%q won't work — use letters, digits, and underscores, "+ + "%q won't work — no hyphens or spaces (use _); "+ + "use letters, digits, and underscores, "+ "starting with a letter or underscore (e.g. churn_train)", table) } diff --git a/internal/push/spec_test.go b/internal/push/spec_test.go index 2e131bae..225183dd 100644 --- a/internal/push/spec_test.go +++ b/internal/push/spec_test.go @@ -666,3 +666,20 @@ func TestBuild_EmitsDetectedExtension_PassesSchema(t *testing.T) { }) } } + +// TestValidateTableName_ErrorMentionsHyphensAndSpaces pins the #386 wording: +// the pattern-rejection error's first clause must spell out that hyphens and +// spaces are not allowed and that _ is the separator — kebab-case (tsc-train) +// is the common mistake, so the rejection has to say so up front. +func TestValidateTableName_ErrorMentionsHyphensAndSpaces(t *testing.T) { + err := ValidateTableName("tsc-train") + if err == nil { + t.Fatal("ValidateTableName(\"tsc-train\") = nil, want a rejection") + } + msg := err.Error() + for _, want := range []string{"no hyphens or spaces", "use _"} { + if !strings.Contains(msg, want) { + t.Errorf("error %q does not mention %q", msg, want) + } + } +}