RUN-38777 - added suspendDefinition and supporting statuses#56
Open
RUN-38777 - added suspendDefinition and supporting statuses#56
Conversation
TomB30
requested review from
AviadHayumi,
Isan-Rivkin,
rogirun,
ronlv10,
shaked-bouktus and
yuval-gr
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a suspend/resume lifecycle: three new ResourceStatus values (Resuming, Suspending, Suspended), a Component-level SuspendDefinition with ordered JQ suspend/resume action lists, CRD/schema and deepcopy updates, an action-expression JQ validator, and wiring/tests to apply and validate the actions. ChangesSuspend/Resume Lifecycle Feature
Sequence DiagramsequenceDiagram
autonumber
participant Comp as Component
participant Acc as Accessor
participant JQ as JQ Runner
participant Obj as Resource Object
rect rgba(135,206,235,0.5)
Comp->>Acc: Suspend(ctx)
Acc->>JQ: Mutate(ctx, suspendActions[0])
JQ->>Obj: apply mutation (in-place)
JQ-->>Acc: success / error
alt more actions
loop for each remaining suspendActions
Acc->>JQ: Mutate(ctx, suspendActions[i])
JQ->>Obj: apply mutation
JQ-->>Acc: success / error
end
end
Acc-->>Comp: success / DefinitionNotFoundError treated as nil or propagate error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/api/runai/v1alpha1/structure.go`:
- Around line 367-374: The Entries() comment promises "first match wins" but the
matching code still appends every match to MatchedStatuses; update the matching
loop so it short-circuits on the first successful StatusMatchEntry match instead
of accumulating all matches. Locate the code that iterates
StatusMappings.Entries() and appends into MatchedStatuses, change it to
break/return immediately after the first entry whose matcher matches (using the
StatusMatchEntry matcher/fields), ensuring suspend-related entries (e.g.,
ResumingStatus, SuspendingStatus, SuspendedStatus) take precedence as intended.
- Around line 65-77: Add a minimum-item constraint and non-empty-entry
validation for SuspendActions and ResumeActions: annotate both SuspendActions
and ResumeActions with +kubebuilder:validation:MinItems=1 to prevent empty
arrays, and update the resource validation methods (e.g., ValidateCreate and
ValidateUpdate for the type declared in this file) to iterate over
SuspendActions and ResumeActions and return an error if any entry is the empty
string (""), rejecting blank entries in the validator.
In `@pkg/jq/validation.go`:
- Around line 220-230: ValidateActionExpression currently accepts plain
selectors; update validation to require at least one assignment/update operator
in the parsed JQ AST before accepting an action. Modify validateActionParsedJQ
(or add a helper it calls) to traverse the gojq parsed AST for assignment/update
nodes (e.g., operators such as =, |=, +=, -=, //=, etc.) and return an error
when none are found; keep the existing parse error handling in
ValidateActionExpression and call the new check after gojq.Parse returns the
parsed AST.
- Around line 235-270: The validator currently only recurses into Func.Args and
binary Op Left/Right inside validateActionParsedJQ; extend it to walk all Term
fields that can contain nested *Query to close AST traversal gaps: when q.Term
!= nil, after handling Func and TermType, recursively call
validateActionParsedJQ on Term.Array elements, Term.Object field values, Term.If
(Cond, Then, Else), Term.Try body, Term.Reduce subqueries (init/accum/body as
present), Term.Foreach subqueries (init/update/result as present), Term.Query
nested Query fields, and Term.SuffixList entries; also ensure Object.Fields and
SuffixList slices are iterated and each contained *Query passed to
validateActionParsedJQ so constructs like If, Array, Object, Try, Reduce,
Foreach, Query and SuffixList cannot bypass the blacklist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 20b0f8bb-1dc5-4eda-9955-953c17eb03e3
📒 Files selected for processing (8)
charts/karta/crds/run.ai_kartas.yamlpkg/api/runai/v1alpha1/structure.gopkg/api/runai/v1alpha1/validation.gopkg/api/runai/v1alpha1/validation_test.gopkg/api/runai/v1alpha1/zz_generated.deepcopy.gopkg/jq/validation.gopkg/jq/validation_test.gopkg/resource/accessor_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: TomB30 <82462817+TomB30@users.noreply.github.com>
Extend validateActionParsedJQ to recursively walk every *Query-bearing
field inside gojq Term nodes: Array.Query, Object KeyQuery/Val,
If/Elif/Else branches, Try Body/Catch, Reduce and Foreach sub-expressions,
parenthesized Term.Query, and both Term.Index and SuffixList index
Start/End bounds.
Previously, constructs like [del(.x)], {a: del(.x)}, (recurse),
try del(.x), reduce/foreach bodies, and index expressions such as
.[range(5)] could bypass the blacklist entirely.
Add 25 new test entries that prove each bypass path is now caught.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/jq/validation.go (1)
376-388: 💤 Low valueAdd explicit traversal of
Term.Bind.BodyandTerm.Label.Bodyfor complete AST coverage.While variable bindings (
. as $x | …) and labels (label $out | …) typically involve pipes at the top level and are thus covered by binary operator traversal, explicit traversal of these Term fields would close a minor gap and improve defense in depth. gojq v0.12.18 defines these fields with Body sub-queries that should be validated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/jq/validation.go` around lines 376 - 388, The validateActionParsedJQ function currently validates binary operator children but misses explicit traversal of Term.Bind.Body and Term.Label.Body; update validateActionParsedJQ to detect when q.Term is a Bind or Label and iterate over each sub-query in Term.Bind.Body and Term.Label.Body respectively, calling validateActionParsedJQ on each element so those nested bodies are validated as well (referencing q.Term, Term.Bind.Body, Term.Label.Body and the validateActionParsedJQ function).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/jq/validation_test.go`:
- Around line 571-572: Add a new test in pkg/jq/validation_test.go to cover
string interpolation bypass after fixing Term.Str traversal: add a DescribeTable
that calls ValidateActionExpression for expressions like `"value: \(del(.x))"`
and `"count: \(range(5))"`, asserting an error occurs and the message contains
the expected substring (e.g., "del function is not allowed" and "function
'range'"), so that ValidateActionExpression rejects dangerous functions embedded
in interpolated strings; reference ValidateActionExpression and Term.Str in case
you need to ensure the validator visits Term.Str nodes.
In `@pkg/jq/validation.go`:
- Around line 245-375: The validator is missing traversal of string
interpolation expressions (q.Term.Str.Queries), so add validation for each
interpolated query by iterating over q.Term.Str.Queries and calling
validateActionParsedJQ on each; place this block after the Term.Index handling
(i.e., after checking q.Term.Index.Start/End) inside the if q.Term != nil
branch, and ensure any returned error is propagated just like other checks in
validateActionParsedJQ.
---
Nitpick comments:
In `@pkg/jq/validation.go`:
- Around line 376-388: The validateActionParsedJQ function currently validates
binary operator children but misses explicit traversal of Term.Bind.Body and
Term.Label.Body; update validateActionParsedJQ to detect when q.Term is a Bind
or Label and iterate over each sub-query in Term.Bind.Body and Term.Label.Body
respectively, calling validateActionParsedJQ on each element so those nested
bodies are validated as well (referencing q.Term, Term.Bind.Body,
Term.Label.Body and the validateActionParsedJQ function).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5e80d480-5293-4303-ba4f-9770ced1f552
📒 Files selected for processing (2)
pkg/jq/validation.gopkg/jq/validation_test.go
Layer 1 — pkg/jq/execution: - Add Mutator interface with Mutate(ctx, expr) for self-contained mutation expressions (e.g. ".spec.suspend = true"); Runner now embeds Mutator - Implement Mutate on runner via assignWithExpression with no variables - Update MockRunner/add MockMutator; add missing Ginkgo suite bootstrap - Add tests for Mutate; fix pre-existing Assign error-type assertion Layer 2 — pkg/resource/accessor: - Add ApplySuspendActions and ApplyResumeActions; each chains Mutate calls over SuspendDefinition.SuspendActions / ResumeActions in order Layer 3 — pkg/resource/component_factory: - Extend ComponentWriter interface and regenerate mocks for both new methods Layer 4 — pkg/resource/component: - Add HasSuspendDefinition, Suspend, Resume; no-op on DefinitionNotFoundError - Tests cover happy path, no-definition no-op, and error propagation Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/jq/execution/runner.go`:
- Around line 90-92: Mutate currently forwards any expression to
assignWithExpression which accepts multiple emitted results; change Mutate to
enforce single-result semantics by validating the expression emits exactly one
value and returning an error if it emits zero or more than one. Locate
runner.Mutate and either call a new helper (e.g.,
assignSingleResultWithExpression) or add a flag/branch in assignWithExpression
to collect all emitted values for the given expression, ensure len(results) ==
1, and only then perform the update; return a clear error from Mutate when
result count != 1. Use the existing symbols runner.Mutate and
assignWithExpression to find where to add this validation.
In `@pkg/resource/accessor.go`:
- Line 359: The function declaration for ExtractInstanceIds is malformed because
the opening brace and the if statement are on the same line; update the
declaration so the opening brace is followed by a newline and the if block
starts on the next line (i.e., change "func (a *Accessor) ExtractInstanceIds(...
) { if definition.InstanceIdPath == nil {" to separate the brace and the if),
ensuring the code around ExtractInstanceIds and the reference to
definition.InstanceIdPath is properly formatted and passes gofmt.
In `@pkg/resource/component.go`:
- Line 340: The declaration of HasPodDefinition has a gofmt violation because
the opening brace and the first statement are on the same line; update the
function signature and body formatting in the HasPodDefinition method so the
opening brace is followed by a newline and the first statement is on its own
indented line (e.g., change "func (c *Component) HasPodDefinition() bool { spec
:= c.definition.SpecDefinition" to put "spec := c.definition.SpecDefinition" on
the next line with proper indentation), ensuring the function body conforms to
standard Go formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a6f8f97e-9fda-49fb-8bcb-028f3a2aadfc
📒 Files selected for processing (11)
pkg/jq/execution/interface.gopkg/jq/execution/runner.gopkg/jq/execution/runner_mock.gopkg/jq/execution/runner_test.gopkg/jq/execution/suite_test.gopkg/resource/accessor.gopkg/resource/accessor_mock.gopkg/resource/accessor_test.gopkg/resource/component.gopkg/resource/component_factory.gopkg/resource/component_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/resource/accessor_mock.go
Dangerous functions embedded in interpolated strings like "\(del(.x))" bypassed validateActionParsedJQ because Term.Str.Queries was never visited. Add a loop over Str.Queries so each interpolated segment is recursed into, and add two test entries confirming del and range are now caught. Co-authored-by: Cursor <cursoragent@cursor.com>
rogirun
reviewed
May 5, 2026
rogirun
reviewed
May 5, 2026
rogirun
reviewed
May 5, 2026
rogirun
reviewed
May 5, 2026
rogirun
reviewed
May 5, 2026
rogirun
reviewed
May 5, 2026
Isan-Rivkin
reviewed
May 5, 2026
Isan-Rivkin
left a comment
Isan-Rivkin
left a comment
There was a problem hiding this comment.
Some comments, still reviewing 🙏
yuval-gr
reviewed
May 5, 2026
added 4 commits
May 6, 2026 09:09
Isan-Rivkin
reviewed
May 6, 2026
| return r.assignWithExpression(ctx, updateExpression, []string{"$val"}, []any{values}) | ||
| } | ||
|
|
||
| func (r *runner) Mutate(ctx context.Context, expression string) error { |
There was a problem hiding this comment.
Adding here for visibility we talked f2f: why Mutate is needed VS using Assign
Author
There was a problem hiding this comment.
Basically, this is exactly what we discussed f2f - it simplifies the use of suspend for users who consume the expression directly and want to execute it.
In my opinion, we should keep it. If you’d prefer to limit the operators we support for the time being, I’m fine with that. If we had opted for the object-based approach with path and value, the assign might have been more convenient since it provides clear separation and spares the user from having to parse it. However, with the current structure, I think it’s more practical to inject the suspendAction directly into the mutate.
I agree that this is relatively "open," but it’s the user's responsibility. Besides, we already have our own safeguards in place against actions that could potentially crash the system.
rogirun
requested changes
May 6, 2026
Isan-Rivkin
requested changes
May 6, 2026
…ume actions - Remove Mutator interface and Mutate method from Runner; suspend/resume actions now use Assign after parsing the = expression - Add parseAssignAction + findAssignOpIndex helpers in accessor.go to split a "PATH = VALUE" string into path and Go value via gojq - Tighten ValidateActionExpression to only accept the = operator; all other update operators (|=, +=, etc.) are now rejected at validation time - Simplify ValidateParsedJQ: drop allowMutation bool parameter, always enforce read-only rules - Remove ValidateParsedJQ allowMutation=true tests; update ValidateActionExpression tests to reflect = -only contract - Remove condition-mutation examples from integration tests (not realistic) - Make validateParsedJQ public per reviewer request; add FuncDefs traversal to prevent blacklisted operations being hidden inside def bodies Co-authored-by: Cursor <cursoragent@cursor.com>
Isan-Rivkin
requested changes
May 7, 2026
Isan-Rivkin
left a comment
Isan-Rivkin
left a comment
There was a problem hiding this comment.
Hey! asking "changing" purely for notification reason, I want to make sure I don't block you and make sure I review this PR when it's ready so please ask for "re-review" I'll get notified when it's ready for review since it's seems to be still WIP 🙏
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
What does this PR do?
Related issue(s)
Fixes #
Checklist
git commit -s)make check)Summary by CodeRabbit
New Features
Validation
Tests