refactor(engine): remove direct db dependency from actions logic by sachin9058 · Pull Request #6305 · mindersec/minder

sachin9058 · 2026-04-08T16:05:57Z

Summary

This PR builds on the recently merged #6289 and further simplifies the engine layer by removing remaining direct dependencies on internal/db types from the actions logic.

Previously, the engine layer (internal/engine/actions) relied on database-specific types such as db.ListRuleEvaluationsByProfileIdRow, creating tight coupling between business logic and persistence.

While #6289 introduced an adapter-based approach for mapping database types, this PR refines the design by:

Introducing engine-level abstractions (EvalStatus, PreviousEval)
Refactoring action decision logic (shouldRemediate, shouldAlert) to operate solely on these types
Performing conversion at the boundary (DoActions) instead of relying on adapter mappings

Additionally, this PR fixes incorrect handling of evaluation states by properly distinguishing between:

evaluation failures
skipped evaluations
generic errors

This ensures correct action behavior (e.g., avoiding unintended alerts or remediation on generic errors).

Changes

Removed direct usage of internal/db types from engine logic
Introduced engine-level domain types (EvalStatus, PreviousEval)
Refactored remediation and alert decision logic to use engine abstractions
Updated tests to align with the new types
Corrected behavior for error vs failure handling

Benefits

Improves separation of concerns between engine and persistence layers
Simplifies architecture by reducing indirection
Avoids import cycle risks introduced by adapter-based approaches
Improves correctness of action decisions
Enhances testability and maintainability

No new dependencies are introduced.

Testing

The changes were validated using the following steps:

Build Verification

go build ./...

Linting

golangci-lint run

Unit Tests

go test ./...

Updated actions_test.go to use engine-level abstractions
Verified correct behavior across:
- success, failure, skipped, and error scenarios
- nil previous evaluation cases
- remediation and alert transitions

Manual Verification

Validated that:

errors no longer incorrectly trigger remediation/alert actions
logic behaves consistently with expected evaluation outcomes

All checks pass successfully with no lint issues or test failures.

Fixes #NONE

sachin9058 · 2026-04-08T16:10:58Z

Open to feedback if a different approach is preferred.

coveralls · 2026-04-08T17:49:35Z

coverage: 59.368% (-0.02%) from 59.39% — sachin9058:refactor/engine-actions-remove-db-dependency into mindersec:main

evankanderson

