Skip to content

Commit dac7c25

Browse files
jawwad-aliclaude
andcommitted
fix(bundle): match catalog removal target exactly first, canonical only as fallback
Address Copilot review: canonicalizing the removal target unconditionally could let 'remove <id>' 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) <noreply@anthropic.com>
1 parent e94b3dd commit dac7c25

2 files changed

Lines changed: 35 additions & 12 deletions

File tree

src/specify_cli/bundler/commands_impl/catalog_config.py

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -179,19 +179,19 @@ def remove_source(project_root: Path, id_or_url: str) -> str:
179179
"(add a same-id source to override it instead)."
180180
)
181181

182-
# add_source canonicalizes a local path to an absolute url before storing,
183-
# so match the canonical form too -- otherwise `remove ./cat.json` cannot
184-
# undo `add ./cat.json` (the stored url is absolute). For ids and remote
185-
# urls _canonicalize_url is a no-op, so this only adds the local-path case.
186-
canonical = _canonicalize_url(target)
187182
catalogs = _read(project_root)
188-
remaining = [
189-
c
190-
for c in catalogs
191-
if c.get("id") != target
192-
and c.get("url") != target
193-
and c.get("url") != canonical
194-
]
183+
# Prefer an exact id/url match.
184+
remaining = [c for c in catalogs if c.get("id") != target and c.get("url") != target]
185+
if len(remaining) == len(catalogs):
186+
# No exact match. add_source canonicalizes a local path to an absolute
187+
# url before storing, so fall back to a canonicalized-url match -- this
188+
# lets `remove ./cat.json` undo `add ./cat.json` (stored absolute).
189+
# Only as a *fallback*: _canonicalize_url treats a bare id as a local
190+
# path (empty scheme), so applying it unconditionally could also delete a
191+
# different source whose url equals the id's canonicalized path.
192+
canonical = _canonicalize_url(target)
193+
if canonical != target:
194+
remaining = [c for c in catalogs if c.get("url") != canonical]
195195
if len(remaining) == len(catalogs):
196196
raise BundlerError(
197197
f"No project-scoped catalog source matching '{target}' was found."

tests/unit/test_bundler_catalog_config.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,29 @@ def test_remove_source_accepts_relative_local_path(tmp_path: Path, monkeypatch):
8989
cc.remove_source(project, "sub/cat.json")
9090

9191

92+
def test_remove_by_id_does_not_also_delete_canonical_url_match(tmp_path: Path, monkeypatch):
93+
"""`remove <id>` must remove only the exact-id source, not also a different
94+
source whose url happens to equal the id's canonicalized path. (_canonicalize_url
95+
treats a bare id as a local path, so the canonical match is only a fallback when
96+
there is no exact id/url match.)"""
97+
project = tmp_path / "proj"
98+
(project / ".specify").mkdir(parents=True)
99+
monkeypatch.chdir(project)
100+
# Source A: id "local", a remote url.
101+
cc.add_source(
102+
project, "https://example.com/a.json", source_id="local",
103+
policy="install-allowed", priority=10,
104+
)
105+
# Source B: a local path that canonicalizes to <cwd>/local, with a distinct id.
106+
cc.add_source(project, "local", source_id="bsource", policy="install-allowed", priority=20)
107+
108+
removed = cc.remove_source(project, "local")
109+
assert removed == "local"
110+
ids = {c["id"] for c in cc._read(project)}
111+
assert "local" not in ids # the exact-id source was removed
112+
assert "bsource" in ids # the canonical-url source survives (not collateral)
113+
114+
92115
def test_add_source_refuses_symlinked_specify_escape(tmp_path: Path):
93116
project = tmp_path / "proj"
94117
project.mkdir()

0 commit comments

Comments
 (0)