Skip to content

feat: support named positional parameters in workflow harness (#19) - #20

Merged
tbrandenburg merged 4 commits into
mainfrom
fix/issue-19-named-positional-params
Jun 2, 2026
Merged

feat: support named positional parameters in workflow harness (#19)#20
tbrandenburg merged 4 commits into
mainfrom
fix/issue-19-named-positional-params

Conversation

@tbrandenburg

Copy link
Copy Markdown
Owner

Summary

The generated harness only accepted --dry-run and rejected everything else with exit 2. Runtime values had to be passed via environment variables (ISSUE_NUMBER=42 bash .harness/foo.sh), making harnesses feel like internal implementation details rather than first-class CLI tools.

Root Cause

The Workflow Pydantic model had no params field, and the harness generator hard-coded a simple if/elif block that only recognised --dry-run.

Changes

File Change
src/flowsh_cli/models.py Added WorkflowParam model (name, description, required) + params: list[WorkflowParam] = [] field on Workflow
src/flowsh_cli/render.py Replaced hard-coded 8-line arg block with _render_arg_block() helper; emits while-loop positional parser when params declared
tests/test_workflow_to_harness.py Added 10 tests: schema validation, positional binding, env-var override, required/optional enforcement, --dry-run interop

Testing

  • Lint passes
  • Type/compile check passes
  • All 85 unit tests pass
  • Existing harnesses (no params key) produce byte-for-byte identical output (zero regression)

Validation

make qa

Issue

Fixes #19


Implementation Details

Implementation followed artifact:

.claude/PRPs/issues/issue-19.md

Deviations from plan:

None


Automated implementation from investigation artifact

Tom Brandenburg added 2 commits June 2, 2026 13:25
Add WorkflowParam model and params field to Workflow so harnesses can
accept positional CLI arguments instead of requiring env-var injection.

Changes:
- Add WorkflowParam Pydantic model (name, description, required) with
  uppercase env-var name validator to models.py
- Add params: list[WorkflowParam] = [] field to Workflow (backward-compatible)
- Replace hard-coded 8-line arg block in render_harness() with
  _render_arg_block() helper that emits a while-loop parser when params
  are declared, falling back to the original block when params is empty
- Add 10 tests covering schema validation, positional binding, env-var
  override, required/optional enforcement, and --dry-run interop

Fixes #19
@tbrandenburg

Copy link
Copy Markdown
Owner Author

Self Code Review

Summary

The implementation correctly addresses the root cause: WorkflowParam model added to schema, _render_arg_block() helper replaces the hard-coded arg block, and 10 focused tests validate all specified behaviours.

Findings

Strengths

  • Zero regression: _render_arg_block([]) returns identical lines to the original hard-coded block, so existing harnesses are unchanged.
  • Env-var override works naturally: positional binding only writes when POSITIONAL_ARGS index exists; required check uses ${VAR:-} so a pre-set env var satisfies it without a positional arg.
  • WorkflowParam.name validator mirrors the existing VarsStep variable name pattern — consistent convention.
  • Tests are end-to-end (actually exec bash), not mocked — high signal.

Suggestions (non-blocking)

  • src/flowsh_cli/render.py:_render_arg_block — extra positional args beyond declared params are silently ignored. Acceptable per spec, but could be documented in a comment if desired.
  • WorkflowParam.name doesn't have an explicit denylist for clashes with internal vars (DRY_RUN, POSITIONAL_ARGS, etc.). Low risk since DRY_RUN contains lowercase chars blocked by the regex, but worth a note.

Security

No security concerns identified. Param names are validated to [A-Z_][A-Z0-9_]* before shell emission; no user-supplied strings reach unquoted shell positions.

Checklist

  • Fix addresses root cause from investigation
  • Code follows codebase patterns (StrictModel, field_validator, bash_quote style)
  • Tests cover the change (10 new tests, all passing)
  • No obvious bugs introduced

Self-reviewed by Claude • Ready for human review

@tbrandenburg
tbrandenburg merged commit a4e742a into main Jun 2, 2026
1 check passed
@tbrandenburg
tbrandenburg deleted the fix/issue-19-named-positional-params branch June 2, 2026 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: support named positional parameters in workflow harness

1 participant