Просмотр исходного кода

fix: intermediate output and comfirmation

jamesread 3 недель назад
Родитель
Сommit
839d244e51

+ 2 - 0
docs/modules/ROOT/pages/args/input_confirmation.adoc

@@ -3,6 +3,8 @@
 
 The `confirmation` type argument is a special argument type, which simply disables the "Start" button until a checkbox is ticked. This can be useful if you have an action with no other arguments, but you want to prevent accidental button-clicks starting the action.
 
+Confirmation arguments are usually left unnamed, so nothing is substituted into the command. If you do give the argument a `name`, the only allowed values are `0` (unchecked) and `1` (checked), so confirmation is safe to use with `shell:` actions.
+
 [source,yaml]
 ----
 actions:

+ 1 - 1
docs/modules/ROOT/pages/args/types.adoc

@@ -20,7 +20,7 @@ A full list of argument types are below;
 | regex:...                   | xref:args/input.adoc[Textbox]           | Version 2024.03.081 and above support custom regex patterns. See xref:args/regex.adoc[Custom regex arguments].
 | int                         | xref:args/input.adoc[Textbox]           | Any number, made up of the characters 0 to 9. Negative numbers are not supported.
 | url                         | xref:args/input.adoc[Textbox]           | A URL (e.g. https://example.com). Accepts any scheme, including `file://` and `ftp://`. See warning below.
-| confirmation                | xref:args/input_confirmation.adoc[Confirmation] | A "hidden" argument that makes the action require a confirmation before launching.
+| confirmation                | xref:args/input_confirmation.adoc[Confirmation] | A UI gate that requires a checkbox before starting. Usually unnamed (nothing is substituted). If named, the value is only `0` or `1`.
 | checklist                   | xref:args/input_checklist.adoc[Checklist]     | Multiple checkboxes from predefined choices. Selected values are passed as a comma-separated string.
 | n/a, but `choices` used     | xref:args/input_dropdown.adoc[Dropdown]         | A "hidden" argument that makes the action require a confirmation before launching.
 | raw_string_multiline        | xref:args/input_textarea.adoc[Textarea]         | Anything. This is **dangerous**, as effectively people can type anything they like

+ 19 - 7
service/internal/executor/arguments.go

@@ -150,12 +150,7 @@ func redactExecArgs(execArgs []string, arguments []config.ActionArgument, argume
 }
 
 func argumentSkipsValidation(arg *config.ActionArgument) bool {
-	switch arg.Type {
-	case "confirmation", "html":
-		return true
-	default:
-		return false
-	}
+	return arg.Type == "html"
 }
 
 func typecheckActionArgument(arg *config.ActionArgument, value string, action *config.Action) error {
@@ -163,6 +158,10 @@ func typecheckActionArgument(arg *config.ActionArgument, value string, action *c
 		return nil
 	}
 
+	if arg.Type == "confirmation" {
+		return typecheckConfirmation(arg, value)
+	}
+
 	if arg.Name == "" {
 		return fmt.Errorf("argument name cannot be empty")
 	}
@@ -170,6 +169,20 @@ func typecheckActionArgument(arg *config.ActionArgument, value string, action *c
 	return typecheckActionArgumentFound(value, arg)
 }
 
+// typecheckConfirmation allows unnamed confirmation args as UI-only gates.
+// Named confirmation values are only ever "0" or "1", matching the web UI.
+func typecheckConfirmation(arg *config.ActionArgument, value string) error {
+	if arg.Name == "" {
+		return nil
+	}
+
+	if value == "0" || value == "1" {
+		return nil
+	}
+
+	return fmt.Errorf("argument %q of type confirmation must be \"0\" or \"1\"", arg.Name)
+}
+
 // ValidateArgument validates a single argument value using the same logic as the executor.
 // It applies mangling transformations and performs full validation including null checks,
 // choice validation, and type safety checks.
@@ -403,7 +416,6 @@ var shellUnsafeArgumentTypes = map[string]struct{}{
 	"very_dangerous_raw_string": {},
 	"password":                  {},
 	"html":                      {},
-	"confirmation":              {},
 }
 
 func isUnsafeShellArgumentType(arg *config.ActionArgument) bool {

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

@@ -523,8 +523,20 @@ func TestCheckShellArgumentSafetyWithConfirmation(t *testing.T) {
 	}
 
 	err := checkShellArgumentSafety(&a1)
-	assert.NotNil(t, err)
-	assert.Contains(t, err.Error(), "unsafe argument type 'confirmation'")
+	assert.Nil(t, err, "confirmation is constrained to 0/1 and is safe with shell")
+}
+
+func TestCheckShellArgumentSafetyWithUnnamedConfirmation(t *testing.T) {
+	a1 := config.Action{
+		Title: "Confirm shell unnamed",
+		Shell: "echo ok",
+		Arguments: []config.ActionArgument{
+			{Type: "confirmation", Title: "Are you sure?!"},
+		},
+	}
+
+	err := checkShellArgumentSafety(&a1)
+	assert.Nil(t, err)
 }
 
 func TestCheckShellArgumentSafetyWithChoicelessCheckbox(t *testing.T) {
@@ -930,8 +942,27 @@ func TestTypecheckActionArgumentConfirmation(t *testing.T) {
 	}
 	action := config.Action{Title: "Test"}
 
+	assert.Nil(t, typecheckActionArgument(&arg, "0", &action))
+	assert.Nil(t, typecheckActionArgument(&arg, "1", &action))
+
 	err := typecheckActionArgument(&arg, "any_value", &action)
-	assert.Nil(t, err, "Confirmation type should always pass validation")
+	assert.NotNil(t, err)
+	assert.Contains(t, err.Error(), "must be \"0\" or \"1\"")
+
+	err = typecheckActionArgument(&arg, "", &action)
+	assert.NotNil(t, err)
+	assert.Contains(t, err.Error(), "must be \"0\" or \"1\"")
+}
+
+func TestTypecheckActionArgumentUnnamedConfirmation(t *testing.T) {
+	arg := config.ActionArgument{
+		Type:  "confirmation",
+		Title: "Are you sure?!",
+	}
+	action := config.Action{Title: "Test"}
+
+	assert.Nil(t, typecheckActionArgument(&arg, "", &action))
+	assert.Nil(t, typecheckActionArgument(&arg, "ignored", &action))
 }
 
 func TestTypecheckActionArgumentHtmlWithoutName(t *testing.T) {

+ 25 - 2
service/internal/executor/executor.go

@@ -1097,6 +1097,7 @@ func appendErrorToStderr(req *ExecutionRequest, err error) {
 
 type OutputStreamer struct {
 	Req    *ExecutionRequest
+	mu     sync.Mutex
 	output bytes.Buffer
 }
 
@@ -1105,10 +1106,31 @@ func (ost *OutputStreamer) Write(o []byte) (n int, err error) {
 		listener.OnOutputChunk(o, ost.Req.TrackingID)
 	}
 
-	return ost.output.Write(o)
+	ost.mu.Lock()
+	n, err = ost.output.Write(o)
+	outputSoFar := ""
+	if err == nil {
+		outputSoFar = ost.output.String()
+	}
+	ost.mu.Unlock()
+
+	if err != nil {
+		return n, err
+	}
+
+	// Keep the log entry's Output in sync while the command is still running so
+	// ExecutionStatus / mid-run result views can show output produced so far.
+	ost.Req.mutateLogEntry(func(entry *InternalLogEntry) {
+		entry.Output = outputSoFar
+	})
+
+	return n, nil
 }
 
 func (ost *OutputStreamer) String() string {
+	ost.mu.Lock()
+	defer ost.mu.Unlock()
+
 	return ost.output.String()
 }
 
@@ -1155,9 +1177,10 @@ func stepExec(req *ExecutionRequest) bool {
 	})
 	ctx.setProcess(cmd.Process)
 	waiterr := cmd.Wait()
+	finalOutput := streamer.String()
 	req.mutateLogEntry(func(entry *InternalLogEntry) {
 		entry.ExitCode = int32(commandExitCode(cmd))
-		entry.Output = streamer.String()
+		entry.Output = finalOutput
 	})
 
 	appendErrorToStderr(req, runerr)

+ 47 - 0
service/internal/executor/executor_test.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/OliveTin/OliveTin/internal/auth"
 	authpublic "github.com/OliveTin/OliveTin/internal/auth/authpublic"
@@ -947,3 +948,49 @@ func TestStepSaveLogReturnsFalseWhenDependenciesMissing(t *testing.T) {
 	missingCfg.Cfg = nil
 	assert.False(t, stepSaveLog(&missingCfg))
 }
+
+func TestLogEntryOutputAvailableWhileRunning(t *testing.T) {
+	cfg := config.DefaultConfig()
+	e := DefaultExecutor(cfg)
+	action := &config.Action{
+		Title: "Slow output",
+		Shell: "echo hello-mid-run; sleep 2",
+	}
+	cfg.Actions = append(cfg.Actions, action)
+	cfg.Sanitize()
+	e.RebuildActionMap()
+
+	binding := e.FindBindingWithNoEntity(action)
+	require.NotNil(t, binding)
+
+	wg, trackingID := e.ExecRequest(&ExecutionRequest{
+		Binding:           binding,
+		Cfg:               cfg,
+		AuthenticatedUser: auth.UserFromSystem(cfg, "testuser"),
+	})
+
+	var sawOutputWhileRunning bool
+	require.Eventually(t, func() bool {
+		snapshot, ok := e.SnapshotLog(trackingID)
+		if !ok {
+			return false
+		}
+		if snapshot.ExecutionFinished {
+			return false
+		}
+		if strings.Contains(snapshot.Output, "hello-mid-run") {
+			sawOutputWhileRunning = true
+			return true
+		}
+		return false
+	}, 2*time.Second, 10*time.Millisecond)
+
+	wg.Wait()
+
+	require.True(t, sawOutputWhileRunning, "expected Output to contain printed text before ExecutionFinished")
+
+	snapshot, ok := e.SnapshotLog(trackingID)
+	require.True(t, ok)
+	assert.True(t, snapshot.ExecutionFinished)
+	assert.Contains(t, snapshot.Output, "hello-mid-run")
+}

+ 2 - 1
service/internal/executor/log_arguments.go

@@ -128,7 +128,8 @@ func RestartArgumentsIncomplete(action *config.Action, entity *entities.Entity,
 }
 
 func restartArgumentRequired(arg *config.ActionArgument, entity *entities.Entity) bool {
-	if argumentSkipsValidation(arg) {
+	// confirmation is a UI gate; html skips validation. Neither needs a stored value to restart.
+	if argumentSkipsValidation(arg) || arg.Type == "confirmation" {
 		return false
 	}