chore: coderabbit knows better than me

Makefile

@@ -79,4 +79,4 @@ config-tool:
 devcheck:
 	python3 scripts/devcheck.py $(ARGS)
 
-.PHONY: proto service windows-resources windows-msi devcheck
+.PHONY: proto service windows-resources windows-msi frontend-unittests devcheck

docs/modules/ROOT/pages/action_execution/aftercompletion.adoc

@@ -15,8 +15,8 @@ actions:
 
 When running shellAfterCompleted, you *cannot* use user-defined argument values - they are not passed to the command. However the following special arguments are defined;
 
-* `{{ exitCode }}` / `{{ .Arguments.exitCode }}` - The exit code of the previous command. OliveTin rewrites these placeholders to the quoted `"$EXITCODE"` environment reference when running `shellAfterCompleted`, so shell metacharacters in the value cannot break quoting.
+* `{{ exitCode }}` / `{{ .Arguments.exitCode }}` - The exit code of the previous command. OliveTin rewrites these placeholders to the quoted `"$EXITCODE"` environment reference when running `shellAfterCompleted`, so shell metacharacters in the value cannot break quoting. Placeholders inside single-quoted shell arguments are rewritten so the environment reference can still expand.
-* `{{ output }}` / `{{ .Arguments.output }}` - The standard output of the previous command. OliveTin rewrites these placeholders to the quoted `"$OUTPUT"` environment reference, so shell metacharacters in command output cannot be executed. You can also reference `$OUTPUT` directly in your `shellAfterCompleted` command. Do not place these placeholders inside single-quoted shell arguments; single quotes prevent `$OUTPUT` and `$EXITCODE` from expanding after substitution.
+* `{{ output }}` / `{{ .Arguments.output }}` - The standard output of the previous command. OliveTin rewrites these placeholders to the quoted `"$OUTPUT"` environment reference, so shell metacharacters in command output cannot be executed. You can also reference `$OUTPUT` directly in your `shellAfterCompleted` command. Placeholders inside single-quoted shell arguments are rewritten so the environment reference can still expand.
 * `{{ .Arguments.ot_executionTrackingId }}` - The unique execution tracking id for this execution (version 3k; in 2k use `{{ ot_executionTrackingId }}`)
 * `{{ .Arguments.ot_username }}` - The username of the user who started the execution (version 3k; in 2k use `{{ ot_username }}`). May be `guest` or `cron` for unauthenticated or automated runs.
 

docs/modules/ROOT/pages/action_execution/onwebhook.adoc

@@ -81,9 +81,11 @@ actions:
       - matchQuery:
           action: deploy
           env: production
+        extract:
+          service: "$.service"
 ----
 
-A request to `/webhooks?action=deploy&env=production` would match this action.
+A request to `/webhooks?action=deploy&env=production` with a JSON body containing `"service"` would match this action and pass that field into the `service` argument.
 
 === Match by JSON Body Path
 
@@ -101,6 +103,8 @@ actions:
         type: ascii
     execOnWebhook:
       - matchPath: "$.event_type=push"
+        extract:
+          branch: "$.branch"
 ----
 
 The `matchPath` format is `jsonpath=value`. You can also just specify a JSONPath without a value to match if the path exists:
@@ -190,7 +194,7 @@ actions:
   - title: Log Request
     exec:
       - echo
-      - "Request ID: {{ webhook_header_x-request-id }}"
+      - 'Request ID: {{ index .Arguments "webhook_header_x-request-id" }}'
     arguments:
       - name: webhook_header_x-request-id
         type: ascii
@@ -199,6 +203,8 @@ actions:
           X-Event-Type: log
 ----
 
+Header names that contain hyphens become argument keys with the same hyphens (for example `webhook_header_x-request-id`). Use the `index` map-lookup form shown above, because Go templates treat hyphens in bare identifiers as subtraction.
+
 == Webhook Authentication
 
 OliveTin supports several authentication methods to verify webhook requests:

