fix: bound ruff to >=0.16,<0.17, make the lint gate real, fix the defects it found

.github/workflows/test.yml

@@ -31,13 +31,30 @@ jobs:
       - name: Install dependencies
         run: uv sync --all-extras
 
+      # This step used to read:
+      #
+      #     run: uv run ruff check src/openadapt_viewer/ || true
+      #     continue-on-error: true
+      #
+      # `|| true` AND `continue-on-error: true` -- belt and braces. It could not
+      # fail, and so had never enforced anything: at the point it was made real
+      # the tree had 261 findings under ruff 0.16's default rule set. It is now
+      # a blocking gate over the whole repository (src, tests, scripts and the
+      # root-level scripts), which is what the residual was cleared against, and
+      # the ruff version is bounded to >=0.16,<0.17 in pyproject.toml so a new
+      # ruff release cannot turn this red without a person raising the ceiling.
       - name: Run ruff linter (check)
-        run: uv run ruff check src/openadapt_viewer/ || true
-        continue-on-error: true
-
-      - name: Run ruff formatter (check)
-        run: uv run ruff format --check src/openadapt_viewer/ || true
-        continue-on-error: true
+        run: uv run ruff check .
+
+      # The companion `ruff format --check src/openadapt_viewer/` step was
+      # deleted rather than made real. It carried the same `|| true` +
+      # continue-on-error, and 31 of the 41 files under src/ do not match
+      # `ruff format` today (90 of 143 across the repo). Making it blocking here
+      # would mean reformatting the tree inside a lint-configuration change,
+      # burying every real fix in whitespace. A check that cannot fail is worse
+      # than no check -- it reads as coverage that does not exist -- so it is
+      # gone until someone does the reformat as its own reviewable commit and
+      # adds the step back.
 
       - name: Run pytest
         run: uv run pytest tests/ -v

pyproject.toml

