Repository navigation
fix: read a subdir app's art from beside its manifest - #51
Merged
Merged
Conversation
`fetch_manifest` joins `source.subdir` before reading `app.json`, but every asset that manifest names -- `iconPath`, `iconPathDark`, `heroImage`, `heroImageDetail`, `screenshots[]` -- was handed to `fetch_blob` with the bare manifest path, so `git show` looked for a monorepo app's icon at the repository root. A correct manifest declaring `iconPath: "icon.png"` beside its `app.json` published a card with no icon, hero or screenshots. Thread the entry's `subdir` into every `_FetchedImages.add(...)` call and join it there, once, before the size probe and the read, so the sizer and the reader can never name two different blobs. The join is contained at the point where it becomes a git path (no absolute result, no `..` segment, no backslash or NUL) rather than trusted from the schemas upstream. The manifest key keeps its app-relative meaning -- the installed app and the store client both resolve it that way -- and the published ref is still the hosted, content-addressed path. Root apps read the same bare path as before; builtin entries never reach an ingester and are unchanged. Tests pin the path handed to the reader for every asset kind, the unchanged root-app and builtin cases, the same warn-and-continue finding for a subdir asset that is missing, containment of the join, and the measured reproduction end to end through a real `git show`. Every case fails on the previous `publish.py`; each `subdir=` call site and both containment checks are mutation-verified. The client-side twin of this defect was fixed in kirodotdev/KiroCrew#12903. Fixes #50
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. |
Opus 5 Review — ✅ no blocking findingsOpus 5 completed its review of This comment is updated in place on each push. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
For a git entry with
source.subdir,tools/publish.pyreadapp.jsonfrom the subdirectory but read every asset that manifest names (iconPath,iconPathDark,heroImage,heroImageDetail,screenshots[]) from the repository root. A monorepo app declaringiconPath: "icon.png"beside itsapp.jsontherefore published a card with no icon, hero or screenshots. This PR joins the entry'ssubdirinto every asset read.Cause
tools/publish.py:454-457--fetch_manifest(url, commit, subdir)joinssubdir/app.jsonbeforegit show.tools/publish.py:924--fetch_blob(url, commit, path)runsgit show commit:pathwith whatever it is handed.tools/publish.py:1464-1468,:1546,:1553-1557--IconAssets.add, the hero/detail-heroingester.add, andScreenshotAssets.addall pass the bare manifest path.origin.get("subdir")reached only the manifest read at:1629; no ingester ever saw it.Fix
contained_blob_path(subdir, rel_path)next tofetch_blob: joinssubdir/<manifest path>with the samePathnormalizationfetch_manifestuses, and refuses an absolute result, a..segment, a backslash or a NUL. Containment is re-stated at the point where the value becomes a git path rather than trusted from the two schemas upstream._FetchedImages.add(...)takes a keyword-onlysubdir=Noneand joins once, before the size probe and the read, so sizer and reader can never name two different blobs. Diagnostics keep naming the manifest's own spelling (rel_path).bake_entryreadssource.get("subdir")and passessubdir=subdirto the icon, hero, detail-hero and screenshot.add(...)calls.Not changed: the meaning of
iconPath(app-relative stays app-relative),validate.py,changed_entries.py, the schemas, the publishedsource.subdirkey. Root apps read the same bare path as before; builtin entries never reach an ingester and are byte-identical (pinned by a test).Tests
22 new cases in
tests/test_publish.py(TestSubdirAppsReadArtBesideTheirManifest,TestContainedBlobPath):source.subdir="apps/beta"+iconPath="icon.png"handsapps/beta/icon.pngto the reader; same foriconPathDark,heroImage,heroImageDetail,screenshots[], and the size probe.subdirstill passes the bare path; a builtin entry is byte-identical.cannot read icon 'icon.png'warning, no error, no raise.git show: two apps in one repo, onlyapps/beta/icon.pngis admitted (notapps/alpha/icon.png, not a rooticon.png).Red-first: 20 of the 22 fail on
main'spublish.py(the two that pass are the root-app and builtin regression guards, by design). Mutation-verified: droppingsubdir=from any one of the three call sites, feedingrel_pathto the sizer or the reader, or deleting either containment check each fails at least one test. A redundant pre-check that no mutation could kill was removed.One existing test (
test_release_entry_ingests_art_from_the_repository_not_the_asset_url) had a hand-rolledSpyAssets.addwith a fixed positional signature; it now accepts thesubdirkeyword.Locally (Python 3.12):
pytest tests/ -q500 passed before, 522 passed after;python tools/validate.pyOK (3 pre-existing warnings);python tools/format.py --checkall 3 files normalized.Related
The client-side twin of this defect was fixed in kirodotdev/KiroCrew#12903 (the store card joins
subdirectoryinto every asset URL); with this change the two sides agree on what an app-relative path means.Fixes #50