mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-09-13 16:30:09 +00:00
fix(migrate): discover directory-form packs instead of silently skipping them
iter_packs() searched only files, so a directory-form pack (`song.sloppak/`, the authoring shape) was walked INTO and never yielded — silently missed by a run that's meant to be exhaustive. Discover suffix-named directories too (yielded whole, not descended into), and route packs through migrate_pack/verify_pack. Directory packs are REPORTED as `dir-form-unsupported`, not rewritten in place: a single-file pack is replaced atomically (a fully-built temp archive swapped in with one os.replace), but a populated directory can't be swapped that way, so an interrupted in-place rewrite could leave an authoring pack half-migrated. The status is a problem status, so it counts against the run's exit code and shows in the summary — the operator re-packs or migrates it as a `.feedpak` instead of it vanishing from the report. Addresses a CodeRabbit review finding. Signed-off-by: Kris Anderson <topkoa@gmail.com>
This commit is contained in:
@@ -315,3 +315,62 @@ def test_a_corrupt_archive_is_reported_not_fatal(tmp_path: Path, capsys):
|
|||||||
assert "migrated" in out
|
assert "migrated" in out
|
||||||
assert mig.verify_zip(good) == "ok" # the healthy pack still got migrated
|
assert mig.verify_zip(good) == "ok" # the healthy pack still got migrated
|
||||||
assert bad.read_bytes() == b"this is not a zip file at all" # untouched
|
assert bad.read_bytes() == b"this is not a zip file at all" # untouched
|
||||||
|
|
||||||
|
|
||||||
|
# ── Directory-form (authoring) packs are discovered, not silently skipped ────
|
||||||
|
|
||||||
|
def _write_dir_pack(path: Path, manifest: dict, files: dict[str, bytes] | None = None) -> Path:
|
||||||
|
"""Build a directory-form pack (`song.sloppak/`), the authoring shape."""
|
||||||
|
files = files or {
|
||||||
|
"original/full.ogg": b"MIXDOWN",
|
||||||
|
"stems/guitar.ogg": b"g",
|
||||||
|
"stems/drums.ogg": b"d",
|
||||||
|
"arrangements/lead.json": b"{}",
|
||||||
|
}
|
||||||
|
path.mkdir()
|
||||||
|
(path / "manifest.yaml").write_text(yaml.safe_dump(manifest, sort_keys=False))
|
||||||
|
for name, data in files.items():
|
||||||
|
p = path / name
|
||||||
|
p.parent.mkdir(parents=True, exist_ok=True)
|
||||||
|
p.write_bytes(data)
|
||||||
|
return path
|
||||||
|
|
||||||
|
|
||||||
|
def test_iter_packs_discovers_directory_form_packs(tmp_path: Path):
|
||||||
|
"""A `song.sloppak/` directory is a pack; os.walk must yield it whole and
|
||||||
|
NOT descend into it (its stems/ are contents, not packs)."""
|
||||||
|
d = _write_dir_pack(tmp_path / "song.sloppak", _manifest())
|
||||||
|
z = _write_pack(tmp_path / "other.feedpak", _manifest())
|
||||||
|
found = set(mig.iter_packs(tmp_path))
|
||||||
|
assert d in found and z in found
|
||||||
|
# Nothing inside the directory pack was yielded as its own pack.
|
||||||
|
assert not any(d in p.parents for p in found)
|
||||||
|
|
||||||
|
|
||||||
|
def test_iter_packs_yields_a_directly_passed_dir_pack(tmp_path: Path):
|
||||||
|
d = _write_dir_pack(tmp_path / "song.sloppak", _manifest())
|
||||||
|
assert list(mig.iter_packs(d)) == [d]
|
||||||
|
|
||||||
|
|
||||||
|
def test_directory_form_pack_is_reported_not_silently_skipped(tmp_path: Path, capsys):
|
||||||
|
"""The migrator rewrites single-file packs atomically; a directory can't be
|
||||||
|
swapped that way, so it is surfaced as a problem rather than vanishing from
|
||||||
|
the run (the silent-skip this guards against) or being rewritten unsafely."""
|
||||||
|
d = _write_dir_pack(tmp_path / "song.sloppak", _manifest())
|
||||||
|
good = _write_pack(tmp_path / "good.feedpak", _manifest())
|
||||||
|
|
||||||
|
rc = mig.main([str(tmp_path)])
|
||||||
|
out = capsys.readouterr().out
|
||||||
|
|
||||||
|
assert rc == 1 # a reported problem fails the exit code
|
||||||
|
assert "dir-form-unsupported" in out
|
||||||
|
assert mig.verify_zip(good) == "ok" # the zip pack still migrated
|
||||||
|
# The directory pack is untouched: legacy key intact, mixdown not moved.
|
||||||
|
manifest = yaml.safe_load((d / "manifest.yaml").read_text())
|
||||||
|
assert manifest.get("original_audio") == "original/full.ogg"
|
||||||
|
assert (d / "original" / "full.ogg").read_bytes() == b"MIXDOWN"
|
||||||
|
|
||||||
|
|
||||||
|
def test_verify_reports_directory_form_packs(tmp_path: Path):
|
||||||
|
d = _write_dir_pack(tmp_path / "song.sloppak", _manifest())
|
||||||
|
assert mig.verify_pack(d) == "dir-form-unsupported"
|
||||||
|
|||||||
@@ -261,14 +261,53 @@ def verify_zip(path: Path) -> str:
|
|||||||
return "ok"
|
return "ok"
|
||||||
|
|
||||||
|
|
||||||
|
def migrate_pack(path: Path, dry_run: bool) -> str:
|
||||||
|
"""Dispatch by pack form. ZIP-file packs are rewritten in place; directory
|
||||||
|
(authoring) packs are REPORTED, not rewritten.
|
||||||
|
|
||||||
|
A single-file pack is replaced atomically — a fully-built temp archive
|
||||||
|
swapped in with one os.replace(), so an interrupted run leaves it either
|
||||||
|
fully migrated or untouched. A directory can't be swapped that way (no
|
||||||
|
atomic replace of a populated directory), so an in-place rewrite could leave
|
||||||
|
an authoring pack half-migrated. Rather than risk that, directory packs are
|
||||||
|
surfaced as `dir-form-unsupported` (a problem status, so the run's exit code
|
||||||
|
and summary flag them) for the operator to re-pack or migrate as a `.feedpak`.
|
||||||
|
"""
|
||||||
|
if path.is_dir():
|
||||||
|
return "dir-form-unsupported"
|
||||||
|
return migrate_zip(path, dry_run)
|
||||||
|
|
||||||
|
|
||||||
|
def verify_pack(path: Path) -> str:
|
||||||
|
"""Verify a pack; directory (authoring) packs are reported, see migrate_pack."""
|
||||||
|
if path.is_dir():
|
||||||
|
return "dir-form-unsupported"
|
||||||
|
return verify_zip(path)
|
||||||
|
|
||||||
|
|
||||||
def iter_packs(root: Path):
|
def iter_packs(root: Path):
|
||||||
|
"""Yield every pack under `root`. A pack is a suffix-named ZIP FILE or a
|
||||||
|
suffix-named DIRECTORY (the authoring form) — both are discovered so a
|
||||||
|
directory-form pack is never silently walked past. A directory pack is
|
||||||
|
yielded whole, not descended into: its `stems/` and `arrangements/` are pack
|
||||||
|
contents, not packs. (migrate/verify then report directory packs rather than
|
||||||
|
rewriting them in place — see migrate_pack.)"""
|
||||||
if root.is_file():
|
if root.is_file():
|
||||||
yield root
|
yield root
|
||||||
return
|
return
|
||||||
for dirpath, _dirnames, filenames in os.walk(root):
|
# A directory whose OWN name is a pack suffix is a single directory-form
|
||||||
|
# pack passed directly, not a tree of packs to search.
|
||||||
|
if root.name.endswith(PACK_EXTS):
|
||||||
|
yield root
|
||||||
|
return
|
||||||
|
for dirpath, dirnames, filenames in os.walk(root):
|
||||||
for fn in sorted(filenames):
|
for fn in sorted(filenames):
|
||||||
if fn.endswith(PACK_EXTS):
|
if fn.endswith(PACK_EXTS):
|
||||||
yield Path(dirpath) / fn
|
yield Path(dirpath) / fn
|
||||||
|
for dn in sorted(dn for dn in dirnames if dn.endswith(PACK_EXTS)):
|
||||||
|
yield Path(dirpath) / dn
|
||||||
|
# Don't descend INTO a pack directory — its contents aren't packs.
|
||||||
|
dirnames[:] = [dn for dn in dirnames if not dn.endswith(PACK_EXTS)]
|
||||||
|
|
||||||
|
|
||||||
def main(argv: list[str] | None = None) -> int:
|
def main(argv: list[str] | None = None) -> int:
|
||||||
@@ -288,7 +327,7 @@ def main(argv: list[str] | None = None) -> int:
|
|||||||
print(f"no packs found under {args.root}", file=sys.stderr)
|
print(f"no packs found under {args.root}", file=sys.stderr)
|
||||||
return 2
|
return 2
|
||||||
|
|
||||||
action = verify_zip if args.verify else (lambda p: migrate_zip(p, args.dry_run))
|
action = verify_pack if args.verify else (lambda p: migrate_pack(p, args.dry_run))
|
||||||
|
|
||||||
def work(pack: Path) -> str:
|
def work(pack: Path) -> str:
|
||||||
"""Never raise. One unreadable pack must not kill a 50,000-pack run.
|
"""Never raise. One unreadable pack must not kill a 50,000-pack run.
|
||||||
|
|||||||
Reference in New Issue
Block a user