Skip to content

fix(bundler): treat an explicit-null manifest field as missing, not the text "None" - #3798

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/manifest-explicit-null-fields
Open

fix(bundler): treat an explicit-null manifest field as missing, not the text "None"#3798
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/manifest-explicit-null-fields

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

BundleManifest.from_dict reads every required scalar as str(raw.get(key, "")).strip(). The "" default only covers a missing key. A key that is present but null — exactly how YAML spells an empty field — yields None, and str(None) is the literal string "None". That value is non-empty, so it sails straight past the if not value required-field checks in structural_errors().

bundle:
  id: demo
  name: Demo
  version: 1.0.0
  role: developer
  description:      # <- empty
  author:           # <- empty
  license:          # <- empty

Reproduced on main:

description='None'   author='None'   license='None'
structural_errors() == []
specify bundle validate -> exit 0: "demo is well-formed and valid."

So an empty required field is silently accepted, and the bundle ships the literal text "None" as its author/license/description — which is what bundle info and a catalog entry then display to users. A null provides.<kind>[].id likewise became a component literally named "None".

Fix

Add a _text() helper beside the existing _parse_str_list — matching this file's established "one coercion helper, applied at every site" shape — that maps an explicit null to "", and route the required scalars (schema_version, the six bundle.* fields, requires.speckit_version, component id) through it.

After:

description=''  author=''
structural_errors():
  - Missing required field: bundle.description.
  - Missing required field: bundle.author.
  - Missing required field: bundle.license.

This is the same silent-acceptance class as the already-merged guards in this exact function — #3629 (present-but-non-mapping integration: was silently dropped) and #3661 (falsy non-mapping requires/provides).

Not a behaviour change for valid input

_text is equivalent to the code it replaces for every input except an explicit None: an absent key already produced "", and non-null scalars are still str()-coerced and stripped. Verified — a valid manifest still yields structural_errors() == [] with author='Ali'.

Verification

  • test_explicit_null_bundle_field_is_reported_as_missing (5 parametrized fields) + null speckit_version + null component id: 7 fail before, pass after.
  • tests/contract/test_manifest_schema.py: 35 passed (28 pre-existing untouched).
  • tests/contract/ + tests/unit/: 272 passed — the 3 remaining failures are pre-existing Windows symlink-privilege tests (need admin; pass on CI's Linux/macOS runners).
  • uvx [email protected] check src tests clean.

AI-assisted: authored with Claude Code. I reproduced the "None" values and the clean bundle validate on main first, then confirmed valid manifests are unaffected after the change.

@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner July 28, 2026 14:27
…he text "None"

BundleManifest.from_dict read every required scalar as
`str(raw.get(key, "")).strip()`. The `""` default only covers a MISSING key. A
key present but null -- exactly how YAML spells an empty field (`author:` with
nothing after it) -- yields None, and `str(None)` is the literal string "None".
That value is non-empty, so it sailed past the `if not value` required-field
checks in structural_errors().

Reproduced on main:

    bundle.yml with description:/author:/license: left empty
    -> description='None'  author='None'  license='None'
    -> structural_errors() == []
    -> specify bundle validate: exit 0, "demo is well-formed and valid."

So an empty required field was silently accepted and the bundle shipped the
literal text "None" as its author/license/description -- which is what
`bundle info` and a catalog entry then display. A null `provides.<kind>[].id`
likewise became a component literally named "None".

Add a `_text()` helper beside the existing `_parse_str_list` (the file's
established "one coercion helper applied at every site" shape) mapping an
explicit null to "", and route the required scalars through it. Same
silent-acceptance class as the already-merged guards in this function: github#3629
(non-mapping `integration:`) and github#3661 (falsy non-mapping requires/provides).

Non-null values are still `str()`-coerced and stripped, and an absent key
already produced "" -- so valid manifests are byte-for-byte unaffected.

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

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@jawwad-ali
jawwad-ali force-pushed the fix/manifest-explicit-null-fields branch from 0988775 to ba47a6a Compare July 28, 2026 14:57
@mnriem
mnriem requested a review from Copilot July 28, 2026 21:40

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 manifest parsing so explicit YAML nulls normalize to empty strings and trigger required-field validation.

Changes:

  • Adds centralized null-safe scalar coercion.
  • Applies it to required manifest fields and component IDs.
  • Adds regression tests for null metadata, version requirements, and component IDs.
Show a summary per file
File Description
src/specify_cli/bundler/models/manifest.py Normalizes explicit-null required scalars.
tests/contract/test_manifest_schema.py Covers explicit-null parsing regressions.

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

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