fix(workflows): reject falsy non-mapping workflow-catalogs.yml top level#3707
Open
jawwad-ali wants to merge 3 commits into
Open
fix(workflows): reject falsy non-mapping workflow-catalogs.yml top level#3707jawwad-ali wants to merge 3 commits into
jawwad-ali wants to merge 3 commits into
Conversation
WorkflowCatalog._load_catalog_config parsed the config with
`yaml.safe_load(...) or {}`, then checked `isinstance(data, dict)`. The
`or {}` coerces a FALSY non-mapping top level (`[]`, `false`, `0`, `''`) to
`{}` *before* the guard runs, so those are silently swallowed as "empty
config" and fall back to the built-in defaults -- while a TRUTHY non-mapping
(`5`, a bare list) correctly raises. Same silent-swallow inconsistency the
bundler catalog reader fixed for its own config.
Drop the `or {}` and branch on `None` (empty document / explicit `null`)
explicitly: `None` stays a valid no-op, every non-mapping (falsy or truthy)
now raises the same actionable error. Correct configs are unaffected.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment said a None return means "no project catalogs, fall back to the built-in defaults". Both halves were imprecise: _load_catalog_config serves the project AND user configs, and get_active_catalogs falls through env -> project -> user -> built-in, so a None from the project layer moves on to the USER config; the built-in defaults apply only once every layer returned None. Reword the loader comment and the mirror test docstring. Comments only -- no behaviour change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes workflow catalog validation so falsy non-mapping YAML values are rejected while empty or null documents remain valid no-ops.
Changes:
- Handles
Noneexplicitly before mapping validation. - Adds regression tests for falsy non-mappings and empty/null documents.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/catalog.py |
Corrects top-level YAML validation. |
tests/test_workflows.py |
Covers invalid falsy values and valid no-op inputs. |
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: 0
- Review effort level: Medium
…atalog
Self-review follow-up: the top-level fix left the identical asymmetry live five
lines below, and again in this file's twin loader.
1. WorkflowCatalog._load_catalog_config: the ``catalogs`` shape check sat behind
an emptiness check, so a FALSY non-list (``catalogs: {}``/``''``/``0``/
``false``) was silently swallowed as "no catalogs" while ``catalogs: 5``
raised. Verified before this commit: ``catalogs: {}`` -> None (no error).
Shape now checked first; absent/explicit-null and empty-list stay no-ops
(matching the bundler's reader).
2. StepCatalog._load_catalog_config -- the step-catalog twin, read the same way
-- still had ``yaml.safe_load(...) or {}``, so falsy non-mappings bypassed its
isinstance guard (``[]`` -> None while ``5`` raised). Same two guards applied,
keeping the two loaders in lockstep.
Eight new parametrized cases, all failing before this commit.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Problem
WorkflowCatalog._load_catalog_config(insrc/specify_cli/workflows/catalog.py) parses the project/userworkflow-catalogs.ymllike this:The
or {}coerces a falsy non-mapping top level to{}before theisinstanceguard runs, so those are silently swallowed instead of raising — while a truthy non-mapping raises as intended. Reproduced onmain:[],false,0,''(falsy non-mappings)None→ built-in defaults, no error5, a bare list (truthy non-mappings)expected a mapping✅nullNone(valid no-op) ✅So a malformed config like a top-level
[](probably meantcatalogs: [...]) is ignored with no diagnostic. This is the same silent-swallow the bundler catalog reader already guards against for its own config.Fix
Drop the
or {}and branch onNoneexplicitly:None(empty file / explicitnull) stays a valid no-op; every non-mapping — falsy or truthy — now raises the same actionable error. Correct configs are unaffected (no behaviour change).Verification
test_falsy_non_mapping_config_rejected([]/false/0/''): fails before (silently returnsNone), passes after (raises).test_empty_or_null_config_is_noop(empty/comment-only/null/~): passes before and after — regression guard that the no-op path is preserved.TestWorkflowCatalog: 32 passed.ruffclean.AI-assisted: authored with Claude Code. I reproduced the falsy/truthy split on
mainwith a small script before writing the fix, and confirmed the empty/null no-op is preserved.