github/github-spec-kit
1
0
Fork 0
mirror of https://github.com/github/spec-kit.git synced 2026-08-03 06:26:30 +08:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* fix(workflows): reject falsy non-mapping workflow-catalogs.yml top level

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>

* docs(workflows): describe the catalog-config fallthrough accurately

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>

* fix(workflows): close the same falsy-mask gap in 'catalogs' and StepCatalog

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>

* test(workflows): move StepCatalog guards into TestStepCatalog and add the nested case

Address three review points:

1. The StepCatalog regression tests sat inside TestWorkflowCatalog, so a
   targeted `pytest ...::TestStepCatalog` run skipped them entirely. Moved into
   that class, where the duplicated twin loader belongs.
2. StepCatalog had no nested-value coverage (only top-level). Added the
   parametrized falsy ``catalogs:`` case, plus the absent/null/empty no-op
   cases. Verified against upstream/main's catalog.py: 8 fail there, pass here.
3. Dropped the inaccurate parity parenthetical. src/specify_cli/catalogs.py
   RAISES for missing/empty ``catalogs`` and coerces a null document to {}, so
   it is not the behavior this loader matches -- the comment now just states
   what changed (only the misreported shapes) without claiming parity.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ali jawwad
2026-07-29 03:20:21 +05:00
committed by GitHub
parent 3dad624e5d
commit 52a6514e38
2 changed files with 133 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
src/specify_cli/workflows/catalog.py
View File

@@ -335,26 +335,45 @@ class WorkflowCatalog:
if not config_path.exists():
return None
try:
data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {}
data = yaml.safe_load(config_path.read_text(encoding="utf-8"))
except (yaml.YAMLError, OSError, UnicodeError) as exc:
raise WorkflowValidationError(
f"Failed to read catalog config {config_path}: {exc}"
) from exc
# An empty document (or explicit ``null``) parses to None -> this config
# layer contributes nothing, so ``get_active_catalogs`` moves on to the
# next layer (this loader serves both the project and user configs;
# the built-in defaults apply only once every layer has returned None).
# Do NOT coerce with ``or {}`` here: that also turns a FALSY non-mapping
# (top-level ``[]``, ``false``, ``0``, ``''``) into ``{}`` and silently
# swallows it, while a TRUTHY non-mapping (``5``, a bare list) correctly
# raises below -- an inconsistency. Only None means "no document".
if data is None:
return None
if not isinstance(data, dict):
raise WorkflowValidationError(
f"Invalid catalog config: expected a mapping, "
f"got {type(data).__name__}"
)
catalogs_data = data.get("catalogs", [])
if not catalogs_data:
# Empty catalogs list (e.g. after removing last entry)
# is valid — fall back to built-in defaults.
# Same asymmetry as the top level above, one nesting level down: the
# shape check has to run BEFORE the emptiness check, or a FALSY non-list
# (``catalogs: {}``/``''``/``0``/``false``) is silently swallowed as
# "no catalogs" while a TRUTHY non-list (``catalogs: 5``) correctly
# raises. An absent key, an explicit ``catalogs:`` null, and an empty
# list all keep their existing "nothing configured here" behavior --
# only the misreported shapes change.
catalogs_data = data.get("catalogs")
if catalogs_data is None:
return None
if not isinstance(catalogs_data, list):
raise WorkflowValidationError(
f"Invalid catalog config: 'catalogs' must be a list, "
f"got {type(catalogs_data).__name__}"
)
if not catalogs_data:
# Empty catalogs list (e.g. after removing last entry)
# is valid — fall back to built-in defaults.
return None
entries: list[WorkflowCatalogEntry] = []
for idx, item in enumerate(catalogs_data):
@@ -1018,24 +1037,33 @@ class StepCatalog:
if not config_path.exists():
return None
try:
data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {}
data = yaml.safe_load(config_path.read_text(encoding="utf-8"))
except (yaml.YAMLError, OSError, UnicodeError) as exc:
raise StepValidationError(
f"Failed to read catalog config {config_path}: {exc}"
) from exc
# Same two guards as WorkflowCatalog._load_catalog_config above, kept in
# lockstep: this is the step-catalog twin of that loader and read the
# same way. Dropping ``or {}`` stops a falsy non-mapping top level from
# being coerced past the isinstance check, and the ``catalogs`` shape
# check runs before the emptiness check for the same reason.
if data is None:
return None
if not isinstance(data, dict):
raise StepValidationError(
f"Invalid catalog config: expected a mapping, "
f"got {type(data).__name__}"
)
catalogs_data = data.get("catalogs", [])
if not catalogs_data:
catalogs_data = data.get("catalogs")
if catalogs_data is None:
return None
if not isinstance(catalogs_data, list):
raise StepValidationError(
f"Invalid catalog config: 'catalogs' must be a list, "
f"got {type(catalogs_data).__name__}"
)
if not catalogs_data:
return None
entries: list[StepCatalogEntry] = []
for idx, item in enumerate(catalogs_data):

97
tests/test_workflows.py
View File

