diff --git a/CHANGELOG.md b/CHANGELOG.md index b3af75ded..45fc993a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,8 @@ All notable changes to Bambuddy will be documented in this file. - **Print Log page: per-row delete (#1687 part 1, reported by @IndividualGhost1905)** — Reporter noted that the existing "Also remove this print from Quick Stats" toggle on archive delete is one-shot: if you tick "keep stats" at delete time, there was no later way to drop the row from /stats; and rows that aren't tied to an archive (errors, aborts, manual entries) had no delete affordance at all. **Fix:** every row in the Archives → Print Log table now has a trash icon next to the filament cell, gated on `archives:delete_own` (own rows) or `archives:delete_all` (any row), matching the archive-delete permission shape. Click → confirm modal → row is gone, and because /archives/stats aggregates over `PrintLogEntry` the filament / time / cost contribution drops out of Quick Stats in the same response cycle. The matching archive (if any) is untouched — the log row is a sibling, not a child. **Backend:** new `DELETE /print-log/{entry_id}` mirrors `delete_archive`'s ownership flow via `require_ownership_permission(ARCHIVES_DELETE_ALL, ARCHIVES_DELETE_OWN)`; owners can drop their own rows, admins can drop any row, missing IDs return 404 rather than 200-silently. **Frontend:** new `deletePrintLogEntry` API helper, per-row mutation that invalidates both `print-log` and `archives-stats` query keys so the totals re-render without a manual refresh. **i18n:** 4 new keys (`deleteEntryTitle`, `deleteEntryConfirm`, `entryDeleted`, `entryDeleteFailed`) translated across all 11 locales (de / en / es / fr / it / ja / ko / pt-BR / tr / zh-CN / zh-TW). **Tests:** 3 backend integration cases — delete drops the row from /stats while keeping the linked archive listed, missing ID returns 404, delete-one does not touch siblings (regression guard against an accidental `delete(PrintLogEntry)` without a `where`). Frontend ArchivesPage / PrintLogModal vitests stay green (31 / 31). i18n parity green (5099 leaves × 11 locales). Issue #1687 also asks for per-row tagging (already covered by `EditArchiveModal`'s tags field) and per-row filament-usage-history edits (deferred — see the issue thread for the reasoning). ### Fixed +- **In-app updater fails when DATA_DIR is on a separate mount from the install (#1715, reported by @francescocozzi)** — Native installs that follow the systemd template `WorkingDirectory=/opt/bambuddy` with `Environment="DATA_DIR=/srv/bambuddy/data"` (or any layout where `DATA_DIR` is not a subdirectory of the install path) couldn't apply in-app updates. Every git step in `_perform_update` (`remote get-url`, `remote set-url`, `fetch`, `reset --hard`) used `cwd=settings.base_dir`, and `safe.directory` was pointed at `base_dir` too. On the standard install (DATA_DIR=INSTALL_PATH/data) this happened to work by accident — git walks up from a subdirectory of the repo to find `.git` — but on a separate-mount layout the data dir is not under the install, the walk-up has nowhere to go, and every operation returns "fatal: not a git repository." Even on the standard install `safe.directory={base_dir}` was wrong (it must equal the repo root git discovers, not the data dir), surfacing on hardened systemd units as "fatal: detected dubious ownership." **Fix:** route every git subprocess in `_perform_update` and `_origin_points_at_repo` through `cwd=settings.app_dir` (the working tree), and set `safe.directory={app_dir}` to match. `app_dir` is now resolved once at the top of `_perform_update` instead of lazily re-resolved before the pip step. The `base_dir` parameter on `_origin_points_at_repo` is renamed to `app_dir` so the signature documents the contract. The pip-install step keeps `cwd=app_dir` (unchanged — that step was already correct). **Tests:** new `test_perform_update_runs_git_in_app_dir_when_data_dir_on_separate_mount` integration case constructs a sibling-paths layout (`tmp/opt/bambuddy` + `tmp/srv/bambuddy/data` — the exact #1715 shape), mocks `asyncio.create_subprocess_exec` to capture every call's cwd, and pins (a) every git subprocess runs with `cwd=app_dir`, (b) the embedded `safe.directory=` config equals `app_dir` on every git call. The existing pip-cwd test stays green (pip's cwd was already `app_dir`). Existing SSH-origin-preserve + origin-rewrite + reset-target tests stay green (they don't assert on git cwd). Full `test_updates_api.py` 21/21 green; ruff clean. **Credit:** root cause + fix shape from francescocozzi via PR #1716 (couldn't be merged as-is — that branch had drifted off an older `dev` and pulled in unrelated upstream commits including a version regression). + - **SliceModal preset-lookup precedence + cross-tier dedup + signed-out banner (#1712, reported by @IndividualGhost1905)** — After the Orca Cloud integration shipped (2026-06-04), every user — including Bambu-Cloud-only / Bambu-Studio-preferred users — got Orca Cloud as the top tier across the SliceModal preset picker, the per-preset auto-pick scoring, the dropdown's optgroup rendering, the AMS slot picker's filament sort, and the backend dedup precedence. A Bambu-Cloud / X1C user reported seeing his Bambu Cloud profiles disappear from auto-pick because Orca Cloud's empty tier shadowed them. The cross-tier dedup (introduced with #1150 and inherited as-is by the Orca change) compounded the problem: a name that existed in multiple tiers showed in only ONE group, so a user with a local-imported and Orca-synced "Bambu PLA Basic" never saw the Orca copy as a picker option — even though they curate both sources. And the cloud-status banner (`CloudStatusBanner`) nagged signed-out users with a permanent *"Sign in to Orca Cloud (Profiles → Orca Cloud) to see your Orca presets"* at the top of every SliceModal open — even after a user had explicitly logged out of Orca Cloud. The Bambu Cloud banner had the symmetric problem. **Fix — order:** precedence is `local > orca_cloud > cloud > standard` across `SliceModal.tsx` (`SLICE_MODAL_TIER_ORDER` + `TIER_BONUS` + dropdown tier list), `ConfigureAmsSlotModal.tsx` (`sourceOrder`), and docstrings in `slicer_presets.py` / `schemas/slicer_presets.py` / `client.ts`. Local imports win (the user did them on purpose), Orca Cloud comes next, Bambu Cloud, bundled fallback last. The order drives auto-pick + visual group order; it does NOT hide profiles. **Fix — no dedup, full lists:** `_dedupe_by_name` is replaced by `_enrich_cloud_metadata`, which returns every tier's full preset list across all three slots (printer / process / filament) — a name in local AND orca_cloud AND cloud renders in EACH of their groups so the user can pick any source. The only work the function still does is filament-metadata backfill: a Bambu Cloud filament without its own `filament_type` / `filament_colour` inherits values from a same-named local / orca_cloud / standard entry so it can still score in `pickFilamentForSlot`. Printer + process presets carry their metadata inline and need no enrich. Frontend code already iterates tiers in priority order and surfaces every entry — no change needed there once the backend stops filtering. **Fix — banner:** `CloudStatusBanner` now silently returns null on `not_authenticated` in addition to `ok` — applies symmetrically to both Bambu and Orca cloud banners. `expired` (token broke) and `unreachable` (network / service down) still surface — those are real breakage states a previously-signed-in user needs to see. Sign-in lives on the Profiles page; the modal doesn't need to advertise it. The `slice.cloud.notAuthenticated` / `slice.orcaCloud.notAuthenticated` i18n keys stay in the locale files (dormant) so re-enabling later doesn't need a re-translation pass. **Fix — ConfigureAmsSlotModal source badges:** before this change, the per-row source badge fired three branches independently — `local` got a green "Local" badge, `builtin` got an amber "Built-in" badge, and a blue "Custom" badge appeared on top of those when `isUser` was true. Since ALL Orca Cloud entries are marked `isUser: true` and Bambu Cloud user presets also get the same flag, the result was visually inconsistent: Orca Cloud rows showed *only* "Custom" (no source identification, no way to tell them from Bambu Cloud user presets), Bambu Cloud built-in rows had NO badge at all, and the "Custom" badge collided with the actual source. Replaced with a single source badge per row: green "Local", purple "Orca Cloud" (new), bambu-blue "Bambu Cloud" (new), amber "Built-in". One badge per row; one colour per source; the `isUser` distinction within the Bambu Cloud tier is dropped (the preset name itself carries the "is this user-authored" signal). Same change in both render blocks (the filament-list code is duplicated in the modal — kept the duplication local rather than refactoring out a helper component in this PR to keep the diff tight). i18n: 2 new keys (`configureAmsSlot.orcaCloud`, `configureAmsSlot.bambuCloud`) translated to all 11 locales — both are brand names, already on the per-locale `IDENTICAL_TO_EN_ALLOWED` lists so the parity check is satisfied without per-locale variants. The dormant `configureAmsSlot.custom` key stays in the locale files. **Tests:** `TestEnrichCloudMetadata` replaces `TestDedupeByName` (5 cases): regression guard pinning that a name in all four tiers appears in EACH (not just local), tier order preserved within a tier, Bambu Cloud filament metadata backfilled from local, backfill falls through to orca / standard when local doesn't carry the name, backfill does NOT overwrite Bambu Cloud's own metadata when present. The "renders a sign-in banner when cloud_status is not_authenticated" case flipped to assert no banner appears, with the test name updated to call out the #1712 reason. Backend `test_slicer_presets.py` 47/47 green; `SliceModal.test.tsx` 34/34 green; `ConfigureAmsSlotModal.test.tsx` 24/24 green; ruff clean; frontend build clean; i18n parity 5120 leaves × 11 locales green. - **Telegram (and other image-bearing) finish notification on a reprint-from-archive showed the original print's finish photo instead of the new run's (#1707, reported by @kycrna)** — P2S user reprinted an archived job and observed the Telegram notification arriving with the photo of the *original* print (white box) attached to the completion message for the *new* run (black box). **Root cause:** reprints reuse the source archive row — `register_expected_print` stores the source `archive_id` in `_expected_prints`, and the on-print-start expected-archive promotion branch at `main.py:2207-2245` updates the row's status / started_at / printer_id / subtask_id but never reset `archive.timelapse_path`. Two failure modes cascaded from the stale path: (a) `_scan_for_timelapse_with_retries` early-returns at `main.py:3062` with `if archive.timelapse_path: return` — the reprint's new timelapse MP4 sitting on the printer's SD card was never downloaded, the archive's `timelapse_path` kept pointing at the original run's local file; (b) `_capture_finish_photo_from_timelapse` polls `archive.timelapse_path` and immediately found the *original* video, extracted ITS last frame as `finish__.jpg`, and handed those bytes to `_background_notifications` as `image_data` — which then went out to Telegram via the `sendPhoto` path. The filename was new (so log lines and the archive's `photos` list looked correct), but the pixels were the original run's finish frame. Surface was specific to the timelapse-prefer path: with `data.timelapse_was_active` true and no external camera, `prefer_timelapse_source` was True, which is the exact configuration on P2S with timelapse-on for both runs. External-camera, buffered-frame, and fresh-RTSP fallback paths grab the *current* camera frame, so users on those paths saw correct photos and the bug stayed hidden. **Fix:** at expected-archive promotion, capture and clear `archive.timelapse_path` to None before the commit, and `os.unlink` the stale on-disk video so reprints don't accumulate orphaned MP4s in the archive directory. Photos list is left alone — accumulating one finish photo per run across the archive's lifetime is the right behaviour. The unlink is wrapped in `OSError`-catching best-effort logging so a missing file (manual delete, archive purge, container rebuild with bind-mount drift) doesn't break promotion. The clear-and-unlink runs unconditionally when `timelapse_path` is set, so even if a user has been reprinting under the buggy build for months, the next reprint self-heals. **Tests:** 3 new cases in `test_reprint_clears_stale_timelapse.py` exercise the full `on_print_start` callback through the expected-archive branch — happy path (path cleared + file unlinked), no-prior-timelapse (no-op, promotion still succeeds), missing-stale-file (best-effort unlink doesn't raise). Full `test_print_start_expected_promotion.py` + `test_print_start_assigns_printer_id_to_vp_archive.py` suite (28/28) stays green; ruff clean. diff --git a/backend/app/api/routes/updates.py b/backend/app/api/routes/updates.py index 30a147624..7586f864c 100644 --- a/backend/app/api/routes/updates.py +++ b/backend/app/api/routes/updates.py @@ -181,12 +181,16 @@ def _parse_github_remote(url: str) -> tuple[str, str] | None: return (parts[0], parts[1]) -async def _origin_points_at_repo(git_path: str, git_config: list[str], base_dir, expected_repo: str) -> bool: +async def _origin_points_at_repo(git_path: str, git_config: list[str], app_dir, expected_repo: str) -> bool: """Return True iff the working tree's `origin` already resolves to `/` matching `expected_repo` (e.g. "maziggy/bambuddy"), regardless of whether it's the SSH or HTTPS form. Used to skip the `git remote set-url origin https://...` rewrite when the developer's - SSH origin is already correct — see `_perform_update` for context.""" + SSH origin is already correct — see `_perform_update` for context. + + ``app_dir`` is the working tree (where ``.git`` lives), not the data + dir — see #1715 for the separate-mount layout that proved why this + must NOT be ``base_dir``.""" try: process = await asyncio.create_subprocess_exec( git_path, @@ -194,7 +198,7 @@ async def _origin_points_at_repo(git_path: str, git_config: list[str], base_dir, "remote", "get-url", "origin", - cwd=str(base_dir), + cwd=str(app_dir), stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, ) @@ -555,7 +559,16 @@ async def _perform_update(target_ref: str): global _update_status try: - base_dir = settings.base_dir + # Every git step runs against the working tree (app_dir), NOT base_dir. + # On a standard install with DATA_DIR=INSTALL_PATH/data, git happens + # to walk up from a subdirectory of the repo to find .git so cwd=base_dir + # used to silently work — but only by accident. On a native install with + # DATA_DIR mounted at an unrelated path (e.g. /srv/bambuddy/data while + # the install is /opt/bambuddy — see #1715), git can't walk up and every + # operation fails with "not a git repository". safe.directory has the + # same requirement: it must equal the repo root git discovers, not the + # data dir, or every call returns "fatal: detected dubious ownership." + app_dir = settings.app_dir # Find git executable (may not be in PATH when running as systemd service) git_path = _find_executable("git") @@ -570,8 +583,9 @@ async def _perform_update(target_ref: str): logger.info("Using git at: %s", git_path) - # Git config to avoid safe.directory issues - git_config = ["-c", f"safe.directory={base_dir}"] + # Git config to avoid safe.directory issues — must point at the working + # tree (where .git lives), see app_dir comment above. + git_config = ["-c", f"safe.directory={app_dir}"] _update_status = { "status": "downloading", @@ -593,7 +607,7 @@ async def _perform_update(target_ref: str): # correct repo are preserved; only missing / wrong / corrupted # origins get reset to HTTPS. https_url = f"https://github.com/{GITHUB_REPO}.git" - if not await _origin_points_at_repo(git_path, git_config, base_dir, GITHUB_REPO): + if not await _origin_points_at_repo(git_path, git_config, app_dir, GITHUB_REPO): process = await asyncio.create_subprocess_exec( git_path, *git_config, @@ -601,7 +615,7 @@ async def _perform_update(target_ref: str): "set-url", "origin", https_url, - cwd=str(base_dir), + cwd=str(app_dir), stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, ) @@ -635,7 +649,7 @@ async def _perform_update(target_ref: str): "--tags", "--force", "origin", - cwd=str(base_dir), + cwd=str(app_dir), stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, ) @@ -671,7 +685,7 @@ async def _perform_update(target_ref: str): "reset", "--hard", target_ref, - cwd=str(base_dir), + cwd=str(app_dir), stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, ) @@ -696,12 +710,9 @@ async def _perform_update(target_ref: str): } # Install Python dependencies — must run from the source-code directory - # (where requirements.txt lives), not the data dir. On native installs - # systemd sets DATA_DIR=INSTALL_PATH/data, so `base_dir` is the data dir, - # not the working tree. `git reset` above worked from base_dir because - # git walks up looking for .git, but `pip install -r requirements.txt` - # needs the file in cwd literally. - app_dir = settings.app_dir + # (where requirements.txt lives). app_dir is already resolved at the top + # of this function; see the comment there for why every step uses it + # instead of base_dir. process = await asyncio.create_subprocess_exec( sys.executable, "-m", diff --git a/backend/tests/integration/test_updates_api.py b/backend/tests/integration/test_updates_api.py index 63c42b539..bb1ec72f1 100644 --- a/backend/tests/integration/test_updates_api.py +++ b/backend/tests/integration/test_updates_api.py @@ -569,3 +569,71 @@ class TestUpdatesAPI: # at the captured cwd. If this fails the cwd is wrong even if it isn't # base_dir — useful diagnostic if someone refactors path handling. assert (Path(pip_cwd) / "requirements.txt").exists() + + @pytest.mark.asyncio + async def test_perform_update_runs_git_in_app_dir_when_data_dir_on_separate_mount(self, tmp_path): + """Regression for #1715: when DATA_DIR is on a path separate from the + install (e.g. WorkingDirectory=/opt/bambuddy + DATA_DIR=/srv/bambuddy/data), + ``base_dir`` and the repo working tree are on different mounts. Pre-fix, + every git subprocess (`remote get-url`, `remote set-url`, `fetch`, + `reset --hard`) used ``cwd=base_dir`` — and git could no longer walk up + to find ``.git`` because the data dir is not a subdir of the repo. + Every update failed with "not a git repository". The fix routes every + git step (and the embedded ``safe.directory`` config) through + ``app_dir`` instead. This test pins the cwd of all four git steps so a + future refactor that re-introduces ``base_dir`` for any of them surfaces + loudly here instead of silently re-breaking native installs.""" + from backend.app.api.routes import updates as updates_module + + # Separate-mount layout: app_dir and data_dir are SIBLINGS, not parent/ + # child. base_dir is not under app_dir, so git cannot walk up. + app_dir = tmp_path / "opt" / "bambuddy" + data_dir = tmp_path / "srv" / "bambuddy" / "data" + app_dir.mkdir(parents=True) + data_dir.mkdir(parents=True) + (app_dir / "requirements.txt").write_text("fastapi\n") + + calls: list[dict] = [] + + async def fake_create_subprocess_exec(*args, **kwargs): + calls.append({"args": args, "cwd": kwargs.get("cwd")}) + proc = MagicMock() + if "get-url" in args and "origin" in args: + proc.communicate = AsyncMock(return_value=(b"git@github.com:maziggy/bambuddy.git\n", b"")) + else: + proc.communicate = AsyncMock(return_value=(b"", b"")) + proc.returncode = 0 + return proc + + with ( + patch.object(updates_module.settings, "base_dir", data_dir), + patch.object(updates_module.settings, "app_dir", app_dir), + patch.object(updates_module, "_find_executable", return_value="/usr/bin/git"), + patch.object( + updates_module.asyncio, + "create_subprocess_exec", + side_effect=fake_create_subprocess_exec, + ), + ): + await updates_module._perform_update("v0.2.4b1") + + # Every git subprocess must run in app_dir (the working tree). A + # regression to base_dir would silently break #1715-class installs. + git_calls = [c for c in calls if c["args"] and c["args"][0] == "/usr/bin/git"] + assert git_calls, "no git subprocess was invoked; setup is wrong" + wrong_cwd = [c for c in git_calls if c["cwd"] != str(app_dir)] + assert not wrong_cwd, ( + "git subprocess ran with cwd != app_dir; #1715 would resurface. " + f"Offending calls: {[(c['args'][1:5], c['cwd']) for c in wrong_cwd]}" + ) + + # ``safe.directory`` must equal app_dir (the repo root git discovers), + # not the data dir — otherwise git refuses with "dubious ownership" + # even when the cwd is technically correct. + safe_dir_configs = [ + arg for c in git_calls for arg in c["args"] if isinstance(arg, str) and arg.startswith("safe.directory=") + ] + assert safe_dir_configs, "safe.directory config was never set on git calls" + assert all(s == f"safe.directory={app_dir}" for s in safe_dir_configs), ( + f"safe.directory must point at app_dir ({app_dir}); got {safe_dir_configs}" + )