Skip to content

fix(presets): reject malformed catalog entries before removal - #4992

Open
dlwlgnsrhy wants to merge 1 commit into
github:mainfrom
dlwlgnsrhy:fix/preset-catalog-remove-invalid-entries
Open

dlwlgnsrhy wants to merge 1 commit into
github:mainfrom
dlwlgnsrhy:fix/preset-catalog-remove-invalid-entries

Conversation

@dlwlgnsrhy

@dlwlgnsrhy dlwlgnsrhy commented Oct 16, 2026 •

Copy link
Copy Markdown

Description

specify preset catalog remove <name> silently discards non-mapping configuration entries and interprets the shortened list as a successful removal. It can therefore rewrite a malformed file and report success even when the requested name does not exist.

For example, with .specify/preset-catalogs.yml containing:

catalogs:
  - name: keep
    url: https://example.com/catalog.json
  - broken-entry

specify preset catalog remove does-not-exist on baseline f6e72bc exits 0, reports the nonexistent catalog as removed, and deletes broken-entry. With this change, it reports Invalid catalog entry at index 1: expected a mapping, got str, exits 1, and preserves the file byte-for-byte. Removing an existing name from the same malformed file is also rejected before any write.

Validate all entries before filtering; use the same index/type error convention as the preset catalog loader. Valid removal behavior is unchanged. This is a constructed CLI reproduction, not a claim of an independently reported incident.

Testing

  • Tested locally with uv run specify --help
  • Tested with a temporary sample project using the real local specify executable
  • Ran the full suite through the project interpreter: .venv/bin/python -m pytest tests -q

Full run: 12,316 passed, 376 skipped, 11 failed in 710 seconds. The 11 failures were subprocess template-composition tests: the initial shell PATH did not include the project virtualenv, so child scripts selected the host Python, which has no PyYAML. Prepending this checkout's .venv/bin to PATH for the test process made all 11 previously failing cases pass (2.80 seconds), without any product-code changes. This was a targeted rerun, not a second full-suite run. No tests or assertions were disabled or weakened.

Regression evidence on macOS arm64, Python 3.12.13, using the checkout's own environment after uv sync --extra test:

  • The target test module reports 20 failed, 6 passed with unchanged baseline production code, and 26 passed with the fix.
  • Malformed-entry tests cover string, integer, boolean, null, and list values; first/last position; existing/missing removal targets; byte preservation including comments; index/type diagnostics; and absence of a success message.
  • Valid cases cover removing one entry, removing the last entry, a missing name, an empty list, and preservation of remaining entries and unrelated configuration keys. Existing markup-rendering tests also pass.
  • Real CLI checks reject both existing and missing targets in a malformed file without modifying it, and successfully remove only the requested entry from a valid file.
  • uvx ruff@0.15.0 check src tests and git diff --check pass.

All execution above was performed by the agent; no human testing is claimed.

AI Disclosure

  • I did use AI assistance

AI disclosure: Codex (GPT-6 family; exact deployment identifier and reasoning-effort setting unavailable), operating autonomously on behalf of @dlwlgnsrhy after the contributor selected and authorized the scope, investigated and reproduced the issue, authored the implementation and tests, ran validation, reviewed the diff, and drafted the commit message and this PR. No human authorship, testing, or line-by-line review is claimed. The commit carries an Assisted-by: trailer.

Validate each catalog entry before filtering so malformed configuration is
reported without silently discarding entries or rewriting the original file.
Add before/after regressions and valid-removal coverage.

Assisted-by: Codex (model: GPT-6, autonomous)
@dlwlgnsrhy
dlwlgnsrhy marked this pull request as ready for review October 16, 2026 05:23
@dlwlgnsrhy
dlwlgnsrhy requested a review from mnriem as a code owner October 16, 2026 05:23
@mnriem mnriem added the triage-must-have Verdict: high-value, important work for Spec Kit — do first label Oct 16, 2026

This branch has not been deployed

No deployments
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