Working on the loop action - #60

Code Review Overview

Overview

This PR adds a new loop action plus the request-context/template plumbing needed to expose loop_item and loop_index during iteration. The feature is well-scoped and the tests cover the main success, edge, and error paths, including nested loop visibility and normal unwinding on returned errors.

Risk: moderate. The change is mostly additive, but it does extend request-scoped execution state and template behavior, so correctness of loop-state cleanup matters for reliability in more complex flows.

One correctness concern was called out inline around loop-frame cleanup on panic; aside from that, the implementation looks cohesive and the test coverage is solid for the intended behavior.

@Servflow, also check how this can be improved

Thanks for your feedback! Here are a few ideas for possible improvements to the current implementation:

1. Error Handling Granularity:

   • In the Execute method, you might consider whether you want to support a "tolerate errors" mode (i.e., continue on error by collecting per-iteration errors) for certain use cases, or if stopping immediately is always the right choice for ServFlow concepts.

2. Performance: Pre-resolving the Body Chain Plan:

   • If the action chain defined by Start is known to stay the same for all iterations, consider resolving it once outside the loop instead of in each iteration, unless plan state is intended to be context-local per run. This is a minor optimization, but if the planning overhead ever grows, it could help.

3. Template Function Clarity:

   • The template docs in the registration (init function) and in the code are clear, but consider referencing the field-access pattern (e.g., {{ loop_item "id" }}) with a brief code snippet in user-facing documentation, since it's a little different from common Go or JS templating metaphors.

4. Tiny Code Style/Idioms:

   • In PopLoop, you could technically check if n := len(rc.loopStack); n > 0 { rc.loopStack = rc.loopStack[:n-1] } to avoid two traversals of .len and .slice. Minor, but for style consistency.

5. Concurrency Guards:

   • The RequestContext locking seems sufficient (uses Lock()/Unlock()), but if the context is ever shared in concurrent chains (not just nested), consider documenting thread-safety expectations more explicitly for others reading the code.

6. Testing Nested Error-After-Nest:

   • Tests already check loop frame restoration on error, which is great. Consider adding a test with more than one nested loop where an inner loop errors, to be absolutely sure loop stack unwinding is robust at multiple levels.

7. Generalization for Future Extensibility:

   • If you anticipate similar "foreach"/"map"/"reduce" primitives, you might extract the loop stack management to a more reusable/context-agnostic helper, but for now the direct approach is clear and understandable.

Overall, the core logic and integration look robust, and these are just suggestions for incremental improvement or future consideration. If you have any specific areas you'd like more feedback on (API surface, template ergonomics, error model), just let me know!

@Servflow are you here

Yes — I’m here.

The main follow-up from my review is still the inline correctness point: in loop.Execute, rc.PopLoop() should ideally be guaranteed with a defer inside a per-iteration closure so loop state is cleaned up even if plan.ExecuteFromContext(...) panics.

Other than that, the implementation looks solid and the test coverage is good. If you want, I can do one more focused pass on:

• panic-safety / unwinding
• API ergonomics of loop_item / loop_index
• possible enhancements like continue-on-error or result collection

PR Overview

Summary

This PR adds a new loop action in pkg/engine/actions/executables/loop that iterates over a runtime-resolved JSON array and runs a body action chain once per element. It also adds request-context support for loop-scoped template functions, and wires the new action into server startup.

Architectural changes

• Introduces a new executable action package: pkg/engine/actions/executables/loop/loop.go.
• Extends pkg/engine/requestctx/aggregationcontext.go with a per-request loopStack and PushLoop/PopLoop helpers for nested iteration state.
• Adds new template functions in pkg/engine/requestctx/templates.go: loop_item and loop_index.
• Registers the new action by importing pkg/engine/actions/executables/loop in pkg/engine/server/engine.go.
• Adds tests for the new loop action and loop template behavior in:
  • pkg/engine/actions/executables/loop/loop_test.go
  • pkg/engine/requestctx/loop_funcs_test.go

Functional / behavioral changes

• New loop action configuration:
  • items: template string that must resolve to a JSON array
  • start: action id for the loop body chain
• loop executes sequentially, one iteration per array element, and returns a count field with the number of items processed.
• The current element and zero-based index become available to templates via {{ loop_item }}, {{ loop_item "field" }}, and {{ loop_index }}.
• Empty or whitespace-only resolved items is treated as a no-op; invalid JSON or non-array JSON is rejected.

Worth a careful look

• pkg/engine/actions/executables/loop/loop.go: the iteration flow around PushLoop, plan.ExecuteFromContext, and PopLoop, especially error handling and how the loop state is restored.
• pkg/engine/requestctx/templates.go and aggregationcontext.go: the new template functions and loop stack semantics, including nested loop behavior and outside-loop defaults.
• pkg/engine/actions/executables/loop/loop_test.go: the test coverage around empty input, object field access, and cleanup after body errors.