fix(workflows): guard non-mapping 'workflow:' block in WorkflowDefinition#3694
fix(workflows): guard non-mapping 'workflow:' block in WorkflowDefinition#3694jawwad-ali wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Prevents malformed workflow: blocks from crashing workflow parsing.
Changes:
- Normalizes non-mapping workflow headers locally.
- Adds regression coverage for null, string, and list values.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/engine.py |
Guards workflow header access. |
tests/test_workflows.py |
Tests malformed header handling. |
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: 1
- Review effort level: Medium
|
Please address Copilot feedback |
d347029 to
81abc3c
Compare
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
…tion
A present-but-non-mapping top-level `workflow:` block (bare `workflow:` ->
YAML null, or `workflow: <str>` / `workflow: [..]`) crashed
WorkflowDefinition.__init__ with AttributeError: the `{}` default of
`data.get("workflow", {})` only applies when the key is ABSENT, so a non-dict
value reached `workflow.get("id", ...)`. This fires inside from_yaml/
from_string — before validate_workflow can report the malformed shape — and
in the CLI escapes as a raw traceback (load_workflow is wrapped to catch only
FileNotFoundError/ValueError).
Normalize the local `workflow` to {} when it is not a mapping (self.data keeps
the raw value so validate_workflow still reports it), mirroring the adjacent
default_options guard.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…low value
Address review: the previous assertion only proved the key stayed present; it
would pass even if construction replaced the malformed value with {}. Assert
definition.data["workflow"] equals the original parsed value and is still a
non-mapping, proving the guard normalizes only the local variable.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
81abc3c to
4b26886
Compare
|
Correct — fixed in 4b26886. I verified
Also rebased onto latest |
What
A present-but-non-mapping top-level
workflow:block crashedWorkflowDefinition.__init__withAttributeError:workflow:→ YAMLnullworkflow: some-string→strworkflow: [a, b]→listworkflow = data.get("workflow", {})only substitutes{}when the key is absent, so a present non-dict value reachedworkflow.get("id", "")→AttributeError: 'NoneType'/'str'/'list' object has no attribute 'get'.This fires inside
from_yaml/from_string— beforevalidate_workflowcan report the malformed shape — and in the CLI (workflow run/resume, whoseload_workflowis wrapped to catch onlyFileNotFoundError/ValueError) it escapes as a raw traceback instead of a clean "Invalid workflow" message.Fix
Normalize the local
workflowto{}when it isn't a mapping (self.datakeeps the raw value sovalidate_workflowstill reports it), mirroring the adjacentdefault_optionsguard (if not isinstance(...): = {}).Tests
tests/test_workflows.py::TestWorkflowDefinition::test_non_mapping_workflow_block_parses_then_validates(parametrized null/str/list) — construction no longer raises, andvalidate_workflowreports the missingworkflow.id. Fails before the fix (all three crash).ruffclean.AI-assisted: authored with Claude Code. Verified against the
default_optionssibling guard and confirmed fail-before/pass-after, incl.self.dataround-trip.