From 42f2572616603854b2ef8da1de2489abfa398eb8 Mon Sep 17 00:00:00 2001 From: jamesread Date: Tue, 7 Jul 2026 11:57:20 +0100 Subject: [PATCH] fix(config): reject ambiguous checklist comma format and propagate encode errors ParseChecklistValue now accepts JSON arrays or single bare values and rejects legacy comma-separated input. Empty JSON segments are validated consistently, and FormatChecklistValue returns marshal errors instead of an empty string --- service/internal/config/checklist.go | 29 +++++++---------- service/internal/config/checklist_test.go | 31 ++++++++++++++----- service/internal/executor/arguments.go | 11 +++++-- service/internal/executor/arguments_test.go | 18 +++++------ .../internal/executor/log_arguments_test.go | 2 +- 5 files changed, 53 insertions(+), 38 deletions(-) diff --git a/service/internal/config/checklist.go b/service/internal/config/checklist.go index ab53516..1714b0f 100644 --- a/service/internal/config/checklist.go +++ b/service/internal/config/checklist.go @@ -7,7 +7,7 @@ import ( ) // ParseChecklistValue parses a checklist argument wire value. -// New values are JSON arrays; legacy comma-separated values are still accepted. +// Values must be JSON arrays, or a single choice without commas. func ParseChecklistValue(value string) ([]string, error) { trimmed := strings.TrimSpace(value) if trimmed == "" { @@ -18,7 +18,11 @@ func ParseChecklistValue(value string) ([]string, error) { return parseJSONChecklistValue(trimmed) } - return parseLegacyChecklistValue(value) + if strings.Contains(trimmed, ",") { + return nil, fmt.Errorf("checklist value uses legacy comma-separated format; use a JSON array instead") + } + + return []string{trimmed}, nil } func parseJSONChecklistValue(value string) ([]string, error) { @@ -27,34 +31,25 @@ func parseJSONChecklistValue(value string) ([]string, error) { return nil, fmt.Errorf("invalid checklist JSON value: %w", err) } - return values, nil -} - -func parseLegacyChecklistValue(value string) ([]string, error) { - segments := strings.Split(value, ",") - values := make([]string, 0, len(segments)) - for _, segment := range segments { - trimmedSegment := strings.TrimSpace(segment) - if trimmedSegment == "" { + for _, segment := range values { + if strings.TrimSpace(segment) == "" { return nil, fmt.Errorf("checklist value contains an empty segment") } - - values = append(values, trimmedSegment) } return values, nil } // FormatChecklistValue serializes selected checklist values for API transport. -func FormatChecklistValue(values []string) string { +func FormatChecklistValue(values []string) (string, error) { if len(values) == 0 { - return "" + return "", nil } encoded, err := json.Marshal(values) if err != nil { - return "" + return "", fmt.Errorf("encoding checklist value: %w", err) } - return string(encoded) + return string(encoded), nil } diff --git a/service/internal/config/checklist_test.go b/service/internal/config/checklist_test.go index f735215..efd378e 100644 --- a/service/internal/config/checklist_test.go +++ b/service/internal/config/checklist_test.go @@ -19,25 +19,40 @@ func TestParseChecklistValueJSON(t *testing.T) { assert.Equal(t, []string{"kitchen,bedroom", "hallway"}, values) } -func TestParseChecklistValueLegacyCommaSeparated(t *testing.T) { +func TestParseChecklistValueSingleValue(t *testing.T) { t.Parallel() - values, err := ParseChecklistValue("documents, photos") + values, err := ParseChecklistValue("documents") require.NoError(t, err) - assert.Equal(t, []string{"documents", "photos"}, values) + assert.Equal(t, []string{"documents"}, values) } -func TestParseChecklistValueRejectsEmptyLegacySegment(t *testing.T) { +func TestParseChecklistValueRejectsLegacyCommaSeparated(t *testing.T) { t.Parallel() - _, err := ParseChecklistValue("documents,,photos") + _, err := ParseChecklistValue("documents, photos") + require.Error(t, err) +} + +func TestParseChecklistValueRejectsEmptyJSONSegment(t *testing.T) { + t.Parallel() + + _, err := ParseChecklistValue(`["documents","","photos"]`) require.Error(t, err) } func TestFormatChecklistValueJSON(t *testing.T) { t.Parallel() - assert.Equal(t, `["documents","photos"]`, FormatChecklistValue([]string{"documents", "photos"})) - assert.Equal(t, `["kitchen,bedroom"]`, FormatChecklistValue([]string{"kitchen,bedroom"})) - assert.Empty(t, FormatChecklistValue(nil)) + encoded, err := FormatChecklistValue([]string{"documents", "photos"}) + require.NoError(t, err) + assert.Equal(t, `["documents","photos"]`, encoded) + + encoded, err = FormatChecklistValue([]string{"kitchen,bedroom"}) + require.NoError(t, err) + assert.Equal(t, `["kitchen,bedroom"]`, encoded) + + encoded, err = FormatChecklistValue(nil) + require.NoError(t, err) + assert.Empty(t, encoded) } diff --git a/service/internal/executor/arguments.go b/service/internal/executor/arguments.go index c4da44c..4b8ac4b 100644 --- a/service/internal/executor/arguments.go +++ b/service/internal/executor/arguments.go @@ -499,16 +499,21 @@ func mangleChecklistValue(arg *config.ActionArgument, value string, actionTitle return value } - return mangleChecklistSegments(arg, segments, actionTitle) + return mangleChecklistSegments(arg, segments, value, actionTitle) } -func mangleChecklistSegments(arg *config.ActionArgument, segments []string, actionTitle string) string { +func mangleChecklistSegments(arg *config.ActionArgument, segments []string, fallback string, actionTitle string) string { mangled := make([]string, len(segments)) for i, segment := range segments { mangled[i] = mangleChecklistSegment(arg, segment, actionTitle) } - return config.FormatChecklistValue(mangled) + formatted, err := config.FormatChecklistValue(mangled) + if err != nil { + return fallback + } + + return formatted } func mangleChecklistSegment(arg *config.ActionArgument, segment string, actionTitle string) string { diff --git a/service/internal/executor/arguments_test.go b/service/internal/executor/arguments_test.go index 4c82a29..3c2a917 100644 --- a/service/internal/executor/arguments_test.go +++ b/service/internal/executor/arguments_test.go @@ -136,10 +136,10 @@ func TestValidateArgumentChecklistSelections(t *testing.T) { err := ValidateArgument(&arg, "documents", &action) assert.Nil(t, err) - err = ValidateArgument(&arg, "documents,photos", &action) + err = ValidateArgument(&arg, `["documents","photos"]`, &action) assert.Nil(t, err) - err = ValidateArgument(&arg, "documents,unknown", &action) + err = ValidateArgument(&arg, `["documents","unknown"]`, &action) assert.NotNil(t, err) } @@ -149,7 +149,7 @@ func TestValidateArgumentChecklistTitleMangling(t *testing.T) { arg := checklistTestArg() action := config.Action{Title: "Test checklist title mangling"} - err := ValidateArgument(&arg, "Documents,Photos", &action) + err := ValidateArgument(&arg, `["Documents","Photos"]`, &action) assert.Nil(t, err) } @@ -186,7 +186,7 @@ func TestValidateArgumentChecklistRejectsEmptySegment(t *testing.T) { arg := checklistTestArg() action := config.Action{Title: "Test checklist empty segment"} - err := ValidateArgument(&arg, "documents,,photos", &action) + err := ValidateArgument(&arg, `["documents","","photos"]`, &action) assert.NotNil(t, err) } @@ -195,10 +195,10 @@ func TestMangleArgumentValueChecklist(t *testing.T) { arg := checklistTestArg() - out := MangleArgumentValue(&arg, "Documents,Music", "Test action") + out := MangleArgumentValue(&arg, `["Documents","Music"]`, "Test action") assert.Equal(t, `["documents","music"]`, out) - out = MangleArgumentValue(&arg, "documents,photos", "Test action") + out = MangleArgumentValue(&arg, `["documents","photos"]`, "Test action") assert.Equal(t, `["documents","photos"]`, out) } @@ -225,10 +225,10 @@ func TestValidateArgumentChecklistEntitySelections(t *testing.T) { err := ValidateArgument(&arg, "attic", &action) assert.Nil(t, err) - err = ValidateArgument(&arg, "attic,basement", &action) + err = ValidateArgument(&arg, `["attic","basement"]`, &action) assert.Nil(t, err) - err = ValidateArgument(&arg, "attic,unknown", &action) + err = ValidateArgument(&arg, `["attic","unknown"]`, &action) assert.NotNil(t, err) } @@ -247,7 +247,7 @@ func TestMangleArgumentValueChecklistEntityTitles(t *testing.T) { }, } - out := MangleArgumentValue(&arg, "attic room,basement room", "Test checklist entity titles") + out := MangleArgumentValue(&arg, `["attic room","basement room"]`, "Test checklist entity titles") assert.Equal(t, `["attic","basement"]`, out) } diff --git a/service/internal/executor/log_arguments_test.go b/service/internal/executor/log_arguments_test.go index e97f6bc..32642b2 100644 --- a/service/internal/executor/log_arguments_test.go +++ b/service/internal/executor/log_arguments_test.go @@ -92,7 +92,7 @@ func TestStorableArgumentsFromRequestStoresMangledChecklistValue(t *testing.T) { }, } req.Arguments = map[string]string{ - "directories": "Documents,Photos", + "directories": `["Documents","Photos"]`, } mangleInvalidArgumentValues(req)