fix: coderabbit suggestions on the justification

Makefile

@@ -57,6 +57,7 @@ devrun: compile
 devcontainer: compile podman-image podman-container
 
 webui-dist:
+	$(call delete-files,webui)
 	$(MAKE) -wC frontend dist
 	mv frontend/dist webui
 

config.yaml

@@ -133,6 +133,7 @@ actions:
   # Docs: https://docs.olivetin.app/args/input_confirmation.html
   - title: Delete old backups
     icon: ashtonished
+    # A single space requires justification with no prefilled template (empty disables it).
     justification: " "
     shell: rm -rf /opt/oliveTinOldBackups/ && sleep 5
     arguments:

frontend/resources/scripts/gen/olivetin/api/v1/olivetin_pb.d.ts

@@ -92,7 +92,7 @@ export declare type Action = Message<"olivetin.api.v1.Action"> & {
   execOnWebhooks: ActionWebhookExecHint[];
 
   /**
-   * @generated from field: string justification = 16;
+   * @generated from field: string justification = 20;
    */
   justification: string;
 

frontend/resources/vue/views/ArgumentForm.vue

@@ -70,6 +70,11 @@ import { useRouter } from 'vue-router'
 import { requestReconnectNow } from '../../../js/websocket.js'
 import ChoiceCombobox from '../components/ChoiceCombobox.vue'
 import ChoiceChecklist from '../components/ChoiceChecklist.vue'
+import {
+  actionJustificationTemplate,
+  actionRequiresJustification,
+  applyArgumentTemplate
+} from '../utils/justificationTemplate.js'
 
 const router = useRouter()
 
@@ -114,8 +119,7 @@ async function setup() {
     icon.value = action.icon
     popupOnStart.value = action.popupOnStart || ''
     actionArguments.value = action.arguments || []
-    justificationRequired.value = action.justification || false
+    justificationRequired.value = actionRequiresJustification(action.justification)
-    justificationValue.value = ''
     argValues.value = {}
     formErrors.value = {}
     confirmationChecked.value = false
@@ -151,6 +155,12 @@ async function setup() {
     }
     })
 