@@ -6422,6 +6422,57 @@ class TestWorkflowCatalog:
assert len(entries) == 1
assert entries[0].name == "custom"
@pytest.mark.parametrize("body", ["[]\n", "false\n", "0\n", "''\n"])
def test_falsy_non_mapping_config_rejected(self, project_dir, body):
"""A FALSY non-mapping top-level config ([], false, 0, '') must raise,
like a truthy non-mapping (5, a bare list) already does. The previous
``yaml.safe_load(...) or {}`` coerced these to {} and silently swallowed
them, diverging from the truthy case."""
from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError
config_path = project_dir / ".specify" / "workflow-catalogs.yml"
config_path.write_text(body, encoding="utf-8")
catalog = WorkflowCatalog(project_dir)
with pytest.raises(WorkflowValidationError, match="expected a mapping"):
catalog._load_catalog_config(config_path)
@pytest.mark.parametrize("body", ["catalogs: {}\n", "catalogs: ''\n", "catalogs: 0\n", "catalogs: false\n"])
def test_falsy_non_list_catalogs_rejected(self, project_dir, body):
"""A FALSY non-list ``catalogs:`` value must raise, like a truthy one
(``catalogs: 5``) already does. The shape check sat behind the emptiness
check, so these were silently swallowed as "no catalogs"."""
from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError
config_path = project_dir / ".specify" / "workflow-catalogs.yml"
config_path.write_text(body, encoding="utf-8")
catalog = WorkflowCatalog(project_dir)
with pytest.raises(WorkflowValidationError, match="'catalogs' must be a list"):
catalog._load_catalog_config(config_path)
@pytest.mark.parametrize("body", ["catalogs:\n", "catalogs: []\n"])
def test_absent_or_empty_catalogs_is_noop(self, project_dir, body):
"""An explicit ``catalogs:`` null or an empty list stays a valid no-op —
the layer contributes nothing and resolution falls through."""
from specify_cli.workflows.catalog import WorkflowCatalog
config_path = project_dir / ".specify" / "workflow-catalogs.yml"
config_path.write_text(body, encoding="utf-8")
catalog = WorkflowCatalog(project_dir)
assert catalog._load_catalog_config(config_path) is None
@pytest.mark.parametrize("body", ["", "# only a comment\n", "null\n", "~\n"])
def test_empty_or_null_config_is_noop(self, project_dir, body):
"""An empty document, comment-only file, or explicit top-level null is a
valid no-op: the loader returns None so that config layer is skipped and
get_active_catalogs falls through to the next one. It must NOT be
confused with a falsy non-mapping, which raises."""
from specify_cli.workflows.catalog import WorkflowCatalog
config_path = project_dir / ".specify" / "workflow-catalogs.yml"
config_path.write_text(body, encoding="utf-8")
catalog = WorkflowCatalog(project_dir)
assert catalog._load_catalog_config(config_path) is None
@pytest.mark.parametrize("bad_priority", [True, False, float("inf")])
def test_config_priority_bool_or_inf_rejected(self, project_dir, bad_priority):
"""`priority: true` must not be silently coerced to 1, and `priority: .inf`
@@ -7045,6 +7096,52 @@ class TestStepRegistryCustom:
class TestStepCatalog:
"""Test StepCatalog catalog resolution."""
# -- Config shape guards ----------------------------------------------
# StepCatalog._load_catalog_config is a duplicated twin of
# WorkflowCatalog._load_catalog_config, so it needs its own coverage: a
# regression in one loader would not be caught by the other's tests.
@pytest.mark.parametrize("body", ["[]\n", "false\n", "0\n", "''\n"])
def test_falsy_non_mapping_config_rejected(self, project_dir, body):
"""A FALSY non-mapping top level had the same ``or {}`` coercion, which
bypassed the isinstance guard. It must raise like a truthy non-mapping."""
from specify_cli.workflows.catalog import StepCatalog, StepValidationError
config_path = project_dir / ".specify" / "step-catalogs.yml"
config_path.write_text(body, encoding="utf-8")
catalog = StepCatalog(project_dir)
with pytest.raises(StepValidationError, match="expected a mapping"):
catalog._load_catalog_config(config_path)
@pytest.mark.parametrize(
"body", ["catalogs: {}\n", "catalogs: ''\n", "catalogs: 0\n", "catalogs: false\n"]
)
def test_falsy_non_list_catalogs_rejected(self, project_dir, body):
"""...and the same nested guard: a FALSY non-list ``catalogs:`` value must
raise rather than being swallowed as "no catalogs"."""
from specify_cli.workflows.catalog import StepCatalog, StepValidationError
config_path = project_dir / ".specify" / "step-catalogs.yml"
config_path.write_text(body, encoding="utf-8")
catalog = StepCatalog(project_dir)
with pytest.raises(StepValidationError, match="'catalogs' must be a list"):
catalog._load_catalog_config(config_path)
@pytest.mark.parametrize(
"body",
["", "# only a comment\n", "null\n", "~\n", "catalogs:\n", "catalogs: []\n"],
)
def test_empty_or_null_config_is_noop(self, project_dir, body):
"""An empty document, explicit null, or absent/empty ``catalogs:`` stays a
valid no-op — the layer contributes nothing and resolution falls
through."""
from specify_cli.workflows.catalog import StepCatalog
config_path = project_dir / ".specify" / "step-catalogs.yml"
config_path.write_text(body, encoding="utf-8")
catalog = StepCatalog(project_dir)
assert catalog._load_catalog_config(config_path) is None
def test_default_catalogs(self, project_dir, monkeypatch):
from specify_cli.workflows.catalog import StepCatalog