service/.golangci.yml

@@ -53,7 +53,17 @@ linters:
       - path: scripts/
         linters:
           - gosec
-      - path: cmd/
+      # Local config-tool CLI: operator-supplied path and 0644 config backups.
+      - path: cmd/config-tool/main\.go
+        text: "G304:"
+        linters:
+          - gosec
+      - path: cmd/config-tool/main\.go
+        text: "G306:"
+        linters:
+          - gosec
+      - path: cmd/config-tool/main\.go
+        text: "G703:"
         linters:
           - gosec
 
@@ -102,11 +112,33 @@ linters:
           - gosec
 
       # Secure is set dynamically from TLS / ForceSecureCookies; gosec wants a literal true.
-      - text: "G124:"
+      - path: internal/api/api\.go
+        text: "G124:"
+        linters:
+          - gosec
+      - path: internal/auth/otoauth2/
+        text: "G124:"
         linters:
           - gosec
 
-      # Protobuf / process exit codes mapped into int32 fields.
+      # Protobuf / process exit codes and collection sizes mapped into int32 fields.
-      - text: "G115:"
+      - path: internal/api/apiActions\.go
+        text: "G115:"
+        linters:
+          - gosec
+      - path: internal/api/api_entities_list\.go
+        text: "G115:"
+        linters:
+          - gosec
+      - path: internal/api/api_queue\.go
+        text: "G115:"
+        linters:
+          - gosec
+      - path: internal/api/config_issues\.go
+        text: "G115:"
+        linters:
+          - gosec
+      - path: internal/executor/executor\.go
+        text: "G115:"
         linters:
           - gosec

service/internal/api/api.go

@@ -43,7 +43,6 @@ type oliveTinAPI struct {
 	streamingClientsMutex sync.RWMutex
 }
 
-// Caps concurrent EventStream connections to limit memory/FD/goroutine exhaustion.
 const maxEventStreamClients = 16
 
 var errEventStreamClientLimit = errors.New("too many concurrent event stream clients")
