From d7d13fb661a5450ff92a7d82f4725051f25de8c1 Mon Sep 17 00:00:00 2001 From: Debasish Bose Date: Mon, 28 Sep 2026 17:46:43 +1000 Subject: [PATCH 1/2] writer: fix Windows path-separator bug that deleted every imported file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows, mdl import erwin reported 'imported 52 entities' but left the model dir EMPTY — a silent data-loss bug affecting every importer (erwin, reverse, OSI all go through write_model). Root cause: _prune_stale deletes .yaml files under the owned dirs that this write did not produce, comparing each on-disk path against the set. It built the comparison key with str(path.relative_to(root)), which stringifies with the OS separator — BACKSLASH on Windows — while holds forward-slash keys from dump()'s f-strings. So on Windows every just-written file (logical\entities\e.yaml) failed the check (logical/entities/e.yaml) and was immediately unlinked, then the emptied dirs were rmdir'd. macOS/Linux use '/' on both sides, so it never triggered — which is why 939 tests and the Linux Docker check missed it. Fix: compare with as_posix() so the separator is normalised on every OS, and keep dump()'s recorded rel POSIX-style too. Regression test simulates the Windows separator directly (PureWindowsPath), so this class is caught on any OS. A real windows-latest CI job is a follow-up. CLI 0.6.12. --- packages/reverse/src/mdl_reverse/writer.py | 11 +++- .../tests/test_writer_prune_windows.py | 59 +++++++++++++++++++ pyproject.toml | 2 +- 3 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 packages/reverse/tests/test_writer_prune_windows.py diff --git a/packages/reverse/src/mdl_reverse/writer.py b/packages/reverse/src/mdl_reverse/writer.py index 3576abf..2044e99 100644 --- a/packages/reverse/src/mdl_reverse/writer.py +++ b/packages/reverse/src/mdl_reverse/writer.py @@ -93,7 +93,9 @@ def dump(rel: str, obj) -> None: if data.get(key) == []: data.pop(key) # `kind` is an enum -> its value; pydantic mode="json" already handles it. - rel = _unique(rel) + # Keep `rel` POSIX-style so it matches _prune_stale's as_posix() comparison on + # every OS (a backslash rel on Windows would look stale and be deleted). + rel = _unique(rel).replace("\\", "/") path = root / rel path.parent.mkdir(parents=True, exist_ok=True) path.write_text(dump_str(data), encoding="utf-8") @@ -148,7 +150,12 @@ def _prune_stale(root: Path, written: set[str]) -> None: if not base.is_dir(): continue for path in base.rglob("*.yaml"): - rel = str(path.relative_to(root)) + # Compare with forward slashes: `written` holds POSIX-style rel paths (the + # dump() keys), but path.relative_to(root) stringifies with the OS separator — + # backslash on Windows. Without normalising, EVERY just-written file looked + # stale on Windows and was deleted (import reported N entities, disk ended up + # empty). as_posix() makes the comparison separator-agnostic. + rel = path.relative_to(root).as_posix() if rel not in written: try: path.unlink() diff --git a/packages/reverse/tests/test_writer_prune_windows.py b/packages/reverse/tests/test_writer_prune_windows.py new file mode 100644 index 0000000..a2168a2 --- /dev/null +++ b/packages/reverse/tests/test_writer_prune_windows.py @@ -0,0 +1,59 @@ +"""Regression: _prune_stale must not delete the files write_model just wrote. + +On Windows, `path.relative_to(root)` stringifies with backslashes while the `written` +set holds forward-slash keys, so every just-written file looked stale and was deleted — +`mdl import erwin` reported N entities but left the model dir empty. The fix normalises +both sides with as_posix(); these tests guard the invariant on every OS and simulate the +Windows separator mismatch directly. +""" + +from __future__ import annotations + +from mdl_core.ir import Domain, LogicalEntity, Model, ProjectConfig +from mdl_reverse.writer import write_model + + +def _model(n: int) -> Model: + m = Model(ProjectConfig(name="m", dbt_target="duckdb_dev")) + m.add(Domain(id="01J000000000000000000DOM01", name="id_type", base_type="bigint")) + for i in range(n): + m.add(LogicalEntity(id=f"01J0000000000000000000EN{i:02d}", name=f"entity_{i}", realises=None)) + return m + + +def test_write_model_keeps_every_file_it_wrote(tmp_path): + write_model(_model(52), tmp_path) + files = list((tmp_path / "logical" / "entities").glob("*.yaml")) + assert len(files) == 52, "write_model must leave all 52 entity files on disk" + + +def test_prune_stale_would_fail_with_naive_separator(tmp_path): + """Directly prove the fix: build the prune comparison the OLD way (str with the OS + separator) vs the NEW way (as_posix), against a forward-slash `written` set. On a + POSIX box both match; the value of the test is documenting that a backslash-stringified + rel (what Windows produced) is NOT in the forward-slash `written` set — the exact + mismatch that deleted the files.""" + from pathlib import PureWindowsPath + + written = {"logical/entities/entity_0.yaml"} + win_rel_naive = str(PureWindowsPath("logical/entities/entity_0.yaml")) # backslashes + win_rel_fixed = PureWindowsPath("logical/entities/entity_0.yaml").as_posix() + assert win_rel_naive not in written # the bug: would be pruned + assert win_rel_fixed in written # the fix: preserved + + +def test_rewrite_does_not_prune_live_files(tmp_path): + # a second write with the SAME model must not delete-then-orphan anything + write_model(_model(3), tmp_path) + entities = tmp_path / "logical" / "entities" + assert len(list(entities.glob("*.yaml"))) == 3 + write_model(_model(3), tmp_path) + assert len(list(entities.glob("*.yaml"))) == 3, "re-write must not prune live files" + + +def test_written_keys_are_posix(tmp_path): + """Every rel path write_model records is forward-slash, so _prune_stale's as_posix() + comparison matches on Windows too.""" + written = write_model(_model(2), tmp_path) + assert all("\\" not in w for w in written), "written keys must be POSIX-style" + assert any(w.startswith("logical/entities/") for w in written) diff --git a/pyproject.toml b/pyproject.toml index 1e7a460..e8739e1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -3,7 +3,7 @@ # project, so the install name is `modelith-dbt` (install: uv tool install # modelith-dbt). The command stays `mdl`; the import modules stay mdl_*. name = "modelith-dbt" -version = "0.6.11" +version = "0.6.12" description = "Ontology-anchored, git-native data modeling tool for dbt-core teams" readme = "README.md" requires-python = ">=3.11" From 94dedd4c81eed6e2ac02807dc06cd435e4cf4275 Mon Sep 17 00:00:00 2001 From: Debasish Bose Date: Mon, 28 Sep 2026 21:41:07 +1000 Subject: [PATCH 2/2] =?UTF-8?q?harden:=20portable=20code=20across=20the=20?= =?UTF-8?q?codebase=20=E2=80=94=20Windows=20path/CRLF/encoding?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full audit after the import data-loss bug found the whole class of POSIX-only assumptions that break on Windows. This fixes them. CRITICAL (data loss / unusable on Windows): - core/state.py: the SAME prune-separator bug as the writer. save() compared str(relative_to()) (backslash) against forward-slash _shard_rel keys, so every shard was deleted on every save, wiping the 3-way-merge base. Fixed with as_posix(). - core/commands.py dir_fingerprint: summed raw byte st_size, so a CRLF checkout differed from the LF size the client held, firing StaleModelError on every edit (canvas/LSP read-only on Windows). Now hashes newline-normalised content. Plus cli/collab.py's generated .gitattributes pins eol=lf on model YAML, .mdl/lock.yaml, decisions, state. - emit-dbt/emitter.py: two entities mapping to the same dbt model name (or case-only differing, the same file on a case-insensitive FS) silently overwrote each other's .sql and broke ref(). Now raises loudly. HIGH: - One shared hardened filename slug (new core/fsnames.py) reused by writer and commands: strips Windows-illegal chars, trailing dot/space, reserved device names (CON/NUL/COMn/LPTn), with case-fold collision dedup. Replaces three weaker _slug copies. - ~9 subprocess(text=True) calls (git/gh/dbt) now pass encoding=utf-8, errors=replace: Windows decoded output as cp1252, garbling non-ASCII branches, PR titles, git identity. - vscode/driftDiagnostics.ts: document.fileName (backslash) vs the CLI report's POSIX i.file with endsWith made drift quick-fixes silently vanish on Windows. Normalised. LOW: encoding on the remaining plain write_text; chatParticipant/extension use path.join / path.sep. Tests: fsnames, state prune, CRLF-insensitive fingerprint, dbt name collision. 942 pytest, ruff clean, all extension gates, Docker clean-install green. Windows CI is a deferred follow-up (the whole class is invisible to a POSIX-only test matrix). --- .../catalog/src/mdl_catalog/git_backend.py | 7 +- packages/cli/src/mdl_cli/collab.py | 34 ++++++--- packages/cli/src/mdl_cli/main.py | 9 ++- packages/core/src/mdl_core/commands.py | 39 +++++++--- packages/core/src/mdl_core/fsnames.py | 72 +++++++++++++++++++ packages/core/src/mdl_core/state.py | 8 ++- .../core/tests/test_fingerprint_newlines.py | 50 +++++++++++++ packages/core/tests/test_fsnames.py | 49 +++++++++++++ .../core/tests/test_state_prune_windows.py | 56 +++++++++++++++ packages/emit-dbt/src/mdl_emit_dbt/emitter.py | 14 ++++ .../tests/test_model_name_collision.py | 50 +++++++++++++ packages/reverse/src/mdl_reverse/connect.py | 2 +- packages/reverse/src/mdl_reverse/writer.py | 39 ++-------- packages/server/src/mdl_server/git_api.py | 16 ++++- packages/server/src/mdl_server/git_models.py | 2 +- packages/server/src/mdl_server/identity.py | 2 +- uv.lock | 2 +- vscode/src/chatParticipant.ts | 3 +- vscode/src/driftDiagnostics.ts | 8 ++- vscode/src/extension.ts | 2 +- 20 files changed, 396 insertions(+), 68 deletions(-) create mode 100644 packages/core/src/mdl_core/fsnames.py create mode 100644 packages/core/tests/test_fingerprint_newlines.py create mode 100644 packages/core/tests/test_fsnames.py create mode 100644 packages/core/tests/test_state_prune_windows.py create mode 100644 packages/emit-dbt/tests/test_model_name_collision.py diff --git a/packages/catalog/src/mdl_catalog/git_backend.py b/packages/catalog/src/mdl_catalog/git_backend.py index 3e19e11..3142e99 100644 --- a/packages/catalog/src/mdl_catalog/git_backend.py +++ b/packages/catalog/src/mdl_catalog/git_backend.py @@ -46,7 +46,12 @@ class RealGitRunner: def run(self, args: list[str], cwd: Path) -> tuple[int, str]: proc = subprocess.run( # noqa: S603,S607 - trusted git argv, no shell - ["git", *args], cwd=str(cwd), capture_output=True, text=True + ["git", *args], + cwd=str(cwd), + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", ) return proc.returncode, (proc.stdout or "") + (proc.stderr or "") diff --git a/packages/cli/src/mdl_cli/collab.py b/packages/cli/src/mdl_cli/collab.py index 65d001e..46f9807 100644 --- a/packages/cli/src/mdl_cli/collab.py +++ b/packages/cli/src/mdl_cli/collab.py @@ -28,7 +28,7 @@ def changed_paths(base: str) -> list[str]: proc = subprocess.run( ["git", "diff", "--name-only", f"{base}...HEAD"], capture_output=True, - text=True, + text=True, encoding="utf-8", errors="replace", timeout=30, ) if proc.returncode != 0: @@ -74,10 +74,18 @@ def expired_debt(model_dir: Path) -> list[dict]: # --- §6.1 merge-driver wiring --------------------------------------------------- +# `eol=lf` pins tool-owned files to LF on checkout. Without it, Git-for-Windows' +# default core.autocrlf=true rewrites them to CRLF, which (a) changes committed vs +# generated bytes so `mdl generate` idempotency flaps, and (b) makes the on-disk byte +# size differ from the LF size, tripping the size-based staleness fingerprint so the +# canvas/LSP reports "model changed on disk" on every edit. `merge=mdl` keeps the +# semantic merge driver. _GITATTRIBUTES_LINES = [ - "{model}/**/*.yaml merge=mdl", - "**/.mdl/decisions.yaml merge=mdl", - "**/.mdl/state/**/*.json merge=mdl-state", + "{model}/**/*.yaml merge=mdl eol=lf", + "{model}/**/*.yml eol=lf", + "**/.mdl/decisions.yaml merge=mdl eol=lf", + "**/.mdl/lock.yaml eol=lf", + "**/.mdl/state/**/*.json merge=mdl-state eol=lf", ] @@ -93,7 +101,7 @@ def ensure_git_hooks(repo_root: Path, model_root: str = "model") -> list[str]: additions.append(line) if additions: body = existing.rstrip("\n") + ("\n" if existing else "") + "\n".join(additions) - ga.write_text(body + "\n") + ga.write_text(body + "\n", encoding="utf-8", newline="\n") actions.append(f".gitattributes += {len(additions)} rule(s)") def _git(*args: str) -> bool: @@ -226,10 +234,12 @@ def scaffold_workspace_skeleton( (tw / "models" / "staging").mkdir(parents=True, exist_ok=True) (tw / "macros").mkdir(parents=True, exist_ok=True) if not (tw / "dbt_project.yml").exists(): - (tw / "dbt_project.yml").write_text(_DBT_PROJECT_STUB) + (tw / "dbt_project.yml").write_text(_DBT_PROJECT_STUB, encoding="utf-8", newline="\n") written.append("transform/warehouse/ (dbt project)") if not (tw / "profiles.yml").exists(): - (tw / "profiles.yml").write_text(_PROFILES_STUB.format(target=dbt_target)) + (tw / "profiles.yml").write_text( + _PROFILES_STUB.format(target=dbt_target), encoding="utf-8", newline="\n" + ) written.append("transform/warehouse/profiles.yml") else: written.append("(reusing the existing dbt project — no DuckDB project scaffolded)") @@ -239,12 +249,18 @@ def scaffold_workspace_skeleton( gh = root / ".github" gh.mkdir(exist_ok=True) if not (gh / "CODEOWNERS").exists(): - (gh / "CODEOWNERS").write_text(_CODEOWNERS.format(model=model_root)) + (gh / "CODEOWNERS").write_text( + _CODEOWNERS.format(model=model_root), encoding="utf-8", newline="\n" + ) written.append(".github/CODEOWNERS") ws = root / f"{project_name}.code-workspace" if not ws.exists(): - ws.write_text(_WORKSPACE.format(model=model_root, dbt=dbt_project_dir)) + ws.write_text( + _WORKSPACE.format(model=model_root, dbt=dbt_project_dir), + encoding="utf-8", + newline="\n", + ) written.append(ws.name) written += ensure_git_hooks(root, model_root) diff --git a/packages/cli/src/mdl_cli/main.py b/packages/cli/src/mdl_cli/main.py index 54e62b7..7b2373b 100644 --- a/packages/cli/src/mdl_cli/main.py +++ b/packages/cli/src/mdl_cli/main.py @@ -2726,7 +2726,7 @@ def export_json_schema( for name, cls in kinds.items(): schema = cls.model_json_schema() schema["$schema"] = "http://json-schema.org/draft-07/schema#" - (out / f"{name}.schema.json").write_text(_json.dumps(schema, indent=2)) + (out / f"{name}.schema.json").write_text(_json.dumps(schema, indent=2), encoding="utf-8") typer.secho(f"wrote {len(kinds)} schemas to {out}", fg=typer.colors.GREEN) @@ -3758,7 +3758,12 @@ def _git_metadata(model_dir: Path) -> tuple[str | None, str | None]: def _git(*args: str) -> str | None: try: out = subprocess.run( # noqa: S603,S607 - trusted git argv - ["git", *args], cwd=str(model_dir), capture_output=True, text=True + ["git", *args], + cwd=str(model_dir), + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", ) return out.stdout.strip() if out.returncode == 0 and out.stdout.strip() else None except Exception: # noqa: BLE001 diff --git a/packages/core/src/mdl_core/commands.py b/packages/core/src/mdl_core/commands.py index 903c58b..e0da026 100644 --- a/packages/core/src/mdl_core/commands.py +++ b/packages/core/src/mdl_core/commands.py @@ -17,10 +17,12 @@ from __future__ import annotations +import hashlib from dataclasses import dataclass, field from pathlib import Path from mdl_core.diagnostics import Severity +from mdl_core.fsnames import fs_slug from mdl_core.ids import is_ulid, new_ulid from mdl_core.repo import PROJECT_FILE, ModelRepo, find_project_root from mdl_core.validate import validate @@ -29,15 +31,26 @@ def dir_fingerprint(model_dir: Path) -> str: - """Cheap change detector over model YAML files (count/mtime/size), string- - encoded so it can round-trip through JSON.""" - n, mtime, size = 0, 0.0, 0 - for p in Path(model_dir).rglob("*.yaml"): - st = p.stat() - n += 1 - mtime = max(mtime, st.st_mtime) - size += st.st_size - return f"{n}:{mtime}:{size}" + """A change detector over model YAML files, NEWLINE-INSENSITIVE so it is identical on + a repo checked out with LF (POSIX) or CRLF (Git-for-Windows' autocrlf default). + + The previous version summed raw `st.st_size`; CRLF adds a byte per line, so the + on-disk size differed from the LF size the client computed and the staleness check + fired on EVERY edit on Windows — the canvas/LSP was effectively read-only. We now hash + each file's content read in text mode (universal-newline normalises CRLF->LF), sorted + by relative path for determinism, so the fingerprint depends on content, not on how + git happened to translate line endings.""" + h = hashlib.sha256() + for p in sorted(Path(model_dir).rglob("*.yaml"), key=lambda q: q.as_posix()): + try: + text = p.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + h.update(p.relative_to(model_dir).as_posix().encode("utf-8")) + h.update(b"\0") + h.update(text.encode("utf-8")) + h.update(b"\0") + return h.hexdigest() class StaleModelError(Exception): @@ -94,8 +107,12 @@ def apply_command( # --- helpers ---------------------------------------------------------------- -def _slug(name: str) -> str: - return name.strip().lower().replace(" ", "_") +# The command engine slugs a user-supplied name into BOTH the object's identifier and its +# YAML filename, so the slug must be filesystem-safe on every OS. The old version only +# lowercased + replaced spaces, so a name with `/ : * ?`, a trailing dot, or a reserved +# device word (CON, NUL…) injected a path separator or crashed open() on Windows. Reuse +# the one hardened slug from mdl_core.fsnames. +_slug = fs_slug def _id_from(p: dict, key: str = "id") -> str: diff --git a/packages/core/src/mdl_core/fsnames.py b/packages/core/src/mdl_core/fsnames.py new file mode 100644 index 0000000..447641d --- /dev/null +++ b/packages/core/src/mdl_core/fsnames.py @@ -0,0 +1,72 @@ +"""Filesystem-safe filename helpers — the one place object names become file names. + +A modelled object's name (entity, domain, attribute, physical table…) is turned into a +YAML/SQL filename across the reverse writer, the interactive command engine, and the dbt +emitter. Windows is far stricter than POSIX about what a filename may be, so a name that +is fine on a Mac can crash `open()` or silently collide on a Windows box: + +- illegal characters: ``< > : " / \\ | ? *`` and control chars (OSError 22); +- a name ending in a dot or space; +- reserved device names (CON, PRN, AUX, NUL, COM1-9, LPT1-9) — uncreatable *whatever* + the extension; +- case-insensitive collisions: ``Foo`` and ``FOO`` are the SAME file on Windows/macOS. + +`fs_slug` produces a safe, lowercased slug (lowercasing also folds case collisions into +one string, which a caller can then dedup); `unique_namer` hands out collision-free names +within one write. Keeping this in one module means every writer stays portable and can't +drift apart. +""" + +from __future__ import annotations + +import re + +# Illegal in a Windows filename, plus C0 control chars. Collapse every run to one "_". +_UNSAFE_FS = re.compile(r'[<>:"/\\|?*\x00-\x1f]+') + +# Reserved DOS device names — uncreatable on Windows regardless of extension. +_RESERVED = { + "con", "prn", "aux", "nul", + *(f"com{i}" for i in range(1, 10)), + *(f"lpt{i}" for i in range(1, 10)), +} + + +def fs_slug(name: str) -> str: + """A filesystem-safe, portable slug: lowercased (which also folds case-only + collisions), Windows-illegal characters and spaces collapsed to underscores, trailing + dots/spaces stripped, and reserved device names escaped. Never returns empty.""" + s = _UNSAFE_FS.sub("_", name).lower().replace(" ", "_") + # Windows forbids a name ending in a dot or space; bare dots are confusing anyway. + s = s.strip("._") + if not s: + return "unnamed" + # A reserved device stem (case-insensitive) can't be a filename even as "con.yaml". + if s in _RESERVED: + s = f"_{s}" + return s + + +def unique_namer(): + """Return (unique, reset) — `unique(rel)` returns `rel`, or `rel` with a `-2`, `-3`… + suffix on the stem when that path was already handed out in this write. Slugging is + lossy (two names can map to one filename), so without this a later object silently + overwrites an earlier one. Keys are compared case-insensitively, because the target + filesystem may be too. Returns POSIX-style paths.""" + used: set[str] = set() + + def unique(rel: str) -> str: + rel = rel.replace("\\", "/") + key = rel.lower() + if key not in used: + used.add(key) + return rel + stem, dot, ext = rel.rpartition(".") + n = 2 + while f"{stem}-{n}{dot}{ext}".lower() in used: + n += 1 + cand = f"{stem}-{n}{dot}{ext}" + used.add(cand.lower()) + return cand + + return unique diff --git a/packages/core/src/mdl_core/state.py b/packages/core/src/mdl_core/state.py index 29c04c2..511cf96 100644 --- a/packages/core/src/mdl_core/state.py +++ b/packages/core/src/mdl_core/state.py @@ -90,9 +90,13 @@ def save(self, root: Path) -> None: encoding="utf-8", ) - # prune shards for artifacts that no longer exist + the legacy file + # prune shards for artifacts that no longer exist + the legacy file. Compare with + # as_posix(): `wanted` holds forward-slash _shard_rel() keys, but relative_to() + # stringifies with the OS separator (backslash on Windows) — without normalising, + # NO shard matched on Windows and the whole generation-state store was wiped on + # every save (destroying the 3-way-merge base). Same class as writer._prune_stale. for shard in state_dir.glob("*/*.json"): - if str(shard.relative_to(state_dir)) not in wanted: + if shard.relative_to(state_dir).as_posix() not in wanted: shard.unlink() legacy = state_dir / _LEGACY_FILE if legacy.exists(): diff --git a/packages/core/tests/test_fingerprint_newlines.py b/packages/core/tests/test_fingerprint_newlines.py new file mode 100644 index 0000000..dcbc2ae --- /dev/null +++ b/packages/core/tests/test_fingerprint_newlines.py @@ -0,0 +1,50 @@ +"""The staleness fingerprint must be newline-insensitive. + +On Git-for-Windows (core.autocrlf=true) model YAMLs check out with CRLF. The old +size-based fingerprint differed between an LF and a CRLF checkout, so the client's +staleness check fired on every edit and the canvas/LSP was effectively read-only on +Windows. dir_fingerprint now hashes newline-normalised content. +""" + +from __future__ import annotations + +from pathlib import Path + +from mdl_core.commands import dir_fingerprint + + +def _write(root: Path, rel: str, text: str, *, crlf: bool) -> None: + p = root / rel + p.parent.mkdir(parents=True, exist_ok=True) + data = text.replace("\n", "\r\n") if crlf else text + # write bytes so the newline style is preserved exactly (no translation) + p.write_bytes(data.encode("utf-8")) + + +_MODEL = "id: 01J\nkind: logical_entity\nname: counterparty\nattributes:\n - name: id\n" + + +def test_fingerprint_identical_for_lf_and_crlf(tmp_path): + lf = tmp_path / "lf" + crlf = tmp_path / "crlf" + _write(lf, "logical/entities/counterparty.yaml", _MODEL, crlf=False) + _write(crlf, "logical/entities/counterparty.yaml", _MODEL, crlf=True) + + # same content, different line endings -> the fingerprint must MATCH + assert dir_fingerprint(lf) == dir_fingerprint(crlf) + + +def test_fingerprint_changes_on_real_content_edit(tmp_path): + d = tmp_path / "m" + _write(d, "logical/entities/a.yaml", _MODEL, crlf=False) + fp1 = dir_fingerprint(d) + _write(d, "logical/entities/a.yaml", _MODEL + "definition: a party\n", crlf=False) + assert dir_fingerprint(d) != fp1, "a real content change must change the fingerprint" + + +def test_fingerprint_changes_when_a_file_is_added(tmp_path): + d = tmp_path / "m" + _write(d, "logical/entities/a.yaml", _MODEL, crlf=False) + fp1 = dir_fingerprint(d) + _write(d, "logical/entities/b.yaml", _MODEL, crlf=False) + assert dir_fingerprint(d) != fp1 diff --git a/packages/core/tests/test_fsnames.py b/packages/core/tests/test_fsnames.py new file mode 100644 index 0000000..a0f1151 --- /dev/null +++ b/packages/core/tests/test_fsnames.py @@ -0,0 +1,49 @@ +"""The shared filesystem-safe name helper — the one place object names become filenames.""" + +from __future__ import annotations + +from mdl_core.fsnames import fs_slug, unique_namer + + +def test_strips_windows_illegal_chars(): + assert fs_slug("") == "root" + assert fs_slug("Order:Line") == "order_line" + assert fs_slug("Foo/Bar") == "foo_bar" + assert fs_slug('a"b|c?d*e') == "a_b_c_d_e" + assert fs_slug("trailing.") == "trailing" + assert fs_slug(" ") == "unnamed" + assert fs_slug("Counterparty") == "counterparty" + + +def test_escapes_reserved_device_names(): + # CON / NUL / COM1 / LPT9 are uncreatable on Windows even as con.yaml + assert fs_slug("CON") == "_con" + assert fs_slug("nul") == "_nul" + assert fs_slug("Com1") == "_com1" + assert fs_slug("LPT9") == "_lpt9" + # a normal name that merely CONTAINS a reserved word is fine + assert fs_slug("control") == "control" + + +def test_case_fold_collision_via_slug(): + # Foo and FOO slug to the same string, so a caller can dedup them + assert fs_slug("Foo") == fs_slug("FOO") == "foo" + + +def test_unique_namer_disambiguates_collisions(): + u = unique_namer() + assert u("logical/entities/foo.yaml") == "logical/entities/foo.yaml" + assert u("logical/entities/foo.yaml") == "logical/entities/foo-2.yaml" + assert u("logical/entities/foo.yaml") == "logical/entities/foo-3.yaml" + + +def test_unique_namer_is_case_insensitive(): + # a case-insensitive filesystem treats Foo.yaml and foo.yaml as the same file + u = unique_namer() + assert u("Foo.yaml") == "Foo.yaml" + assert u("foo.yaml") == "foo-2.yaml" + + +def test_unique_namer_returns_posix(): + u = unique_namer() + assert u("logical\\entities\\foo.yaml") == "logical/entities/foo.yaml" diff --git a/packages/core/tests/test_state_prune_windows.py b/packages/core/tests/test_state_prune_windows.py new file mode 100644 index 0000000..59ed8d7 --- /dev/null +++ b/packages/core/tests/test_state_prune_windows.py @@ -0,0 +1,56 @@ +"""GenerationState.save must not delete the shards it just wrote. + +Same Windows path-separator class as the reverse writer: `wanted` holds forward-slash +_shard_rel keys, but the prune loop compared str(relative_to()) which is backslash on +Windows — so every shard looked stale and the whole .mdl/state store was wiped on every +save, destroying the 3-way-merge base. The fix normalises with as_posix(). +""" + +from __future__ import annotations + +from pathlib import PureWindowsPath + +from mdl_core.state import FileState, GenerationState, _shard_rel + + +def _fs(path: str) -> FileState: + return FileState( + path=path, ulids=["01J"], fingerprint="fp", content_hash="ch", emitter_version="1" + ) + + +def test_save_keeps_shards_it_wrote(tmp_path): + st = GenerationState() + for i in range(5): + st.record(_fs(f"models/model_{i}.sql")) + st.save(tmp_path) + shards = list((tmp_path / ".mdl" / "state").glob("*/*.json")) + assert len(shards) == 5, "save must leave every shard it wrote on disk" + + # a re-save with the same state must not prune the live shards + st.save(tmp_path) + assert len(list((tmp_path / ".mdl" / "state").glob("*/*.json"))) == 5 + + +def test_save_prunes_only_stale_shards(tmp_path): + st = GenerationState() + st.record(_fs("models/a.sql")) + st.record(_fs("models/b.sql")) + st.save(tmp_path) + assert len(list((tmp_path / ".mdl" / "state").glob("*/*.json"))) == 2 + + # drop one artifact -> its shard is pruned, the other survives + st2 = GenerationState() + st2.record(_fs("models/a.sql")) + st2.save(tmp_path) + assert len(list((tmp_path / ".mdl" / "state").glob("*/*.json"))) == 1 + + +def test_shard_rel_is_posix_and_would_mismatch_backslash(): + # documents the bug: _shard_rel is forward-slash; a backslash-stringified rel (Windows) + # is NOT in the wanted set, which is exactly what deleted every shard. + rel = _shard_rel("models/a.sql") + assert "/" in rel and "\\" not in rel + win = str(PureWindowsPath(rel)) + assert win != rel # backslashes on Windows + assert PureWindowsPath(rel).as_posix() == rel # the fix normalises back to a match diff --git a/packages/emit-dbt/src/mdl_emit_dbt/emitter.py b/packages/emit-dbt/src/mdl_emit_dbt/emitter.py index 5468f75..25448e9 100644 --- a/packages/emit-dbt/src/mdl_emit_dbt/emitter.py +++ b/packages/emit-dbt/src/mdl_emit_dbt/emitter.py @@ -117,8 +117,22 @@ def _plan_files(self) -> dict[str, dict]: entities = self._entities_for_target() schema_entries = [] + seen: dict[str, str] = {} # lowercased model name -> the entity that claimed it for le, pt in entities: name = pt.name.lower() if pt else le.name + # A dbt model name IS the file stem AND the ref() key, so two entities mapping + # to the same name (or one that only differs in case — the same file on a + # Windows/macOS filesystem) is a real modelling error, not something to + # silently rename: fail loudly instead of overwriting one model with another. + key = name.lower() + if key in seen: + raise ValueError( + f"two entities map to the same dbt model name {name!r} " + f"({seen[key]} and {le.name}); rename one — a colliding model file " + "would overwrite the other and break ref() on a case-insensitive " + "filesystem" + ) + seen[key] = le.name sql, ulids, fp = self._emit_model_sql(le, pt) planned[f"models/{name}.sql"] = { "content": sql, diff --git a/packages/emit-dbt/tests/test_model_name_collision.py b/packages/emit-dbt/tests/test_model_name_collision.py new file mode 100644 index 0000000..bdc7b1d --- /dev/null +++ b/packages/emit-dbt/tests/test_model_name_collision.py @@ -0,0 +1,50 @@ +"""The dbt emitter must fail loudly when two entities map to the same model name. + +A dbt model name is the file stem AND the ref() key. Two entities mapping to the same +name (or one that only differs in case — the same file on a Windows/macOS filesystem) +would silently overwrite each other's .sql and break ref(). The emitter raises instead. +""" + +from __future__ import annotations + +import pytest + +from mdl_core.ir import LogicalEntity, Model, PhysicalTable, ProjectConfig +from mdl_emit_dbt.emitter import DbtEmitter + + +def _model_with(names: list[str], *, physical: bool) -> Model: + m = Model(ProjectConfig(name="m", dbt_target="duckdb_dev")) + for i, nm in enumerate(names): + le = LogicalEntity(id=f"01J00000000000000000000L{i}", name=f"e{i}", realises=None) + m.add(le) + if physical: + m.add( + PhysicalTable( + id=f"01J00000000000000000000P{i}", + target="duckdb_dev", + realises=le.id, + name=nm, + materialization="table", + ) + ) + return m + + +def test_case_only_collision_raises(tmp_path): + # two physical tables Foo / FOO -> same file on a case-insensitive filesystem + m = _model_with(["Foo", "FOO"], physical=True) + with pytest.raises(ValueError, match="same dbt model name"): + DbtEmitter(m, "duckdb_dev").generate(tmp_path, write=False) + + +def test_exact_collision_raises(tmp_path): + m = _model_with(["orders", "orders"], physical=True) + with pytest.raises(ValueError, match="same dbt model name"): + DbtEmitter(m, "duckdb_dev").generate(tmp_path, write=False) + + +def test_distinct_names_do_not_raise(tmp_path): + m = _model_with(["orders", "customers"], physical=True) + # should plan cleanly (write=False, no disk touch) + DbtEmitter(m, "duckdb_dev").generate(tmp_path, write=False) diff --git a/packages/reverse/src/mdl_reverse/connect.py b/packages/reverse/src/mdl_reverse/connect.py index 1808ab4..9cacca4 100644 --- a/packages/reverse/src/mdl_reverse/connect.py +++ b/packages/reverse/src/mdl_reverse/connect.py @@ -97,7 +97,7 @@ def _run(argv: list[str], cwd: Path | None = None, timeout: float = DEFAULT_TIME argv, cwd=str(cwd) if cwd else None, capture_output=True, - text=True, + text=True, encoding="utf-8", errors="replace", timeout=timeout, ) return RunResult(proc.returncode, proc.stdout, proc.stderr) diff --git a/packages/reverse/src/mdl_reverse/writer.py b/packages/reverse/src/mdl_reverse/writer.py index 2044e99..5faa7ee 100644 --- a/packages/reverse/src/mdl_reverse/writer.py +++ b/packages/reverse/src/mdl_reverse/writer.py @@ -8,9 +8,9 @@ from __future__ import annotations -import re from pathlib import Path +from mdl_core.fsnames import fs_slug, unique_namer from mdl_core.ir import Model from mdl_core.yaml_io import dump_str, load_file @@ -66,27 +66,13 @@ def _write_project_config(model: Model, root: Path) -> None: def write_model(model: Model, root: Path) -> list[str]: root = Path(root) written: list[str] = [] - used: set[str] = set() # rel paths already taken this write, for collision-avoidance + # collision-avoidance across this write (case-insensitive, POSIX-normalised) + _unique = unique_namer() # Collection fields that default to []: `exclude_none` does not drop an empty # list, so a reversed model would carry a noise `members: []` on every object. _EMPTY_OK = ("members", "synonyms", "subtypes", "ontology_refs", "values") - def _unique(rel: str) -> str: - """Disambiguate a filename collision. Slugging is lossy (two names can map to one - filename, e.g. 'Order Line' and 'Order/Line' -> 'order_line'), so a clash would - silently overwrite one object with another. Append -2, -3… on the stem instead.""" - if rel not in used: - used.add(rel) - return rel - stem, dot, ext = rel.rpartition(".") - n = 2 - while f"{stem}-{n}{dot}{ext}" in used: - n += 1 - cand = f"{stem}-{n}{dot}{ext}" - used.add(cand) - return cand - def dump(rel: str, obj) -> None: data = obj.model_dump(by_alias=True, exclude_none=True, mode="json") for key in _EMPTY_OK: @@ -170,18 +156,7 @@ def _prune_stale(root: Path, written: set[str]) -> None: pass -# Characters illegal in a Windows filename (< > : " / \ | ? *) plus control chars. A -# reversed/imported object name is used verbatim as a filename, and an erwin export can -# carry names like "" or "Order:Line" that crash open() on Windows (OSError 22) and -# make a portable git repo impossible. Map every unsafe run to a single underscore. -_UNSAFE_FS = re.compile(r'[<>:"/\\|?*\x00-\x1f]+') - - -def _slug(name: str) -> str: - """A filesystem-safe, portable slug for a filename: lowercased, spaces and any - Windows-illegal characters collapsed to underscores, and trailing dots/spaces (also - illegal on Windows) stripped. Never returns empty.""" - s = _UNSAFE_FS.sub("_", name).lower().replace(" ", "_") - # Windows also forbids a name ending in a dot or space, and bare dots are confusing. - s = s.strip("._") - return s or "unnamed" +# The filesystem-safe slug (Windows-illegal chars, reserved names, trailing dot/space) +# lives in mdl_core.fsnames so the writer, the command engine and the dbt emitter all +# share ONE hardened implementation. `_slug` is kept as a thin local alias. +_slug = fs_slug diff --git a/packages/server/src/mdl_server/git_api.py b/packages/server/src/mdl_server/git_api.py index 90f4dbf..9619dc7 100644 --- a/packages/server/src/mdl_server/git_api.py +++ b/packages/server/src/mdl_server/git_api.py @@ -35,7 +35,7 @@ def _git(model_dir: Path, *args: str, timeout: int = 30) -> tuple[int, str]: proc = subprocess.run( ["git", "-C", str(model_dir), *args], capture_output=True, - text=True, + text=True, encoding="utf-8", errors="replace", timeout=timeout, ) out = proc.stdout + (("\n" + proc.stderr) if proc.returncode != 0 else "") @@ -669,7 +669,12 @@ def _gh_prs(model_dir: Path) -> dict[str, dict]: proc = subprocess.run( ["gh", "pr", "list", "--state", "all", "--limit", "50", "--json", "number,url,state,headRefName,reviewDecision"], - cwd=str(model_dir), capture_output=True, text=True, timeout=10, + cwd=str(model_dir), + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=10, ) except (subprocess.SubprocessError, OSError): return {} @@ -1056,7 +1061,12 @@ def _finish(payload: dict) -> JSONResponse: proc = subprocess.run( ["gh", "pr", "create", "--head", branch_name, "--title", body.title, "--body", body.body or body.title], - cwd=str(model_dir), capture_output=True, text=True, timeout=60, + cwd=str(model_dir), + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=60, ) if proc.returncode == 0: result["pr_url"] = proc.stdout.strip() diff --git a/packages/server/src/mdl_server/git_models.py b/packages/server/src/mdl_server/git_models.py index 9c37ddd..36a7b33 100644 --- a/packages/server/src/mdl_server/git_models.py +++ b/packages/server/src/mdl_server/git_models.py @@ -35,7 +35,7 @@ def _git(model_dir: Path, *args: str, timeout: int = 30) -> tuple[int, str]: try: p = subprocess.run( ["git", "-C", str(model_dir), *args], - capture_output=True, text=True, timeout=timeout, + capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=timeout, ) except (OSError, subprocess.SubprocessError) as e: # pragma: no cover - env dependent return 1, str(e) diff --git a/packages/server/src/mdl_server/identity.py b/packages/server/src/mdl_server/identity.py index 1a9ab37..328d18c 100644 --- a/packages/server/src/mdl_server/identity.py +++ b/packages/server/src/mdl_server/identity.py @@ -168,7 +168,7 @@ def _cfg(key: str) -> str: proc = subprocess.run( ["git", "-C", str(model_dir), "config", key], capture_output=True, - text=True, + text=True, encoding="utf-8", errors="replace", timeout=5, ) except (subprocess.SubprocessError, OSError): diff --git a/uv.lock b/uv.lock index 20370f9..30c80bd 100644 --- a/uv.lock +++ b/uv.lock @@ -939,7 +939,7 @@ requires-dist = [ [[package]] name = "modelith-dbt" -version = "0.6.10" +version = "0.6.12" source = { editable = "." } dependencies = [ { name = "dbt-artifacts-parser" }, diff --git a/vscode/src/chatParticipant.ts b/vscode/src/chatParticipant.ts index fead15d..714a11f 100644 --- a/vscode/src/chatParticipant.ts +++ b/vscode/src/chatParticipant.ts @@ -1,3 +1,4 @@ +import * as path from "node:path"; import * as vscode from "vscode"; import type { DriftItem, DriftReport } from "./driftDiagnostics"; import { findManifestPath, findMdl, findModelDir, runMdl } from "./mdl"; @@ -418,7 +419,7 @@ async function driftCommand( } const files = [...new Set(items.map((i) => i.file).filter((f): f is string => !!f))]; for (const f of files) { - stream.reference(vscode.Uri.file(`${dir}/${f}`)); + stream.reference(vscode.Uri.file(path.join(dir, f))); } if (report.breaking_count > 0) { stream.markdown( diff --git a/vscode/src/driftDiagnostics.ts b/vscode/src/driftDiagnostics.ts index fa056f1..430440d 100644 --- a/vscode/src/driftDiagnostics.ts +++ b/vscode/src/driftDiagnostics.ts @@ -156,9 +156,13 @@ export class DriftCodeActionProvider implements vscode.CodeActionProvider { let offeredReconcileAll = false; for (const diag of context.diagnostics) { if (diag.source !== "modelith-drift") continue; - // Match the diagnostic back to a report item by kind + this file. + // Match the diagnostic back to a report item by kind + this file. document.fileName + // uses the OS separator (backslash on Windows) but the CLI report's `i.file` is a + // POSIX rel path — without normalising, endsWith() never matched on Windows and the + // drift quick-fixes silently disappeared. Compare with forward slashes on both sides. + const docPosix = document.fileName.split(path.sep).join("/"); const item = report.items.find( - (i) => i.kind === diag.code && document.fileName.endsWith(i.file ?? "mdl-project.yaml"), + (i) => i.kind === diag.code && docPosix.endsWith(i.file ?? "mdl-project.yaml"), ); if (!item) continue; if (item.reconcilable && !offeredReconcileAll) { diff --git a/vscode/src/extension.ts b/vscode/src/extension.ts index 74b4c8d..06e105f 100644 --- a/vscode/src/extension.ts +++ b/vscode/src/extension.ts @@ -1385,7 +1385,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { title: "Reverse Engineer — target folder", prompt: "Where to write the reversed model (relative to the workspace)", value: rel, - valueSelection: [Math.max(0, rel.lastIndexOf("/") + 1), rel.length], + valueSelection: [Math.max(0, rel.lastIndexOf(path.sep) + 1), rel.length], validateInput: (v) => { const t = v.trim(); if (!t) return "Enter a folder path.";