+    const prefilledJustification = applyArgumentTemplate(
+      actionJustificationTemplate(action.justification),
+      argValues.value
+    )
+    justificationValue.value = prefilledJustification.trim() === '' ? '' : prefilledJustification
+
     // Run initial validation on all fields after DOM is updated
     await nextTick()
     for (const arg of actionArguments.value) {

integration-tests/Makefile

@@ -1,8 +1,12 @@
-default: test-install test-run
+default: test-install prep test-run
 
 test-install:
 	npm install --no-fund
 
+prep:
+	$(MAKE) -wC .. webui-dist
+	$(MAKE) -wC ../service compile-currentenv
+
 test-run:
     # GitHub Actions fails badly on the default timeout of 2000ms
 	npx mocha tests --recursive -t 10000
@@ -24,4 +28,4 @@ getsnapshot:
 	rm -rf /opt/OliveTin-snapshot/*
 	gh run download -D /opt/OliveTin-snapshot/
 
-.PHONY: default find-flakey-tests find-flakey-tests-inf
+.PHONY: default find-flakey-tests find-flakey-tests-inf prep

proto/olivetin/api/v1/olivetin.proto

@@ -20,7 +20,8 @@ message Action {
 	repeated string exec_on_file_changed_in_dir = 13;
 	string exec_on_calendar_file = 14;
 	repeated ActionWebhookExecHint exec_on_webhooks = 15;
-	string justification = 16;
+	reserved 16;
+	string justification = 20;
 	bool has_running_instance = 17;
 	bool has_queued_instance = 18;
 	repeated ActionGroupMembership groups = 19;

service/gen/olivetin/api/v1/olivetin.pb.go

@@ -38,7 +38,7 @@ type Action struct {
 	ExecOnFileChangedInDir   []string                 `protobuf:"bytes,13,rep,name=exec_on_file_changed_in_dir,json=execOnFileChangedInDir,proto3" json:"exec_on_file_changed_in_dir,omitempty"`
 	ExecOnCalendarFile       string                   `protobuf:"bytes,14,opt,name=exec_on_calendar_file,json=execOnCalendarFile,proto3" json:"exec_on_calendar_file,omitempty"`
 	ExecOnWebhooks           []*ActionWebhookExecHint `protobuf:"bytes,15,rep,name=exec_on_webhooks,json=execOnWebhooks,proto3" json:"exec_on_webhooks,omitempty"`
-	Justification            string                   `protobuf:"bytes,16,opt,name=justification,proto3" json:"justification,omitempty"`
+	Justification            string                   `protobuf:"bytes,20,opt,name=justification,proto3" json:"justification,omitempty"`
 	HasRunningInstance       bool                     `protobuf:"varint,17,opt,name=has_running_instance,json=hasRunningInstance,proto3" json:"has_running_instance,omitempty"`
 	HasQueuedInstance        bool                     `protobuf:"varint,18,opt,name=has_queued_instance,json=hasQueuedInstance,proto3" json:"has_queued_instance,omitempty"`
 	Groups                   []*ActionGroupMembership `protobuf:"bytes,19,rep,name=groups,proto3" json:"groups,omitempty"`
@@ -4575,7 +4575,7 @@ var File_olivetin_api_v1_olivetin_proto protoreflect.FileDescriptor
 
 const file_olivetin_api_v1_olivetin_proto_rawDesc = "" +
 	"\n" +
-	"\x1eolivetin/api/v1/olivetin.proto\x12\x0folivetin.api.v1\"\xd1\x06\n" +
+	"\x1eolivetin/api/v1/olivetin.proto\x12\x0folivetin.api.v1\"\xd7\x06\n" +
 	"\x06Action\x12\x1d\n" +
 	"\n" +
 	"binding_id\x18\x01 \x01(\tR\tbindingId\x12\x14\n" +
@@ -4595,10 +4595,10 @@ const file_olivetin_api_v1_olivetin_proto_rawDesc = "" +
 	"\x1bexec_on_file_changed_in_dir\x18\r \x03(\tR\x16execOnFileChangedInDir\x121\n" +
 	"\x15exec_on_calendar_file\x18\x0e \x01(\tR\x12execOnCalendarFile\x12P\n" +
 	"\x10exec_on_webhooks\x18\x0f \x03(\v2&.olivetin.api.v1.ActionWebhookExecHintR\x0eexecOnWebhooks\x12$\n" +
-	"\rjustification\x18\x10 \x01(\tR\rjustification\x120\n" +
+	"\rjustification\x18\x14 \x01(\tR\rjustification\x120\n" +
 	"\x14has_running_instance\x18\x11 \x01(\bR\x12hasRunningInstance\x12.\n" +
 	"\x13has_queued_instance\x18\x12 \x01(\bR\x11hasQueuedInstance\x12>\n" +
-	"\x06groups\x18\x13 \x03(\v2&.olivetin.api.v1.ActionGroupMembershipR\x06groups\"q\n" +
+	"\x06groups\x18\x13 \x03(\v2&.olivetin.api.v1.ActionGroupMembershipR\x06groupsJ\x04\b\x10\x10\x11\"q\n" +
 	"\x15ActionGroupMembership\x12\x12\n" +
 	"\x04name\x18\x01 \x01(\tR\x04name\x12%\n" +
 	"\x0emax_concurrent\x18\x02 \x01(\x05R\rmaxConcurrent\x12\x1d\n" +

service/internal/api/api_justification.go

@@ -12,6 +12,7 @@ import (
 	"github.com/OliveTin/OliveTin/internal/entities"
 	"github.com/OliveTin/OliveTin/internal/executor"
 	"github.com/OliveTin/OliveTin/internal/tpl"
+	log "github.com/sirupsen/logrus"
 )
 
 func validateJustificationRequired(action *config.Action, justification string, user *authpublic.AuthenticatedUser) error {
@@ -58,6 +59,11 @@ 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")
 		return fallback
 	}
 

service/internal/api/api_justification_test.go

@@ -21,7 +21,7 @@ func TestStartActionRequiresJustificationForGuest(t *testing.T) {
 	action := &config.Action{
 		Title:         "Send email",
 		ID:            "send_email",
-		Justification: " ",
+		Justification: config.JustificationRequiredNoTemplate,
 		Shell:         "echo done",
 	}
 	cfg.Actions = append(cfg.Actions, action)
@@ -85,7 +85,7 @@ func TestBuildActionExposesBlankRequiredJustification(t *testing.T) {
 	action := &config.Action{
 		Title:         "Audited action",
 		ID:            "audited",
-		Justification: " ",
+		Justification: config.JustificationRequiredNoTemplate,
 		Shell:         "echo hi",
 	}
 	cfg.Actions = append(cfg.Actions, action)
@@ -101,7 +101,7 @@ func TestBuildActionExposesBlankRequiredJustification(t *testing.T) {
 	})
 
 	require.NotNil(t, pb)
-	assert.Equal(t, " ", pb.Justification)
+	assert.Equal(t, config.JustificationRequiredNoTemplate, pb.Justification)
 }
 
 func TestResolveStartJustificationUsesTemplateWhenClientValueEmpty(t *testing.T) {
@@ -168,7 +168,7 @@ func TestStartActionResolvesJustificationTemplateForGuest(t *testing.T) {
 
 func TestValidateJustificationRequiredAllowsSystemUser(t *testing.T) {
 	cfg := config.DefaultConfig()
-	action := &config.Action{Title: "Cron job", Justification: " "}
+	action := &config.Action{Title: "Cron job", Justification: config.JustificationRequiredNoTemplate}
 
 	err := validateJustificationRequired(action, "", auth.UserFromSystem(cfg, "cron"))
 	require.NoError(t, err)

service/internal/config/config.go

@@ -7,6 +7,9 @@ import (
 // ReservedArgumentNamePrefix is reserved for OliveTin-injected system arguments.
 const ReservedArgumentNamePrefix = "ot_"
 
+// JustificationRequiredNoTemplate requires a manual justification with no prefilled template.
+const JustificationRequiredNoTemplate = " "
+
 // Action represents the core functionality of OliveTin - commands that show up
 // as buttons in the UI.
 type Action struct {
@@ -43,7 +46,7 @@ func (action *Action) RequiresJustification() bool {
 }
 
 func (action *Action) JustificationTemplateText() string {
-	if action == nil || !action.RequiresJustification() {
+	if !action.RequiresJustification() {
 		return ""
 	}
 

service/internal/config/config_reloader.go

@@ -55,6 +55,7 @@ func unmarshalRoot(k *koanf.Koanf, cfg *Config) bool {
 		DecoderConfig: &mapstructure.DecoderConfig{
 			DecodeHook: mapstructure.ComposeDecodeHookFunc(
 				envDecodeHookFunc,
+				justificationDecodeHookFunc,
 				mapstructure.StringToTimeDurationHookFunc(),
 				mapstructure.TextUnmarshallerHookFunc(),
 			),
@@ -259,6 +260,18 @@ func mergeFunc(src map[string]interface{}, dest map[string]interface{}) error {
 
 var envRegex = regexp.MustCompile(`\${{ *?(\S+) *?}}`)
 
+func justificationDecodeHookFunc(from reflect.Type, to reflect.Type, data any) (any, error) {
+	if to.Kind() != reflect.String || from.Kind() != reflect.Bool {
+		return data, nil
+	}
+
+	if data.(bool) {
+		return JustificationRequiredNoTemplate, nil
+	}
+
+	return "", nil
+}
+
 func envDecodeHookFunc(from reflect.Type, to reflect.Type, data any) (any, error) {
 	log.Debugf("envDecodeHookFunc called: from=%v, to=%v, data=%v", from, to, data)
 	if from.Kind() != reflect.String {

service/internal/config/justification_compat_test.go

@@ -0,0 +1,58 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/knadh/koanf/parsers/yaml"
+	"github.com/knadh/koanf/providers/rawbytes"
+	"github.com/knadh/koanf/v2"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestJustificationDecodeHookMigratesLegacyBooleanFalse(t *testing.T) {
+	cfg := loadJustificationCompatConfig(t, `
+actions:
+  - title: Legacy disabled
+    shell: echo hi
+    justification: false
+`)
+
+	require.Len(t, cfg.Actions, 1)
+	assert.Empty(t, cfg.Actions[0].Justification)
+}
+
+func TestJustificationDecodeHookMigratesLegacyBooleanTrue(t *testing.T) {
+	cfg := loadJustificationCompatConfig(t, `
+actions:
+  - title: Legacy required
+    shell: echo hi
+    justification: true
+`)
+
+	require.Len(t, cfg.Actions, 1)
+	assert.Equal(t, JustificationRequiredNoTemplate, cfg.Actions[0].Justification)
+}
+
+func TestSanitizeJustificationMigratesWeaklyTypedLegacyStrings(t *testing.T) {
+	action := &Action{Justification: "false"}
+	action.sanitizeJustification()
+	assert.Empty(t, action.Justification)
+
+	action.Justification = "true"
+	action.sanitizeJustification()
+	assert.Equal(t, JustificationRequiredNoTemplate, action.Justification)
+}
+
+func loadJustificationCompatConfig(t *testing.T, yamlBody string) *Config {
+	t.Helper()
+
+	k := koanf.New(".")
+	require.NoError(t, k.Load(rawbytes.Provider([]byte(yamlBody)), yaml.Parser()))
+
+	cfg := DefaultConfig()
+	require.True(t, unmarshalRoot(k, cfg))
+	cfg.Sanitize()
+
+	return cfg
+}

service/internal/config/sanitize.go

@@ -227,6 +227,7 @@ func (action *Action) sanitize(cfg *Config) {
 	action.ID = getActionID(action)
 	action.Icon = lookupHTMLIcon(action.Icon, cfg.DefaultIconForActions)
 	migrateActionOnClick(action)
+	action.sanitizeJustification()
 	action.OnClick = sanitizeOnClick(action.OnClick, cfg)
 	action.PopupOnStart = action.OnClick
 
@@ -449,6 +450,15 @@ func migrateActionOnClick(action *Action) {
 	}
 }
 
+func (action *Action) sanitizeJustification() {
+	switch action.Justification {
+	case "false":
+		action.Justification = ""
+	case "true":
+		action.Justification = JustificationRequiredNoTemplate
+	}
+}
+
 func shouldMigrateDefaultOnClickFromPopup(onClick, popupOnStart string) bool {
 	if popupOnStart == "" {
 		return false