Skip to content

fix(bundler): validate a catalog source before persisting it - #4535

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/catalog-add-validate-before-write
Open

fix(bundler): validate a catalog source before persisting it#4535
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/catalog-add-validate-before-write

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

add_source writes the new entry and only then constructs it:

catalogs.append(entry)
_write(project_root, catalogs)
return CatalogSource.from_dict(entry, Scope.PROJECT)   # <-- validation, too late

CatalogSource.from_dict already rejects an empty id — but by the time it runs, the entry is on disk.

Reproduction on current main (c173bf1)

before     : []
add_source -> raised: A catalog source is missing its 'id'.
AFTER      : [{"id": "", "url": "https://example.test/c.json", "priority": 5, "install_policy": "install-allowed"}]

The command reports a failure, so the user reasonably assumes nothing happened.

It wedges the project

Every command that loads the catalog stack now fails on that stray entry:

load_source_stack -> BROKEN: BundlerError A catalog source is missing its 'id'.

So a single mistyped catalog add --id " " leaves the project's catalog config unusable until bundle-catalogs.yml is hand-edited — by hand, for a file the bundle catalog commands exist to manage.

Fix

Construct before writing, reusing the validation that already exists rather than adding a second check that could drift:

source = CatalogSource.from_dict(entry, Scope.PROJECT)
catalogs.append(entry)
_write(project_root, catalogs)
return source

After the fix:

add with blank id -> refused: A catalog source is missing its 'id'.
config after      : []                      <-- nothing persisted
stack still loads : ['default', 'community']
normal add works  : ok https://example.test/ok.json

Verification

  • Fail-before / pass-after: 1 new-vs-baseline failure with the source reverted to upstream/main → passing with the fix.
  • The test asserts both halves: the config is left empty and load_source_stack still succeeds — the latter is what the stray entry actually broke.
  • Scoped regression over tests/unit: 536 passed vs a clean-main baseline of 535 passed, same 2 pre-existing failures, none new.
  • uvx ruff@0.15.0 check src tests → clean

No behaviour change for any input that previously succeeded. The reordered call is the same construction, just earlier, and it raises for exactly the input that already raised — the difference is that the failure no longer leaves a file behind.

Note on overlap: this touches commands_impl/catalog_config.py, as does my #4533, but a different function (add_source vs remove_source). Happy to rebase whichever lands second.


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

🤖 Generated with Claude Code

`add_source` wrote the new entry and only then constructed it:

    catalogs.append(entry)
    _write(project_root, catalogs)
    return CatalogSource.from_dict(entry, Scope.PROJECT)

`CatalogSource.from_dict` already rejects an empty id, but by the time it ran
the entry was on disk. A whitespace-only `--id` (or a url from which no id can
be derived) therefore reported an error while leaving a broken entry behind:

    add_source -> raised: A catalog source is missing its 'id'.
    AFTER      : [{"id": "", "url": "https://example.test/c.json", ...}]

The user sees a failure and reasonably assumes nothing happened. In fact the
project's catalog config is now unusable -- every command that loads the stack
fails on that entry:

    load_source_stack -> BROKEN: BundlerError A catalog source is missing its 'id'.

so the project stays wedged until bundle-catalogs.yml is hand-edited.

Constructing before writing reuses the validation that already exists; nothing
is persisted when it fails.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner September 11, 2026 16:17
@mnriem mnriem added the triage-must-have Verdict: high-value, important work for Spec Kit — do first label Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triage-must-have Verdict: high-value, important work for Spec Kit — do first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants