fix(workflows): guard non-mapping 'workflow:' block in WorkflowDefinition#3694
Open
jawwad-ali wants to merge 1 commit into
Open
fix(workflows): guard non-mapping 'workflow:' block in WorkflowDefinition#3694jawwad-ali wants to merge 1 commit into
jawwad-ali wants to merge 1 commit into
Conversation
…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>
Contributor
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
| errors = validate_workflow(definition) | ||
| assert any("workflow.id" in e for e in errors) | ||
| # The raw value is preserved on .data for validation/inspection. | ||
| assert "workflow" in definition.data |
Collaborator
|
Please address Copilot feedback |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.