diff --git a/CHANGELOG.md b/CHANGELOG.md index ba4205381..c2759b460 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ All notable changes to Bambuddy will be documented in this file. ### Security - **Path-traversal hardening across the upload / import / file-write surface (routes + services); fifth CI backstop ships alongside** — A private path-traversal report against `POST /api/v1/projects/import/file` traced two attacker-controlled strings being joined to `library_dir` with no resolve + containment check: (a) `linked_folders[*].name` from the request's `project.json` ("Vector A" — an absolute path in this field collapsed `library_dir / "/anywhere"` to `Path("/anywhere")` because pathlib discards the left side when the right is absolute, letting the next `write_bytes` land anywhere the backend could write), and (b) per-entry `zf.namelist()` paths from the ZIP itself ("Vector B" — ZIP filenames carry `..` segments by spec and the join `library_dir / folder_name / relative_path` had no per-component check). Concrete escalation: drop a `.pth` file into the venv's `site-packages` directory for code execution on next service restart; overwrite the JWT signing-secret file to forge an admin token; overwrite `~/.ssh/authorized_keys` or `~/.bashrc` on native installs. **Fix is structural, not just patch the diff** (per [[feedback_dont_dismiss_preexisting]]). New `backend/app/utils/safe_path.py::safe_join_under(parent, *parts)` helper joins under a trusted parent, resolves both sides, asserts `is_relative_to(parent.resolve())`, and rejects up-front empty / null-byte / absolute path components. Wired into `import_project_file` at both vectors. **Adjacent fix from the routes audit**: `GET /api/v1/archives/{id}/photos/{filename}` had NO validation on `filename` and FileResponse-served arbitrary paths — the existing DELETE endpoint at least had a membership check against `archive.photos` (which is UUID-generated on upload), but GET shared neither the check nor any traversal guard. Both GET and DELETE now route through `safe_join_under` for defence-in-depth on top of the membership check. **Second adjacent fix from the services audit**: `ArchiveService.attach_timelapse(archive_id, data, filename)` in `backend/app/services/archive.py:1456` wrote `archive_dir / filename` where `filename` ultimately comes from either a printer's FTP listing (compromised-printer threat model — the printer is part of the trust surface) or the `?filename=...` query param on `POST /api/v1/archives/{id}/timelapse/select`. A malicious printer that returns a directory listing entry with `..` segments could write the timelapse bytes outside the archive directory; the `f.get("name") == filename` gate in the route did not prevent it because the gate is satisfied by whatever the printer claims is on disk. `attach_timelapse` now routes through `safe_join_under(..., http=False)` and returns `False` (logging the rejection) when the join would escape — matching the existing not-found contract of the function rather than raising 400 from inside a background task. **Audit sweep methodology**: AST-walked every Python file under `backend/app/api/routes/` AND `backend/app/services/` for `Path / Name` shapes (the exact shape that produced the original report). 25 additional route-layer sites and 8 additional service-layer sites confirmed safe case-by-case (UUID-generated filenames written by Bambuddy itself, `_safe_filename(...)` / `Path(arg).name` basename-stripped inputs, `os.walk`-discovered names, denylist + format-validated backup names, hardcoded constants iterated through a tuple, DB-stored paths whose write origin already goes through a resolved-and-containment-checked helper). Each safe site got a `# SEC-PATH-OK: ` marker so future audits can trust the inline guard at a glance. Six pre-existing safe-with-marker sites (`library.py` external upload, `archives.py` timelapse output, `projects.py` attachment download/delete, `settings.py` backup extractall) carry the same marker shape. **Fifth CI backstop** `test_route_path_arithmetic_is_safe_joined_or_marked` (`backend/tests/unit/test_no_unsafe_path_joins.py`) AST-walks every Python file in `backend/app/api/routes/` AND `backend/app/services/` and fails the build on any ` / ` join that doesn't either route through `safe_join_under` or carry the marker on the join line. Joins matching the higher-structure shapes (Attribute access, Subscript, f-string, `str(...)` call) are categorically different and out of scope — those are caught by the broader audit sweep, not the regression backstop. The services layer is in scope because it receives values from the routes verbatim AND from external sources Bambuddy has no control over (the printer FTP-listing case above). **Tests**: 17 unit tests for `safe_join_under` covering every escape vector (absolute path, Windows abs path, `..` segments, embedded `..`, null byte, empty string, no parts, non-str, plus legitimate nested-path round-trip); 4 integration tests against `POST /api/v1/projects/import/file` exercising the full FastAPI stack with the verbatim shape from the report (absolute path in `folder_name` → 400 + filesystem assertion that the target file doesn't exist; `..` in `folder_name` → 400; `..` in `relative_path` → 400; legitimate nested ZIP still imports cleanly to guard against the fix being over-strict); 3 unit tests against `ArchiveService.attach_timelapse` exercising the compromised-printer threat model (filename with `..` segments → returns False + no file at the escape target; absolute filename → returns False + no file at `/tmp`; legitimate `timelapse_YYYY-MM-DD_HH-MM-SS.mp4` → returns True + file lands inside archive_dir, guarding against the fix being over-strict). **SECURITY.md** gains a fifth rule + a fifth row in the CI-test mapping table; the rule explicitly names the printer FTP-listing case as in-scope to set the expectation for future services-layer audits. Full 5500+ test backend suite green; ruff clean. ### Fixed +- **Archive card's Print Time + accuracy badge are now consistent for multi-run / multi-plate archives (#1608, reported via an AI-assisted diagnosis that included the failing SQL, file line numbers, and a worked example for archive #65)** — Reporter's case: 3-plate `.gcode.3mf` printed plate-by-plate over 9 runs. Card showed `1h 46m +188%` next to the now-correct `156.7g` / `$9.81`. The 1h 46m = 6364 s = one run's `completed_at − started_at`; the +188% = `print_time_seconds / 6364` − 100 % where `print_time_seconds` is 18354 s (the whole-file estimate the #1593 parser fix correctly stores). The two halves describe different scopes — apples-to-oranges. **Root cause** — `backend/app/api/routes/archives.py::compute_time_accuracy` (line 152) only inspects the archive row's own `started_at` / `completed_at`, which reflect the latest run, while `archive.print_time_seconds` is the sum across plates post-#1593. The existing 5-500 % sanity band catches truly broken values but lets the deterministic N×100% shape through (300% for a 3-plate file). `archive_to_response` calls `compute_time_accuracy` on every list / detail / search / project-archive / patch render (line 275), so the bad number reaches the frontend on every card surface. The stats endpoint (`/api/v1/archives/stats`, line 940-988) has its OWN per-run accuracy loop with a tighter 50-200 % band filter shipped with #1593 — that's untouched and stays correct. **Fix** — `compute_time_accuracy(archive, run_aggregate=None)` gains an optional `run_aggregate` argument. When `run_aggregate["run_count"] > 1`, both `actual_time_seconds` and `time_accuracy` are returned as `None`. The frontend already falls through to `archive.print_time_seconds` for the Time display (`archive.actual_time_seconds || archive.print_time_seconds`) and conditionally renders the badge only when `archive.time_accuracy` is truthy, so multi-run archives now show "Estimated 5h 6m" with no badge instead of "Actual 1h 46m +188%". Single-run archives — the case the badge was designed for, and the only case where one-run actual versus whole-file estimate is a meaningful ratio — keep the original behaviour verbatim. **Audit-wide** — `archive_to_response` now passes `run_aggregate` through to `compute_time_accuracy` at the response-conversion call site. The 3 endpoints that did NOT previously load run aggregates (`backend/app/api/routes/archives.py` search endpoint's pre-FTS fast path at line 583 and FTS path at line 610, the single-archive PATCH endpoint at line 1419, and `backend/app/api/routes/projects.py::list_project_archives` at line 706) now batch-load `_load_run_aggregates` and pass it through, so the badge-suppression applies on every card surface — not just the main list and detail endpoints. One extra `SELECT … GROUP BY archive_id` per endpoint (the helper is already batched), cheap. Per the [[feedback_pr_reviews_thorough]] HARD RULE the fix is shipped across every call site that renders an archive card. **What this does NOT change** — the stats endpoint's per-run accuracy aggregation at line 940-988, its 50-200% band filter, the `archive.started_at` / `completed_at` source-of-truth for the latest-run timestamps, the frontend `ArchivesPage.tsx:1004-1022` rendering logic, or the time-accuracy computation for single-run archives. Reprint scope is unchanged (the reporter's option A — comparing summed run durations against the whole-file estimate — was not pursued because it produces a different but equally misleading number for the reprints-of-a-single-plate-file shape, where sum-of-runs = N × estimate). **Tests** — `backend/tests/unit/test_archive_run_aggregation.py::TestComputeTimeAccuracyMultiRun`: 4 new direct unit tests for the function — single-run archive keeps original badge, no `run_aggregate` argument keeps original badge (defends the legacy caller pattern), multi-run archive (reporter's exact 9-run case) clears both fields, and `run_count: 0` edge case keeps original behaviour. Two new integration tests against the live archives list endpoint: `test_archive_list_suppresses_time_accuracy_for_multi_run_archives` is the #1608 regression (3-plate plate-by-plate fixture, asserts both `actual_time_seconds is None` and `time_accuracy is None` AND that the estimate `print_time_seconds` survives so the card has something to render), and `test_archive_list_keeps_time_accuracy_for_single_run_archives` is the sanity check that the badge still shows for the happy path. Full backend pytest 3680 passed under `-n 30`; ruff clean. **No frontend change required** — the existing rendering logic in `ArchivesPage.tsx:1004-1026` (`formatDuration(archive.actual_time_seconds || archive.print_time_seconds || 0)` for the time + `{archive.time_accuracy && …}` conditional badge) naturally produces the desired "show estimate, no badge" presentation when the backend returns null for both fields. - **Queue / Review / Archive virtual-printer modes now complete the TLS handshake on hardened-distro hosts (#1610, reported by an AI-assisted diagnosis)** — Reporter ran Bambuddy in a container on a hardened-policy host (Fedora / RHEL with `update-crypto-policies` or similar) and OrcaSlicer / BambuStudio failed to connect to any non-proxy-mode virtual printer with `code=-1` after the TCP handshake completed. Switching the same VP to Proxy mode worked. They grepped the container and pinpointed that the #620 cipher-suite fix only patched `tcp_proxy.py::_create_client_ssl_context` (printer-facing) and missed every other slicer-facing VP TLS context. Diagnosis confirmed: real Bambu printers (and slicer paths written to mimic them) offer only the plain-RSA AES-GCM suites `AES256-GCM-SHA384` / `AES128-GCM-SHA256`; on stock OpenSSL these stay in `DEFAULT`, but a system crypto policy that strips them leaves the server side offering ECDHE-only and the slicer's ClientHello finds no overlap — handshake aborts before any data flows. **Audit-wide fix (4 contexts patched, one regression test class per site)**: (a) `backend/app/services/virtual_printer/bind_server.py::_create_tls_context` (port 3002, used by Queue/Review/Archive modes — the literal reported failure) — added `ctx.set_ciphers("DEFAULT:AES256-GCM-SHA384:AES128-GCM-SHA256")`; (b) `backend/app/services/virtual_printer/mqtt_server.py::SimpleMQTTServer.start` (port 8883, slicer MQTT-over-TLS) — same cipher pin; (c) `backend/app/services/virtual_printer/tcp_proxy.py::TLSProxy._create_server_ssl_context` (slicer side of Proxy-mode 3002 — the #620 fix's *other* half that was never written) — same cipher pin; (d) `backend/app/services/virtual_printer/ftp_server.py::VirtualPrinterFTPServer.start` (port 990, FTPS upload) — changed from the historical `HIGH:!aNULL:!MD5:!RC4` to `HIGH:AES256-GCM-SHA384:AES128-GCM-SHA256:!aNULL:!MD5:!RC4`. The `HIGH` baseline is kept verbatim (not replaced with `DEFAULT`) so the cipher set stays a strict superset of what shipped before — the original `HIGH` set offers ~58 ciphers `DEFAULT` doesn't (CCM, ARIA, CAMELLIA, DSS variants); none are picked by any known Bambu slicer, but the [[feedback_dont_remove_compat_pinning]] HARD RULE says don't narrow a compat surface without proof. The `!aNULL:!MD5:!RC4` exclusions are preserved as well. For the three new contexts (a/b/c), the cipher string is `DEFAULT:AES256-GCM-SHA384:AES128-GCM-SHA256` — verbatim match with the #620 client-side fix. **Verified strict-superset** at audit time (`{c['name'] for c in old.get_ciphers()}.issubset({c['name'] for c in new.get_ciphers()})` returns True for all four call sites). Per the [[feedback_vp_regression_matrix]] HARD RULE, the slicer-facing TLS surface gets the fix in one drop instead of a per-mode-per-issue ticket trail. Per [[feedback_dont_remove_compat_pinning]], the `minimum_version = TLSv1_2` and the FTPS `maximum_version = TLSv1_2` pins (the BambuStudio FTPS-data-channel-PSK-reuse compat) are unchanged — only the cipher list is widened, never narrowed. **Tests** (`backend/tests/unit/test_vp_tls_ciphers.py`, new file): 5 cases — one per slicer-facing surface (bind / mqtt / proxy-server / ftp) asserting `AES256-GCM-SHA384` AND `AES128-GCM-SHA256` are in the SSL context's offered cipher list, plus a guard test that the original #620 client-side `tcp_proxy._create_client_ssl_context` still has them so this audit's edits can't accidentally regress that fix. Cipher assertions use the production `CertificateService` to generate a real self-signed CA + per-VP cert pair into `tmp_path` (rather than mocking `load_cert_chain`) so the SSL contexts are configured exactly as production would. Full backend unit suite 3674 passed under `-n 30`; ruff clean. **Why neither maintainer nor the local CI box reproduces the bug**: stock OpenSSL 3.x ships `AES256-GCM-SHA384` / `AES128-GCM-SHA256` in `DEFAULT` already, so the missing `set_ciphers()` calls were harmless on most builds — verifiable with `python -c "import ssl; print(c['name'] for c in ssl.SSLContext().get_ciphers())"`. The bug only manifests on builds where a system crypto policy or vendor build flag narrows the default to forward-secrecy-only. The explicit cipher pins now survive any such narrowing. **What this does NOT do**: the printer-side TLS surface (proxy client context, MQTT printer-client context in `bambu_mqtt.py`) is unchanged — printers always sit behind the unfiltered #620 client pin, and there's no equivalent failure mode reported there. - **External-spool usage is now tracked when the AMS has empty slots in between loaded ones (#1607, reported by @ahmtcnby)** — Reporter had AMS slots 0–2 loaded, slot 3 empty, and an external spool. After every multi-filament print, the external's weight never decremented; assigning the external spool to the empty AMS slot 3 in Bambuddy made the deduction appear correctly. Root cause: when no explicit slot-to-tray mapping is available (path 5 of 6 in `usage_tracker.py::_track_from_3mf`, e.g. the very first print after a fresh container start before the request-topic subscription that captures `ams_mapping` from `print_command` is accepted — the bundle showed `Request topic subscription accepted. ams_mapping capture enabled` only fired at 19:36:29), the tracker falls back to a position-based mapping built from `spoolman_tracking.py::build_ams_tray_lookup`. That helper enumerated every AMS tray by `id` regardless of whether a spool was loaded, so the reporter's layout yielded `available_trays = [0, 1, 2, 3, 254]`. BambuStudio / OrcaSlicer compact their filament-assignment UI by hiding unloaded AMS slots — the slicer's 4th filament is the external, so the 3MF carries slot_ids 1–4 with slot 4 = external. Position-based mapping then routed slot 4 → `available_trays[3]` = AMS0-T3 (the empty slot) instead of 254 (the external). No spool assignment exists at AMS0-T3, so usage was silently skipped at line 1273-1274 and the external spool's weight stayed unchanged. **Fix** (`backend/app/services/usage_tracker.py:1232-1245`): the position-based fallback now filters `build_ams_tray_lookup`'s output to slots whose `tray_type` is non-empty before sorting. `build_ams_tray_lookup` itself is unchanged (its other callers — `spoolman_tracking.store_print_data`, `routes/printers`, `spool_assignment_notifications` — want every physical slot for AMS-state purposes); the filter is applied at the call site so we only narrow what the fallback uses. vt_tray entries are already filtered the same way inside `build_ams_tray_lookup` at line 174 (`if vt.get("tray_type"):`) — this mirrors that behaviour for the AMS side. **Why the reporter's workaround helped**: assigning the external spool to AMS0-T3 in Bambuddy made the wrong-target rewrite accidentally land on the right spool — the empty AMS slot resolved to the same spool the external was fed from. The fix removes the need for that workaround. **Tests**: 2 new in `backend/tests/unit/test_usage_tracker.py::TestPositionBasedFallbackEmptyAmsSlot` — `test_external_routed_correctly_when_ams_has_empty_middle_slot` is the literal #1607 regression (3 AMS slots loaded + 1 empty + external loaded, slicer's slot 4 must charge spool at AMS255-T0, **must NOT** charge anything at AMS0-T3 — explicit `(0, 3) not in handled_trays` assertion); `test_dense_ams_unchanged_no_empty_slots` is the no-empty-slots sanity check that confirms the fix doesn't regress the everyday case (4 AMS slots all loaded + external → slot 5 still maps to external). Path priority order unchanged: explicit `print_cmd` / MQTT / queue / color-match mappings still override the position-based fallback, so this only changes behaviour when none of those paths fired. Full backend pytest 3669 passed under `-n 30`; ruff clean. - **Custom maintenance type "documentation URL" now persists on create (#1596, reported by @BurntOutHylian — with the exact root cause pre-triaged in the issue body)** — POST `/api/v1/maintenance/types` hard-coded every field on the `MaintenanceType` constructor by name (`name`, `description`, `default_interval_hours`, `interval_type`, `icon`, `is_system`) and silently dropped `wiki_url`, even though the Pydantic schema accepted it and the response model echoed it back as `null`. PATCH was fine because it used `data.model_dump(exclude_unset=True) + setattr`, which is why editing a freshly-created type DID save the URL — masking the bug under any "save then immediately fix it" test. **Fix**: add `wiki_url=data.wiki_url` to the constructor call at `routes/maintenance.py:206`. **Frontend nit also addressed in the same drop** (#1596 nit section): `MaintenancePage.tsx:1131` `updateTypeMutation`'s inline `Partial<{...}>` shape listed `name | default_interval_hours | interval_type | icon` only. The value reached the API correctly at runtime because `api.updateMaintenanceType` accepts `Partial` (which includes `wiki_url`), but the local type was misleading — anyone reading the mutation would wrongly conclude `wiki_url` wasn't part of the update payload. Extended the inline shape to include `wiki_url?: string | null`. **Tests**: one new integration test in `test_maintenance_api.py::test_create_custom_type_persists_wiki_url` — POSTs a custom type with a `wiki_url`, asserts the POST response carries it, and verifies via a separate GET round-trip that the value actually committed (defending against the "response echoes request body" failure mode the bug would have masked). Full 5565-test backend suite green; ruff clean; frontend build clean; ESLint zero output; touched MaintenancePage vitest green. diff --git a/backend/app/api/routes/archives.py b/backend/app/api/routes/archives.py index 43adf4a8b..4c9afcf53 100644 --- a/backend/app/api/routes/archives.py +++ b/backend/app/api/routes/archives.py @@ -149,7 +149,7 @@ def _apply_run_user_filter(conditions: list, created_by_id: int | None): conditions.append(PrintLogEntry.created_by_id == created_by_id) -def compute_time_accuracy(archive: PrintArchive) -> dict: +def compute_time_accuracy(archive: PrintArchive, run_aggregate: dict | None = None) -> dict: """Compute actual print time and accuracy for an archive. Returns dict with actual_time_seconds and time_accuracy. @@ -157,8 +157,26 @@ def compute_time_accuracy(archive: PrintArchive) -> dict: - 100% = perfect estimate - >100% = print was faster than estimated - <100% = print took longer than estimated + + When ``run_aggregate`` indicates the archive has more than one logged + run (multi-plate file printed plate-by-plate, or reprints), both + fields are suppressed: ``archive.started_at / completed_at`` reflect + the LATEST run only, while ``archive.print_time_seconds`` is the + whole-file estimate (post-#1593 the parser sums across plates), so + comparing the two describes different scopes. The card-rendering + frontend falls through to ``archive.print_time_seconds`` for the + time display and hides the badge when ``time_accuracy`` is null — + that's the desired "show estimate, no badge" presentation for + multi-run archives (#1608). Single-run archives keep the original + badge behaviour verbatim. """ - result = {"actual_time_seconds": None, "time_accuracy": None} + result: dict[str, int | float | None] = {"actual_time_seconds": None, "time_accuracy": None} + + # Multi-run archives: the per-run actual (started_at..completed_at on + # the archive row) is incommensurable with the whole-file estimate. + # Both fields are cleared so the card shows estimate + no badge. + if run_aggregate and (run_aggregate.get("run_count") or 0) > 1: + return result if archive.started_at and archive.completed_at and archive.status == "completed": actual_seconds = int((archive.completed_at - archive.started_at).total_seconds()) @@ -271,8 +289,11 @@ def archive_to_response( "created_by_username": archive.created_by.username if archive.created_by else None, } - # Add computed time accuracy fields - accuracy_data = compute_time_accuracy(archive) + # Add computed time accuracy fields. ``run_aggregate`` lets + # ``compute_time_accuracy`` suppress the badge for multi-run archives + # where the per-run actual / whole-file estimate scopes don't match + # (#1608). + accuracy_data = compute_time_accuracy(archive, run_aggregate) data.update(accuracy_data) if run_aggregate: @@ -580,7 +601,10 @@ async def search_archives( query = query.limit(limit).offset(offset) result = await db.execute(query) archives = result.scalars().all() - return [archive_to_response(a) for a in archives] + # Load run aggregates so multi-run archives' time/accuracy badge is + # suppressed consistently with the main list endpoint (#1608). + run_aggregates = await _load_run_aggregates(db, [a.id for a in archives]) + return [archive_to_response(a, run_aggregate=run_aggregates.get(a.id)) for a in archives] if not matched_ids: return [] @@ -607,7 +631,10 @@ async def search_archives( ordered_archives = [archives_dict[id] for id in matched_ids if id in archives_dict] paginated = ordered_archives[offset : offset + limit] - return [archive_to_response(a) for a in paginated] + # Load run aggregates so multi-run archives' time/accuracy badge is + # suppressed consistently with the main list endpoint (#1608). + run_aggregates = await _load_run_aggregates(db, [a.id for a in paginated]) + return [archive_to_response(a, run_aggregate=run_aggregates.get(a.id)) for a in paginated] @router.post("/search/rebuild-index") @@ -1416,7 +1443,11 @@ async def update_archive( ) archive = result.scalar_one_or_none() - return archive_to_response(archive) + # Load run aggregate so the time/accuracy badge stays consistent with + # the list / detail endpoints when the frontend re-renders the card + # after a PATCH (#1608). + run_aggregates = await _load_run_aggregates(db, [archive.id]) if archive else {} + return archive_to_response(archive, run_aggregate=run_aggregates.get(archive.id) if archive else None) @router.post("/{archive_id}/favorite", response_model=ArchiveResponse) diff --git a/backend/app/api/routes/projects.py b/backend/app/api/routes/projects.py index 1b44287c6..28bf9852a 100644 --- a/backend/app/api/routes/projects.py +++ b/backend/app/api/routes/projects.py @@ -701,9 +701,13 @@ async def list_project_archives( archives = result.scalars().all() # Import the response converter from archives module - from backend.app.api.routes.archives import archive_to_response + from backend.app.api.routes.archives import _load_run_aggregates, archive_to_response - return [archive_to_response(a) for a in archives] + # Load run aggregates so multi-run archives' time/accuracy badge is + # suppressed consistently with the main archives list endpoint (#1608). + run_aggregates = await _load_run_aggregates(db, [a.id for a in archives]) + + return [archive_to_response(a, run_aggregate=run_aggregates.get(a.id)) for a in archives] @router.get("/{project_id}/queue") diff --git a/backend/tests/unit/test_archive_run_aggregation.py b/backend/tests/unit/test_archive_run_aggregation.py index cac502672..ee834e6ee 100644 --- a/backend/tests/unit/test_archive_run_aggregation.py +++ b/backend/tests/unit/test_archive_run_aggregation.py @@ -312,3 +312,187 @@ async def test_time_accuracy_excludes_multi_plate_plate_by_plate_outliers( body = (await async_client.get("/api/v1/archives/stats")).json() assert body["average_time_accuracy"] == pytest.approx(97.3, abs=0.1) assert body["time_accuracy_by_printer"][str(printer.id)] == pytest.approx(97.3, abs=0.1) + + +# --------------------------------------------------------------------------- +# #1608: compute_time_accuracy suppresses the per-card badge for multi-run +# archives where the whole-file estimate is incommensurable with the +# latest-run actual. +# --------------------------------------------------------------------------- + + +class TestComputeTimeAccuracyMultiRun: + """The card-level ``compute_time_accuracy`` runs against the archive's own + ``started_at`` / ``completed_at`` (latest run only) and + ``print_time_seconds`` (post-#1593 sum across plates). For multi-run + archives those describe different scopes — a 3-plate file printed + plate-by-plate over 3 runs produces estimate/actual = 300% → +200% badge, + which is pure noise. The reporter (#1608, archive #65) verified the + bug surfaces at +188% for a 3-plate file with 9 runs. + + The fix: when the archive has more than one logged run, suppress BOTH + fields. The frontend then falls through to ``print_time_seconds`` for + the time display (so the user sees the slicer's whole-file estimate + instead of one run's wall-clock) and hides the badge. + """ + + def _make_archive(self, *, print_time_seconds, started_at, completed_at, status="completed"): + from types import SimpleNamespace + + return SimpleNamespace( + print_time_seconds=print_time_seconds, + started_at=started_at, + completed_at=completed_at, + status=status, + ) + + def test_single_run_keeps_original_behaviour(self): + """``run_count == 1`` is the case the badge was designed for — + compute and return both actual + accuracy as before.""" + from backend.app.api.routes.archives import compute_time_accuracy + + archive = self._make_archive( + print_time_seconds=3600, + started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc), + completed_at=datetime(2026, 5, 1, 11, 0, tzinfo=timezone.utc), + ) + result = compute_time_accuracy(archive, run_aggregate={"run_count": 1}) + + assert result["actual_time_seconds"] == 3600 + assert result["time_accuracy"] == 100.0 + + def test_no_run_aggregate_keeps_original_behaviour(self): + """Endpoints that don't yet load run_aggregates (legacy callers, or + contexts where the data isn't relevant) must keep the pre-fix + per-archive computation — never silently drop the badge.""" + from backend.app.api.routes.archives import compute_time_accuracy + + archive = self._make_archive( + print_time_seconds=3600, + started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc), + completed_at=datetime(2026, 5, 1, 10, 50, tzinfo=timezone.utc), + ) + result = compute_time_accuracy(archive) # no run_aggregate + + assert result["actual_time_seconds"] == 3000 + assert result["time_accuracy"] == 120.0 # 3600/3000 + + def test_multi_run_archive_suppresses_both_fields(self): + """Reporter's case (archive #65, 3 plates, 9 logged runs): one-run + actual (6364s) vs whole-file estimate (18354s) → +188% badge that + means nothing. Multi-run must clear both fields so the card falls + through to the estimate display with no badge.""" + from backend.app.api.routes.archives import compute_time_accuracy + + archive = self._make_archive( + print_time_seconds=18354, + started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc), + completed_at=datetime(2026, 5, 1, 11, 46, 4, tzinfo=timezone.utc), # ~6364s + ) + result = compute_time_accuracy(archive, run_aggregate={"run_count": 9}) + + assert result["actual_time_seconds"] is None + assert result["time_accuracy"] is None + + def test_run_count_zero_keeps_original_behaviour(self): + """A run_aggregate that exists but reports zero runs (edge case from + the LEFT JOIN-style helper) must not trigger suppression — that's + not the multi-run shape, it's the no-runs shape, and the + per-archive timestamps are still meaningful (the archive was + marked completed without a PrintLogEntry trail, e.g. legacy + imports).""" + from backend.app.api.routes.archives import compute_time_accuracy + + archive = self._make_archive( + print_time_seconds=3600, + started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc), + completed_at=datetime(2026, 5, 1, 11, 0, tzinfo=timezone.utc), + ) + result = compute_time_accuracy(archive, run_aggregate={"run_count": 0}) + + assert result["actual_time_seconds"] == 3600 + assert result["time_accuracy"] == 100.0 + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_archive_list_suppresses_time_accuracy_for_multi_run_archives( + async_client: AsyncClient, archive_factory, printer_factory, db_session +): + """#1608 integration: the card response from the main list endpoint + must report ``actual_time_seconds = null`` and ``time_accuracy = null`` + for an archive with multiple logged runs, so the frontend renders the + slicer estimate without the misleading +N% badge.""" + printer = await printer_factory() + + # 3-plate file: estimate is the whole-file sum, latest run is one plate. + archive = await archive_factory( + printer.id, + status="completed", + print_time_seconds=18354, # all-plates estimate (post-#1593 parser) + started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc), + completed_at=datetime(2026, 5, 1, 11, 46, 4, tzinfo=timezone.utc), # ~6364s = one plate + with_run=False, + ) + # Three runs each ~6364s — plate-by-plate. + for day in (1, 2, 3): + db_session.add( + PrintLogEntry( + archive_id=archive.id, + printer_id=archive.printer_id, + status="completed", + started_at=datetime(2026, 5, day, 10, 0, tzinfo=timezone.utc), + completed_at=datetime(2026, 5, day, 11, 46, 4, tzinfo=timezone.utc), + duration_seconds=6364, + ) + ) + await db_session.commit() + + response = await async_client.get("/api/v1/archives/") + assert response.status_code == 200 + row = next(r for r in response.json() if r["id"] == archive.id) + + # Frontend renders archive.actual_time_seconds || archive.print_time_seconds — + # with actual cleared, it falls through to the estimate; with accuracy + # cleared, no badge renders. + assert row["actual_time_seconds"] is None, "multi-run actual is incommensurable with the estimate — must be null" + assert row["time_accuracy"] is None, "no badge for multi-run archives — the scopes don't match" + # The estimate itself is preserved so the card has something to display. + assert row["print_time_seconds"] == 18354 + assert row["run_count"] == 3 + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_archive_list_keeps_time_accuracy_for_single_run_archives( + async_client: AsyncClient, archive_factory, printer_factory, db_session +): + """Sanity check for the #1608 fix: single-run archives (the case the + badge was designed for) keep their original badge behaviour.""" + printer = await printer_factory() + + archive = await archive_factory( + printer.id, + status="completed", + print_time_seconds=3600, + started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc), + completed_at=datetime(2026, 5, 1, 11, 0, tzinfo=timezone.utc), + with_run=False, + ) + db_session.add( + PrintLogEntry( + archive_id=archive.id, + printer_id=archive.printer_id, + status="completed", + started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc), + completed_at=datetime(2026, 5, 1, 11, 0, tzinfo=timezone.utc), + duration_seconds=3600, + ) + ) + await db_session.commit() + + row = next(r for r in (await async_client.get("/api/v1/archives/")).json() if r["id"] == archive.id) + + assert row["actual_time_seconds"] == 3600 + assert row["time_accuracy"] == 100.0 + assert row["run_count"] == 1