feat: add for step type for basic loop support (#21)

[tbrandenburg]

Summary

Workflows that need to iterate over a list of items had to inline a bash for/while loop in a single bash step, losing per-step logging, dry-run support, and failure catching. This adds a first-class for step type that wraps child steps and iterates them over a newline-delimited variable.

Root Cause

No for step type existed in the step model; users had no structured way to iterate within a workflow.

Changes

File	Change
src/flowsh_cli/models.py	Add ForStep model with in_, item, steps fields; update Step union; call ForStep.model_rebuild()
src/flowsh_cli/render.py	Import ForStep; extract _render_step_body helper; add ForStep branch generating inner functions + while-read loop; add _for_inner_function_name for unique naming; update default_step_title
tests/test_workflow_to_harness.py	Add 11 tests (parse accept/reject, render structure, E2E execution, dry-run); fix pre-existing env isolation issue in required-param test

Testing

• Lint passes
• Type check passes
• All 91 unit/integration tests pass
• E2E: harness generates, executes, iterates over items correctly
• Dry-run: inner steps skipped with [DRY-RUN] in stderr

Validation

make qa

Issue

Closes #21

[tbrandenburg]

Automated Code Review

Summary

The implementation correctly follows the investigation plan: ForStep is added to the model, render.py is refactored to extract _render_step_body, and 11 tests cover parse, render, E2E, and dry-run. A pre-existing test isolation bug (env var leakage) was also fixed as a bonus.

Findings

Strengths

• _render_step_body extraction is clean — all three existing step types render identically to before, no regression risk
• ForStep correctly uses run_stateful_step so the loop body runs in the main shell and export ITEM is visible downstream
• _for_inner_function_name mirrors the existing step_function_name deduplication pattern exactly
• Nested for explicitly raises AssertionError (guarded) rather than silently producing broken output
• field_validator on both in_ and item enforces the [A-Z_][A-Z0-9_]* constraint at parse time

Suggestions (non-blocking)

• render.py now has 3 private helpers (_render_step_body, _for_inner_function_name, _render_arg_block). A future refactor could move them into a dataclass/renderer object, but not needed now.
• The done <<< "${ITEMS}" line uses brace form — consistent with the rest of the bash idioms in the file.

Security

• No security concerns: loop variable is exported via export ITEM, not interpolated into a shell eval. The heredoc delimiter collision logic already in use applies to inner steps too.

Checklist

• Feature addresses the proposal from investigation
• Code follows codebase patterns (discriminated union, field validators, run_stateful_step)
• Tests cover: parse accept, parse reject (bad name, empty steps), render structure, E2E execution, dry-run
• No regressions: 91/91 tests pass

Self-reviewed by Claude • Ready for human review

[tbrandenburg]

PR #26 Review: feat: add for step type for basic loop support (#21)

Reviewer: Human review (assisted by OpenCode)
Date: 2026-06-03
Branch: feat/issue-21-for-step → main
Recommendation: ✅ APPROVE (with non-blocking suggestions)

---

Summary

PR #26 adds a first-class for step type that iterates inner steps over a newline-delimited
variable, addressing the gap where users had to inline bash for/while loops in a single
bash step — losing per-step logging, dry-run support, and failure catching.

The implementation is clean, well-tested, and follows all existing patterns. All automated
checks pass: 91/91 tests, lint, type check, and build.

---

Validation Results

Check	Result	Detail
Lint (ruff check)	✅ PASS	All checks passed
Format (ruff format)	✅ PASS	9 files already formatted
Syntax check (py_compile)	✅ PASS
Tests (pytest)	✅ PASS	91/91 passed in 19.13s
Build (uv build)	✅ PASS	flowsh_cli-0.5.0 wheel + sdist

---

Issues Found

Severity	Count
Critical	0
High	0
Medium	2
Suggestions	2

---

Detailed Findings

Medium: Inner VarsStep inside for will not export variables to outer scope

File: src/flowsh_cli/render.py — render_step() for-branch
Lines: The inner function call uses run_step for all inner step types

*[f"    run_step {fn}" for fn in inner_fns],

All inner steps are dispatched with run_step, which runs the function in a subshell.
VarsStep normally uses run_stateful_step (no subshell) so its export statements
propagate to the current shell. Inside a for loop, a vars inner step would run in
a subshell — its exports would not propagate back to the outer loop or subsequent steps.

Impact: Any user who puts a vars step inside a for loop expecting cross-iteration
or post-loop variable state will get silent failures — the vars will appear set inside the
inner function body but the outer shell won't see them.

Recommendation: Either:

1. Use run_stateful_step for inner VarsStep calls (mirror how top-level vars work), or
2. Document this limitation explicitly in AGENTS.md / user-facing docs.

Given the PR description states "flat, non-nested iteration", option 2 may be intentional.
If so, add a parse-time guard or at minimum a code comment explaining why run_step is
used uniformly for inner steps.

---

Medium: Empty ITEMS variable runs one empty iteration

File: src/flowsh_cli/render.py
Line: f' done <<< "${{{step.in_}}}"'

In bash, while IFS= read -r ITEM; done <<< "" processes exactly one iteration with
ITEM="" rather than zero iterations. If a vars step sets ITEMS to an empty string
(e.g. when a find or grep returns nothing), the loop body still executes once with
an empty value — which is almost certainly unintended behavior for the user.

Recommendation: Guard against the empty-string case in the generated loop:

while IFS= read -r ITEM && [[ -n "$ITEM" ]]; do
  export ITEM
  ...
done <<< "${ITEMS}"

Or add a preamble check:

[[ -z "${ITEMS}" ]] && { log INFO "ITEMS is empty, skipping loop"; return 0; }

No test currently covers this edge case.

---

Suggestion: _for_inner_function_name deduplication uses same used_function_names set

File: src/flowsh_cli/render.py — _for_inner_function_name()

Inner functions are added to the shared used_function_names set, so they won't collide
with outer step function names. This is correct. However, the naming scheme for_{outer}_{slug}
is not mirrored in step_function_name — if an outer step happened to be named
for_1_process, it would collide. This is an unlikely edge case, but if it ever bites, the
deduplication suffix (_2, _3) would silently rename the inner function. Low risk in
practice, just worth a note.

---

Suggestion: Missing test for empty ITEMS edge case

File: tests/test_workflow_to_harness.py

There is no test covering what happens when the variable iterated over (in: ITEMS) is
an empty string at runtime. Given the bash behavior described above (one empty iteration),
this should be tested to lock down the expected behavior — whether it is "skip loop body"
or "run once with empty ITEM".

---

Strengths

• _render_step_body extraction is excellent. The refactor cleanly separates "what does
  a step body look like" from "how is a step wrapped and dispatched". Existing step types
  render identically to before — zero regression risk. The helper is small, single-purpose,
  and has a clear docstring.

• ForStep.model_rebuild() is correctly placed after the Step union redefinition. This
  is the idiomatic Pydantic v2 pattern for forward-reference cycles and is easy to miss.

• validation_alias="in" cleanly handles the Python keyword conflict without any runtime
  workaround or string manipulation at the call site.

• run_stateful_step for the outer for function is the right call — the loop sets
  export ITEM which must be visible in the main shell for downstream steps.

• Nested for raises AssertionError with a clear message rather than silently producing
  broken shell. The guard is in _render_step_body which is the right place since
  render_step handles the outer for case before calling the helper.

• field_validator on in_ and item enforces the same [A-Z_][A-Z0-9_]* constraint
  used throughout the model layer, keeping variable name validation consistent.

• Test coverage is thorough: 11 new tests cover parse-accept, parse-reject (bad name,
  empty steps), render structure (assert generated bash constructs), E2E execution (actual
  bash runs and produces expected output), and dry-run (inner steps skipped, [DRY-RUN]
  in stderr).

• Pre-existing test isolation fix (env var leakage for ISSUE_NUMBER) is a good
  incidental fix — isolated, minimal, and correct.

• AGENTS.md updated to reflect the new supported step type. This keeps the project
  constitution accurate.

---

Conclusion

The implementation is correct, complete, and follows all existing patterns. The two medium
issues represent real edge cases (inner vars not propagating, empty-variable loop behavior)
but neither is a correctness regression — they are newly introduced behaviors with the new
step type. The empty-ITEMS case is the more actionable one; adding a guard or a test to
document the expected behavior is recommended before a future release if for steps see
widespread use.

No high or critical issues. The PR is ready to merge.