This looks like it is making good progress. A couple small comments (your LLM is still rewriting a lot of comments it doesn't need to), but this is looking promising.

evankanderson · 2026-04-08T20:53:13Z

+		var prev *PreviousEval
+		if row := params.GetEvalStatusFromDb(); row != nil {
+			prev = &PreviousEval{
+				RemediationStatus: RemediationStatus(row.RemStatus),
+				AlertStatus:       AlertStatus(row.AlertStatus),
+				RemediationMeta:   &row.RemMetadata,
+				AlertMeta:         &row.AlertMetadata,
+			}
+		}


Should we construct prev once for both remediate and alert?

evankanderson · 2026-04-08T20:54:52Z

 				Msg("panic in action execution")
-			finalErr = enginerr.ErrInternal
+
+			err = enginerr.ErrInternal // restore behavior


I don't think this comment makes sense. LLM leftover?

evankanderson · 2026-04-08T20:55:42Z

+// previous remediation state.
+func shouldRemediate(prevEval *PreviousEval, evalStatus EvalStatus) engif.ActionCmd {
+	// Determine current evaluation status
+	newEval := evalStatus


Why do this rename?

evankanderson · 2026-04-08T20:59:10Z

+	// Determine current evaluation status
+	newEval := evalStatus


Again, I think we can just rename the evalStatus argument to newEval and avoid this assignment.

evankanderson · 2026-04-08T20:59:59Z

-	// Case 1 - Successful remediation of a type that is not PR is considered instant.
+	// Case: successful remediation (non-PR type)


The original comment explained that non-PR remediations were "instant", so they would already be fixed. That's useful context for why this case is what it is.

evankanderson · 2026-04-08T21:01:09Z

-		// Case 3 - Evaluation changed from something else to ERROR -> Alert should be ON
-		// Case 4 - Evaluation has changed from something else to FAILED -> Alert should be ON
-		// The Alert should be on (if it wasn't already)
-		if db.AlertStatusTypesOn != prevAlert {


Can you keep these "Case X" statements? It helps understand the original intent of the code -- unless you feel like you've substantially adjusted the logic here on purpose.

evankanderson · 2026-04-08T21:04:33Z

 	if err != nil {
-		logger.Error().Err(err).Msg("error marshaling empty json.RawMessage")
+		logger.Error().Err(err).Msg("error marshaling empty json")
 	}


We should just replace this with a direct assignment, and avoid the need for error handling.

Suggested change

}

// Even though meta is an empty json struct by default, there's no risk of overwriting

// any existing meta entry since we don't upsert in case of conflict while skipping

m := json.RawMessage("{}")

sachin9058 · 2026-04-08T22:46:11Z

@evankanderson Thanks for the feedback !!! really helpful! I’ve gone through and addressed the points you mentioned, including cleaning up some leftover comments, reusing prev, and restoring the original case comments to keep the intent clear.

Happy to tweak anything further if needed.

evankanderson

There's still a lot of comment movement; it makes it somewhat harder to review the diff (since about 25% of it is no-op rewordings or addition of "." to the end of sentences). I can work around it, but it's generally helpful to practice producing minimal diffs when there's another human reviewing the code.

evankanderson · 2026-04-10T07:06:28Z

-		// Case 2 - Evaluation changed from something else to ERROR -> Remediation should be OFF
-		// Case 3 - Evaluation changed from something else to PASSING -> Remediation should be OFF


You're still deleting these "Case N" statements. Are you sure that the code is matching the comments? If not, I'd leave in the (conflicting) comments until we figure out whether the code or the comments are correct.

evankanderson · 2026-04-10T07:09:55Z

-		// Case 4 - Evaluation has changed from something else to FAILED -> Remediation should be ON
-		// We should remediate only if the previous remediation was skipped, so we don't risk endless remediation loops


And keep these comments, please.

(Keep this comment about endless remediation loops, too)

evankanderson · 2026-04-10T07:12:09Z

-
 	// Case 1 - Successful remediation of a type that is not PR is considered instant.
 	if remType != pull_request.RemediateType && remErr == nil {
-		// If this is the case either skip alerting or turn it off if it was on


Again, I think this comment helps clarify the intent beyond simply what the code does.

Please keep this comment about skipping alerting or turning it off -- this code is more subtle than I'd like (it tries to interpret past and present to "understand" what edge-change is happening, rather than simply copying status from A to B).

evankanderson · 2026-04-10T07:13:33Z

-		// Case 5 - Evaluation changed from something else to PASSING -> Alert should be OFF
-		// The Alert should be turned OFF (if it wasn't already)


Again, keep this comment if you can. If you can keep the indentation the same, it will help GitHub line up the diff correctly (right now, it's anchoring on blank lines stupidly, because it can't figure out which lines are common).

evankanderson · 2026-04-10T07:14:40Z

-		// If the action is not found, definitely skip it
-		logger.Msg("action type not found, skipping")
+		zerolog.Ctx(ctx).Info().
+			Str("eval_status", fmt.Sprintf("%v", evalErr)).


If evalErr is an error, you can simply call evalErr.Error() to get the string representation.

evankanderson · 2026-04-10T07:15:39Z

-		logger.Msg("unknown action option, skipping")
 		return true
 	case models.ActionOptDryRun, models.ActionOptOn:
-		// Action is on or dry-run, do not skip yet. Check the evaluation error


I think this error helps to explain why this is not simply false or true. Can you leave at least this comment?

Can we keep some comment about some on and dry-run actions still get skipped because the evaluation requested a "skip" result.

evankanderson · 2026-04-10T07:18:03Z

+// getAlertMeta returns alert metadata from previous evaluation.
+func getAlertMeta(prevEval *PreviousEval) *json.RawMessage {
+	if prevEval != nil {
+		return prevEval.AlertMeta
 	}
 	return nil
 }


Now that PreviousEval is a struct we define here (rather than a DB row), we can define a GetAlertMeta() *json.RawMessage function that will do this even if the pointer is nil, rather than needing a free function:

Suggested change

// getAlertMeta returns alert metadata from previous evaluation.

func getAlertMeta(prevEval *PreviousEval) *json.RawMessage {

if prevEval != nil {

return prevEval.AlertMeta

}

return nil

}

// getAlertMeta returns alert metadata from previous evaluation.

func (p *PreviousEval) GetAlertMeta() *json.RawMessage {

if p!= nil {

return p.AlertMeta

}

return nil

}

It's also not clear to me that PreviousEval needs to be exported, so it might be spelled previousEval...

evankanderson · 2026-04-10T07:21:45Z

@@ -213,7 +223,17 @@ func TestShouldAlert(t *testing.T) {
 			if !tt.nilRow {
 				prevRow = &db.ListRuleEvaluationsByProfileIdRow{AlertStatus: tt.prevAlert}
 			}
-			got := shouldAlert(prevRow, tt.evalErr, tt.remErr, tt.remType)
+			var prev *PreviousEval
+			if prevRow != nil {
+				prev = &PreviousEval{
+					AlertStatus: AlertStatus(prevRow.AlertStatus),
+				}
+			}


There's no need to have prevRow be a DB type anymore, right? It could just be a *PreviousEval?

Suggested change

}

var prev *PreviousEval

if !tt.nilRow {

prev = &PreviousEval{

AlertStatus: AlertStatus(tt.prevAlert),

}

}

(Some of the rows are outside the suggestion window, so it presents this like a comment on a single line.

sachin9058 · 2026-04-10T20:49:23Z

@evankanderson
Thanks for the feedback!

I've addressed the review comments:

restored the original "Case X" comments to preserve intent
removed unnecessary comment rewrites to keep the diff minimal
added the TODO for EvalStatusError as suggested
fixed logging to use evalErr.Error()

I also rebased the branch on the latest main and resolved conflicts.

Please let me know if anything else should be adjusted.

evankanderson

One lint error, and a bit more return of the comments, and this should be good to go.

evankanderson · 2026-04-10T22:21:22Z

 	"github.com/rs/zerolog"
 	"google.golang.org/protobuf/reflect/protoreflect"

+	"strings"


"strings" should be in the block with "time" and "fmt", as it's part of the go standard library (and the linter is complaining)

evankanderson · 2026-04-10T22:24:48Z

+		if prevRemediation != RemediationStatus("skipped") {
 			return engif.ActionCmdOff
 		}
-		// We should do nothing if remediation was already skipped


This comment again is helpful explaining why we return "Do Nothing" when the comment says " -> OFF". It might have been better to store the previous state On/Off state, but that's not what's going on here, so it's useful to understand the intents.

evankanderson · 2026-04-10T22:25:05Z

-		// Case 4 - Evaluation has changed from something else to FAILED -> Remediation should be ON
-		// We should remediate only if the previous remediation was skipped, so we don't risk endless remediation loops


(Keep this comment about endless remediation loops, too)

evankanderson · 2026-04-10T22:26:47Z

-
 	// Case 1 - Successful remediation of a type that is not PR is considered instant.
 	if remType != pull_request.RemediateType && remErr == nil {
-		// If this is the case either skip alerting or turn it off if it was on


Please keep this comment about skipping alerting or turning it off -- this code is more subtle than I'd like (it tries to interpret past and present to "understand" what edge-change is happening, rather than simply copying status from A to B).

evankanderson · 2026-04-10T22:28:44Z

-		logger.Msg("unknown action option, skipping")
 		return true
 	case models.ActionOptDryRun, models.ActionOptOn:
-		// Action is on or dry-run, do not skip yet. Check the evaluation error


Can we keep some comment about some on and dry-run actions still get skipped because the evaluation requested a "skip" result.

sachin9058 · 2026-04-11T13:46:58Z

@evankanderson Thanks for the detailed feedback, that clarified the intent a lot.

I’ve updated the comments to better reflect the reasoning behind remediation decisions and alert transitions, especially around avoiding repeated remediation and handling state changes correctly. I also addressed the lint issues and rebased on the latest upstream.

Happy to make further adjustments if needed.

evankanderson

You need to revert the changes to entities/properties/service from this PR.

(In general, it's good ta take a look at the actual file diff to make sure that the contents roughly match what you expect the reviewer to be looking at.)

evankanderson · 2026-04-12T13:52:08Z

+		// handle duplicate key error gracefully (acts like update)
+		if strings.Contains(err.Error(), "duplicate key") {


Why is this here (why is it part of this PR, at least)?

Secondarily (meaning: if you are opening another PR for this), It looks like UpsertPropertyValueV1 bottoms out in:

INSERT INTO properties ( entity_id, key, value, updated_at ) VALUES ($1, $2, $3, NOW()) ON CONFLICT (entity_id, key) DO UPDATE SET value = $4, updated_at = NOW() RETURNING id, entity_id, key, value, updated_at

Which should already handle the duplicate key case. If we get an error, it seems like we probably have some other problem that prevented this.

sachin9058 · 2026-04-13T09:45:28Z

@evankanderson
I've cleaned up the branch and squashed the changes into a single commit so the PR only includes the intended engine refactor.

It now only contains the actions logic updates along with the new internal types to decouple from database representations.

I've also addressed the previous feedback around comments and structure.

Happy to refine further if needed.

sachin9058 requested a review from a team as a code owner April 8, 2026 16:05

sachin9058 force-pushed the refactor/engine-actions-remove-db-dependency branch 3 times, most recently from 589de8f to 8e9d9d1 Compare April 8, 2026 17:38

evankanderson reviewed Apr 8, 2026

View reviewed changes

sachin9058 force-pushed the refactor/engine-actions-remove-db-dependency branch from 71f4d58 to 24ffab7 Compare April 8, 2026 22:45

evankanderson reviewed Apr 10, 2026

View reviewed changes

sachin9058 force-pushed the refactor/engine-actions-remove-db-dependency branch from f09e0c6 to af3b673 Compare April 10, 2026 20:47

evankanderson reviewed Apr 10, 2026

View reviewed changes

sachin9058 force-pushed the refactor/engine-actions-remove-db-dependency branch from af3b673 to 8781cf7 Compare April 11, 2026 13:44

evankanderson requested changes Apr 12, 2026

View reviewed changes

refactor(engine): decouple actions logic from database types

22c3fbc

sachin9058 force-pushed the refactor/engine-actions-remove-db-dependency branch from 1e2a2a0 to 22c3fbc Compare April 13, 2026 09:44

sachin9058 requested a review from evankanderson April 13, 2026 09:46

		// Case 1 - Successful remediation of a type that is not PR is considered instant.
		// Case: successful remediation (non-PR type)

-	}
+	// Even though meta is an empty json struct by default, there's no risk of overwriting
+	// any existing meta entry since we don't upsert in case of conflict while skipping
+	m := json.RawMessage("{}")

		// Case 2 - Evaluation changed from something else to ERROR -> Remediation should be OFF
		// Case 3 - Evaluation changed from something else to PASSING -> Remediation should be OFF

		// Case 4 - Evaluation has changed from something else to FAILED -> Remediation should be ON
		// We should remediate only if the previous remediation was skipped, so we don't risk endless remediation loops

		// Case 5 - Evaluation changed from something else to PASSING -> Alert should be OFF
		// The Alert should be turned OFF (if it wasn't already)

		// handle duplicate key error gracefully (acts like update)
		if strings.Contains(err.Error(), "duplicate key") {

Conversation

sachin9058 commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Benefits

Testing

Build Verification

Linting

Unit Tests

Manual Verification

Uh oh!

sachin9058 commented Apr 8, 2026

Uh oh!

coveralls commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachin9058 commented Apr 8, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachin9058 commented Apr 10, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachin9058 commented Apr 11, 2026

sachin9058 commented Apr 8, 2026 •

edited

Loading

coveralls commented Apr 8, 2026 •

edited

Loading