Skip to content

fix(workflows): reject a fan-in wait_for that names an unknown step at validation#3225

Merged
mnriem merged 2 commits into
github:mainfrom
doquanghuy:fix/3223-fanin-unknown-wait-for
Jun 29, 2026
Merged

fix(workflows): reject a fan-in wait_for that names an unknown step at validation#3225
mnriem merged 2 commits into
github:mainfrom
doquanghuy:fix/3223-fanin-unknown-wait-for

Conversation

@doquanghuy

Copy link
Copy Markdown
Contributor

Description

Closes #3223. Sibling to #2957 (fail loud when a fan-out items expression does not resolve to a list).

A fan-in whose wait_for names a step id that isn't declared — a typo, or a forward reference to a step declared after the fan-in — currently aggregates an empty result and reports COMPLETED, masking the authoring error. validate_workflow now flags it: each wait_for id must name a step declared at or before the fan-in, checked against the seen_ids set _validate_steps already maintains.

A step that is declared but only conditionally executed (e.g. inside an if/switch branch) is still "seen", so a legitimately-empty join stays valid — only genuinely unknown or forward-referenced ids are rejected. This is validation-time, so there is no runtime behavior change for correctly-authored workflows.

Testing

  • Ran the workflow suite with .venv/bin/python -m pytest tests/test_workflows.py — 314 passed, including 4 new TestFanInWaitForValidation cases: unknown id rejected, declared-earlier id passes, conditionally-declared (if-branch) id passes, forward reference rejected.
  • uvx ruff check src/ tests/test_workflows.py — clean
  • Tested locally with uv run specify --help

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (describe below)

Code, tests, and this description were authored with AI assistance (Claude Code), from a fan-out/fan-in audit; everything was verified by running the repo's test suite and ruff locally.

@mnriem — small validation-only change; would appreciate your review when you have a moment.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens workflow validation to prevent fan-in steps from silently producing empty join results when wait_for references an invalid step id, addressing #3223 and aligning with prior “fail loud” validation work.

Changes:

  • Add engine-level validation to ensure fan-in.wait_for only references step ids already declared at the point of the fan-in.
  • Add unit tests covering unknown wait_for ids, valid earlier ids, conditionally declared ids, and forward references.
Show a summary per file
File Description
src/specify_cli/workflows/engine.py Adds fan-in wait_for validation against already-seen step ids during workflow validation.
tests/test_workflows.py Adds new test cases asserting validation rejects unknown/forward-referenced wait_for ids and allows earlier/conditional ids.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Low

Comment thread src/specify_cli/workflows/engine.py
Comment thread tests/test_workflows.py
…entries

Address review feedback on the fan-in wait_for validator:

- A fan-in's own id is added to seen_ids before the wait_for check, so
  `wait_for: [<self>]` passed validation while producing a silent empty
  join at runtime. Reject self-references explicitly.
- Non-string entries (e.g. YAML `wait_for: [123]`) were skipped by the
  isinstance(str) guard and validated even though they can never match a
  real step id. Flag them as wiring errors.

Add coverage for both cases.
@doquanghuy

Copy link
Copy Markdown
Contributor Author

Thanks @copilot for the review — both points addressed at root cause in d9c867b:

  • Self-reference: a fan-in's own id is already in seen_ids by the time wait_for is checked, so wait_for: [<self>] passed validation while still producing a silent empty join at runtime. Now rejected explicitly.
  • Non-string entries: wait_for: [123] was skipped by the isinstance(str) guard and silently validated even though it can never match a step id. Now flagged as a wiring error.

Added coverage: test_self_reference_is_rejected, test_non_string_wait_for_entry_is_rejected.

@mnriem would you mind taking a look when you have a moment? 🙏

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Low

@mnriem mnriem merged commit 3036fe6 into github:main Jun 29, 2026
12 checks passed
@mnriem

mnriem commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

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.

[Bug]: fan-in silently joins empty when wait_for names an unknown step

3 participants