From e94b3ddf69a0bc805d6221c188a01371a4695516 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Mon, 29 Jun 2026 21:42:09 +0500 Subject: [PATCH 1/2] fix(bundle): allow 'catalog remove' by the same relative path used to 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) --- .../bundler/commands_impl/catalog_config.py | 11 +++++++++- tests/unit/test_bundler_catalog_config.py | 20 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index 477099b7d7..34ee0d458f 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -179,9 +179,18 @@ def remove_source(project_root: Path, id_or_url: str) -> str: "(add a same-id source to override it instead)." ) + # 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) catalogs = _read(project_root) remaining = [ - c for c in catalogs if c.get("id") != target and c.get("url") != target + c + for c in catalogs + if c.get("id") != target + and c.get("url") != target + and c.get("url") != canonical ] if len(remaining) == len(catalogs): raise BundlerError( diff --git a/tests/unit/test_bundler_catalog_config.py b/tests/unit/test_bundler_catalog_config.py index 0ccb219a53..df9176ef3f 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -69,6 +69,26 @@ def test_add_source_persists_absolute_local_path(tmp_path: Path, monkeypatch): assert Path(source.url) == catalog.resolve() +def test_remove_source_accepts_relative_local_path(tmp_path: Path, monkeypatch): + """add_source stores a local path as an absolute url, so remove_source must + accept the same relative path the caller added; otherwise `remove ./cat.json` + cannot undo `add ./cat.json`.""" + project = tmp_path / "proj" + (project / ".specify").mkdir(parents=True) + catalog = project / "sub" / "cat.json" + catalog.parent.mkdir() + catalog.write_text("{}", encoding="utf-8") + monkeypatch.chdir(project) + + cc.add_source(project, "sub/cat.json", policy="install-allowed", priority=50) + # Removing with the same relative path must succeed (stored absolute). + removed = cc.remove_source(project, "sub/cat.json") + assert removed == "sub/cat.json" + # And it is actually gone now. + with pytest.raises(BundlerError, match="No project-scoped catalog source"): + cc.remove_source(project, "sub/cat.json") + + def test_add_source_refuses_symlinked_specify_escape(tmp_path: Path): project = tmp_path / "proj" project.mkdir() From dac7c252e9652ae498948ea8601d24dec65c91af Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 30 Jun 2026 19:56:33 +0500 Subject: [PATCH 2/2] fix(bundle): match catalog removal target exactly first, canonical only as fallback Address Copilot review: canonicalizing the removal target unconditionally could let 'remove ' also delete a different source whose url equals that id's canonicalized path (ids are treated as local paths by _canonicalize_url, empty scheme). Try an exact id/url match first; only fall back to a canonicalized-url match when no exact match is found, so relative-path removal still works without collateral deletion. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../bundler/commands_impl/catalog_config.py | 24 +++++++++---------- tests/unit/test_bundler_catalog_config.py | 23 ++++++++++++++++++ 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index 34ee0d458f..e0650ce4b5 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -179,19 +179,19 @@ def remove_source(project_root: Path, id_or_url: str) -> str: "(add a same-id source to override it instead)." ) - # 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) catalogs = _read(project_root) - remaining = [ - c - for c in catalogs - if c.get("id") != target - and c.get("url") != target - and c.get("url") != canonical - ] + # Prefer an exact id/url match. + remaining = [c for c in catalogs if c.get("id") != target and c.get("url") != target] + if len(remaining) == len(catalogs): + # No exact match. add_source canonicalizes a local path to an absolute + # url before storing, so fall back to a canonicalized-url match -- this + # lets `remove ./cat.json` undo `add ./cat.json` (stored absolute). + # Only as a *fallback*: _canonicalize_url treats a bare id as a local + # path (empty scheme), so applying it unconditionally could also delete a + # different source whose url equals the id's canonicalized path. + canonical = _canonicalize_url(target) + if canonical != target: + remaining = [c for c in catalogs if c.get("url") != canonical] if len(remaining) == len(catalogs): raise BundlerError( f"No project-scoped catalog source matching '{target}' was found." diff --git a/tests/unit/test_bundler_catalog_config.py b/tests/unit/test_bundler_catalog_config.py index df9176ef3f..e2505ec14d 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -89,6 +89,29 @@ def test_remove_source_accepts_relative_local_path(tmp_path: Path, monkeypatch): cc.remove_source(project, "sub/cat.json") +def test_remove_by_id_does_not_also_delete_canonical_url_match(tmp_path: Path, monkeypatch): + """`remove ` must remove only the exact-id source, not also a different + source whose url happens to equal the id's canonicalized path. (_canonicalize_url + treats a bare id as a local path, so the canonical match is only a fallback when + there is no exact id/url match.)""" + project = tmp_path / "proj" + (project / ".specify").mkdir(parents=True) + monkeypatch.chdir(project) + # Source A: id "local", a remote url. + cc.add_source( + project, "https://example.com/a.json", source_id="local", + policy="install-allowed", priority=10, + ) + # Source B: a local path that canonicalizes to /local, with a distinct id. + cc.add_source(project, "local", source_id="bsource", policy="install-allowed", priority=20) + + removed = cc.remove_source(project, "local") + assert removed == "local" + ids = {c["id"] for c in cc._read(project)} + assert "local" not in ids # the exact-id source was removed + assert "bsource" in ids # the canonical-url source survives (not collateral) + + def test_add_source_refuses_symlinked_specify_escape(tmp_path: Path): project = tmp_path / "proj" project.mkdir()