mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-09-30 03:01:21 +02:00
fix(updates): preserve SSH origin pointing at the right repo
The in-app Apply Update path unconditionally ran `git remote set-url origin https://github.com/maziggy/bambuddy.git` before fetching, on the theory that systemd service users wouldn't have SSH keys. True in production, but it also clobbered every developer's SSH origin the moment they tested the upgrade flow against their own checkout. Next `git push` then prompted for HTTPS credentials and bounced. New behaviour: read `origin` first via `git remote get-url`, parse out the (owner, repo) pair using a small helper that handles all four canonical forms (git@github.com:owner/repo[.git] and https://github.com/owner/repo[.git]), and only rewrite if it doesn't already resolve to maziggy/bambuddy. Native installs with no remote or pointing at a fork still get reset to the canonical HTTPS URL. Three new regression tests in test_updates_api.py: - parser accepts SSH/HTTPS, with/without .git, rejects non-GitHub - SSH origin pointing at maziggy/bambuddy is preserved (the developer-footgun case) - origin pointing at a fork still gets rewritten to HTTPS (the original behaviour we don't want to lose)
This commit is contained in:
@@ -49,6 +49,7 @@ All notable changes to Bambuddy will be documented in this file.
|
||||
- **i18n: full key parity across all 8 locales** — `en` is the reference; every other locale (`de`, `fr`, `it`, `ja`, `pt-BR`, `zh-CN`, `zh-TW`) is checked identically and any drift fails CI. Until now, the parity script at `frontend/scripts/check-i18n-parity.mjs` only enforced parity for `de` / `zh-CN` / `zh-TW` and demoted `fr` / `it` / `ja` / `pt-BR` to an "informational" tier — drift was reported but never gated. Result: 78 missing keys in fr and it, 66 in pt-BR, 54 in ja, accumulated across every release that added new en strings. Backfilled real translations (not English fallbacks) for every gap: `login.resetPassword.*` (12 keys, fr/it/ja), `printers.firmwareModal.*` extension (7 keys × 4 locales) from the firmware modal redesign, the full `settings.spoolbuddy.*` device-control admin block (~40 keys) for unregister / reboot / shutdown / update / restart confirms, the kiosk-side `spoolbuddy.settings.*` block (13 keys × 4) for backend & auth + diagnostics, and the new `virtualPrinter.archiveNameSource.*` block from this release ([#1152](https://github.com/maziggy/bambuddy/issues/1152)). The parity script itself dropped the two-tier `STRICT` / `info` machinery — every non-en locale is now treated equally — so any future feature that adds en strings without translating them everywhere fails CI uniformly. All 8 locales sit at 4492 leaves.
|
||||
|
||||
### Fixed
|
||||
- **In-app upgrade clobbered SSH `origin` on developer checkouts** — The in-app *Apply Update* path unconditionally ran `git remote set-url origin https://github.com/maziggy/bambuddy.git` before fetching, on the assumption that systemd service users wouldn't have SSH keys configured. That assumption holds for production native installs, but anyone testing the upgrade flow against their own development checkout (where `origin` is legitimately `git@github.com:maziggy/bambuddy.git` and authentication is via SSH keys) had their SSH origin silently rewritten to HTTPS — so the very next `git push` prompted for HTTPS credentials they didn't have configured and bounced. Fix: the updater now reads the current `origin` first via `git remote get-url`, parses the URL into an `(owner, repo)` pair (handling all four canonical forms — `git@github.com:owner/repo[.git]` and `https://github.com/owner/repo[.git]`), and only rewrites if it doesn't already resolve to `maziggy/bambuddy`. Native installs with no remote set, or origins pointing at a fork / wrong repo, still get reset to the canonical HTTPS URL. Three regression tests in `test_updates_api.py` cover the parser, the SSH-preservation case, and the fork-rewrite case so a future refactor can't regress either side of the contract.
|
||||
- **Native-install in-app upgrade silently skipped `pip install` and the new dependencies never landed** — On a native install (where systemd sets `DATA_DIR=$INSTALL_PATH/data`), the in-app *Apply Update* button shipped the new code via `git reset --hard origin/main` correctly but then logged `ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'requirements.txt'` and continued without installing the new deps. `pip install -r requirements.txt` was running with `cwd=settings.base_dir`, which on a native install resolves to the data dir (e.g. `/opt/bambuddy/data`), not the source-code dir (`/opt/bambuddy`); pip doesn't walk up looking for the requirements file the way `git` walks up looking for `.git`, so the file wasn't found, the install was effectively skipped, and the user ended up with new code but stale dependencies — which surfaces as cryptic import / runtime errors on the next restart. Same bug affected the optional `npm install` / `npm run build` step (it tested `frontend_dir = base_dir / "frontend"`, which doesn't exist on native installs, and silently fell through to the pre-built static files). Fix: introduce `settings.app_dir` alongside `settings.base_dir` pointing at the source-tree root, and run `pip install` and the npm steps with `cwd=settings.app_dir`. Git operations keep using `base_dir` since they already worked (git walks up to find `.git`). Docker users were unaffected — Docker doesn't use the in-app updater (image pull replaces it). Regression test in `test_updates_api.py` mocks every subprocess invocation in `_perform_update`, captures their cwd, and asserts the pip step runs in `app_dir` and that `requirements.txt` actually exists there, so a future refactor that re-introduces `cwd=base_dir` for the pip step fails CI before another user trips over it.
|
||||
- **Postgres restore from a SQLite Local Backup aborted with `cannot drop table printers`** — Settings → Backup → Restore on a Bambuddy running against external Postgres failed with `asyncpg.exceptions.DependentObjectsStillExistError: cannot drop table printers because other objects depend on it` whenever the live database carried orphan tables from removed features — for example legacy `spoolman_slot_assignments` / `spoolman_k_profile` from an earlier Spoolman integration that has since been removed from the ORM but whose tables and `*_printer_id_fkey` constraints still sat in the live schema, pointing at `printers`. The restore path (`_import_sqlite_to_postgres` in `settings.py`) called `metadata.drop_all`, which only enumerates tables defined by SQLAlchemy ORM models and emits plain `DROP TABLE` (no `CASCADE`); Postgres correctly refused to drop `printers` while external constraints still referenced it, the entire restore aborted before any rows landed, and the user was left without a working DB. The drop phase now executes `DROP TABLE … CASCADE` on every table in the `public` schema (via a `pg_tables`-iterating PL/pgSQL `DO` block, after FKs have been stripped from the ORM metadata) before `metadata.create_all` rebuilds the schema. CASCADE is the right tool for a destructive restore — the user has explicitly chosen to wipe the DB and replace it from backup, so taking out orphan tables alongside ORM tables is correct behaviour, not surprise data loss. SQLite restores are unaffected (they go through a separate path). Discovered while attempting to restore a 0.2.4b1 backup onto a Postgres instance that had been upgraded across the Spoolman integration rewrite. Two regression tests in `test_postgres_restore_drop_cascade.py` mock the Postgres engine, run `_import_sqlite_to_postgres` against a tiny SQLite source, and assert (1) the captured SQL stream contains a CASCADE-aware iteration over `pg_tables` (so a regression to `metadata.drop_all` fails CI loudly, before another user trips on it) and (2) the CASCADE drop is scoped to `schemaname = 'public'` so a shared Postgres instance holding non-Bambuddy data in other schemas isn't taken out by a restore. All 44 existing settings-API tests still pass unchanged.
|
||||
- **H2D Pro multi-plate dispatch double-/triple-fire** ([#1157](https://github.com/maziggy/bambuddy/issues/1157)) — Scheduling 3 plates of a multi-plate file to the same H2D Pro caused the scheduler to fire all three `project_file` commands within ~60 seconds, even though the printer hadn't transitioned out of `FINISH` for the first one yet. The H2D Pro can sit at `FINISH` for 80–210 s after accepting `project_file` before the `gcode_state` flips to `PREPARE`, and during that window the existing DB `busy_printers` seed (querying queue items in `printing` status) was empirically missing the in-flight item — observed in support logs as items 139/140/141 all dispatching with status='printing' yet only the third actually triggering a state transition. User-visible symptoms: layer count flapping, all queued plates showing as printing simultaneously, MQTT disconnect storms (33 in a single 5-minute window), eventual print failure. Root-cause fix is a defensive in-memory dispatch hold layer in `print_scheduler.py`: when `_start_print` succeeds we record `(printer_id, dispatched_at, pre_state, pre_subtask_id)`, and the next `check_queue` tick adds that printer to `busy_printers` until either (a) the watchdog observes a state/subtask transition (success path — release immediately past a 60 s minimum cooldown), or (b) a 180 s hard timeout expires (escape hatch for lost MQTT sessions). The minimum cooldown also prevents a spurious double-dispatch if the printer pulses through PREPARE→RUNNING→PREPARE in the first second after acceptance. The hold is purely additive — sits alongside the existing seed query and `_is_printer_idle` checks, doesn't depend on DB row visibility, doesn't depend on `on_print_complete` firing correctly. Per-printer isolation: a hold on printer A never blocks printer B. Edge cases covered by 12 new unit tests (`test_scheduler_dispatch_hold.py`): no-pre-state fallback (printer was offline at dispatch time), status-unavailable keeps hold (printer disconnected post-dispatch — don't release on missing data), idempotent release, hard-timeout self-cleanup, transition-during-cooldown still holds. The 90 s watchdog still owns the unhappy-path revert (queue item back to `pending` for retry) — this fix runs alongside it, not instead of it. All 179 existing scheduler tests still pass unchanged.
|
||||
|
||||
@@ -78,6 +78,78 @@ def _find_executable(name: str) -> str | None:
|
||||
return None
|
||||
|
||||
|
||||
def _parse_github_remote(url: str) -> tuple[str, str] | None:
|
||||
"""Extract `(owner, repo)` from a GitHub remote URL, or None if it isn't a
|
||||
GitHub URL we recognise.
|
||||
|
||||
Handles the four forms `git remote -v` typically prints:
|
||||
- `git@github.com:owner/repo.git` (SSH, the dev default)
|
||||
- `git@github.com:owner/repo` (SSH without .git suffix)
|
||||
- `https://github.com/owner/repo.git` (HTTPS, what _perform_update sets)
|
||||
- `https://github.com/owner/repo` (HTTPS without .git)
|
||||
|
||||
Anything else (a fork URL, a different host, a malformed value, the empty
|
||||
string from a missing origin) returns None so the caller treats it as
|
||||
"not pointing at our repo" and resets it.
|
||||
"""
|
||||
s = url.strip()
|
||||
if not s:
|
||||
return None
|
||||
# SSH form: git@github.com:owner/repo[.git]
|
||||
ssh_prefix = "git@github.com:"
|
||||
https_prefix_a = "https://github.com/"
|
||||
https_prefix_b = "http://github.com/" # tolerated for legacy
|
||||
if s.startswith(ssh_prefix):
|
||||
path = s[len(ssh_prefix) :]
|
||||
elif s.startswith(https_prefix_a):
|
||||
path = s[len(https_prefix_a) :]
|
||||
elif s.startswith(https_prefix_b):
|
||||
path = s[len(https_prefix_b) :]
|
||||
else:
|
||||
return None
|
||||
if path.endswith(".git"):
|
||||
path = path[:-4]
|
||||
parts = path.strip("/").split("/")
|
||||
if len(parts) != 2 or not parts[0] or not parts[1]:
|
||||
return 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:
|
||||
"""Return True iff the working tree's `origin` already resolves to
|
||||
`<owner>/<repo>` 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."""
|
||||
try:
|
||||
process = await asyncio.create_subprocess_exec(
|
||||
git_path,
|
||||
*git_config,
|
||||
"remote",
|
||||
"get-url",
|
||||
"origin",
|
||||
cwd=str(base_dir),
|
||||
stdout=asyncio.subprocess.PIPE,
|
||||
stderr=asyncio.subprocess.PIPE,
|
||||
)
|
||||
stdout, _ = await process.communicate()
|
||||
except (OSError, asyncio.CancelledError):
|
||||
# Fail closed: let the caller go through the rewrite branch if we
|
||||
# can't even invoke git. The unconditional set-url is the safer
|
||||
# fallback, only mildly destructive.
|
||||
return False
|
||||
if process.returncode != 0:
|
||||
# Most likely cause: no `origin` defined yet (fresh clone-style
|
||||
# checkout). Caller will set it.
|
||||
return False
|
||||
parsed = _parse_github_remote(stdout.decode().strip())
|
||||
if parsed is None:
|
||||
return False
|
||||
owner, repo = parsed
|
||||
expected_owner, expected_repo_name = expected_repo.split("/", 1)
|
||||
return owner == expected_owner and repo == expected_repo_name
|
||||
|
||||
|
||||
def parse_version(version: str) -> tuple:
|
||||
"""Parse version string into tuple for comparison.
|
||||
|
||||
@@ -341,20 +413,32 @@ async def _perform_update():
|
||||
"error": None,
|
||||
}
|
||||
|
||||
# Ensure remote uses HTTPS (SSH may not be available)
|
||||
# Ensure remote points at the expected repo. We previously rewrote
|
||||
# origin to HTTPS unconditionally on the assumption that systemd
|
||||
# service users wouldn't have SSH keys configured — which is fine
|
||||
# for that case, but stomps on developer checkouts where origin is
|
||||
# legitimately `git@github.com:maziggy/bambuddy.git` and the user
|
||||
# auths via SSH keys. After the rewrite, `git push` prompts for
|
||||
# HTTPS credentials and fails.
|
||||
# New behaviour: read the current origin, parse out the
|
||||
# `<owner>/<repo>` pair, and only rewrite if it doesn't already
|
||||
# resolve to the right GitHub repo. SSH origins pointing at the
|
||||
# correct repo are preserved; only missing / wrong / corrupted
|
||||
# origins get reset to HTTPS.
|
||||
https_url = f"https://github.com/{GITHUB_REPO}.git"
|
||||
process = await asyncio.create_subprocess_exec(
|
||||
git_path,
|
||||
*git_config,
|
||||
"remote",
|
||||
"set-url",
|
||||
"origin",
|
||||
https_url,
|
||||
cwd=str(base_dir),
|
||||
stdout=asyncio.subprocess.PIPE,
|
||||
stderr=asyncio.subprocess.PIPE,
|
||||
)
|
||||
await process.communicate()
|
||||
if not await _origin_points_at_repo(git_path, git_config, base_dir, GITHUB_REPO):
|
||||
process = await asyncio.create_subprocess_exec(
|
||||
git_path,
|
||||
*git_config,
|
||||
"remote",
|
||||
"set-url",
|
||||
"origin",
|
||||
https_url,
|
||||
cwd=str(base_dir),
|
||||
stdout=asyncio.subprocess.PIPE,
|
||||
stderr=asyncio.subprocess.PIPE,
|
||||
)
|
||||
await process.communicate()
|
||||
|
||||
_update_status = {
|
||||
"status": "downloading",
|
||||
|
||||
@@ -47,6 +47,135 @@ class TestUpdatesAPI:
|
||||
|
||||
assert is_newer_version("0.1.5", "0.1.5b7") is True
|
||||
|
||||
def test_parse_github_remote_recognises_ssh_https_and_dotgit(self):
|
||||
"""`_parse_github_remote` must accept the four canonical forms `git
|
||||
remote -v` prints; anything else returns None so callers can treat
|
||||
it as 'reset to expected URL'."""
|
||||
from backend.app.api.routes.updates import _parse_github_remote
|
||||
|
||||
assert _parse_github_remote("git@github.com:maziggy/bambuddy.git") == (
|
||||
"maziggy",
|
||||
"bambuddy",
|
||||
)
|
||||
assert _parse_github_remote("git@github.com:maziggy/bambuddy") == (
|
||||
"maziggy",
|
||||
"bambuddy",
|
||||
)
|
||||
assert _parse_github_remote("https://github.com/maziggy/bambuddy.git") == (
|
||||
"maziggy",
|
||||
"bambuddy",
|
||||
)
|
||||
assert _parse_github_remote("https://github.com/maziggy/bambuddy") == (
|
||||
"maziggy",
|
||||
"bambuddy",
|
||||
)
|
||||
# Non-GitHub host → None (we don't claim ownership over arbitrary
|
||||
# forge URLs).
|
||||
assert _parse_github_remote("git@gitlab.com:maziggy/bambuddy.git") is None
|
||||
# Empty / malformed → None.
|
||||
assert _parse_github_remote("") is None
|
||||
assert _parse_github_remote("not-a-url") is None
|
||||
assert _parse_github_remote("https://github.com/maziggy") is None # no /repo
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_perform_update_preserves_ssh_origin_when_pointing_at_correct_repo(self, tmp_path):
|
||||
"""Regression for the developer-checkout footgun: if origin already
|
||||
points at github.com/maziggy/bambuddy via SSH, the updater must
|
||||
leave it alone instead of clobbering it with HTTPS. Pre-fix, every
|
||||
Apply Update click rewrote `git@github.com:...` to `https://...`,
|
||||
breaking subsequent `git push` for any developer testing the
|
||||
upgrade flow against their own checkout."""
|
||||
from backend.app.api.routes import updates as updates_module
|
||||
|
||||
app_dir = tmp_path / "app"
|
||||
data_dir = tmp_path / "app" / "data"
|
||||
app_dir.mkdir()
|
||||
data_dir.mkdir()
|
||||
(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()
|
||||
# When the updater asks `git remote get-url origin`, return the
|
||||
# SSH URL. Every other subprocess returns successfully with no
|
||||
# output.
|
||||
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()
|
||||
|
||||
# The updater MUST NOT have run `git remote set-url origin <https>`
|
||||
# because origin already pointed at the right repo over SSH.
|
||||
set_url_calls = [c for c in calls if "set-url" in c["args"] and "origin" in c["args"]]
|
||||
assert not set_url_calls, (
|
||||
"Updater clobbered an SSH origin pointing at the correct repo. "
|
||||
"Captured set-url calls: " + repr([c["args"] for c in set_url_calls])
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_perform_update_resets_origin_when_pointing_elsewhere(self, tmp_path):
|
||||
"""Defensive: if origin points at a fork or unrelated repo (or is
|
||||
missing), the updater should still rewrite it to the canonical
|
||||
HTTPS URL so subsequent fetch / reset works against the right
|
||||
repo. This is the original behaviour that the SSH-preservation
|
||||
fix above must NOT regress."""
|
||||
from backend.app.api.routes import updates as updates_module
|
||||
from backend.app.core.config import GITHUB_REPO
|
||||
|
||||
app_dir = tmp_path / "app"
|
||||
data_dir = tmp_path / "app" / "data"
|
||||
app_dir.mkdir()
|
||||
data_dir.mkdir()
|
||||
(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()
|
||||
# origin is set to a fork — must be rewritten.
|
||||
if "get-url" in args and "origin" in args:
|
||||
proc.communicate = AsyncMock(return_value=(b"git@github.com:somefork/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()
|
||||
|
||||
set_url_calls = [c for c in calls if "set-url" in c["args"] and "origin" in c["args"]]
|
||||
assert set_url_calls, "Updater must rewrite origin when it points at a fork."
|
||||
rewritten_to = set_url_calls[0]["args"][-1]
|
||||
assert rewritten_to == f"https://github.com/{GITHUB_REPO}.git", (
|
||||
f"Expected origin to be reset to canonical HTTPS URL; got: {rewritten_to}"
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_perform_update_runs_pip_in_app_dir_not_data_dir(self, tmp_path):
|
||||
"""Native install: `requirements.txt` lives at INSTALL_PATH (the source-
|
||||
|
||||
Reference in New Issue
Block a user