From 3ea077be1dab73350c4acb7b584b4df53dfb377a Mon Sep 17 00:00:00 2001 From: Philippe Parage Date: Wed, 3 Jun 2026 11:19:44 +0200 Subject: [PATCH 1/4] fix(catalog): resolve entry detail across all manifest formats (#77) The /v1/catalog/entries detail endpoint only read range42.yaml, so every container (meta.json) and Ansible-role (meta/main.yml) entry surfaced by the browse walker 404'd on detail. Add _detail_at_path() mirroring _discover's manifest recognition and wire get_entry to use it. --- app/routes/v1/catalog/entries.py | 42 +++++++++--- tests/routes/test_catalog_detail_resolver.py | 68 ++++++++++++++++++++ 2 files changed, 101 insertions(+), 9 deletions(-) create mode 100644 tests/routes/test_catalog_detail_resolver.py diff --git a/app/routes/v1/catalog/entries.py b/app/routes/v1/catalog/entries.py index 1c06962..887f15b 100644 --- a/app/routes/v1/catalog/entries.py +++ b/app/routes/v1/catalog/entries.py @@ -112,6 +112,31 @@ def _entry_from_meta_main_yml(p: Path, repo_dir: Path) -> dict | None: } +def _detail_at_path(repo_dir: Path, path: str) -> dict | None: + """Resolve a single entry at ``path`` across the three manifest formats. + + Mirrors :func:`_discover`'s recognition so the detail endpoint surfaces + every entry the browse endpoint lists — not just ``range42.yaml``. + Returns the summary dict plus a parsed ``document``, or ``None`` if no + recognised manifest exists at ``path``. + """ + base = repo_dir / path + candidates = [ + (base / "range42.yaml", _entry_from_range42_yaml, _yaml.load), + (base / "meta.json", _entry_from_meta_json, json.loads), + (base / "meta" / "main.yml", _entry_from_meta_main_yml, _yaml.load), + ] + for manifest, parse_entry, load_doc in candidates: + if not manifest.exists(): + continue + entry = parse_entry(manifest, repo_dir) + if entry is None: + continue + doc = load_doc(manifest.read_text()) or {} + return {**entry, "document": doc} + return None + + def _discover(repo_dir: Path) -> list[dict]: """Walk ``repo_dir`` for the three known manifest types. @@ -210,19 +235,18 @@ async def get_entry( workdir = Path(td) for repo in repos: dest = _clone_repo(src, repo, workdir) - manifest = dest / path / "range42.yaml" - if manifest.exists(): - doc = _yaml.load(manifest.read_text()) or {} + entry = _detail_at_path(dest, path) + if entry is not None: readme = dest / path / "README.md" return CatalogEntryDetail( source_id=src.id, - path=path, - kind=doc.get("kind", "unknown"), - name=doc.get("name", path), - description=doc.get("description"), - tags=doc.get("tags", []), + path=entry["path"], + kind=entry["kind"], + name=entry["name"], + description=entry.get("description"), + tags=entry.get("tags") or [], sha=None, - document=doc, + document=entry["document"], readme_md=readme.read_text() if readme.exists() else None, ) raise Range42Error( diff --git a/tests/routes/test_catalog_detail_resolver.py b/tests/routes/test_catalog_detail_resolver.py new file mode 100644 index 0000000..f0ffbfd --- /dev/null +++ b/tests/routes/test_catalog_detail_resolver.py @@ -0,0 +1,68 @@ +"""Entry-detail resolution must recognise all three manifest formats. + +The detail endpoint used to only read ``range42.yaml`` at the requested +path, so every container (``meta.json``) or Ansible-role (``meta/main.yml``) +entry that the browse endpoint surfaced 404'd on detail. ``_detail_at_path`` +must resolve all three, returning the parsed document alongside the summary +fields. +""" +from __future__ import annotations + +import json + +from app.routes.v1.catalog.entries import _detail_at_path + + +def _write(path, text): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + +def test_detail_resolves_range42_yaml(tmp_path): + _write(tmp_path / "sub_a" / "range42.yaml", "kind: lab\nname: x\n") + d = _detail_at_path(tmp_path, "sub_a") + assert d is not None + assert d["kind"] == "lab" + assert d["name"] == "x" + assert d["document"]["kind"] == "lab" + + +def test_detail_resolves_meta_json_container(tmp_path): + _write( + tmp_path / "sub_b" / "meta.json", + json.dumps( + { + "x_range42": { + "exercise": {"id": "exer-1"}, + "catalog": {"tags": ["t1"]}, + "vuln": {"title": "vuln title"}, + } + } + ), + ) + d = _detail_at_path(tmp_path, "sub_b") + assert d is not None + assert d["kind"] == "container" + assert d["name"] == "exer-1" + assert d["description"] == "vuln title" + assert d["tags"] == ["t1"] + assert d["document"]["x_range42"]["exercise"]["id"] == "exer-1" + + +def test_detail_resolves_meta_main_yml_role(tmp_path): + _write( + tmp_path / "sub_c" / "meta" / "main.yml", + "galaxy_info:\n description: role d\n galaxy_tags:\n - g1\n", + ) + d = _detail_at_path(tmp_path, "sub_c") + assert d is not None + assert d["kind"] == "ansible_role" + assert d["name"] == "sub_c" + assert d["description"] == "role d" + assert d["tags"] == ["g1"] + assert d["document"]["galaxy_info"]["description"] == "role d" + + +def test_detail_missing_path_returns_none(tmp_path): + _write(tmp_path / "sub_a" / "range42.yaml", "kind: lab\nname: x\n") + assert _detail_at_path(tmp_path, "nope") is None From c60116ebb929a6a0f69bd6028d2c5d3825a3207d Mon Sep 17 00:00:00 2001 From: Philippe Parage Date: Wed, 3 Jun 2026 11:19:44 +0200 Subject: [PATCH 2/4] fix(catalog): count refresh entries via shared walker (#78) _count_entries_in_repo counted only range42.yaml + a nonexistent catalog.yaml, so entries_indexed was always 0 for real catalogs. Delegate to _discover so the refresh count agrees with what /v1/catalog/entries surfaces. --- app/routes/v1/catalog/refresh.py | 20 +++++++++----- tests/routes/test_catalog_refresh_count.py | 31 ++++++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) create mode 100644 tests/routes/test_catalog_refresh_count.py diff --git a/app/routes/v1/catalog/refresh.py b/app/routes/v1/catalog/refresh.py index 70ee1f0..483018a 100644 --- a/app/routes/v1/catalog/refresh.py +++ b/app/routes/v1/catalog/refresh.py @@ -69,6 +69,18 @@ async def refresh_source( ) +def _count_manifests(root: "Path") -> int: + """Count catalog entries under ``root`` using the shared walker. + + Delegates to :func:`app.routes.v1.catalog.entries._discover` so the + refresh count always agrees with what ``/v1/catalog/entries`` surfaces + (range42.yaml, meta.json, and meta/main.yml — not just range42.yaml). + """ + from app.routes.v1.catalog.entries import _discover + + return len(_discover(root)) + + def _count_entries_in_repo(src: Source, repo: SourceRepo) -> int: import tempfile from pathlib import Path @@ -78,10 +90,4 @@ def _count_entries_in_repo(src: Source, repo: SourceRepo) -> int: url = f"{str(src.base_url).rstrip('/')}/{repo.owner}/{repo.repo}.git" with tempfile.TemporaryDirectory() as td: git.Repo.clone_from(url, td, depth=1, branch=repo.branch) - root = Path(td) - count = 0 - for _p in root.rglob("range42.yaml"): - count += 1 - for _p in root.rglob("catalog.yaml"): - count += 1 - return count + return _count_manifests(Path(td)) diff --git a/tests/routes/test_catalog_refresh_count.py b/tests/routes/test_catalog_refresh_count.py new file mode 100644 index 0000000..170314c --- /dev/null +++ b/tests/routes/test_catalog_refresh_count.py @@ -0,0 +1,31 @@ +"""Refresh entry-count must match the walker's manifest recognition. + +The refresh endpoint reports ``entries_indexed`` after shallow-cloning each +repo. That count must agree with what ``/v1/catalog/entries`` would surface, +i.e. it must recognise all three manifest formats (range42.yaml, meta.json, +meta/main.yml) — not just range42.yaml. +""" +from __future__ import annotations + +import json + +from app.routes.v1.catalog.refresh import _count_manifests + + +def _write(path, text): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + +def test_count_manifests_recognises_all_three_formats(tmp_path): + _write(tmp_path / "sub_a" / "range42.yaml", "kind: lab\nname: x\n") + _write( + tmp_path / "sub_b" / "meta.json", + json.dumps({"x_range42": {"exercise": {"id": "exer-1"}}}), + ) + _write( + tmp_path / "sub_c" / "meta" / "main.yml", + "galaxy_info:\n description: role d\n", + ) + + assert _count_manifests(tmp_path) == 3 From 4cdc574cf8304f976246153b0e6998212c17ccf9 Mon Sep 17 00:00:00 2001 From: Philippe Parage Date: Wed, 3 Jun 2026 11:19:44 +0200 Subject: [PATCH 3/4] test(core): run alembic via sys.executable to use venv SQLAlchemy (#76) The migration roundtrip test invoked bare 'alembic', which resolved to a stale ~/.local copy bound to an old SQLAlchemy lacking declarative_base. Invoke via sys.executable -m alembic so it runs against the venv. --- tests/core/test_alembic.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/core/test_alembic.py b/tests/core/test_alembic.py index fe1f746..f7cccb4 100644 --- a/tests/core/test_alembic.py +++ b/tests/core/test_alembic.py @@ -1,4 +1,5 @@ import subprocess +import sys from pathlib import Path @@ -7,8 +8,12 @@ def test_alembic_upgrade_downgrade_roundtrip(tmp_path, monkeypatch): monkeypatch.setenv("RANGE42_DB_URL", f"sqlite+aiosqlite:///{db}") monkeypatch.setenv("RANGE42_WORKSPACE_ROOT", str(tmp_path)) repo = Path(__file__).resolve().parents[2] - r = subprocess.run(["alembic", "upgrade", "head"], cwd=repo, check=True, + # Invoke alembic via the active interpreter (sys.executable -m) so the + # migration runs against the venv's SQLAlchemy, not whatever bare + # ``alembic`` happens to resolve to on PATH (e.g. a stale ~/.local copy). + alembic = [sys.executable, "-m", "alembic"] + r = subprocess.run(alembic + ["upgrade", "head"], cwd=repo, check=True, capture_output=True, text=True) assert db.exists() - r = subprocess.run(["alembic", "downgrade", "base"], cwd=repo, check=True, + r = subprocess.run(alembic + ["downgrade", "base"], cwd=repo, check=True, capture_output=True, text=True) From 0439eb9272b2d09662ba033a8015686ce96df577 Mon Sep 17 00:00:00 2001 From: Philippe Parage Date: Wed, 3 Jun 2026 11:26:16 +0200 Subject: [PATCH 4/4] fix(catalog): coerce non-dict manifest document in entry detail (#77) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A syntactically valid manifest with a non-mapping top level (e.g. a JSON/YAML list) is still listed by the browse walker, but the detail resolver passed the raw value through as document — failing CatalogEntryDetail.document: dict response validation with a 500. Coerce non-dict documents to {} so the entry resolves cleanly. Addresses Codex review feedback. --- app/routes/v1/catalog/entries.py | 7 ++++++- tests/routes/test_catalog_detail_resolver.py | 11 +++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/routes/v1/catalog/entries.py b/app/routes/v1/catalog/entries.py index 887f15b..dc37d0b 100644 --- a/app/routes/v1/catalog/entries.py +++ b/app/routes/v1/catalog/entries.py @@ -132,7 +132,12 @@ def _detail_at_path(repo_dir: Path, path: str) -> dict | None: entry = parse_entry(manifest, repo_dir) if entry is None: continue - doc = load_doc(manifest.read_text()) or {} + doc = load_doc(manifest.read_text()) + # CatalogEntryDetail.document is typed dict; a syntactically valid but + # non-mapping top level (e.g. a YAML/JSON list) would otherwise fail + # response validation. Coerce it to {} — the entry still resolves. + if not isinstance(doc, dict): + doc = {} return {**entry, "document": doc} return None diff --git a/tests/routes/test_catalog_detail_resolver.py b/tests/routes/test_catalog_detail_resolver.py index f0ffbfd..ea15e71 100644 --- a/tests/routes/test_catalog_detail_resolver.py +++ b/tests/routes/test_catalog_detail_resolver.py @@ -66,3 +66,14 @@ def test_detail_resolves_meta_main_yml_role(tmp_path): def test_detail_missing_path_returns_none(tmp_path): _write(tmp_path / "sub_a" / "range42.yaml", "kind: lab\nname: x\n") assert _detail_at_path(tmp_path, "nope") is None + + +def test_detail_non_dict_document_is_coerced_to_dict(tmp_path): + # A syntactically valid manifest whose top level is a (truthy) non-dict, + # e.g. a JSON array. The browse walker still synthesises a summary for it, + # so detail must return a CatalogEntryDetail-compatible document (a dict), + # not the raw list — otherwise response validation 500s on document: dict. + _write(tmp_path / "weird" / "meta.json", "[1, 2, 3]") + d = _detail_at_path(tmp_path, "weird") + assert d is not None + assert isinstance(d["document"], dict)