@@ -1360,8 +1359,8 @@ func (api *oliveTinAPI) addCustomDashboardEntries(entries *[]*apiv1.RootDashboar
 	for _, dashboard := range dashboards {
 		// We have to build the dashboard response instead of just looping over config.dashboards,
 		// because we need to check if the user has access to the dashboard
-		db := renderDashboard(rr, dashboard.Title)
+		renderedDashboard := renderDashboard(rr, dashboard.Title)
-		if db != nil {
+		if renderedDashboard != nil {
 			*entries = append(*entries, &apiv1.RootDashboard{
 				Title:    dashboard.Title,
 				Category: dashboard.Category,

service/internal/api/api_test.go

@@ -138,11 +138,11 @@ func TestGetEntities(t *testing.T) {
 	resp, err := client.GetEntities(context.Background(), connect.NewRequest(&apiv1.GetEntitiesRequest{}))
 
 	require.NoError(t, err, "GetEntities should not return an error")
-	assert.NotNil(t, resp, "GetEntities response should not be nil")
+	require.NotNil(t, resp, "GetEntities response should not be nil")
-	assert.NotNil(t, resp.Msg, "GetEntities response message should not be nil")
+	require.NotNil(t, resp.Msg, "GetEntities response message should not be nil")
 
 	entityDefinitions := resp.Msg.EntityDefinitions
-	assert.Len(t, entityDefinitions, 3, "Should return 3 entity definitions")
+	require.Len(t, entityDefinitions, 3, "Should return 3 entity definitions")
 
 	validateEntityOrderAndStructure(t, entityDefinitions)
 	validateNoDuplicates(t, entityDefinitions)
@@ -189,6 +189,8 @@ func setupTestEntities() {
 func validateEntityOrderAndStructure(t *testing.T, entityDefinitions []*apiv1.EntityDefinition) {
 	t.Helper()
 
+	require.GreaterOrEqual(t, len(entityDefinitions), 3, "Need at least three entity definitions before indexing")
+
 	assert.Equal(t, "application", entityDefinitions[0].Title, "First entity should be 'application' (alphabetically first)")
 	assert.Len(t, entityDefinitions[0].Instances, 1, "Application should have 1 instance")
 	assert.Equal(t, "webapp", entityDefinitions[0].Instances[0].UniqueKey, "Application instance should be 'webapp'")
@@ -221,11 +223,12 @@ func validateConsistency(t *testing.T, client apiv1connect.OliveTinApiServiceCli
 
 	resp2, err2 := client.GetEntities(context.Background(), connect.NewRequest(&apiv1.GetEntitiesRequest{}))
 	require.NoError(t, err2, "Second GetEntities call should not return an error")
-	assert.Len(t, entityDefinitions, len(resp2.Msg.EntityDefinitions), "Second call should return same number of entity definitions")
+	require.NotNil(t, resp2.Msg)
+	require.Len(t, resp2.Msg.EntityDefinitions, len(entityDefinitions), "Second call should return same number of entity definitions")
 
 	for i, def := range entityDefinitions {
 		assert.Equal(t, def.Title, resp2.Msg.EntityDefinitions[i].Title, "Entity order should be consistent across calls")
-		assert.Len(t, def.Instances, len(resp2.Msg.EntityDefinitions[i].Instances), "Instance count should be consistent")
+		require.Len(t, resp2.Msg.EntityDefinitions[i].Instances, len(def.Instances), "Instance count should be consistent")
 		for j, inst := range def.Instances {
 			assert.Equal(t, inst.UniqueKey, resp2.Msg.EntityDefinitions[i].Instances[j].UniqueKey, "Instance order should be consistent across calls")
 		}

service/internal/auth/otjwt/jwt_test.go

@@ -17,6 +17,7 @@ import (
 	config "github.com/OliveTin/OliveTin/internal/config"
 	"github.com/golang-jwt/jwt/v5"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func generateRSAKeyPair(t *testing.T) (*rsa.PrivateKey, []byte) {
@@ -116,7 +117,8 @@ func verifyJWTResponse(t *testing.T, res *http.Response, expectCode int) {
 
 	defer func() { _ = res.Body.Close() }()
 	assert.Equal(t, expectCode, res.StatusCode)
-	body, _ := io.ReadAll(res.Body)
+	body, err := io.ReadAll(res.Body)
+	require.NoError(t, err, "reading JWT response body")
 	t.Logf("Response body: %s", string(body))
 }
 
@@ -145,15 +147,9 @@ func testJwkValidationWithAudience(t *testing.T, expire int64, expectCode int, c
 	srv := httptest.NewServer(handler)
 	defer srv.Close()
 
-	res := makeJWTRequest(t, srv, tokenStr)
+	res := makeJWTRequest(t, srv, tokenStr) //nolint:bodyclose // closed by verifyJWTResponse
 
 	verifyJWTResponse(t, res, expectCode)
-
-	err := res.Body.Close()
-
-	if err != nil {
-		t.Error("Could not close response body", err)
-	}
 }
 
 func TestJWTSignatureVerificationSucceeds(t *testing.T) {
@@ -238,10 +234,6 @@ func TestJWTHeader(t *testing.T) {
 	srv := httptest.NewServer(mux)
 	defer srv.Close()
 
-	res := makeJWTRequest(t, srv, tokenStr)
+	res := makeJWTRequest(t, srv, tokenStr) //nolint:bodyclose // closed by verifyJWTResponse
-	defer func() { _ = res.Body.Close() }()
+	verifyJWTResponse(t, res, http.StatusOK)
-
-	assert.Equal(t, http.StatusOK, res.StatusCode)
-	body, _ := io.ReadAll(res.Body)
-	t.Logf("Response body: %s", string(body))
 }

service/internal/auth/otoauth2/restapi_auth_oauth2.go

@@ -353,7 +353,7 @@ func getUserInfo(cfg *config.Config, client *http.Client, provider *config.OAuth
 		return ret
 	}
 
-	res, err := http.DefaultClient.Do(req)
+	res, err := client.Do(req)
 
 	if err != nil {
 		log.Errorf("Failed to get user data: %v", err)

service/internal/executor/arguments_test.go

@@ -358,7 +358,7 @@ func TestArgumentNotProvided(t *testing.T) {
 	out, err := parseActionArguments(req)
 
 	assert.Empty(t, out)
-	assert.Equal(t, "required arg not provided: personName", err.Error())
+	require.EqualError(t, err, "required arg not provided: personName")
 }
 
 func TestExecArrayParsing(t *testing.T) {

service/internal/executor/executor.go

@@ -49,8 +49,6 @@ type ActionBinding struct {
 	ConfigOrder  int
 }
 
-// Executor represents a helper class for executing commands. It's main method
-// is ExecRequest
 type Executor struct {
 	logs                  map[string]*InternalLogEntry
 	LogsByBindingId       map[string][]*InternalLogEntry
@@ -1282,12 +1280,75 @@ var (
 )
 
 func substituteShellAfterCompletedEnvRefs(command string) string {
-	// $$ is required: regexp replacements treat $ as submatch expansion.
+	command = replaceShellAfterEnvRef(command, shellAfterOutputRef, "$OUTPUT")
-	command = shellAfterOutputRef.ReplaceAllString(command, `"$$OUTPUT"`)
+	command = replaceShellAfterEnvRef(command, shellAfterExitCodeRef, "$EXITCODE")
-	command = shellAfterExitCodeRef.ReplaceAllString(command, `"$$EXITCODE"`)
 	return command
 }
 
+func replaceShellAfterEnvRef(command string, pattern *regexp.Regexp, envRef string) string {
+	matches := pattern.FindAllStringIndex(command, -1)
+	for i := len(matches) - 1; i >= 0; i-- {
+		start, end := matches[i][0], matches[i][1]
+		replacement := `"` + envRef + `"`
+		if shellPosInsideSingleQuotes(command, start) {
+			// Break out of single quotes so the env ref can expand at runtime.
+			replacement = `'` + replacement + `'`
+		}
+		command = command[:start] + replacement + command[end:]
+	}
+	return command
+}
+
+func shellPosInsideSingleQuotes(command string, pos int) bool {
+	inSingle := false
+	inDouble := false
+	i := 0
+
+	for i < pos {
+		inSingle, inDouble, i = advanceShellQuoteState(command, i, pos, inSingle, inDouble)
+	}
+
+	return inSingle
+}
+
+func advanceShellQuoteState(command string, i, pos int, inSingle, inDouble bool) (bool, bool, int) {
+	if inSingle {
+		return advanceInsideSingleQuote(command, i, inSingle, inDouble)
+	}
+	if inDouble {
+		return advanceInsideDoubleQuote(command, i, pos, inSingle, inDouble)
+	}
+	return advanceOutsideQuotes(command, i, inSingle, inDouble)
+}
+
+func advanceInsideSingleQuote(command string, i int, inSingle, inDouble bool) (bool, bool, int) {
+	if command[i] == '\'' {
+		return false, inDouble, i + 1
+	}
+	return inSingle, inDouble, i + 1
+}
+
+func advanceInsideDoubleQuote(command string, i, pos int, inSingle, inDouble bool) (bool, bool, int) {
+	if command[i] == '\\' && i+1 < pos {
+		return inSingle, inDouble, i + 2
+	}
+	if command[i] == '"' {
+		return inSingle, false, i + 1
+	}
+	return inSingle, inDouble, i + 1
+}
+
+func advanceOutsideQuotes(command string, i int, inSingle, inDouble bool) (bool, bool, int) {
+	switch command[i] {
+	case '\'':
+		return true, inDouble, i + 1
+	case '"':
+		return inSingle, true, i + 1
+	default:
+		return inSingle, inDouble, i + 1
+	}
+}
+
 // shellAfterTemplateArgs omits output/exitCode so templates cannot expand them
 // raw. Those values are only provided as OUTPUT/EXITCODE process environment.
 func shellAfterTemplateArgs(args map[string]string) map[string]string {

service/internal/executor/executor_test.go

@@ -449,6 +449,45 @@ func TestShellAfterCompletedUsesOutputEnvSafely(t *testing.T) {
 	assert.True(t, os.IsNotExist(err), "shellAfterCompleted must not execute injected commands from output")
 }
 
+func TestShellAfterCompletedExpandsQuotedPlaceholders(t *testing.T) {
+	cases := []struct {
+		name string
+		sac  string
+	}{
+		{"legacy single-quoted", `printf '%s' '{{ output }}'`},
+		{"modern single-quoted", `printf '%s' '{{ .Arguments.output }}'`},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			cfg := config.DefaultConfig()
+			executor := DefaultExecutor(cfg)
+			mainOutput := "quoted-output-ok"
+
+			action := &config.Action{
+				Title:               "sac-quoted-" + tc.name,
+				Shell:               "printf %s \"" + mainOutput + "\"",
+				ShellAfterCompleted: tc.sac,
+			}
+			cfg.Actions = append(cfg.Actions, action)
+			cfg.Sanitize()
+			executor.RebuildActionMap()
+
+			req := ExecutionRequest{
+				AuthenticatedUser: auth.UserFromSystem(cfg, "cron"),
+				Cfg:               cfg,
+				Binding:           executor.FindBindingWithNoEntity(action),
+			}
+			wg, _ := executor.ExecRequest(&req)
+			wg.Wait()
+
+			require.NotNil(t, req.logEntry)
+			assert.Equal(t, int32(0), req.logEntry.ExitCode)
+			assert.Contains(t, req.logEntry.Output, "OliveTin::shellAfterCompleted stdout\n"+mainOutput)
+		})
+	}
+}
+
 func TestShellAfterCompletedBlocksArgumentsOutputInjection(t *testing.T) {
 	payload := func(injectedPath string) string {
 		return "x; touch " + injectedPath + "; #"
@@ -469,25 +508,25 @@ func TestShellAfterCompletedBlocksArgumentsOutputInjection(t *testing.T) {
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
 			cfg := config.DefaultConfig()
-			e := DefaultExecutor(cfg)
+			executor := DefaultExecutor(cfg)
 			injectedPath := filepath.Join(t.TempDir(), "injected")
 			mainPayload := payload(injectedPath)
 
-			a1 := &config.Action{
+			action := &config.Action{
 				Title:               "sac-injection-" + tc.name,
 				Shell:               "printf %s \"" + mainPayload + "\"",
 				ShellAfterCompleted: tc.sac,
 			}
-			cfg.Actions = append(cfg.Actions, a1)
+			cfg.Actions = append(cfg.Actions, action)
 			cfg.Sanitize()
-			e.RebuildActionMap()
+			executor.RebuildActionMap()
 
 			req := ExecutionRequest{
 				AuthenticatedUser: auth.UserFromSystem(cfg, "cron"),
 				Cfg:               cfg,
-				Binding:           e.FindBindingWithNoEntity(a1),
+				Binding:           executor.FindBindingWithNoEntity(action),
 			}
-			wg, _ := e.ExecRequest(&req)
+			wg, _ := executor.ExecRequest(&req)
 			wg.Wait()
 
 			_, err := os.Stat(injectedPath)
@@ -509,6 +548,10 @@ func TestSubstituteShellAfterCompletedEnvRefs(t *testing.T) {
 		{`echo {{ exitCode }}`, `echo "$EXITCODE"`},
 		{`echo {{ .Arguments.exitCode }}`, `echo "$EXITCODE"`},
 		{`echo {{  .Arguments.exitCode  }}`, `echo "$EXITCODE"`},
+		{`printf '%s' '{{ output }}'`, `printf '%s' ''"$OUTPUT"''`},
+		{`printf '%s' '{{ .Arguments.output }}'`, `printf '%s' ''"$OUTPUT"''`},
+		{`printf '%s' '{{ exitCode }}'`, `printf '%s' ''"$EXITCODE"''`},
+		{`printf '%s' '{{ .Arguments.exitCode }}'`, `printf '%s' ''"$EXITCODE"''`},
 	}
 
 	for _, tc := range cases {

specs/config-issues.md

@@ -8,7 +8,7 @@ Operators should see configuration problems in Diagnostics instead of only in se
 
 ## When issues are rebuilt
 
-The issue list is cleared and rebuilt when the action map is rebuilt. That happens after configuration load or reload, and after entity data changes. Some load-time findings that cannot be re-derived after decode (for example unset environment variables already expanded away) are kept across rebuilds until the next configuration load begins.
+The issue list is cleared and rebuilt when configuration is loaded or reloaded, and when entity data changes. Some findings that can only be detected while configuration is first being loaded (for example references to unset environment variables that are expanded away during that load) are kept across later rebuilds until the next configuration load begins.
 
 ## What is collected
 
@@ -16,7 +16,7 @@ Issues include:
 
 - Unknown or unenforced action group references
 - Checklist arguments with missing or invalid choice templates
-- Arguments whose type was left unset (defaulted to ascii)
+- Arguments whose type was left unset (defaulted to a generic text type)
 - Unset environment variables referenced from configuration
 - Missing or invalid include directories
 - Argument default or choice templates that fail to parse
@@ -29,7 +29,7 @@ Issues include:
 
 Each issue has a severity of warning or error, a stable code, a human-readable message, and optional context such as action title, argument name, configuration source file, or detail value.
 
-When the issue list is rebuilt, OliveTin logs only newly appeared issues so startup does not repeat the same warning for every action-map rebuild.
+When the issue list is rebuilt, OliveTin logs only newly appeared issues so startup does not repeat the same warning for every rebuild.
 
 When configuration is loaded from a base file and an include directory, OliveTin records which file defined each action and entity declaration. That path is shown as the configuration source file when available. Some issues (for example unset environment variables) may not have a specific file. Entity data file problems also show the entity data path in the detail column.
 
@@ -45,6 +45,6 @@ Users who are not allowed to view Diagnostics cannot retrieve the issue list.
 
 When Diagnostics is visible and at least one configuration issue exists that the user is allowed to see, the Diagnostics navigation link shows a count badge with the number of those issues. The badge clears when the visible issue count becomes zero after a configuration or entity refresh.
 
-## Init count
+## Startup count
 
-The Init response includes the configuration issue count for users who may view Diagnostics, using the same per-user filtering as the Diagnostics list. For other users the count is zero.
+When the web UI starts, users who may view Diagnostics receive the same filtered configuration issue count used for the Diagnostics list and navigation badge. For other users the count is zero.

specs/dashboard-nav-categories.md

@@ -28,7 +28,7 @@ When building the sidebar:
 3. Within a category, dashboards keep the order they appear in the configuration among visible dashboards in that category.
 4. After all dashboard links, a **System** category lists Entities, Logs, and Diagnostics (each only when the user is allowed to see that item). If none of those links are visible, the System category is omitted.
 
-The default Actions dashboard, when present, is uncategorized unless configuration gives it a category (it is not a configured root entry, so it stays uncategorized).
+The default Actions dashboard, when present, is always uncategorized.
 
 ## 4. Navigation style