From 4541dccf9cb4f47b2309c566adc66faf5b8bb7ba Mon Sep 17 00:00:00 2001 From: Simone Date: Fri, 24 Jul 2026 22:04:25 +0200 Subject: [PATCH] feat(operations): backup-to-template grouping fix + artifact/backup delete _template_of_backup only stripped one "_"-chunk off the archive name, so real Template__%Y%m%d_%H%M%S.7z files (date *and* time) never matched their template -- backups silently failed to show up grouped under it. Replaced with a regex that strips the full optional date+time suffix and tolerates underscores in template names. Also adds delete_artifact/delete_backup (mirrored, containment-checked single-file deletion) and backups_by_template (groups list_backups() output by parsed template name, newest-first) for the web dashboard's per-item delete actions and the Templates panel's per-row backup list. --- src/ci_orchestrator/operations/inventory.py | 126 ++++++++++++++++++++ tests/python/test_operations_inventory.py | 21 ++++ 2 files changed, 147 insertions(+) diff --git a/src/ci_orchestrator/operations/inventory.py b/src/ci_orchestrator/operations/inventory.py index 529e355..1133f5c 100644 --- a/src/ci_orchestrator/operations/inventory.py +++ b/src/ci_orchestrator/operations/inventory.py @@ -17,6 +17,7 @@ outside ``root``. from __future__ import annotations import os +import re import time from dataclasses import dataclass from pathlib import Path @@ -280,9 +281,134 @@ def list_artifacts(config: Config) -> list[FileEntry]: return list_dir(config.paths.artifacts, root=config.paths.root, pattern="*") +@dataclass(frozen=True) +class ArtifactDeleteResult: + """Outcome of a (real or dry-run) single-artifact deletion.""" + + target: Path | None + ok: bool + error: str | None + + +def delete_artifact(config: Config, name: str, *, dry_run: bool) -> ArtifactDeleteResult: + """Delete one artifact file directly under ``config.paths.artifacts``. + + SECURITY: ``name`` is only ever a filename the dashboard already listed, + but this still resolves the candidate and refuses anything whose parent + is not exactly the artifacts directory (a ``..`` segment, a nested + subdir, or a symlink pointing elsewhere) and anything that is not a + regular file -- the same containment discipline + :func:`list_dir`/``web/routes.py`` apply. ``dry_run`` validates and + reports the target without unlinking. Never raises. + """ + artifacts_dir = config.paths.artifacts + try: + resolved_dir = artifacts_dir.resolve() + resolved = (artifacts_dir / name).resolve() + except OSError as exc: + return ArtifactDeleteResult(target=None, ok=False, error=f"cannot resolve path: {exc}") + + if resolved.parent != resolved_dir: + return ArtifactDeleteResult( + target=resolved, ok=False, error="artifact is not contained in the artifacts directory" + ) + if not resolved.is_file(): + return ArtifactDeleteResult(target=resolved, ok=False, error="artifact not found") + + if dry_run: + return ArtifactDeleteResult(target=resolved, ok=True, error=None) + + try: + resolved.unlink() + except OSError as exc: + return ArtifactDeleteResult(target=resolved, ok=False, error=str(exc)) + return ArtifactDeleteResult(target=resolved, ok=True, error=None) + + +#: ``Template__%Y%m%d[_%H%M%S].7z`` -- the timestamp both +#: ``operations/host.py::backup_template`` and the ``template backup`` CLI +#: write is ``_%Y%m%d_%H%M%S`` (a date *and* a time chunk, e.g. +#: ``_20260722_145001``), so ```` is everything before that trailing +#: ``_\d{8}(_\d{6})?``. A greedy ``.+`` lets template names that themselves +#: contain underscores (e.g. ``Win_Build_2025``) survive intact; the optional +#: time group tolerates the older date-only shape too. +_BACKUP_NAME_RE = re.compile(r"^Template_(?P.+)_\d{8}(?:_\d{6})?\.7z$") + + +def _template_of_backup(filename: str) -> str | None: + """Parse the template name out of a ``Template__.7z`` archive. + + Mirrors the naming ``operations/host.py::backup_template`` writes. Returns + ``None`` for any filename that does not match the ``Template_..._....7z`` + shape (see :data:`_BACKUP_NAME_RE`). + """ + match = _BACKUP_NAME_RE.match(filename) + return match.group("name") if match else None + + +def backups_by_template(config: Config) -> dict[str, list[FileEntry]]: + """Group every backup archive by the template it belongs to, newest-first. + + Built on :func:`list_backups` (so it inherits the same containment guard + and newest-first ordering); each value list preserves that ordering. + Archives whose name does not parse to a template are skipped. Never + raises -- an unreadable backups dir degrades to an empty mapping. + """ + out: dict[str, list[FileEntry]] = {} + for entry in list_backups(config): + name = _template_of_backup(entry.name) + if name is None: + continue + out.setdefault(name, []).append(entry) + return out + + +def delete_backup(config: Config, name: str, *, dry_run: bool) -> ArtifactDeleteResult: + """Delete one template backup archive under ``/backups``. + + SECURITY: mirrors :func:`delete_artifact`'s containment discipline -- the + candidate must resolve to a direct child of the backups directory (no + ``..``, no nested subdir, no symlink escaping it) and be a regular file. + It must additionally parse as a real ``Template_..._....7z`` archive (see + :data:`_BACKUP_NAME_RE`) so this can never be used to unlink an arbitrary + file that merely happens to sit in the backups directory. ``dry_run`` + validates and reports the target without unlinking. Never raises. + """ + backups_dir = config.paths.root / "backups" + if _template_of_backup(name) is None: + return ArtifactDeleteResult( + target=None, ok=False, error="not a template backup archive name" + ) + try: + resolved_dir = backups_dir.resolve() + resolved = (backups_dir / name).resolve() + except OSError as exc: + return ArtifactDeleteResult(target=None, ok=False, error=f"cannot resolve path: {exc}") + + if resolved.parent != resolved_dir: + return ArtifactDeleteResult( + target=resolved, ok=False, error="backup is not contained in the backups directory" + ) + if not resolved.is_file(): + return ArtifactDeleteResult(target=resolved, ok=False, error="backup not found") + + if dry_run: + return ArtifactDeleteResult(target=resolved, ok=True, error=None) + + try: + resolved.unlink() + except OSError as exc: + return ArtifactDeleteResult(target=resolved, ok=False, error=str(exc)) + return ArtifactDeleteResult(target=resolved, ok=True, error=None) + + __all__ = [ + "ArtifactDeleteResult", "FileEntry", "TemplateInfo", + "backups_by_template", + "delete_artifact", + "delete_backup", "list_artifacts", "list_backups", "list_dir", diff --git a/tests/python/test_operations_inventory.py b/tests/python/test_operations_inventory.py index bd20ccb..400435c 100644 --- a/tests/python/test_operations_inventory.py +++ b/tests/python/test_operations_inventory.py @@ -162,6 +162,27 @@ def test_list_templates_last_backup_picks_newest_matching_archive(tmp_path: Path assert info.last_backup.name == "Template_LinuxBuild2404_20260601_000000.7z" +@pytest.mark.parametrize( + ("filename", "expected"), + [ + # Real archive shape: ``Template__%Y%m%d_%H%M%S.7z`` -- both the + # date and the time chunk must be stripped (regression: only the time + # chunk was, leaving "LinuxBuild2404_20260722"). + ("Template_LinuxBuild2404_20260722_145001.7z", "LinuxBuild2404"), + ("Template_WinBuild2025_20260101_000000.7z", "WinBuild2025"), + # Template names containing underscores survive intact. + ("Template_Win_Build_2025_20260722_145001.7z", "Win_Build_2025"), + # Date-only shape (no time chunk) is still tolerated. + ("Template_LinuxBuild2404_20260722.7z", "LinuxBuild2404"), + # Non-matching names parse to None. + ("not-a-template-archive.7z", None), + ("Template_no_timestamp.txt", None), + ], +) +def test_template_of_backup_strips_full_timestamp(filename: str, expected: str | None) -> None: + assert inventory_module._template_of_backup(filename) == expected + + def test_list_templates_last_backup_none_when_no_backups_dir(tmp_path: Path) -> None: (tmp_path / "templates" / "LinuxBuild2404").mkdir(parents=True) config = _make_config(tmp_path)