Skip to content

fix(bundle): allow 'catalog remove' by the same relative path used to add#3242

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/bundle-catalog-remove-relative-path
Open

fix(bundle): allow 'catalog remove' by the same relative path used to add#3242
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/bundle-catalog-remove-relative-path

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Description

bundle catalog add canonicalizes a local catalog path to an absolute url before persisting it (add_source_canonicalize_url). But remove_source compared only the raw input against the stored id/url:

remaining = [c for c in catalogs if c.get("id") != target and c.get("url") != target]

So bundle catalog remove ./cat.json could not undo bundle catalog add ./cat.json: the stored url is absolute, the removal target is the relative string, they never match, and the command fails with "No project-scoped catalog source matching … found" even though the source is present.

Fix

In remove_source, also match the canonicalized form of the target (_canonicalize_url is a no-op for ids and remote urls, so this only adds the local-path case). A local source is now removable by the same path it was added with.

Testing

  • uvx ruff check clean.
  • New test_remove_source_accepts_relative_local_path: add sub/cat.json, remove sub/cat.json → succeeds, and a second remove raises the not-found error. Fails before this change, passes after.
  • Verified all paths still work: removal by id, by absolute url, and the not-found error for an unknown source. (The one failing test in this file, test_add_source_refuses_symlinked_specify_escape, is an unrelated pre-existing Windows symlink-privilege skip; it passes on Linux CI and is in the untouched add_source path.)

AI Disclosure

  • I did use AI assistance (describe below)

Found and fixed with Claude Code (Claude Opus 4.8) under my direction. AI spotted the add/remove canonicalization asymmetry; I reproduced the failed removal, confirmed id/absolute-url/not-found paths are unaffected, and reviewed the diff before submitting.

… add

add_source canonicalizes a local catalog path to an absolute url before persisting it, but remove_source compared only the raw input against the stored id/url. So 'bundle catalog remove ./cat.json' could not undo 'bundle catalog add ./cat.json' -- the stored url was absolute, the removal target relative, and they never matched ('No project-scoped catalog source found'). Match the canonicalized form too (a no-op for ids and remote urls), so a local source is removable by the same path it was added with.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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

This PR fixes an asymmetry in the bundler catalog config logic so that a project-scoped local catalog added via a relative path can also be removed using that same relative path (by matching the canonicalized absolute form stored in config). It also adds a unit test to prevent regressions.

Changes:

  • Update remove_source() to also compare against a canonicalized form of the removal target for local-path inputs.
  • Add a unit test covering add/remove using a relative local path and verifying the not-found behavior on a second removal attempt.
Show a summary per file
File Description
tests/unit/test_bundler_catalog_config.py Adds coverage for removing a local catalog source using the same relative path it was added with.
src/specify_cli/bundler/commands_impl/catalog_config.py Extends removal matching to include canonicalized local paths so stored absolute paths can be removed via relative inputs.

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: 1
  • Review effort level: Low

Comment on lines +182 to +186
# add_source canonicalizes a local path to an absolute url before storing,
# so match the canonical form too -- otherwise `remove ./cat.json` cannot
# undo `add ./cat.json` (the stored url is absolute). For ids and remote
# urls _canonicalize_url is a no-op, so this only adds the local-path case.
canonical = _canonicalize_url(target)

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

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