Validate action schemas #632
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughConstraint validation is added to global and resource action execution: action schemas are retrieved and constraints (required-together, mutually exclusive, at-least-one, dependent-on) are validated against provided args before scheduling or invoking handlers; new helpers and error paths for missing schemas were introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ActionInvoker as Invoker
participant SchemaStore as SchemaStore
participant Validator as ConstraintValidator
participant Handler as ActionHandler
Client->>Invoker: Request to invoke action (args, resource?)
Invoker->>SchemaStore: Fetch action schema (global or resource-specific)
SchemaStore-->>Invoker: Return action schema
Invoker->>Validator: validateActionConstraints(schema.constraints, args)
alt validation passes
Validator-->>Invoker: OK
Invoker->>Handler: Invoke action handler (args)
Handler-->>Client: Action result
else validation fails
Validator-->>Invoker: Validation error
Invoker-->>Client: Return validation error (pre-execution)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/actions/actions.go (2)
418-466: Avoid silently skipping validation when schema is missing; also fixdone <- struct{}{}goroutine leak risk.
- Line 422-434: if
handlerexists butschemais nil/missing, constraints are skipped (likely masking registry corruption).- Line 454: if InvokeAction returns via timeout/cancel, the goroutine can block forever on
done <- struct{}{}.Proposed fix
func (a *ActionManager) invokeGlobalAction(ctx context.Context, name string, args *structpb.Struct) (string, v2.BatonActionStatus, *structpb.Struct, annotations.Annotations, error) { a.mu.RLock() handler, ok := a.handlers[name] - schema := a.schemas[name] + schema, schemaOk := a.schemas[name] a.mu.RUnlock() if !ok { return "", v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED, nil, nil, status.Error(codes.NotFound, fmt.Sprintf("handler for action %s not found", name)) } + if !schemaOk || schema == nil { + return "", v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED, nil, nil, status.Error(codes.Internal, fmt.Sprintf("schema for action %s not found", name)) + } // Validate constraints - if schema != nil { - if err := validateActionConstraints(schema.GetConstraints(), args); err != nil { - return "", v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED, nil, nil, status.Error(codes.InvalidArgument, err.Error()) - } - } + if err := validateActionConstraints(schema.GetConstraints(), args); err != nil { + return "", v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED, nil, nil, status.Error(codes.InvalidArgument, err.Error()) + } oa := a.GetNewAction(name) - done := make(chan struct{}) + done := make(chan struct{}) // If handler exits within a second, return result. // If handler takes longer than 1 second, return status pending. // If handler takes longer than an hour, return status failed. go func() { + defer close(done) oa.SetStatus(ctx, v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING) handlerCtx, cancel := context.WithTimeoutCause(context.Background(), 1*time.Hour, errors.New("action handler timed out")) defer cancel() var oaErr error oa.Rv, oa.Annos, oaErr = handler(handlerCtx, args) if oaErr == nil { oa.SetStatus(ctx, v2.BatonActionStatus_BATON_ACTION_STATUS_COMPLETE) } else { oa.SetError(ctx, oaErr) } - done <- struct{}{} }()
469-545: Resource action lookup errors: preferNotFoundfor missing schema/action; fixdone <- struct{}{}goroutine leak risk here too.
- Line 505-509: missing schema for
actionNamereturnscodes.Internal; this looks likeNotFound(vs genuine registry corruption).- Line 532: same potential blocking send to
doneif caller returns early.Proposed fix
schema, ok := schemas[actionName] if !ok { a.mu.RUnlock() - return "", v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED, nil, nil, status.Error(codes.Internal, fmt.Sprintf("schema not found for action %s", actionName)) + return "", v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED, nil, nil, status.Error(codes.NotFound, fmt.Sprintf("schema not found for action %s", actionName)) } a.mu.RUnlock() // Validate constraints if err := validateActionConstraints(schema.GetConstraints(), args); err != nil { return "", v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED, nil, nil, status.Error(codes.InvalidArgument, err.Error()) } oa := a.GetNewAction(actionName) done := make(chan struct{}) // Invoke handler in goroutine go func() { + defer close(done) oa.SetStatus(ctx, v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING) handlerCtx, cancel := context.WithTimeoutCause(context.Background(), 1*time.Hour, errors.New("action handler timed out")) defer cancel() var oaErr error oa.Rv, oa.Annos, oaErr = handler(handlerCtx, args) if oaErr == nil { oa.SetStatus(ctx, v2.BatonActionStatus_BATON_ACTION_STATUS_COMPLETE) } else { oa.SetError(ctx, oaErr) } - done <- struct{}{} }()
🤖 Fix all issues with AI agents
In @pkg/actions/actions.go:
- Around line 547-631: The constraint validation can panic or produce confusing
errors when nils or empty field lists appear: guard against nil Constraint
objects in validateActionConstraints/validateConstraint, skip or return a clear
error for constraints with empty FieldNames (especially for AT_LEAST_ONE,
REQUIRED_TOGETHER, MUTUALLY_EXCLUSIVE), and avoid dereferencing nil
structpb.Value in isNullValue by first checking for nil; likewise make
deduplicateStrings tolerate nil/empty slices. Concretely, in
validateActionConstraints check each constraint != nil before calling
validateConstraint, in validateConstraint return a descriptive error when
len(c.GetFieldNames()) == 0 (or treat empty as no-op) and handle nil secondary
lists, and change isNullValue to return true/false when v == nil without calling
v.GetKind().
🧹 Nitpick comments (1)
pkg/actions/actions_test.go (1)
201-468: Good coverage; add 2 “nil safety” cases to prevent panics (nil constraint, nil value).Right now this suite won’t catch panics from
validateActionConstraintsif a constraint slice containsnil, or ifargs.Fields["x"]is present but the value pointer isnil.Additional test cases
func TestConstraintValidation(t *testing.T) { + t.Run("nil constraint returns error (no panic)", func(t *testing.T) { + constraints := []*config.Constraint{nil} + err := validateActionConstraints(constraints, &structpb.Struct{Fields: map[string]*structpb.Value{}}) + require.Error(t, err) + }) + + t.Run("nil structpb.Value is not considered present (no panic)", func(t *testing.T) { + constraints := []*config.Constraint{ + config.Constraint_builder{ + Kind: config.ConstraintKind_CONSTRAINT_KIND_AT_LEAST_ONE, + FieldNames: []string{"field_a"}, + }.Build(), + } + args := &structpb.Struct{Fields: map[string]*structpb.Value{"field_a": nil}} + err := validateActionConstraints(constraints, args) + require.Error(t, err) + }) + t.Run("RequiredTogether - both present passes", func(t *testing.T) { constraints := []*config.Constraint{ config.Constraint_builder{ Kind: config.ConstraintKind_CONSTRAINT_KIND_REQUIRED_TOGETHER, FieldNames: []string{"field_a", "field_b"},
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/actions/actions.gopkg/actions/actions_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-02T17:21:01.723Z
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:01.723Z
Learning: In tests that verify cleanup behavior (e.g., ensuring temporary artifacts are removed or directories are empty after an operation), treat cleanup failures as test failures by asserting on the cleanup call (e.g., require.NoError(t, err) or assert.NoError(t, err)). This ensures that the cleanup path is explicitly tested and any cleanup error fails the test, confirming correct behavior of the cleanup code.
Applied to files:
pkg/actions/actions_test.go
🧬 Code graph analysis (2)
pkg/actions/actions_test.go (1)
pb/c1/config/v1/config.pb.go (7)
Constraint(309-319)Constraint(332-332)Constraint_builder(412-421)ConstraintKind_CONSTRAINT_KIND_REQUIRED_TOGETHER(30-30)ConstraintKind_CONSTRAINT_KIND_MUTUALLY_EXCLUSIVE(32-32)ConstraintKind_CONSTRAINT_KIND_AT_LEAST_ONE(31-31)ConstraintKind_CONSTRAINT_KIND_DEPENDENT_ON(33-33)
pkg/actions/actions.go (1)
pb/c1/config/v1/config.pb.go (7)
Constraint(309-319)Constraint(332-332)ConstraintKind_CONSTRAINT_KIND_REQUIRED_TOGETHER(30-30)ConstraintKind_CONSTRAINT_KIND_MUTUALLY_EXCLUSIVE(32-32)ConstraintKind_CONSTRAINT_KIND_AT_LEAST_ONE(31-31)ConstraintKind_CONSTRAINT_KIND_DEPENDENT_ON(33-33)ConstraintKind_CONSTRAINT_KIND_UNSPECIFIED(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: go-test (1.25.2, windows-latest)
- GitHub Check: go-test (1.25.2, ubuntu-latest)
Comment on lines
547
to
631
| // validateActionConstraints validates that the provided args satisfy the schema constraints. | ||
| func validateActionConstraints(constraints []*config.Constraint, args *structpb.Struct) error { | ||
| if len(constraints) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| // Build map of present fields (non-null values in struct) | ||
| present := make(map[string]bool) | ||
| if args != nil { | ||
| for fieldName, value := range args.GetFields() { | ||
| if !isNullValue(value) { | ||
| present[fieldName] = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Validate each constraint | ||
| for _, constraint := range constraints { | ||
| if err := validateConstraint(constraint, present); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func validateConstraint(c *config.Constraint, present map[string]bool) error { | ||
| // Deduplicate field names to handle cases where the same field is listed multiple times | ||
| uniqueFieldNames := deduplicateStrings(c.GetFieldNames()) | ||
|
|
||
| // Count how many unique primary fields are present | ||
| var primaryPresent int | ||
| for _, name := range uniqueFieldNames { | ||
| if present[name] { | ||
| primaryPresent++ | ||
| } | ||
| } | ||
|
|
||
| switch c.GetKind() { | ||
| case config.ConstraintKind_CONSTRAINT_KIND_REQUIRED_TOGETHER: | ||
| if primaryPresent > 0 && primaryPresent < len(uniqueFieldNames) { | ||
| return fmt.Errorf("fields required together: %v", uniqueFieldNames) | ||
| } | ||
| case config.ConstraintKind_CONSTRAINT_KIND_MUTUALLY_EXCLUSIVE: | ||
| if primaryPresent > 1 { | ||
| return fmt.Errorf("fields are mutually exclusive: %v", uniqueFieldNames) | ||
| } | ||
| case config.ConstraintKind_CONSTRAINT_KIND_AT_LEAST_ONE: | ||
| if primaryPresent == 0 { | ||
| return fmt.Errorf("at least one required: %v", uniqueFieldNames) | ||
| } | ||
| case config.ConstraintKind_CONSTRAINT_KIND_DEPENDENT_ON: | ||
| if primaryPresent > 0 { | ||
| // Deduplicate secondary field names and check they are all present | ||
| uniqueSecondaryFieldNames := deduplicateStrings(c.GetSecondaryFieldNames()) | ||
| for _, name := range uniqueSecondaryFieldNames { | ||
| if !present[name] { | ||
| return fmt.Errorf("fields %v depend on %v which must also be present", uniqueFieldNames, uniqueSecondaryFieldNames) | ||
| } | ||
| } | ||
| } | ||
| case config.ConstraintKind_CONSTRAINT_KIND_UNSPECIFIED: | ||
| return nil | ||
| default: | ||
| return fmt.Errorf("unknown constraint kind: %v", c.GetKind()) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // deduplicateStrings returns a new slice with duplicate strings removed, preserving order. | ||
| func deduplicateStrings(input []string) []string { | ||
| seen := make(map[string]bool) | ||
| result := make([]string, 0, len(input)) | ||
| for _, s := range input { | ||
| if !seen[s] { | ||
| seen[s] = true | ||
| result = append(result, s) | ||
| } | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| func isNullValue(v *structpb.Value) bool { | ||
| _, isNull := v.GetKind().(*structpb.Value_NullValue) | ||
| return isNull | ||
| } |
There was a problem hiding this comment.
Harden constraint validation against nils/empty field lists to avoid panics and confusing errors.
- Line 564-566 / 572+:
constraint == nilwill panic. - Line 556-559 / 628-630:
value == nilwill panic inisNullValue. - Line 584-611: constraints with empty
FieldNamesprobably should error (or be ignored), especially forAT_LEAST_ONE.
Proposed fix
func validateActionConstraints(constraints []*config.Constraint, args *structpb.Struct) error {
if len(constraints) == 0 {
return nil
}
// Build map of present fields (non-null values in struct)
present := make(map[string]bool)
if args != nil {
for fieldName, value := range args.GetFields() {
- if !isNullValue(value) {
+ if value != nil && !isNullValue(value) {
present[fieldName] = true
}
}
}
// Validate each constraint
for _, constraint := range constraints {
+ if constraint == nil {
+ return errors.New("nil constraint")
+ }
if err := validateConstraint(constraint, present); err != nil {
return err
}
}
return nil
}
func validateConstraint(c *config.Constraint, present map[string]bool) error {
// Deduplicate field names to handle cases where the same field is listed multiple times
uniqueFieldNames := deduplicateStrings(c.GetFieldNames())
+ if len(uniqueFieldNames) == 0 {
+ return errors.New("constraint has no field_names")
+ }
// Count how many unique primary fields are present
var primaryPresent int
for _, name := range uniqueFieldNames {
if present[name] {
primaryPresent++
}
}
@@
func isNullValue(v *structpb.Value) bool {
+ if v == nil {
+ return true
+ }
_, isNull := v.GetKind().(*structpb.Value_NullValue)
return isNull
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // validateActionConstraints validates that the provided args satisfy the schema constraints. | |
| func validateActionConstraints(constraints []*config.Constraint, args *structpb.Struct) error { | |
| if len(constraints) == 0 { | |
| return nil | |
| } | |
| // Build map of present fields (non-null values in struct) | |
| present := make(map[string]bool) | |
| if args != nil { | |
| for fieldName, value := range args.GetFields() { | |
| if !isNullValue(value) { | |
| present[fieldName] = true | |
| } | |
| } | |
| } | |
| // Validate each constraint | |
| for _, constraint := range constraints { | |
| if err := validateConstraint(constraint, present); err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| } | |
| func validateConstraint(c *config.Constraint, present map[string]bool) error { | |
| // Deduplicate field names to handle cases where the same field is listed multiple times | |
| uniqueFieldNames := deduplicateStrings(c.GetFieldNames()) | |
| // Count how many unique primary fields are present | |
| var primaryPresent int | |
| for _, name := range uniqueFieldNames { | |
| if present[name] { | |
| primaryPresent++ | |
| } | |
| } | |
| switch c.GetKind() { | |
| case config.ConstraintKind_CONSTRAINT_KIND_REQUIRED_TOGETHER: | |
| if primaryPresent > 0 && primaryPresent < len(uniqueFieldNames) { | |
| return fmt.Errorf("fields required together: %v", uniqueFieldNames) | |
| } | |
| case config.ConstraintKind_CONSTRAINT_KIND_MUTUALLY_EXCLUSIVE: | |
| if primaryPresent > 1 { | |
| return fmt.Errorf("fields are mutually exclusive: %v", uniqueFieldNames) | |
| } | |
| case config.ConstraintKind_CONSTRAINT_KIND_AT_LEAST_ONE: | |
| if primaryPresent == 0 { | |
| return fmt.Errorf("at least one required: %v", uniqueFieldNames) | |
| } | |
| case config.ConstraintKind_CONSTRAINT_KIND_DEPENDENT_ON: | |
| if primaryPresent > 0 { | |
| // Deduplicate secondary field names and check they are all present | |
| uniqueSecondaryFieldNames := deduplicateStrings(c.GetSecondaryFieldNames()) | |
| for _, name := range uniqueSecondaryFieldNames { | |
| if !present[name] { | |
| return fmt.Errorf("fields %v depend on %v which must also be present", uniqueFieldNames, uniqueSecondaryFieldNames) | |
| } | |
| } | |
| } | |
| case config.ConstraintKind_CONSTRAINT_KIND_UNSPECIFIED: | |
| return nil | |
| default: | |
| return fmt.Errorf("unknown constraint kind: %v", c.GetKind()) | |
| } | |
| return nil | |
| } | |
| // deduplicateStrings returns a new slice with duplicate strings removed, preserving order. | |
| func deduplicateStrings(input []string) []string { | |
| seen := make(map[string]bool) | |
| result := make([]string, 0, len(input)) | |
| for _, s := range input { | |
| if !seen[s] { | |
| seen[s] = true | |
| result = append(result, s) | |
| } | |
| } | |
| return result | |
| } | |
| func isNullValue(v *structpb.Value) bool { | |
| _, isNull := v.GetKind().(*structpb.Value_NullValue) | |
| return isNull | |
| } | |
| // validateActionConstraints validates that the provided args satisfy the schema constraints. | |
| func validateActionConstraints(constraints []*config.Constraint, args *structpb.Struct) error { | |
| if len(constraints) == 0 { | |
| return nil | |
| } | |
| // Build map of present fields (non-null values in struct) | |
| present := make(map[string]bool) | |
| if args != nil { | |
| for fieldName, value := range args.GetFields() { | |
| if value != nil && !isNullValue(value) { | |
| present[fieldName] = true | |
| } | |
| } | |
| } | |
| // Validate each constraint | |
| for _, constraint := range constraints { | |
| if constraint == nil { | |
| return errors.New("nil constraint") | |
| } | |
| if err := validateConstraint(constraint, present); err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| } | |
| func validateConstraint(c *config.Constraint, present map[string]bool) error { | |
| // Deduplicate field names to handle cases where the same field is listed multiple times | |
| uniqueFieldNames := deduplicateStrings(c.GetFieldNames()) | |
| if len(uniqueFieldNames) == 0 { | |
| return errors.New("constraint has no field_names") | |
| } | |
| // Count how many unique primary fields are present | |
| var primaryPresent int | |
| for _, name := range uniqueFieldNames { | |
| if present[name] { | |
| primaryPresent++ | |
| } | |
| } | |
| switch c.GetKind() { | |
| case config.ConstraintKind_CONSTRAINT_KIND_REQUIRED_TOGETHER: | |
| if primaryPresent > 0 && primaryPresent < len(uniqueFieldNames) { | |
| return fmt.Errorf("fields required together: %v", uniqueFieldNames) | |
| } | |
| case config.ConstraintKind_CONSTRAINT_KIND_MUTUALLY_EXCLUSIVE: | |
| if primaryPresent > 1 { | |
| return fmt.Errorf("fields are mutually exclusive: %v", uniqueFieldNames) | |
| } | |
| case config.ConstraintKind_CONSTRAINT_KIND_AT_LEAST_ONE: | |
| if primaryPresent == 0 { | |
| return fmt.Errorf("at least one required: %v", uniqueFieldNames) | |
| } | |
| case config.ConstraintKind_CONSTRAINT_KIND_DEPENDENT_ON: | |
| if primaryPresent > 0 { | |
| // Deduplicate secondary field names and check they are all present | |
| uniqueSecondaryFieldNames := deduplicateStrings(c.GetSecondaryFieldNames()) | |
| for _, name := range uniqueSecondaryFieldNames { | |
| if !present[name] { | |
| return fmt.Errorf("fields %v depend on %v which must also be present", uniqueFieldNames, uniqueSecondaryFieldNames) | |
| } | |
| } | |
| } | |
| case config.ConstraintKind_CONSTRAINT_KIND_UNSPECIFIED: | |
| return nil | |
| default: | |
| return fmt.Errorf("unknown constraint kind: %v", c.GetKind()) | |
| } | |
| return nil | |
| } | |
| // deduplicateStrings returns a new slice with duplicate strings removed, preserving order. | |
| func deduplicateStrings(input []string) []string { | |
| seen := make(map[string]bool) | |
| result := make([]string, 0, len(input)) | |
| for _, s := range input { | |
| if !seen[s] { | |
| seen[s] = true | |
| result = append(result, s) | |
| } | |
| } | |
| return result | |
| } | |
| func isNullValue(v *structpb.Value) bool { | |
| if v == nil { | |
| return true | |
| } | |
| _, isNull := v.GetKind().(*structpb.Value_NullValue) | |
| return isNull | |
| } |
🤖 Prompt for AI Agents
In @pkg/actions/actions.go around lines 547 - 631, The constraint validation can
panic or produce confusing errors when nils or empty field lists appear: guard
against nil Constraint objects in validateActionConstraints/validateConstraint,
skip or return a clear error for constraints with empty FieldNames (especially
for AT_LEAST_ONE, REQUIRED_TOGETHER, MUTUALLY_EXCLUSIVE), and avoid
dereferencing nil structpb.Value in isNullValue by first checking for nil;
likewise make deduplicateStrings tolerate nil/empty slices. Concretely, in
validateActionConstraints check each constraint != nil before calling
validateConstraint, in validateConstraint return a descriptive error when
len(c.GetFieldNames()) == 0 (or treat empty as no-op) and handle nil secondary
lists, and change isNullValue to return true/false when v == nil without calling
v.GetKind().
phoebesimon
approved these changes
Jan 13, 2026
…sent. Error if we can't find a schema for the action. Close channels so we don't leak goroutines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/actions/actions.go (1)
573-614: Consider adding nil check for constraint parameter.The function relies on protobuf getter nil-safety to handle nil constraints (falling through to
CONSTRAINT_KIND_UNSPECIFIED). While this works, an explicit nil check would make the intent clearer and be more defensive.Suggested improvement
func validateConstraint(c *config.Constraint, present map[string]bool) error { + if c == nil { + return nil + } // Deduplicate field names to handle cases where the same field is listed multiple times uniqueFieldNames := deduplicateStrings(c.GetFieldNames())
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/actions/actions.gopkg/actions/actions_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-02T17:21:01.723Z
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:01.723Z
Learning: In tests that verify cleanup behavior (e.g., ensuring temporary artifacts are removed or directories are empty after an operation), treat cleanup failures as test failures by asserting on the cleanup call (e.g., require.NoError(t, err) or assert.NoError(t, err)). This ensures that the cleanup path is explicitly tested and any cleanup error fails the test, confirming correct behavior of the cleanup code.
Applied to files:
pkg/actions/actions_test.go
📚 Learning: 2025-10-24T20:32:19.985Z
Learnt from: kans
Repo: ConductorOne/baton-sdk PR: 531
File: pkg/session/json_session.go:38-42
Timestamp: 2025-10-24T20:32:19.985Z
Learning: In the baton-sdk repository, SetManyJSON and similar batch operations in pkg/session/json_session.go should not add early-return guards for empty inputs. The design preference is to let server-side validation errors (such as min_pairs:1) propagate rather than silently succeeding on the client side.
Applied to files:
pkg/actions/actions.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: go-test (1.25.2, ubuntu-latest)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (8)
pkg/actions/actions.go (7)
422-435: LGTM! Schema retrieval and constraint validation before action execution.Good addition of upfront validation. The use of
codes.Internalfor missing schema is appropriate since handler and schema are registered together, so a missing schema indicates an internal inconsistency.
445-445: Good fix: closing channel prevents goroutine leak.Adding
defer close(done)ensures the select statement doesn't block indefinitely if the handler completes after the 1-second timeout, preventing goroutine leaks.
499-516: LGTM! Consistent schema validation for resource actions.The validation flow mirrors the global action pattern. Error codes are appropriate:
Internalfor missing schemas indicates registration inconsistency.
523-523: Good fix: closing channel prevents goroutine leak.Same fix as in
invokeGlobalAction- ensures proper cleanup.
548-571: LGTM! Clean implementation of constraint validation.The logic correctly:
- Short-circuits on empty constraints
- Treats nil and null values as not present
- Iterates through all constraints to validate
616-627: LGTM!Clean deduplication helper with order preservation.
629-635: LGTM!Correctly identifies both nil pointers and explicit null values as "null".
pkg/actions/actions_test.go (1)
201-486: Comprehensive test coverage for constraint validation.Good coverage including edge cases:
- Nil constraints and nil structpb.Value
- All four constraint kinds with pass/fail scenarios
- Null value behavior
- Field name deduplication
One minor gap: the
defaultbranch invalidateConstraint(unknown constraint kind) is not tested. Consider adding a test case if the protobuf allows unknown enum values to reach the switch.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From #626
Summary by CodeRabbit
Enhancement
Tests
✏️ Tip: You can customize this high-level summary in your review settings.