@@ -40,7 +40,14 @@ dependencies = [
 [project.optional-dependencies]
 dev = [
     "pytest>=8.0.0",
-    "ruff>=0.1.0",
+    # Bounded on purpose. `ruff>=0.1.0` unpinned means `ruff check` here answers
+    # differently depending on the day it is resolved: ruff 0.16.0 grew its
+    # DEFAULT rule set from 59 rules to 413, and this project selected no rules
+    # explicitly, so it silently inherited 354 new ones. This tree reported 0
+    # findings under the 0.14.13 that `uv.lock` happened to pin and 261 under
+    # 0.16.0. Raise the ceiling deliberately, with the findings cleared in the
+    # same change.
+    "ruff>=0.16,<0.17",
 ]
 screenshots = [
     "playwright>=1.40.0",
@@ -64,6 +71,44 @@ packages = ["src/openadapt_viewer"]
 
 [tool.ruff]
 line-length = 100
+# Pinned to the floor of `requires-python = ">=3.10"`. Without it ruff infers the
+# target from `requires-python`, which is the same answer today but would move on
+# its own the day that floor is raised -- changing which pyupgrade rewrites fire.
+target-version = "py310"
+
+# There is deliberately no `select` here: this project keeps ruff's FULL default
+# rule set (413 rules as of 0.16). The residual after safe autofix was 102
+# findings across a 13.8k-line tree, small enough to read and judge one by one,
+# and doing so is what surfaced the defects fixed in this change -- a bare
+# `except:` that hid an unparseable timestamp behind a file mtime, two CSS reads
+# that fell back to a truncated stylesheet in silence, two screenshot scripts
+# that exited 0 after every scenario failed, and a `.lstrip("../")` that was
+# never a prefix strip. Narrowing to `["E","F","I","W"]` would have found none
+# of them. Predictability comes instead from the bounded `ruff>=0.16,<0.17` in
+# the dev extra above: ruff only grows its default set in a minor release, so
+# the rule set cannot move until a person raises that ceiling and owns the new
+# findings in the same change.
+#
+# On DTZ (39 of the 102): every naive datetime here was fixed with a trailing
+# `.astimezone()`, which attaches the machine's local zone WITHOUT moving the
+# wall clock. Both timestamps a user actually sees -- `created_at_formatted` in
+# the segmentation catalog and the `Created:` column of
+# `openadapt-viewer catalog list` -- render through `strftime` with no `%z`, so
+# their output is byte-identical before and after. This was the point: a viewer
+# shows the time the recording happened on this machine, and converting those to
+# UTC would have silently shifted what the user reads. Only `.isoformat()`
+# output changes, gaining a `+HH:MM` suffix, which makes the emitted
+# `generated_at` metadata unambiguous rather than different.
+
+[tool.ruff.lint.per-file-ignores]
+# RUF022 wants `__all__` sorted alphabetically. Both of these are grouped by
+# subsystem with `# Core` / `# Components` / `# Builders` / `# Viewers` /
+# `# Catalog` headings, and an alphabetical sort strands every heading against
+# unrelated names. The grouping documents the package layout and is worth more
+# than the ordering, so the rule is turned off for these two files rather than
+# repo-wide.
+"src/openadapt_viewer/__init__.py" = ["RUF022"]
+"src/openadapt_viewer/components/__init__.py" = ["RUF022"]
 
 [tool.semantic_release]
 version_toml = ["pyproject.toml:project.version"]

scripts/generate_comprehensive_screenshots.py

@@ -15,14 +15,24 @@
 
 import argparse
 import sys
+from collections.abc import Callable
 from pathlib import Path
-from typing import Any, Callable, Optional
 
 # Add parent directory to path for imports
 SCRIPT_DIR = Path(__file__).parent
 REPO_ROOT = SCRIPT_DIR.parent
 sys.path.insert(0, str(REPO_ROOT / "src"))
 
+try:
+    from playwright.sync_api import Error as PlaywrightError
+except ImportError:
+    # playwright is an optional extra; main() refuses to run without it. The
+    # alias only keeps this module importable.
+    PlaywrightError = Exception
+
+# Scenarios that raised during this process, so main() can exit non-zero.
+FAILED_SCENARIOS: list[str] = []
+
 
 class ScreenshotConfig:
     """Configuration for a single screenshot scenario."""
@@ -34,7 +44,7 @@ def __init__(
         viewport_width: int = 1400,
         viewport_height: int = 900,
         full_page: bool = False,
-        interact: Optional[Callable] = None,
+        interact: Callable | None = None,
         wait_after_load: int = 1000,
         wait_after_interact: int = 500,
     ):
@@ -244,7 +254,7 @@ def generate_screenshot(
             try:
                 config.interact(page)
                 page.wait_for_timeout(config.wait_after_interact)
-            except Exception as e:
+            except PlaywrightError as e:
                 print(f"    Warning: Interaction failed: {e}")
 
         # Take screenshot
@@ -292,8 +302,12 @@ def generate_viewer_screenshots(
         try:
             screenshot_path = generate_screenshot(viewer_html, output_path, scenario)
             screenshots.append(screenshot_path)
-        except Exception as e:
+        except Exception as e:  # noqa: BLE001
+            # Deliberately blind: one bad scenario must not abandon the rest of
+            # the batch. The failure is recorded so main() exits non-zero
+            # instead of printing a success line.
             print(f"  ERROR: Failed to generate {scenario.name}: {e}")
+            FAILED_SCENARIOS.append(f"{viewer_type}_{scenario.name}")
 
     return screenshots
 
@@ -418,6 +432,17 @@ def main() -> int:
         total_size_mb = sum(p.stat().st_size for p in all_screenshots) / 1024 / 1024
         print(f"\nTotal size: {total_size_mb:.2f} MB")
 
+    if FAILED_SCENARIOS:
+        # Previously this returned 0 regardless, so a run in which every
+        # scenario raised still printed a success line and exited 0. A viewer
+        # HTML that is simply absent is still a skip, not a failure.
+        print(
+            f"\n✗ {len(FAILED_SCENARIOS)} scenario(s) failed: "
+            f"{', '.join(FAILED_SCENARIOS)}",
+            file=sys.stderr,
+        )
+        return 1
+
     print("\n✓ Screenshot generation completed!")
     return 0
 

scripts/generate_readme_screenshots.py

@@ -28,7 +28,6 @@
 class ScreenshotGenerationError(Exception):
     """Base exception for screenshot generation errors."""
 
-    pass
 
 
 def check_dependencies() -> dict[str, bool]:

scripts/generate_segmentation_screenshots.py

@@ -28,15 +28,25 @@
 import argparse
 import json
 import sys
+from collections.abc import Callable
 from dataclasses import dataclass
 from pathlib import Path
-from typing import Any, Callable, Optional
+from typing import Any
 
 # Add src to path for imports
 SCRIPT_DIR = Path(__file__).parent
 REPO_ROOT = SCRIPT_DIR.parent
 sys.path.insert(0, str(REPO_ROOT / "src"))
 
+try:
+    from playwright.sync_api import Error as PlaywrightError
+except ImportError:
+    # playwright is an optional extra. generate_all_screenshots() raises a
+    # RuntimeError with install instructions before any handler below can run,
+    # so this alias only exists to keep the module importable (the test suite
+    # imports it to check --help and --check-deps).
+    PlaywrightError = Exception
+
 
 @dataclass
 class ScreenshotScenario:
@@ -47,8 +57,8 @@ class ScreenshotScenario:
     viewport_width: int
     viewport_height: int
     full_page: bool = False
-    interact: Optional[Callable] = None
-    wait_for_selector: Optional[str] = None
+    interact: Callable | None = None
+    wait_for_selector: str | None = None
     wait_timeout: int = 1000
 
 
@@ -67,6 +77,9 @@ def __init__(self, output_dir: Path, viewer_path: Path, test_data_path: Path):
         self.viewer_path = viewer_path
         self.test_data_path = test_data_path
         self.screenshots_generated = []
+        # Scenarios that raised. A run in which every scenario failed used
+        # to still exit 0 and print "completed successfully".
+        self.scenarios_failed: list[str] = []
 
         # Create output directory
         self.output_dir.mkdir(parents=True, exist_ok=True)
@@ -278,7 +291,7 @@ def _execute_scenarios(self, browser, scenarios: list[ScreenshotScenario]):
                 if scenario.wait_for_selector:
                     try:
                         page.wait_for_selector(scenario.wait_for_selector, timeout=5000)
-                    except Exception as e:
+                    except PlaywrightError as e:
                         print(f"    Warning: Selector '{scenario.wait_for_selector}' not found: {e}")
 
                 # Additional wait after interaction
@@ -293,8 +306,12 @@ def _execute_scenarios(self, browser, scenarios: list[ScreenshotScenario]):
 
                 self.screenshots_generated.append(output_path)
 
-            except Exception as e:
+            except Exception as e:  # noqa: BLE001
+                # Deliberately blind: one bad scenario must not abandon the
+                # other twelve. The failure is now recorded so main() can exit
+                # non-zero instead of reporting success.
                 print(f"    Error: {e}")
+                self.scenarios_failed.append(scenario.name)
 
             finally:
                 page.close()
@@ -330,7 +347,7 @@ def _load_and_expand_first_episode(self, page):
         try:
             page.click(".episode-card:first-child")
             page.wait_for_timeout(500)
-        except Exception as e:
+        except PlaywrightError as e:
             print(f"    Warning: Could not click first episode: {e}")
 
     def _focus_search(self, page):
@@ -341,8 +358,11 @@ def _focus_search(self, page):
         # Focus search input
         try:
             page.click("#search-input")
-        except Exception:
-            pass
+        except PlaywrightError as e:
+            # Was `except Exception: pass`. A failed click still produced a
+            # screenshot named "search focused" showing an unfocused search box
+            # and reported it as generated.
+            print(f"    Warning: Could not focus search input: {e}")
 
     def _search_nightshift(self, page):
         """Load data and search for 'nightshift'."""
@@ -353,7 +373,7 @@ def _search_nightshift(self, page):
         try:
             page.fill("#search-input", "nightshift")
             page.wait_for_timeout(500)
-        except Exception as e:
+        except PlaywrightError as e:
             print(f"    Warning: Could not search: {e}")
 
     def _show_recording_filter(self, page):
@@ -364,8 +384,11 @@ def _show_recording_filter(self, page):
         # Click recording filter dropdown
         try:
             page.click("#recording-filter")
-        except Exception:
-            pass
+        except PlaywrightError as e:
+            # Was `except Exception: pass`. Same failure mode as _focus_search:
+            # a screenshot named "recording filter open" showing a closed
+            # dropdown, reported as generated.
+            print(f"    Warning: Could not open recording filter: {e}")
 
     def generate_metadata(self) -> dict[str, Any]:
         """Generate metadata about the screenshot generation.
@@ -376,7 +399,7 @@ def generate_metadata(self) -> dict[str, Any]:
         from datetime import datetime
 
         return {
-            "generated_at": datetime.now().isoformat(),
+            "generated_at": datetime.now().astimezone().isoformat(),
             "viewer_path": str(self.viewer_path),
             "test_data_path": str(self.test_data_path),
             "output_dir": str(self.output_dir),
@@ -494,10 +517,23 @@ def main() -> int:
         for path in screenshots:
             print(f"  - {path.name}")
 
+        if generator.scenarios_failed:
+            # Previously this returned 0 however many scenarios raised, so a run
+            # that captured nothing still printed "completed successfully" and
+            # `openadapt-viewer screenshots` exited 0.
+            print(
+                f"\n✗ {len(generator.scenarios_failed)} scenario(s) failed: "
+                f"{', '.join(generator.scenarios_failed)}",
+                file=sys.stderr,
+            )
+            return 1
+
         print("\n✓ Screenshot generation completed successfully!")
         return 0
 
-    except Exception as e:
+    except Exception as e:  # noqa: BLE001
+        # Top of the process: turn any unexpected failure into an exit status
+        # and a traceback rather than an unhandled crash.
         print(f"\nError: {e}", file=sys.stderr)
         import traceback
 

scripts/generate_segmentation_viewer.py

@@ -22,7 +22,7 @@
 def generate_viewer_with_catalog(
     template_path: str,
     output_path: str,
-    segmentation_dirs: list = None
+    segmentation_dirs: list | None = None
 ):
     """
     Generate segmentation viewer with embedded catalog data.