From 63b69c4ae9683451a26a15cbe214d50777c8bdfe Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 15 Aug 2026 19:44:59 +0500 Subject: [PATCH 1/2] fix(workflows): refuse `overlay add` that would overwrite a different overlay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Overlay identity in this module is the manifest `id`, not the filename — `_find_overlay_file`'s own docstring says so, and a whole test class asserts it. So `.specify/workflows/overlays/wf/lint.yml` can legitimately contain `id: format`. When `_find_overlay_file` finds no file carrying the incoming overlay's id, the fallback target is derived purely from the filename and committed onto unconditionally, with no check for who already lives there: BEFORE: lint.yml holds id='format' add exit: 0 AFTER : lint.yml holds id='lint' -> the format overlay is GONE backups left: none The loss is permanent: `_commit_workflow_file` renames the victim to a `.bak` and the success path then discards that backup. Exit code 0, no warning — a silent loss of exactly the project-local customization overlays exist to protect. Refuse instead. The guard fires only for a *different* id, so updating an overlay in place and creating a new file both still work. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/overlays/_commands.py | 20 +++++ tests/workflows/test_overlay_commands.py | 88 +++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index 549f1ea151..7cbcfde024 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -207,6 +207,26 @@ def workflow_overlay_add( target_path = _ensure_contained_path( target_dir / f"{overlay.id}.yml", _overlay_root(project_root) ) + # Overlay identity is the manifest ``id``, not the filename (see + # ``_find_overlay_file``), so ``.yml`` can legitimately already hold + # a DIFFERENT overlay. Committing onto it would destroy that overlay + # permanently -- the commit renames the victim to a ``.bak`` and the + # success path then discards that backup -- while reporting success. + if target_path.is_file(): + occupant, _ = _read_overlay(target_path) + occupant_id = occupant.get("id") if isinstance(occupant, dict) else None + if ( + isinstance(occupant_id, str) + and occupant_id + and occupant_id != overlay.id + ): + err_console.print( + f"[red]Error:[/red] {_escape_markup(str(target_path))} already " + f"holds overlay {_escape_markup(repr(occupant_id))}. Rename or " + f"remove it before adding overlay " + f"{_escape_markup(repr(overlay.id))}." + ) + return None backup: Path | None = None try: diff --git a/tests/workflows/test_overlay_commands.py b/tests/workflows/test_overlay_commands.py index 8a344cacdf..ec324f9f96 100644 --- a/tests/workflows/test_overlay_commands.py +++ b/tests/workflows/test_overlay_commands.py @@ -893,3 +893,91 @@ def test_duplicate_manifest_id_is_rejected(self, project_dir, monkeypatch): with pytest.raises(typer.Exit): _find_overlay_file(project_dir, "wf", "lint") + + +class TestOverlayAddDoesNotClobber: + """`overlay add` must not destroy a different overlay sitting at .yml. + + Overlay identity is the manifest `id`, not the filename (see + `_find_overlay_file`), so `lint.yml` can legitimately contain + `id: format`. When `_find_overlay_file` found no file carrying the new + overlay's id, the fallback target was derived purely from the filename and + committed onto unconditionally — permanently destroying the occupant, since + the commit renames it to a `.bak` and the success path then discards that + backup. Exit code 0, no warning. + """ + + def _setup(self, project_dir: Path, occupant_id: str | None) -> tuple[Path, Path]: + _write_workflow( + project_dir, + "wf", + { + "schema_version": "1.0", + "workflow": {"id": "wf", "name": "WF", "version": "1.0.0"}, + "steps": [{"id": "a", "type": "command", "command": "echo"}], + }, + ) + ov_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf" + ov_dir.mkdir(parents=True, exist_ok=True) + if occupant_id is not None: + (ov_dir / "lint.yml").write_text( + yaml.safe_dump( + { + "id": occupant_id, + "extends": "wf", + "priority": 3, + "edits": [{"remove": "a"}], + } + ), + encoding="utf-8", + ) + incoming = project_dir / "incoming.yml" + incoming.write_text( + yaml.safe_dump( + { + "id": "lint", + "extends": "wf", + "priority": 10, + "edits": [{"remove": "a"}], + } + ), + encoding="utf-8", + ) + return ov_dir, incoming + + def test_add_does_not_clobber_a_different_overlay(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + ov_dir, incoming = self._setup(project_dir, occupant_id="format") + + result = runner.invoke(app, ["workflow", "overlay", "add", str(incoming)]) + + assert result.exit_code == 1, result.output + # The victim must be untouched, and no backup left lying around. + survivor = yaml.safe_load((ov_dir / "lint.yml").read_text(encoding="utf-8")) + assert survivor["id"] == "format", survivor + assert survivor["priority"] == 3, survivor + assert [p.name for p in ov_dir.iterdir() if "bak" in p.name] == [] + + def test_add_still_updates_the_same_overlay_in_place( + self, project_dir, monkeypatch + ): + """The guard must only fire for a *different* overlay id.""" + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + ov_dir, incoming = self._setup(project_dir, occupant_id="lint") + + result = runner.invoke(app, ["workflow", "overlay", "add", str(incoming)]) + + assert result.exit_code == 0, result.output + updated = yaml.safe_load((ov_dir / "lint.yml").read_text(encoding="utf-8")) + assert updated["id"] == "lint" + assert updated["priority"] == 10 + + def test_add_creates_the_file_when_absent(self, project_dir, monkeypatch): + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + ov_dir, incoming = self._setup(project_dir, occupant_id=None) + + result = runner.invoke(app, ["workflow", "overlay", "add", str(incoming)]) + + assert result.exit_code == 0, result.output + created = yaml.safe_load((ov_dir / "lint.yml").read_text(encoding="utf-8")) + assert created["id"] == "lint" From 36bcd53d128d8e2f7a774677f74cd174c6afdd2d Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sun, 30 Aug 2026 21:28:57 +0500 Subject: [PATCH 2/2] fix(workflows): fail closed when the overlay occupant cannot be identified Addresses review feedback: the guard only refused an occupant with a *different valid* id, so every file whose identity could not be established still fell through to `_commit_workflow_file`. That gap is reachable, not theoretical. `_find_overlay_file` matches on the manifest `id` and skips exactly those files -- `_read_overlay` returns None for unreadable, malformed and non-mapping input, and a mapping may carry a missing or non-string `id`. Skipping them is what routes execution to the filename-derived target in the first place, so the old guard let the user's file be discarded just as permanently as a valid overlay, without even being able to name what was lost. Now refuses unless the occupant is provably the same overlay, and uses the errors `_read_overlay` already returns to explain why: * "already holds overlay 'format'" (different valid id) * "could not be parsed as an overlay (Invalid YAML ...)" (read/parse failure) * "is not a readable overlay manifest (no usable 'id')" (missing/non-string) New parametrized regression test covers all five shapes -- malformed, sequence, scalar, missing_id, non_string_id -- and asserts the occupant survives byte-for-byte with no backup left behind. Mutation-tested: restoring the previous guard fails all five, so they pin this specific hole rather than the original clobber bug. The same-id update path and the create-when-absent path are unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/overlays/_commands.py | 29 ++++++++++----- tests/workflows/test_overlay_commands.py | 36 +++++++++++++++++++ 2 files changed, 56 insertions(+), 9 deletions(-) diff --git a/src/specify_cli/workflows/overlays/_commands.py b/src/specify_cli/workflows/overlays/_commands.py index 7cbcfde024..1bc8e83bae 100644 --- a/src/specify_cli/workflows/overlays/_commands.py +++ b/src/specify_cli/workflows/overlays/_commands.py @@ -213,17 +213,28 @@ def workflow_overlay_add( # permanently -- the commit renames the victim to a ``.bak`` and the # success path then discards that backup -- while reporting success. if target_path.is_file(): - occupant, _ = _read_overlay(target_path) + # Fail closed: refuse unless the occupant is provably this same + # overlay. Reaching here means ``_find_overlay_file`` did not match + # this path, and it skips exactly the files whose identity cannot be + # established -- unreadable, malformed, non-mapping, or missing a + # usable ``id``. Letting those through would destroy the user's file + # just as permanently as overwriting a valid one, only without even + # being able to name what was lost. + occupant, read_errors = _read_overlay(target_path) occupant_id = occupant.get("id") if isinstance(occupant, dict) else None - if ( - isinstance(occupant_id, str) - and occupant_id - and occupant_id != overlay.id - ): + if not (isinstance(occupant_id, str) and occupant_id == overlay.id): + if isinstance(occupant_id, str) and occupant_id: + detail = f"already holds overlay {_escape_markup(repr(occupant_id))}" + elif read_errors: + detail = ( + "could not be parsed as an overlay " + f"({_escape_markup('; '.join(read_errors))})" + ) + else: + detail = "is not a readable overlay manifest (no usable 'id')" err_console.print( - f"[red]Error:[/red] {_escape_markup(str(target_path))} already " - f"holds overlay {_escape_markup(repr(occupant_id))}. Rename or " - f"remove it before adding overlay " + f"[red]Error:[/red] {_escape_markup(str(target_path))} {detail}. " + f"Rename or remove it before adding overlay " f"{_escape_markup(repr(overlay.id))}." ) return None diff --git a/tests/workflows/test_overlay_commands.py b/tests/workflows/test_overlay_commands.py index ec324f9f96..9ab10962cc 100644 --- a/tests/workflows/test_overlay_commands.py +++ b/tests/workflows/test_overlay_commands.py @@ -972,6 +972,42 @@ def test_add_still_updates_the_same_overlay_in_place( assert updated["id"] == "lint" assert updated["priority"] == 10 + @pytest.mark.parametrize( + "raw", + [ + "id: [1, 2\n bad: yaml:\n", + "- just\n- a\n- sequence\n", + "just a scalar\n", + "extends: wf\npriority: 3\n", + "id: 5\nextends: wf\npriority: 3\n", + ], + ids=["malformed", "sequence", "scalar", "missing_id", "non_string_id"], + ) + def test_add_fails_closed_when_the_occupant_cannot_be_identified( + self, project_dir, monkeypatch, raw + ): + """An unidentifiable occupant must be refused, not silently destroyed. + + `_find_overlay_file` matches on the manifest `id` and skips exactly the + files whose identity cannot be established — unreadable, malformed, + non-mapping, or missing a usable `id`. Those therefore fall through to + the filename-derived target, so a guard that only refuses a *different + valid* id would still let `_commit_workflow_file` discard the user's + file. It is destroyed just as permanently as a valid overlay, only + without even being able to name what was lost. + """ + monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) + ov_dir, incoming = self._setup(project_dir, occupant_id=None) + occupant = ov_dir / "lint.yml" + occupant.write_text(raw, encoding="utf-8") + + result = runner.invoke(app, ["workflow", "overlay", "add", str(incoming)]) + + assert result.exit_code == 1, result.output + # Byte-for-byte survival, and no backup left behind. + assert occupant.read_text(encoding="utf-8") == raw + assert [p.name for p in ov_dir.iterdir() if "bak" in p.name] == [] + def test_add_creates_the_file_when_absent(self, project_dir, monkeypatch): monkeypatch.setattr("specify_cli._require_specify_project", lambda: project_dir) ov_dir, incoming = self._setup(project_dir, occupant_id=None)