Skip to content

fix(workflows): report a falsy non-mapping overlay manifest as a shape error - #3884

Open
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/overlay-falsy-nonmapping-manifest
Open

fix(workflows): report a falsy non-mapping overlay manifest as a shape error#3884
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/overlay-falsy-nonmapping-manifest

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

ProjectOverlaySource.collect in src/specify_cli/workflows/overlays/layer_sources.py reads an overlay manifest with:

data = yaml.safe_load(path.read_text(encoding="utf-8")) or {}

validate_overlay_yaml opens with if not isinstance(data, dict): return None, ["Overlay manifest must be a mapping."] — so a wrong-shaped manifest is meant to be reported as exactly that. But or {} replaces every falsy non-mapping with an empty mapping before that check runs, so those files are misreported.

Reproduction on current main (81bf741)

Whole-document overlay manifests, same code path:

'- a'    -> ['Overlay manifest must be a mapping.']          <-- correct
'hello'  -> ['Overlay manifest must be a mapping.']          <-- correct
'[]'     -> ["Overlay 'id' is required and must be a non-empty string.",
             "Overlay 'extends' is required and must be a non-empty string.",
             "Overlay 'edits' is required and must be a list."]
'false'  -> same three
'0'      -> same three
"''"     -> same three

So [], false, 0 and '' produce three misleading "required field" errors, sending the author looking for missing keys in a document that has the wrong shape entirely. Their truthy twins get the right answer.

Precedent

The sibling reader for these same files, in the same package_read_overlay in overlays/_commands.py:160 — does not coerce:

data = yaml.safe_load(content)

So the two readers of one file format disagree, and the coercing one is the outlier.

Fix

Drop or {}; normalise only an empty document:

data = yaml.safe_load(path.read_text(encoding="utf-8"))
...
if data is None:
    data = {}

No breaking change. None (an empty file) still becomes {}, so a genuinely empty overlay still reports its missing fields — pinned by a second new test. Every mapping is unaffected. The only inputs whose behaviour changes are the four falsy non-mappings, which move from three wrong errors to one right one.

Verification

  • Fail-before / pass-after: the four parametrized cases fail on unpatched src and pass with the fix — 8 failed → 4 failed, and those remaining 4 are Windows symlink-privilege tests that fail on clean main.
  • Scoped regression over tests/workflows: failure set identical to the clean-main baseline captured on 81bf741 (10 pre-existing in scope).
  • uvx ruff@0.15.0 check src tests → clean

New class sits next to the existing TestProjectOverlaySourceFileReadErrors, which covers the other failure modes of this same read.


Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

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 overlay validation so falsy non-mapping manifests report a shape error.

Changes:

  • Removes broad falsy-value coercion.
  • Preserves empty-document handling.
  • Adds regression tests for falsy manifests.
Show a summary per file
File Description
src/specify_cli/workflows/overlays/layer_sources.py Narrows manifest normalization to None.
tests/workflows/test_overlay_layer_sources.py Tests falsy manifests and empty documents.

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: 2
  • Review effort level: Balanced

Comment thread tests/workflows/test_overlay_layer_sources.py Outdated
Comment thread src/specify_cli/workflows/overlays/layer_sources.py Outdated
jawwad-ali and others added 2 commits July 31, 2026 22:43
…e error

`ProjectOverlaySource.collect` did `yaml.safe_load(...) or {}`.
`validate_overlay_yaml` opens with an `isinstance(data, dict)` check, so a
truthy non-mapping is reported correctly — but `or {}` replaced the falsy
non-mappings with an empty mapping first, so those files were reported as
three bogus missing-field errors instead of the wrong shape:

  '- a'    -> ['Overlay manifest must be a mapping.']
  'hello'  -> ['Overlay manifest must be a mapping.']
  '[]'     -> ["Overlay 'id' is required...", "'extends' is required...",
               "'edits' is required..."]
  'false'  -> same three
  '0'      -> same three
  "''"     -> same three

The sibling reader for these same files in the same package, `_read_overlay`
in overlays/_commands.py, does not coerce.

Only an empty document (None) now becomes an empty mapping, so a genuinely
empty overlay still reports its missing fields.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review catch: `safe_load` returns None for an explicit null scalar
(`null`, `~`, `Null`, `NULL`) as well as for an empty document, so the
`data is None` normalization still converted those manifests to `{}` and
they still received missing-field errors instead of the mapping-shape
error.

Use `yaml.compose`, which yields no node only for a genuinely empty
document, to tell the two apart. Measured:

  empty doc          -> missing-field   (correct)
  explicit null      -> SHAPE
  explicit ~         -> SHAPE
  NULL               -> SHAPE
  [] false 0 ''      -> SHAPE
  - a / hello        -> SHAPE

Extends the parametrized cases with null/~/NULL, and corrects the article
before `isinstance` in the docstring.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jawwad-ali
jawwad-ali force-pushed the fix/overlay-falsy-nonmapping-manifest branch from cd8f706 to 31d69e7 Compare July 31, 2026 17:45
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