Kaynağa Gözat

fix: harden checklist JSON wire format, entity pagination, and review findings across API and UI

jamesread 11 saat önce
ebeveyn
işleme
9dc51df01d

+ 4 - 1
frontend/resources/vue/components/ChoiceCombobox.vue

@@ -152,8 +152,11 @@ function selectedChoiceIndex(choices) {
 
 function openList() {
   document.dispatchEvent(new CustomEvent(closeOthersEvent, { detail: { id: props.id } }))
+  const wasClosed = !isOpen.value
   isOpen.value = true
-  highlightedIndex.value = selectedChoiceIndex(filteredChoices.value)
+  if (wasClosed) {
+    highlightedIndex.value = selectedChoiceIndex(filteredChoices.value)
+  }
 }
 
 function closeList() {

+ 10 - 0
frontend/resources/vue/components/EntityDefinitionSection.vue

@@ -72,6 +72,7 @@
 	const pageSize = ref(10)
 	const tableError = ref('')
 	let fetchTimer = null
+	let fetchSequence = 0
 
 	const hasTable = computed(() => (props.definition.properties?.length ?? 0) > 0)
 
@@ -120,6 +121,7 @@
 			return
 		}
 
+		const requestId = ++fetchSequence
 		tableError.value = ''
 		try {
 			const response = await window.client.getEntities({
@@ -129,10 +131,18 @@
 				pageSize: pageSize.value
 			})
 
+			if (requestId !== fetchSequence) {
+				return
+			}
+
 			const definition = response.entityDefinitions?.find(def => def.title === props.definition.title)
 			tableInstances.value = definition?.instances ?? []
 			totalInstances.value = definition?.totalInstances ?? 0
 		} catch (err) {
+			if (requestId !== fetchSequence) {
+				return
+			}
+
 			console.error('Failed to fetch entity instances:', err)
 			tableError.value = 'Failed to load entity instances.'
 			tableInstances.value = []

+ 20 - 2
frontend/resources/vue/utils/choiceChecklistHelpers.js

@@ -1,9 +1,27 @@
+function parseLegacyChecklistValue(value) {
+  return value.split(',').map((segment) => segment.trim()).filter((segment) => segment !== '')
+}
+
 export function parseChecklistValue(value) {
   if (!value || value === '') {
     return []
   }
 
-  return value.split(',').map((segment) => segment.trim()).filter((segment) => segment !== '')
+  const trimmed = value.trim()
+  if (trimmed.startsWith('[')) {
+    try {
+      const parsed = JSON.parse(trimmed)
+      if (!Array.isArray(parsed)) {
+        return []
+      }
+
+      return parsed.map((segment) => String(segment).trim()).filter((segment) => segment !== '')
+    } catch {
+      return []
+    }
+  }
+
+  return parseLegacyChecklistValue(value)
 }
 
 export function formatChecklistValue(selected) {
@@ -11,7 +29,7 @@ export function formatChecklistValue(selected) {
     return ''
   }
 
-  return selected.join(',')
+  return JSON.stringify(selected)
 }
 
 export function toggleChoice(selected, value) {

+ 10 - 4
frontend/resources/vue/utils/choiceChecklistHelpers.test.mjs

@@ -13,14 +13,20 @@ const choices = [
   { title: 'Photos', value: 'photos' }
 ]
 
-test('parseChecklistValue splits comma-delimited values', () => {
+	test('parseChecklistValue parses JSON-encoded values', () => {
+  assert.deepEqual(parseChecklistValue('["documents","photos"]'), ['documents', 'photos'])
+  assert.deepEqual(parseChecklistValue('["kitchen,bedroom","hallway"]'), ['kitchen,bedroom', 'hallway'])
+  assert.deepEqual(parseChecklistValue(''), [])
+})
+
+test('parseChecklistValue accepts legacy comma-delimited values', () => {
   assert.deepEqual(parseChecklistValue('documents,photos'), ['documents', 'photos'])
   assert.deepEqual(parseChecklistValue('documents, photos'), ['documents', 'photos'])
-  assert.deepEqual(parseChecklistValue(''), [])
 })
 
-test('formatChecklistValue joins selected values', () => {
-  assert.equal(formatChecklistValue(['documents', 'photos']), 'documents,photos')
+test('formatChecklistValue joins selected values as JSON', () => {
+  assert.equal(formatChecklistValue(['documents', 'photos']), '["documents","photos"]')
+  assert.equal(formatChecklistValue(['kitchen,bedroom']), '["kitchen,bedroom"]')
   assert.equal(formatChecklistValue([]), '')
 })
 

+ 8 - 4
frontend/resources/vue/utils/prefilledArguments.js

@@ -7,11 +7,15 @@ export function readPrefilledArgumentsFromNavigation() {
 	return {}
 }
 
-export function getInitialArgumentValue(paramName, prefilledArguments) {
-	if (Object.prototype.hasOwnProperty.call(prefilledArguments, paramName)) {
-		return prefilledArguments[paramName]
+export function getInitialArgumentValue(paramName, prefilledArguments = {}) {
+	const safePrefilledArguments = prefilledArguments && typeof prefilledArguments === 'object'
+		? prefilledArguments
+		: {}
+
+	if (Object.prototype.hasOwnProperty.call(safePrefilledArguments, paramName)) {
+		return safePrefilledArguments[paramName]
 	}
 
-	const params = new URLSearchParams(window.location.search.substring(1))
+	const params = new URLSearchParams(window.location.search)
 	return params.get(paramName)
 }

+ 15 - 16
frontend/resources/vue/views/ArgumentForm.vue

@@ -19,9 +19,12 @@
         <template v-if="actionArguments.length > 0">
 
           <template v-for="arg in actionArguments" :key="arg.name">
-              <label :for="arg.type === 'checklist' ? undefined : arg.name">
+              <label v-if="arg.type !== 'checklist'" :for="arg.name">
                 {{ formatLabel(arg.title) }}
               </label>
+              <div v-else class="argument-label">
+                {{ formatLabel(arg.title) }}
+              </div>
 
               <datalist v-if="(arg.suggestions && Object.keys(arg.suggestions).length > 0) || getBrowserSuggestions(arg).length > 0" :id="`${arg.name}-choices`">
                 <option v-for="(suggestion, key) in arg.suggestions" :key="key" :value="key">
@@ -383,35 +386,31 @@ function formatArgumentValueForApi(arg, rawValue) {
   return rawValue ?? ''
 }
 
-function getArgumentValues() {
-  const ret = []
+function getSelectedArgumentEntries() {
+  const entries = []
 
   for (const arg of actionArguments.value) {
     if (!shouldSendArgument(arg)) {
       continue
     }
 
-    ret.push({
+    entries.push({
       name: arg.name,
       value: formatArgumentValueForApi(arg, argValues.value[arg.name])
     })
   }
 
-  return ret
+  return entries
 }
 
-function getArgumentMapForTemplate() {
-  const args = {}
-
-  for (const arg of actionArguments.value) {
-    if (!shouldSendArgument(arg)) {
-      continue
-    }
-
-    args[arg.name] = formatArgumentValueForApi(arg, argValues.value[arg.name])
-  }
+function getArgumentValues() {
+  return getSelectedArgumentEntries().map(({ name, value }) => ({ name, value }))
+}
 
-  return args
+function getArgumentMapForTemplate() {
+  return Object.fromEntries(
+    getSelectedArgumentEntries().map(({ name, value }) => [name, value])
+  )
 }
 
 function updateJustificationFromTemplate() {

+ 1 - 0
frontend/resources/vue/views/EntityDetailsView.vue

@@ -63,6 +63,7 @@
 	<section v-if="entityDetails && relatedActions.length > 0" class="transparent">
 		<div class="dashboard-row">
 			<fieldset>
+				<legend class="visually-hidden">Related actions</legend>
 				<template v-for="(related, idx) in relatedActions" :key="related.action?.bindingId || idx">
 					<ActionButton
 						v-if="related.action"

+ 15 - 5
integration-tests/tests/checklist/checklist.mjs

@@ -62,7 +62,17 @@ async function waitForTerminalOutput(expectedValue, label = 'Selected segments')
 async function waitForTerminalOutputPattern(pattern) {
   await pollTerminal(
     (output) => pattern.test(output),
-    10000
+    DEFAULT_UI_WAIT_MS
+  )
+}
+
+async function waitForChecklistValue(expectedValue) {
+  await webdriver.wait(
+    new Condition('wait for checklist hidden value', async () => {
+      const valueInput = await webdriver.findElement(By.css('.choice-checklist > input'))
+      return (await valueInput.getAttribute('value')) === expectedValue
+    }),
+    DEFAULT_UI_WAIT_MS
   )
 }
 
@@ -108,7 +118,7 @@ describe('config: checklist', function () {
 
     const selectNone = await webdriver.findElement(By.xpath("//button[normalize-space()='Select none']"))
     await selectNone.click()
-    await webdriver.sleep(300)
+    await waitForChecklistValue('')
 
     const valueInput = await webdriver.findElement(By.css('.choice-checklist > input'))
     expect(await valueInput.getAttribute('value')).to.equal('')
@@ -131,7 +141,7 @@ describe('config: checklist', function () {
     await submitChecklistForm()
     await waitForLogsPage()
     await waitForExecutionComplete()
-    await waitForTerminalOutput('kitchen,bedroom,hallway')
+    await waitForTerminalOutput('["kitchen","bedroom","hallway"]')
   })
 
   it('Checklist toggles individual choices before submit', async function () {
@@ -143,7 +153,7 @@ describe('config: checklist', function () {
     await submitChecklistForm()
     await waitForLogsPage()
     await waitForExecutionComplete()
-    await waitForTerminalOutput('kitchen,bedroom,hallway')
+    await waitForTerminalOutput('["kitchen","bedroom","hallway"]')
   })
 
   it('Checklist entity argument renders choices from entities', async function () {
@@ -163,6 +173,6 @@ describe('config: checklist', function () {
     await submitChecklistForm()
     await waitForLogsPage()
     await waitForExecutionComplete()
-    await waitForTerminalOutput('attic', 'Selected rooms')
+    await waitForTerminalOutput('["attic"]', 'Selected rooms')
   })
 })

+ 14 - 7
service/internal/api/api_entities_list.go

@@ -9,7 +9,10 @@ import (
 	"github.com/OliveTin/OliveTin/internal/entities"
 )
 
-const defaultEntityInstancesPageSize = 10
+const (
+	defaultEntityInstancesPageSize = 10
+	maxEntityInstancesPageSize     = 100
+)
 
 func (api *oliveTinAPI) buildEntityDefinitionsResponse(req *apiv1.GetEntitiesRequest, entityMap entities.EntitiesByClass) []*apiv1.EntityDefinition {
 	if req != nil && req.EntityType != "" {
@@ -91,6 +94,9 @@ func normalizeEntityInstancesPageSize(pageSize int32) int32 {
 	if pageSize < 1 {
 		return defaultEntityInstancesPageSize
 	}
+	if pageSize > maxEntityInstancesPageSize {
+		return maxEntityInstancesPageSize
+	}
 	return pageSize
 }
 
@@ -125,17 +131,18 @@ func entityInstanceMatchesFilter(instance *apiv1.Entity, filter string) bool {
 }
 
 func paginateEntityInstances(instances []*apiv1.Entity, page, pageSize int32) []*apiv1.Entity {
-	start := int((page - 1) * pageSize)
-	if start >= len(instances) {
+	count := int64(len(instances))
+	start := int64(page-1) * int64(pageSize)
+	if start >= count {
 		return []*apiv1.Entity{}
 	}
 
-	end := start + int(pageSize)
-	if end > len(instances) {
-		end = len(instances)
+	end := start + int64(pageSize)
+	if end > count {
+		end = count
 	}
 
-	return instances[start:end]
+	return instances[int(start):int(end)]
 }
 
 func entityFieldsForResponse(data any, properties []config.EntityProperty) map[string]string {

+ 55 - 0
service/internal/api/api_entities_list_test.go

@@ -64,6 +64,61 @@ func TestGetEntitiesPaginatesAndFiltersInstances(t *testing.T) {
 	assert.Equal(t, "1", pagedResp.Msg.EntityDefinitions[0].Instances[0].UniqueKey)
 }
 
+func TestGetEntitiesUnfilteredIncludesConfiguredProperties(t *testing.T) {
+	entities.ClearEntitiesOfType("server")
+	entities.AddEntity("server", "0", map[string]any{"name": "alpha", "hostname": "alpha.example.com", "ip": "10.0.0.1"})
+	t.Cleanup(func() {
+		entities.ClearEntitiesOfType("server")
+	})
+
+	cfg := config.DefaultConfig()
+	cfg.Entities = []*config.EntityFile{
+		{
+			Name: "server",
+			Properties: []config.EntityProperty{
+				{Name: "hostname", Title: "Hostname"},
+			},
+		},
+	}
+	cfg.Sanitize()
+
+	ex := executor.DefaultExecutor(cfg)
+	ex.RebuildActionMap()
+	ts, client := getNewTestServerAndClientWithExecutor(cfg, ex)
+	defer ts.Close()
+
+	resp, err := client.GetEntities(context.Background(), connect.NewRequest(&apiv1.GetEntitiesRequest{}))
+	require.NoError(t, err)
+
+	serverDef := findEntityDefinition(resp.Msg.EntityDefinitions, "server")
+	require.NotNil(t, serverDef)
+	require.Len(t, serverDef.Properties, 1)
+	assert.Equal(t, "hostname", serverDef.Properties[0].Name)
+	assert.Equal(t, int32(1), serverDef.TotalInstances)
+	assert.Empty(t, serverDef.Instances)
+}
+
+func TestPaginateEntityInstancesHandlesLargePageValues(t *testing.T) {
+	instances := []*apiv1.Entity{
+		{UniqueKey: "0"},
+		{UniqueKey: "1"},
+	}
+
+	assert.Empty(t, paginateEntityInstances(instances, 1<<30, 1))
+	assert.Empty(t, paginateEntityInstances(instances, 2, 1<<30))
+	assert.Equal(t, []*apiv1.Entity{{UniqueKey: "1"}}, paginateEntityInstances(instances, 2, 1))
+}
+
+func findEntityDefinition(definitions []*apiv1.EntityDefinition, title string) *apiv1.EntityDefinition {
+	for _, definition := range definitions {
+		if definition.Title == title {
+			return definition
+		}
+	}
+
+	return nil
+}
+
 func TestGetEntityRestrictsFieldsToConfiguredProperties(t *testing.T) {
 	entities.ClearEntitiesOfType("server")
 	entities.AddEntity("server", "0", map[string]any{

+ 9 - 5
service/internal/api/api_entity_related_actions.go

@@ -17,13 +17,13 @@ type relatedActionCandidate struct {
 }
 
 func (api *oliveTinAPI) relatedActionsForEntity(user *authpublic.AuthenticatedUser, entityType string, entity *entities.Entity) []*apiv1.EntityRelatedAction {
-	rr := api.createDashboardRenderRequest(user, entityType, entity.UniqueKey)
-	populateActiveBindingStates(rr)
+	renderRequest := api.createDashboardRenderRequest(user, entityType, entity.UniqueKey)
+	populateActiveBindingStates(renderRequest)
 
 	candidates := collectRelatedActionCandidates(api, user, entityType, entity)
 	sortRelatedActionCandidates(candidates)
 
-	return buildEntityRelatedActions(candidates, rr)
+	return buildEntityRelatedActions(candidates, renderRequest)
 }
 
 func collectRelatedActionCandidates(api *oliveTinAPI, user *authpublic.AuthenticatedUser, entityType string, entity *entities.Entity) []relatedActionCandidate {
@@ -102,8 +102,12 @@ func buildPrefilledArgumentsForEntity(action *config.Action, entityType string,
 }
 
 func sortRelatedActionCandidates(candidates []relatedActionCandidate) {
-	sort.Slice(candidates, func(i, j int) bool {
-		return candidates[i].binding.ConfigOrder < candidates[j].binding.ConfigOrder
+	sort.SliceStable(candidates, func(i, j int) bool {
+		if candidates[i].binding.ConfigOrder != candidates[j].binding.ConfigOrder {
+			return candidates[i].binding.ConfigOrder < candidates[j].binding.ConfigOrder
+		}
+
+		return candidates[i].binding.ID < candidates[j].binding.ID
 	})
 }
 

+ 20 - 5
service/internal/api/api_justification.go

@@ -59,17 +59,32 @@ func resolveJustificationFromTemplate(action *config.Action, binding *executor.A
 
 	resolved, err := tpl.ParseTemplateWithActionContext(templateText, bindingEntity(binding), args)
 	if err != nil {
-		log.WithFields(log.Fields{
-			"template": templateText,
-			"entity":   bindingEntity(binding),
-			"error":    err,
-		}).Warn("Failed to resolve justification template")
+		log.WithFields(justificationTemplateErrorFields(templateText, binding, err)).Warn("Failed to resolve justification template")
 		return fallback
 	}
 
 	return resolved
 }
 
+func justificationTemplateErrorFields(templateText string, binding *executor.ActionBinding, err error) log.Fields {
+	fields := log.Fields{
+		"template": templateText,
+		"error":    err,
+	}
+
+	entity := bindingEntity(binding)
+	if entity == nil {
+		return fields
+	}
+
+	fields["entityKey"] = entity.UniqueKey
+	if binding.Action != nil && binding.Action.Entity != "" {
+		fields["entityType"] = binding.Action.Entity
+	}
+
+	return fields
+}
+
 func bindingEntity(binding *executor.ActionBinding) *entities.Entity {
 	if binding == nil {
 		return nil

+ 2 - 11
service/internal/api/api_justification_test.go

@@ -3,7 +3,6 @@ package api
 import (
 	"context"
 	"testing"
-	"time"
 
 	"connectrpc.com/connect"
 	"github.com/google/uuid"
@@ -49,11 +48,7 @@ func TestStartActionRequiresJustificationForGuest(t *testing.T) {
 	require.NoError(t, err)
 	require.NotEmpty(t, resp.Msg.ExecutionTrackingId)
 
-	time.Sleep(200 * time.Millisecond)
-
-	entry, ok := ex.GetLog(resp.Msg.ExecutionTrackingId)
-	require.True(t, ok)
-	assert.Equal(t, "New user registration foo@example.com", entry.Justification)
+	waitForLogJustification(t, ex, resp.Msg.ExecutionTrackingId, "New user registration foo@example.com")
 }
 
 func TestBuildActionExposesJustificationTemplate(t *testing.T) {
@@ -159,11 +154,7 @@ func TestStartActionResolvesJustificationTemplateForGuest(t *testing.T) {
 	require.NoError(t, err)
 	require.NotEmpty(t, resp.Msg.ExecutionTrackingId)
 
-	time.Sleep(200 * time.Millisecond)
-
-	entry, ok := ex.GetLog(resp.Msg.ExecutionTrackingId)
-	require.True(t, ok)
-	assert.Equal(t, "stuffbox", entry.Justification)
+	waitForLogJustification(t, ex, resp.Msg.ExecutionTrackingId, "stuffbox")
 }
 
 func TestValidateJustificationRequiredAllowsSystemUser(t *testing.T) {

+ 2 - 2
service/internal/api/api_log_arguments_test.go

@@ -286,7 +286,7 @@ func TestRestartActionRequiresJustificationWhenMissingFromStoredLog(t *testing.T
 			Title:         "Dangerous action",
 			Shell:         "echo ok",
 			MaxConcurrent: 1,
-			Justification: " ",
+			Justification: config.JustificationRequiredNoTemplate,
 		},
 	}
 
@@ -319,7 +319,7 @@ func TestRestartActionReusesStoredJustificationViaStartActionPath(t *testing.T)
 			Title:         "Dangerous action",
 			Shell:         "echo ok",
 			MaxConcurrent: 1,
-			Justification: " ",
+			Justification: config.JustificationRequiredNoTemplate,
 		},
 	}
 

+ 4 - 0
service/internal/api/api_test.go

@@ -157,6 +157,7 @@ func validateEntityListProperties(t *testing.T, client apiv1connect.OliveTinApiS
 		PageSize:   10,
 	}))
 	require.NoError(t, err)
+	require.Len(t, resp.Msg.EntityDefinitions, 1)
 
 	serverDef := resp.Msg.EntityDefinitions[0]
 	require.NotNil(t, serverDef, "server entity definition should be present")
@@ -949,6 +950,9 @@ func TestBuildActionIncludesGroups(t *testing.T) {
 func TestBuildChoicesExpandsChecklistEntityChoices(t *testing.T) {
 	entities.AddEntity("room", "0", map[string]any{"hostname": "attic"})
 	entities.AddEntity("room", "1", map[string]any{"hostname": "basement"})
+	t.Cleanup(func() {
+		entities.ClearEntitiesOfType("room")
+	})
 
 	arg := config.ActionArgument{
 		Type:   "checklist",

+ 60 - 0
service/internal/config/checklist.go

@@ -0,0 +1,60 @@
+package config
+
+import (
+	"encoding/json"
+	"fmt"
+	"strings"
+)
+
+// ParseChecklistValue parses a checklist argument wire value.
+// New values are JSON arrays; legacy comma-separated values are still accepted.
+func ParseChecklistValue(value string) ([]string, error) {
+	trimmed := strings.TrimSpace(value)
+	if trimmed == "" {
+		return nil, nil
+	}
+
+	if strings.HasPrefix(trimmed, "[") {
+		return parseJSONChecklistValue(trimmed)
+	}
+
+	return parseLegacyChecklistValue(value)
+}
+
+func parseJSONChecklistValue(value string) ([]string, error) {
+	var values []string
+	if err := json.Unmarshal([]byte(value), &values); err != nil {
+		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 == "" {
+			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 {
+	if len(values) == 0 {
+		return ""
+	}
+
+	encoded, err := json.Marshal(values)
+	if err != nil {
+		return ""
+	}
+
+	return string(encoded)
+}

+ 43 - 0
service/internal/config/checklist_test.go

@@ -0,0 +1,43 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParseChecklistValueJSON(t *testing.T) {
+	t.Parallel()
+
+	values, err := ParseChecklistValue(`["documents","photos"]`)
+	require.NoError(t, err)
+	assert.Equal(t, []string{"documents", "photos"}, values)
+
+	values, err = ParseChecklistValue(`["kitchen,bedroom","hallway"]`)
+	require.NoError(t, err)
+	assert.Equal(t, []string{"kitchen,bedroom", "hallway"}, values)
+}
+
+func TestParseChecklistValueLegacyCommaSeparated(t *testing.T) {
+	t.Parallel()
+
+	values, err := ParseChecklistValue("documents, photos")
+	require.NoError(t, err)
+	assert.Equal(t, []string{"documents", "photos"}, values)
+}
+
+func TestParseChecklistValueRejectsEmptyLegacySegment(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))
+}

+ 4 - 0
service/internal/config/config.go

@@ -50,6 +50,10 @@ func (action *Action) JustificationTemplateText() string {
 		return ""
 	}
 
+	if action.Justification == JustificationRequiredNoTemplate {
+		return ""
+	}
+
 	return action.Justification
 }
 

+ 2 - 3
service/internal/config/sanitize.go

@@ -95,12 +95,11 @@ func validateChecklistChoicesForArgument(actionTitle string, arg ActionArgument)
 	}
 
 	for _, choice := range arg.Choices {
-		if strings.Contains(choice.Value, ",") {
+		if strings.TrimSpace(choice.Value) == "" {
 			return fmt.Errorf(
-				`action %q argument %q choice value %q must not contain commas`,
+				`action %q argument %q choice value must not be empty`,
 				actionTitle,
 				arg.Name,
-				choice.Value,
 			)
 		}
 	}

+ 2 - 3
service/internal/config/sanitize_test.go

@@ -272,7 +272,7 @@ func TestValidateUniqueLocalUserAPIKeys(t *testing.T) {
 	require.NoError(t, err)
 }
 
-func TestValidateChecklistChoiceValuesRejectsCommas(t *testing.T) {
+func TestValidateChecklistChoiceValuesAllowsCommas(t *testing.T) {
 	t.Parallel()
 
 	c := DefaultConfig()
@@ -291,6 +291,5 @@ func TestValidateChecklistChoiceValuesRejectsCommas(t *testing.T) {
 	})
 
 	err := c.validateChecklistChoiceValues()
-	require.Error(t, err)
-	assert.Contains(t, err.Error(), `choice value "kitchen,bedroom" must not contain commas`)
+	require.NoError(t, err)
 }

+ 21 - 5
service/internal/executor/arguments.go

@@ -241,8 +241,17 @@ func typecheckChecklist(value string, arg *config.ActionArgument) error {
 		return fmt.Errorf("checklist argument %q requires choices", arg.Name)
 	}
 
-	for _, segment := range strings.Split(value, ",") {
-		if err := typecheckChecklistSegment(strings.TrimSpace(segment), arg); err != nil {
+	segments, err := config.ParseChecklistValue(value)
+	if err != nil {
+		return err
+	}
+
+	return typecheckChecklistSegments(segments, arg)
+}
+
+func typecheckChecklistSegments(segments []string, arg *config.ActionArgument) error {
+	for _, segment := range segments {
+		if err := typecheckChecklistSegment(segment, arg); err != nil {
 			return err
 		}
 	}
@@ -485,14 +494,21 @@ func mangleChecklistValue(arg *config.ActionArgument, value string, actionTitle
 		return value
 	}
 
-	segments := strings.Split(value, ",")
-	mangled := make([]string, len(segments))
+	segments, err := config.ParseChecklistValue(value)
+	if err != nil {
+		return value
+	}
 
+	return mangleChecklistSegments(arg, segments, actionTitle)
+}
+
+func mangleChecklistSegments(arg *config.ActionArgument, segments []string, actionTitle string) string {
+	mangled := make([]string, len(segments))
 	for i, segment := range segments {
 		mangled[i] = mangleChecklistSegment(arg, segment, actionTitle)
 	}
 
-	return strings.Join(mangled, ",")
+	return config.FormatChecklistValue(mangled)
 }
 
 func mangleChecklistSegment(arg *config.ActionArgument, segment string, actionTitle string) string {

+ 3 - 3
service/internal/executor/arguments_test.go

@@ -196,10 +196,10 @@ func TestMangleArgumentValueChecklist(t *testing.T) {
 	arg := checklistTestArg()
 
 	out := MangleArgumentValue(&arg, "Documents,Music", "Test action")
-	assert.Equal(t, "documents,music", out)
+	assert.Equal(t, `["documents","music"]`, out)
 
 	out = MangleArgumentValue(&arg, "documents,photos", "Test action")
-	assert.Equal(t, "documents,photos", out)
+	assert.Equal(t, `["documents","photos"]`, out)
 }
 
 func checklistEntityTestArg() config.ActionArgument {
@@ -248,7 +248,7 @@ func TestMangleArgumentValueChecklistEntityTitles(t *testing.T) {
 	}
 
 	out := MangleArgumentValue(&arg, "attic room,basement room", "Test checklist entity titles")
-	assert.Equal(t, "attic,basement", out)
+	assert.Equal(t, `["attic","basement"]`, out)
 }
 
 func TestParseActionArgumentsChecklistEmptySelection(t *testing.T) {

+ 5 - 5
service/internal/executor/justification_test.go

@@ -11,7 +11,7 @@ import (
 
 func TestResolveJustificationUsesProvidedValue(t *testing.T) {
 	cfg := config.DefaultConfig()
-	action := &config.Action{Title: "Send email", Justification: " ", Shell: "echo hi"}
+	action := &config.Action{Title: "Send email", Justification: config.JustificationRequiredNoTemplate, Shell: "echo hi"}
 	cfg.Actions = append(cfg.Actions, action)
 	ex := DefaultExecutor(cfg)
 	ex.RebuildActionMap()
@@ -29,7 +29,7 @@ func TestResolveJustificationUsesProvidedValue(t *testing.T) {
 
 func TestResolveJustificationCronDefault(t *testing.T) {
 	cfg := config.DefaultConfig()
-	action := &config.Action{Title: "Nightly backup", Justification: " ", Shell: "echo hi"}
+	action := &config.Action{Title: "Nightly backup", Justification: config.JustificationRequiredNoTemplate, Shell: "echo hi"}
 	cfg.Actions = append(cfg.Actions, action)
 	ex := DefaultExecutor(cfg)
 	ex.RebuildActionMap()
@@ -45,7 +45,7 @@ func TestResolveJustificationCronDefault(t *testing.T) {
 
 func TestResolveJustificationStartupDefault(t *testing.T) {
 	cfg := config.DefaultConfig()
-	action := &config.Action{Title: "Init", Justification: " ", Shell: "echo hi"}
+	action := &config.Action{Title: "Init", Justification: config.JustificationRequiredNoTemplate, Shell: "echo hi"}
 	cfg.Actions = append(cfg.Actions, action)
 	ex := DefaultExecutor(cfg)
 	ex.RebuildActionMap()
@@ -61,7 +61,7 @@ func TestResolveJustificationStartupDefault(t *testing.T) {
 
 func TestResolveJustificationWebhookDefault(t *testing.T) {
 	cfg := config.DefaultConfig()
-	action := &config.Action{Title: "Deploy", Justification: " ", Exec: []string{"echo", "deploy"}}
+	action := &config.Action{Title: "Deploy", Justification: config.JustificationRequiredNoTemplate, Exec: []string{"echo", "deploy"}}
 	cfg.Actions = append(cfg.Actions, action)
 	ex := DefaultExecutor(cfg)
 	ex.RebuildActionMap()
@@ -95,7 +95,7 @@ func TestJustificationNotPassedToShellArgs(t *testing.T) {
 	cfg := config.DefaultConfig()
 	action := &config.Action{
 		Title:         "Echo",
-		Justification: " ",
+		Justification: config.JustificationRequiredNoTemplate,
 		Shell:         "echo {{ message }}",
 		Arguments: []config.ActionArgument{
 			{Name: "message", Type: "ascii_sentence"},

+ 1 - 1
service/internal/executor/log_arguments_test.go

@@ -99,7 +99,7 @@ func TestStorableArgumentsFromRequestStoresMangledChecklistValue(t *testing.T) {
 	args := storableArgumentsFromRequest(req)
 
 	require.Len(t, args, 1)
-	assert.Equal(t, "documents,photos", args["directories"])
+	assert.Equal(t, `["documents","photos"]`, args["directories"])
 }
 
 func TestCopyStorableArgumentsToLogEntry(t *testing.T) {