Repository navigation
fix(extensions): reject malformed catalog entries before removal - #5033
Merged
mnriem merged 1 commit intoOct 20, 2026
Merged
Conversation
`specify extension catalog remove` filtered out every non-mapping entry in `.specify/extension-catalogs.yml` while removing the named catalog. Removing a name that isn't there then reported success and rewrote the file without the malformed entry, and removing the last valid catalog deleted the file with the user's other keys. Validate the entries first and leave the file untouched, as github#4992 does for preset catalogs. Assisted-by: Codex CLI (model: gpt-6.1-sol, autonomous) Assisted-by: Claude Code (model: claude-opus-5-5, autonomous)
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Validation precedes all file mutations, and targeted regression coverage addresses the reported failure paths.
No open issues 🎉
What changed in this PR
Aligns extension catalog removal with #4992’s preset fix, preventing malformed configuration from being silently discarded.
Changes:
- Validate every catalog entry before removal or file changes.
- Add regression coverage for malformed entries, diagnostics, and byte-for-byte file preservation.
Tests were not rerun; regression results are author-reported.
| File | Description |
|---|---|
| tests/specify_cli/extensions/catalog/test_command_remove.py | Adds malformed-entry and initialized-project regression tests. |
| src/specify_cli/extensions/catalog/command_remove.py | Rejects non-mapping entries before filtering or modifying configuration. |
Review effort: Max
Collaborator
|
Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
specify extension catalog remove <name>drops every entry of.specify/extension-catalogs.ymlthat isn't a mapping while it removes<name>(the list comprehension insrc/specify_cli/extensions/catalog/command_remove.py). One bare URL line, such as- https://example.com/private.jsoninstead of aname/urlmapping, is enough:✓ Removed catalog '<name>', exits 0, and rewrites the file without the malformed entry. The comment and the CRLF line endings go with it.#4992 fixed the same thing in
specify preset catalog remove. This PR does the same for extensions: every entry is checked before anything is removed, and a non-mapping entry exits 1 withInvalid catalog entry at index N: expected a mapping, got <type>, the message #4992 uses. The file is left as it was. Valid configs behave as before.specify extension catalog listalready refuses such a file: on the config below it exits 1 withcatalog entry at index 1: expected a mapping, got strfrom the shared catalog loader.removerewrote the same file instead.I haven't seen a user report of this. It came up while checking the other catalog commands after #4992.
Repro:
specify init --here --integration codex --ignore-agent-tools, then this.specify/extension-catalogs.yml(CRLF):main(f68ffca)specify extension catalog remove does-not-exist✓ Removed catalog 'does-not-exist'; file rewritten without the comment, the CRLF endings and the URL entryInvalid catalog entry at index 1: expected a mapping, got str; file unchangedspecify extension catalog remove keep✓ Removed catalog 'keep'andProject catalog override removed; file deleted,metadataand the URL entry with itTesting
uv run specify --helpuv sync && uv run pytestNew tests in
tests/specify_cli/extensions/catalog/test_command_remove.py, in the shape of #4992's:test_malformed_entries_preserve_config: a string, number, boolean, null or list entry, first or last in the list, while removing an existing or a missing name. The command exits 1 with the index and type in the message, doesn't print "Removed catalog", and the file bytes (comment,café, CRLF,metadata) are unchanged.test_remove_malformed_config_in_initialized_project: the two repro commands through the real CLI in a project made byspecify init.With only
src/reverted tomain, the 22 new cases fail; with the fix all 29 tests in the file pass.Full suite on Linux, Python 3.13.15: 12447 passed, 377 skipped, 11 failed. The 11 are template parity tests that run the bash scripts with the host
python3, which has no PyYAML here (PyYAML is required to resolve preset template composition), the same 11 #4992 reports. With the checkout's.venv/binfirst on PATH, those five test files pass (257 passed, 112 skipped).Ruff 0.15.0 passes on the changed files.
AI Disclosure
AI disclosure: Two agents, both autonomous. Codex CLI (model: gpt-6.1-sol, reasoning effort xhigh, autonomous, sandboxed and offline) found this while comparing the catalog commands after #4992, wrote the reproduction, and generated the fix and tests. Claude Code (model: claude-opus-5-5, autonomous) reviewed the change, reran the tests against
mainand the fix, reran the repro with the realspecifyCLI, ran the full suite and lint, and drafted this description. Commits carryAssisted-by:trailers.