fix(catalog): entry-detail manifest resolution, refresh count, alembic test#89
fix(catalog): entry-detail manifest resolution, refresh count, alembic test#89pparage wants to merge 4 commits into
Conversation
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.
_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.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cdc574cf8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| doc = load_doc(manifest.read_text()) or {} | ||
| return {**entry, "document": doc} |
There was a problem hiding this comment.
When a meta.json or meta/main.yml is syntactically valid but its top level is not an object (for example []), the existing summary parsers still synthesize an entry, so the browse endpoint lists it. The new detail resolver then returns that list as document, but CatalogEntryDetail.document is typed as dict, causing FastAPI/Pydantic response validation to fail with a 500 instead of either skipping the entry or returning a valid detail response. This affects malformed-but-parseable manifests in the new non-range42.yaml detail path.
Useful? React with 👍 / 👎.
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.
|
Good catch — valid P2. Fixed in 0439eb9: Minor note: the exact |
1 participant
Three evidence-backed quick-win fixes from the open-issue triage. Each was developed test-first (TDD); full suite 389 passing.
Changes
/v1/catalog/entries/{source}/{path}detail only readrange42.yaml, 404-ing on every container (meta.json) and Ansible-role (meta/main.yml) entry the browse walker surfaces. New_detail_at_path()mirrors_discover's manifest recognition;get_entrynow uses it.entries_indexedcounted onlyrange42.yaml+ a nonexistentcatalog.yaml(always 0 for real catalogs)._count_entries_in_reponow delegates to_discovervia_count_manifests, so the count matches what the browse endpoint lists.alembic, resolving to a stale~/.localcopy bound to an old SQLAlchemy lackingdeclarative_base(the test was actually failing). Now invoked viasys.executable -m alembicagainst the venv.Tests
tests/routes/test_catalog_detail_resolver.py(4),tests/routes/test_catalog_refresh_count.py(1)tests/core/test_alembic.pyBase is
feature/gamenet-authoring-v1— the v1 catalog code these touch does not exist ondev/main.