|
| 1 | +# Investigation: feat: support optional workflow metadata parameters (enabled, schedule, shellScriptPath) |
| 2 | + |
| 3 | +**Issue**: #37 (https://github.com/tbrandenburg/flowsh/issues/37) |
| 4 | +**Type**: ENHANCEMENT |
| 5 | +**Investigated**: 2026-06-22T20:30:00Z |
| 6 | + |
| 7 | +### Assessment |
| 8 | + |
| 9 | +| Metric | Value | Reasoning | |
| 10 | +| ---------- | ------ | ------------------------------------------------------------------------------------------------------ | |
| 11 | +| Priority | MEDIUM | Enables external tooling annotation (schedulers, CI runners) but does not block current workflow use | |
| 12 | +| Complexity | LOW | Single model class change (3 optional fields), 1 existing test inverted, 3 new tests added | |
| 13 | +| Confidence | HIGH | Root cause fully identified: `StrictModel(extra="forbid")` at `models.py:44` rejects unknown fields | |
| 14 | + |
| 15 | +## Problem Statement |
| 16 | + |
| 17 | +External tooling (schedulers, CI runners, orchestrators) needs to annotate workflows with operational metadata (`enabled`, `schedule`, `shellScriptPath`) co-located in the workflow YAML. Currently `Workflow` inherits `StrictModel` which sets `extra="forbid"`, so any YAML containing these fields raises a `WorkflowParseError` at parse time. These three fields must be accepted by the schema and parser but produce **no change** in generated shell scripts. |
| 18 | + |
| 19 | +## Analysis |
| 20 | + |
| 21 | +### Change Rationale |
| 22 | + |
| 23 | +Adding three optional fields to `Workflow` at `models.py:199-203` will: |
| 24 | + |
| 25 | +1. Allow the fields in YAML without validation errors (Pydantic simply populates them) |
| 26 | +2. Auto-expose them in `--schema` output via `WorkflowFile.model_json_schema()` — no CLI change needed |
| 27 | +3. Leave the generated shell script **identical** — `render_harness()` only touches `workflow.id`, `workflow.name`, `workflow.params`, and `workflow.steps`; the new fields are never read by the renderer |
| 28 | + |
| 29 | +### Evidence Chain |
| 30 | + |
| 31 | +WHY: YAML with `enabled`, `schedule`, or `shellScriptPath` fields raises `WorkflowParseError` |
| 32 | +↓ BECAUSE: `parse_workflows()` calls `WorkflowFile.model_validate(data)`, which propagates Pydantic's `ValidationError` |
| 33 | +Evidence: `models.py:261-263` — `return WorkflowFile.model_validate(data).workflows` inside `try/except ValidationError as error: raise WorkflowParseError(...)` |
| 34 | + |
| 35 | +↓ BECAUSE: Pydantic raises a `ValidationError` for extra fields on any model inheriting `StrictModel` |
| 36 | +Evidence: `models.py:43-44` — `class StrictModel(BaseModel): model_config = ConfigDict(extra="forbid", strict=True)` |
| 37 | + |
| 38 | +↓ BECAUSE: `Workflow` inherits `StrictModel` and declares none of the three fields |
| 39 | +Evidence: `models.py:199-203` — |
| 40 | +```python |
| 41 | +class Workflow(StrictModel): |
| 42 | + id: str |
| 43 | + name: str |
| 44 | + params: list[WorkflowParam] = [] |
| 45 | + steps: list[Step] |
| 46 | +``` |
| 47 | + |
| 48 | +↓ ROOT CAUSE: The three metadata fields are absent from `Workflow`; adding them as optional fields resolves the rejection without touching any other code path. |
| 49 | + |
| 50 | +### Affected Files |
| 51 | + |
| 52 | +| File | Lines | Action | Description | |
| 53 | +| ----------------------------------------- | --------- | ------ | --------------------------------------------------------- | |
| 54 | +| `src/flowsh_cli/models.py` | 199-203 | UPDATE | Add 3 optional fields to `Workflow` class | |
| 55 | +| `tests/test_workflow_to_harness.py` | 1161-1179 | UPDATE | Invert rejection test to acceptance test | |
| 56 | +| `tests/test_workflow_to_harness.py` | after 1179| CREATE | Add 3 new tests (partial, absent defaults, script identity) | |
| 57 | +| `tests/test_workflow_to_harness.py` | 633-645 | UPDATE | Add 3 schema assertions for new fields | |
| 58 | + |
| 59 | +### Integration Points |
| 60 | + |
| 61 | +- `src/flowsh_cli/render.py:22-227` — `render_harness()`: uses only `workflow.id`, `workflow.name`, `workflow.params`, `workflow.steps` — **no change needed** |
| 62 | +- `src/flowsh_cli/render.py:18-19` — `harness_path()`: uses only `workflow.id` — **no change needed** |
| 63 | +- `src/flowsh_cli/cli.py` — `--schema` already calls `workflow_schema_yaml()` → `WorkflowFile.model_json_schema()` — auto-updates, **no change needed** |
| 64 | +- `src/flowsh_cli/models.py:266-271` — `workflow_schema_yaml()`: calls `WorkflowFile.model_json_schema()` — schema auto-updates, **no change needed** |
| 65 | + |
| 66 | +### Git History |
| 67 | + |
| 68 | +- **Rejection test introduced**: `ee97fb5` — 2026-06-02 — "feat: support named positional parameters in workflow harness (#19)" |
| 69 | +- **Last `models.py` modification**: `e8d67fb` — "feat: add parallel step type for concurrent workflow execution (#25) (#27)" |
| 70 | +- **Implication**: The `test_cli_rejects_removed_metadata_and_path_fields` test was added to guard against old schema fields leaking back in. This issue reverses that guard intentionally: the fields are now declared as supported optional metadata. |
| 71 | + |
| 72 | +## Implementation Plan |
| 73 | + |
| 74 | +### Step 1: Add optional metadata fields to `Workflow` model |
| 75 | + |
| 76 | +**File**: `src/flowsh_cli/models.py` |
| 77 | +**Lines**: 199-203 |
| 78 | +**Action**: UPDATE |
| 79 | + |
| 80 | +**Current code:** |
| 81 | + |
| 82 | +```python |
| 83 | +class Workflow(StrictModel): |
| 84 | + id: str |
| 85 | + name: str |
| 86 | + params: list[WorkflowParam] = [] |
| 87 | + steps: list[Step] |
| 88 | +``` |
| 89 | + |
| 90 | +**Required change:** |
| 91 | + |
| 92 | +```python |
| 93 | +class Workflow(StrictModel): |
| 94 | + id: str |
| 95 | + name: str |
| 96 | + params: list[WorkflowParam] = [] |
| 97 | + enabled: bool = True |
| 98 | + schedule: str | None = None |
| 99 | + shellScriptPath: str | None = None |
| 100 | + steps: list[Step] |
| 101 | +``` |
| 102 | + |
| 103 | +**Why**: Declares the three metadata fields as optional with sensible defaults. `enabled` defaults to `True` (active by default). `schedule` and `shellScriptPath` default to `None` (not configured). No validators are needed — these fields are passive metadata consumed only by external tooling, never by the parser or renderer. Field order keeps `steps` last to maintain YAML readability. |
| 104 | + |
| 105 | +--- |
| 106 | + |
| 107 | +### Step 2: Invert the existing rejection test to an acceptance test |
| 108 | + |
| 109 | +**File**: `tests/test_workflow_to_harness.py` |
| 110 | +**Lines**: 1161-1179 |
| 111 | +**Action**: UPDATE |
| 112 | + |
| 113 | +**Current code:** |
| 114 | + |
| 115 | +```python |
| 116 | +def test_cli_rejects_removed_metadata_and_path_fields(tmp_path: Path) -> None: |
| 117 | + workflow_file = tmp_path / "workflows.yml" |
| 118 | + workflow_file.write_text( |
| 119 | + """ |
| 120 | +workflows: |
| 121 | + - id: wf_legacy |
| 122 | + name: Legacy Shape |
| 123 | + enabled: true |
| 124 | + schedule: manual |
| 125 | + shellScriptPath: .harness/legacy.sh |
| 126 | + steps: |
| 127 | + - type: bash |
| 128 | + run: echo legacy |
| 129 | +""".lstrip(), |
| 130 | + encoding="utf-8", |
| 131 | + ) |
| 132 | + |
| 133 | + with pytest.raises(WorkflowParseError): |
| 134 | + parse_workflows(workflow_file) |
| 135 | +``` |
| 136 | + |
| 137 | +**Required change:** |
| 138 | + |
| 139 | +```python |
| 140 | +def test_parse_workflows_accepts_all_optional_metadata_fields(tmp_path: Path) -> None: |
| 141 | + workflow_file = tmp_path / "workflows.yml" |
| 142 | + workflow_file.write_text( |
| 143 | + """ |
| 144 | +workflows: |
| 145 | + - id: wf_legacy |
| 146 | + name: Legacy Shape |
| 147 | + enabled: true |
| 148 | + schedule: manual |
| 149 | + shellScriptPath: .harness/legacy.sh |
| 150 | + steps: |
| 151 | + - type: bash |
| 152 | + run: echo legacy |
| 153 | +""".lstrip(), |
| 154 | + encoding="utf-8", |
| 155 | + ) |
| 156 | + |
| 157 | + workflows = parse_workflows(workflow_file) |
| 158 | + assert len(workflows) == 1 |
| 159 | + wf = workflows[0] |
| 160 | + assert wf.enabled is True |
| 161 | + assert wf.schedule == "manual" |
| 162 | + assert wf.shellScriptPath == ".harness/legacy.sh" |
| 163 | +``` |
| 164 | + |
| 165 | +**Why**: The YAML that was explicitly rejected must now parse successfully. The assertions verify the fields are populated with the values supplied. |
| 166 | + |
| 167 | +--- |
| 168 | + |
| 169 | +### Step 3: Add tests for partial presence and default values |
| 170 | + |
| 171 | +**File**: `tests/test_workflow_to_harness.py` |
| 172 | +**Lines**: after 1179 (insert after the test updated in Step 2) |
| 173 | +**Action**: CREATE (new test functions) |
| 174 | + |
| 175 | +```python |
| 176 | +def test_parse_workflows_accepts_partial_metadata_fields(tmp_path: Path) -> None: |
| 177 | + workflow_file = tmp_path / "workflows.yml" |
| 178 | + workflow_file.write_text( |
| 179 | + """ |
| 180 | +workflows: |
| 181 | + - id: wf_partial |
| 182 | + name: Partial Metadata |
| 183 | + enabled: false |
| 184 | + steps: |
| 185 | + - type: bash |
| 186 | + run: echo partial |
| 187 | +""".lstrip(), |
| 188 | + encoding="utf-8", |
| 189 | + ) |
| 190 | + workflows = parse_workflows(workflow_file) |
| 191 | + assert len(workflows) == 1 |
| 192 | + wf = workflows[0] |
| 193 | + assert wf.enabled is False |
| 194 | + assert wf.schedule is None |
| 195 | + assert wf.shellScriptPath is None |
| 196 | + |
| 197 | + |
| 198 | +def test_parse_workflows_applies_default_metadata_when_fields_absent(tmp_path: Path) -> None: |
| 199 | + workflow_file = tmp_path / "workflows.yml" |
| 200 | + workflow_file.write_text( |
| 201 | + """ |
| 202 | +workflows: |
| 203 | + - id: wf_no_meta |
| 204 | + name: No Metadata |
| 205 | + steps: |
| 206 | + - type: bash |
| 207 | + run: echo no-meta |
| 208 | +""".lstrip(), |
| 209 | + encoding="utf-8", |
| 210 | + ) |
| 211 | + workflows = parse_workflows(workflow_file) |
| 212 | + assert len(workflows) == 1 |
| 213 | + wf = workflows[0] |
| 214 | + assert wf.enabled is True |
| 215 | + assert wf.schedule is None |
| 216 | + assert wf.shellScriptPath is None |
| 217 | + |
| 218 | + |
| 219 | +def test_generated_harness_is_identical_with_and_without_metadata_fields(tmp_path: Path) -> None: |
| 220 | + base_file = tmp_path / "base.yml" |
| 221 | + base_file.write_text( |
| 222 | + """ |
| 223 | +workflows: |
| 224 | + - id: wf_meta_test |
| 225 | + name: Meta Test |
| 226 | + steps: |
| 227 | + - type: bash |
| 228 | + run: echo hello |
| 229 | +""".lstrip(), |
| 230 | + encoding="utf-8", |
| 231 | + ) |
| 232 | + |
| 233 | + meta_file = tmp_path / "meta.yml" |
| 234 | + meta_file.write_text( |
| 235 | + """ |
| 236 | +workflows: |
| 237 | + - id: wf_meta_test |
| 238 | + name: Meta Test |
| 239 | + enabled: true |
| 240 | + schedule: "0 * * * *" |
| 241 | + shellScriptPath: .harness/wf_meta_test.sh |
| 242 | + steps: |
| 243 | + - type: bash |
| 244 | + run: echo hello |
| 245 | +""".lstrip(), |
| 246 | + encoding="utf-8", |
| 247 | + ) |
| 248 | + |
| 249 | + base_workflows = parse_workflows(base_file) |
| 250 | + meta_workflows = parse_workflows(meta_file) |
| 251 | + |
| 252 | + from flowsh_cli.render import render_harness |
| 253 | + |
| 254 | + assert render_harness(base_workflows[0]) == render_harness(meta_workflows[0]) |
| 255 | +``` |
| 256 | + |
| 257 | +**Why**: These three tests cover the acceptance criteria in full — partial presence, complete absence (defaults), and script identity preservation. The identity test is the most critical: it must fail immediately if anyone accidentally wires the metadata fields into the renderer. |
| 258 | + |
| 259 | +--- |
| 260 | + |
| 261 | +### Step 4: Assert new fields appear in `--schema` output |
| 262 | + |
| 263 | +**File**: `tests/test_workflow_to_harness.py` |
| 264 | +**Lines**: 633-645 |
| 265 | +**Action**: UPDATE (add 3 assertions to existing test) |
| 266 | + |
| 267 | +**Current code (lines 638-644):** |
| 268 | + |
| 269 | +```python |
| 270 | + assert "title: WorkflowFile" in result.output |
| 271 | + assert "const: vars" in result.output |
| 272 | + assert "const: bash" in result.output |
| 273 | + assert "const: agent" in result.output |
| 274 | + assert "model:" in result.output |
| 275 | + assert "command:" in result.output |
| 276 | + assert "dangerouslySkipPermissions:" in result.output |
| 277 | +``` |
| 278 | + |
| 279 | +**Required change:** |
| 280 | + |
| 281 | +```python |
| 282 | + assert "title: WorkflowFile" in result.output |
| 283 | + assert "const: vars" in result.output |
| 284 | + assert "const: bash" in result.output |
| 285 | + assert "const: agent" in result.output |
| 286 | + assert "model:" in result.output |
| 287 | + assert "command:" in result.output |
| 288 | + assert "dangerouslySkipPermissions:" in result.output |
| 289 | + assert "enabled:" in result.output |
| 290 | + assert "schedule:" in result.output |
| 291 | + assert "shellScriptPath:" in result.output |
| 292 | +``` |
| 293 | + |
| 294 | +**Why**: The `--schema` acceptance criterion requires the three fields to be visible in YAML schema output. Pydantic auto-generates them once the fields are declared; these assertions lock that behavior. |
| 295 | + |
| 296 | +--- |
| 297 | + |
| 298 | +## Patterns to Follow |
| 299 | + |
| 300 | +**From codebase — mirror these exactly:** |
| 301 | + |
| 302 | +```python |
| 303 | +# SOURCE: models.py:48 |
| 304 | +# Pattern for optional string field with None default |
| 305 | +name: str | None = None |
| 306 | + |
| 307 | +# SOURCE: models.py:84-85 |
| 308 | +# Pattern for optional string fields on AgentStep |
| 309 | +agent: str | None = None |
| 310 | +model: str | None = None |
| 311 | + |
| 312 | +# SOURCE: models.py:87-90 |
| 313 | +# Pattern for boolean field with explicit default (no Field() needed for simple case) |
| 314 | +dangerouslySkipPermissions: bool = Field(default=False, ...) |
| 315 | +# Simplified for metadata (no alias or description needed): |
| 316 | +enabled: bool = True |
| 317 | +``` |
| 318 | + |
| 319 | +Use `bool = True` directly (no `Field()` wrapper) since `enabled` needs no alias, description, or validators. Use `str | None = None` directly for `schedule` and `shellScriptPath` — same rationale. |
| 320 | + |
| 321 | +--- |
| 322 | + |
| 323 | +## Edge Cases & Risks |
| 324 | + |
| 325 | +| Risk / Edge Case | Mitigation | |
| 326 | +| ------------------------------------------------------------------ | --------------------------------------------------------------------------------------- | |
| 327 | +| Renderer accidentally reads `shellScriptPath` in a future refactor | The identity test (`test_generated_harness_is_identical_with_and_without_metadata_fields`) will immediately catch any such regression | |
| 328 | +| `schedule` accepts arbitrary strings, not only valid cron | By design — flowsh does not interpret `schedule`; external schedulers own validation | |
| 329 | +| `enabled: false` does not suppress harness generation | By design — `enabled` is metadata for external tooling; suppression is out of scope | |
| 330 | +| `shellScriptPath` does not control actual output path | By design — output path is derived from `workflow.id` via `harness_path()`; path hint is metadata only | |
| 331 | +| Test name `test_cli_rejects_removed_metadata_and_path_fields` remains in git history | Rename fully resolves confusion; no runtime impact | |
| 332 | + |
| 333 | +--- |
| 334 | + |
| 335 | +## Validation |
| 336 | + |
| 337 | +### Automated Checks |
| 338 | + |
| 339 | +```bash |
| 340 | +make qa |
| 341 | +``` |
| 342 | + |
| 343 | +This runs `uv run ruff check`, `uv run mypy`, and `uv run pytest` — all three must pass. |
| 344 | + |
| 345 | +### Manual Verification |
| 346 | + |
| 347 | +1. Create `test.yml` with all three metadata fields and run `uv run flowsh-cli test.yml --dry-run` — must succeed with exit 0 |
| 348 | +2. Run `uv run flowsh-cli --schema` — verify `enabled:`, `schedule:`, and `shellScriptPath:` appear in the output |
| 349 | +3. Generate harness with and without metadata fields, diff the outputs — must be byte-for-byte identical |
| 350 | + |
| 351 | +--- |
| 352 | + |
| 353 | +## Scope Boundaries |
| 354 | + |
| 355 | +**IN SCOPE:** |
| 356 | + |
| 357 | +- Adding `enabled: bool = True`, `schedule: str | None = None`, `shellScriptPath: str | None = None` to `Workflow` at `models.py:199-203` |
| 358 | +- Renaming and inverting `test_cli_rejects_removed_metadata_and_path_fields` → `test_parse_workflows_accepts_all_optional_metadata_fields` |
| 359 | +- Adding 3 new test functions for partial presence, default values, and script identity |
| 360 | +- Adding 3 schema assertions to `test_cli_exposes_schema_without_workflow_argument` |
| 361 | + |
| 362 | +**OUT OF SCOPE (do not touch):** |
| 363 | + |
| 364 | +- `render.py` — must not be changed; generated scripts must remain identical |
| 365 | +- `cli.py` — must not be changed; `--schema` already auto-updates |
| 366 | +- `StrictModel`, `WorkflowFile`, `WorkflowParam`, any step model |
| 367 | +- Making `enabled: false` suppress harness generation (future enhancement) |
| 368 | +- Making `shellScriptPath` control the actual output path (future enhancement) |
| 369 | +- Adding validators on the three new fields |
| 370 | +- Adding any metadata fields beyond the three proposed in the issue |
| 371 | + |
| 372 | +--- |
| 373 | + |
| 374 | +## Metadata |
| 375 | + |
| 376 | +- **Investigated by**: GHAR |
| 377 | +- **Timestamp**: 2026-06-22T20:30:00Z |
| 378 | +- **Artifact**: `.ghar/issues/issue-37.md` |
0 commit comments