Skip to content

fix(workflows): reject falsy non-mapping workflow-catalogs.yml top level#3707

Open
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/workflow-catalog-config-falsy-non-mapping
Open

fix(workflows): reject falsy non-mapping workflow-catalogs.yml top level#3707
jawwad-ali wants to merge 3 commits into
github:mainfrom
jawwad-ali:fix/workflow-catalog-config-falsy-non-mapping

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

WorkflowCatalog._load_catalog_config (in src/specify_cli/workflows/catalog.py) parses the project/user workflow-catalogs.yml like this:

data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {}
...
if not isinstance(data, dict):
    raise WorkflowValidationError(f"Invalid catalog config: expected a mapping, got ...")

The or {} coerces a falsy non-mapping top level to {} before the isinstance guard runs, so those are silently swallowed instead of raising — while a truthy non-mapping raises as intended. Reproduced on main:

top-level value current behaviour
[], false, 0, '' (falsy non-mappings) silently returns None → built-in defaults, no error
5, a bare list (truthy non-mappings) raises expected a mapping
empty document / null returns None (valid no-op) ✅

So a malformed config like a top-level [] (probably meant catalogs: [...]) 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 on None explicitly:

data = yaml.safe_load(...)
if data is None:        # empty document / explicit null -> valid no-op
    return None
if not isinstance(data, dict):
    raise WorkflowValidationError(...)   # now catches [], false, 0, '' too

None (empty file / explicit null) 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

  • New test_falsy_non_mapping_config_rejected ([]/false/0/''): fails before (silently returns None), passes after (raises).
  • New test_empty_or_null_config_is_noop (empty/comment-only/null/~): passes before and after — regression guard that the no-op path is preserved.
  • Full TestWorkflowCatalog: 32 passed. ruff clean.

AI-assisted: authored with Claude Code. I reproduced the falsy/truthy split on main with a small script before writing the fix, and confirmed the empty/null no-op is preserved.

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>
@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner July 24, 2026 08:41
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>

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

Fixes workflow catalog validation so falsy non-mapping YAML values are rejected while empty or null documents remain valid no-ops.

Changes:

  • Handles None explicitly 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>
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.

3 participants