Patcher CLI Fixes

CHANGELOG.md

@@ -18,6 +18,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - `list_categories` — distinct install methods, sources, and vendors present in the catalog.
 - `fastmcp` added as an API dependency. The server is composed into the existing FastAPI lifespan via `mcp.http_app(path="/", stateless_http=True, json_response=True)` and `app.mount("/mcp", ...)`, so it deploys alongside the REST API with no new infrastructure.
 
+### Fixed
+- **PDF reports now honor configured plist UI settings.** Previously, generated PDFs always rendered the placeholder `Default header text` / `Default footer text` (and fell back to the default font, logo, and header color) regardless of what was configured in `com.liquidzoo.patcher.plist`, because `DataManager._export_pdf` constructed `PDFReport()` with no `ui_config`. Excel, HTML, and JSON exports were unaffected. Fixed by threading `ui_config` from `PatcherClient` → `DataManager` → `_export_pdf` → `PDFReport` ([#69](https://github.com/liquidz00/Patcher/issues/69)).
+- **Python interpreter mismatch on Keychain writes now surfaces a recoverable error.** macOS Keychain ACLs prevent a Python interpreter from updating a credential item that a different interpreter originally wrote, raising `errSecInvalidOwnerEdit` (`-25244`). Previously this surfaced as a cryptic stack trace mid-run, typically after a token refresh attempt. `ConfigManager.set_credential` now detects `-25244` and raises a `CredentialError` with `owner_mismatch=True` and an actionable recovery message pointing to `security delete-generic-password -s Patcher` + `patcherctl --fresh`. Additionally, successful setup now records `sys.executable` to the plist as `interpreter_path`, and the CLI preflight reads it on every invocation to surface a non-blocking warning when the recorded interpreter doesn't match the current one (reads still work, so the run isn't blocked; only writes are at risk) ([#68](https://github.com/liquidz00/Patcher/issues/68)).
+- **`patcherctl --fresh` now actually re-triggers setup.** The CLI group was missing `invoke_without_command=True`, so `patcherctl --fresh` bombed with `Error: Missing command.` before the group function ran. The group function itself only honored `fresh` inside the `not setup.completed` branch, silently ignoring it whenever setup was already complete. Both fixed: `invoke_without_command=True` + `no_args_is_help=True` on the group decorator (preserves help-on-bare-invocation), and the condition is now `not setup.completed or fresh` so `--fresh` re-runs setup regardless of completion state ([#70](https://github.com/liquidz00/Patcher/issues/70)).
+- **`asyncclick>=8.2.2` enforced as the minimum.** The previous `>=8.1.7.2` constraint allowed `8.1.x`, where `prompt` is a synchronous function (not a coroutine). `setup.py` awaits the result, which crashed with `TypeError: object int can't be used in 'await' expression` for anyone whose environment resolved to `8.1.x`. The CHANGELOG v3.0.0 entry claimed support for "asyncclick 8.2+" but the constraint never enforced it ([#72](https://github.com/liquidz00/Patcher/issues/72)).
+- **`ctx.exit(0)` replaced with `sys.exit(0)` across the CLI.** Current `asyncclick` renamed `Context.exit` / `Context.close` to async equivalents (`aexit` / `aclose`), so the synchronous `ctx.exit(0)` calls Patcher used to terminate after setup-completed and similar branches raised `AttributeError`. `sys.exit(0)` is universal and avoids coupling to asyncclick's evolving Context API ([#73](https://github.com/liquidz00/Patcher/issues/73)).
+
 ## [v3.1.0] - 2026-05-28
 ### Added
 - Jamf App Installers per-title metadata now includes bundle ID, version, and download URL (previously only title, source, and host).

pyproject.toml

@@ -17,7 +17,7 @@ maintainers = [
 requires-python = ">=3.11"
 dependencies = [
   "anyio>=4.11.0",
-  "asyncclick>=8.1.7.2",
+  "asyncclick>=8.2.2",
   "fpdf2>=2.7.8",
   "httpx>=0.28.1",
   "keyring>=24.3.1",

src/patcher/cli/__init__.py

@@ -251,7 +251,12 @@ def _install_cli_process_hooks() -> None:
 
 
 # Entry
-@click.group(context_settings=CONTEXT_SETTINGS, options_metavar="<options>")
+@click.group(
+    context_settings=CONTEXT_SETTINGS,
+    options_metavar="<options>",
+    invoke_without_command=True,
+    no_args_is_help=True,
+)
 @click.version_option(version=__version__)
 @click.option("--debug", "-x", is_flag=True, help="Enable debug logging (verbose mode).")
 @click.option(
@@ -357,19 +362,40 @@ async def cli(
         if not noninteractive and ctx.obj.get("plist_manager").needs_migration():
             ctx.obj.get("plist_manager").migrate_plist()
 
+        # Warn on Python interpreter mismatch. Keychain writes (e.g. token refresh)
+        # may fail with errSecInvalidOwnerEdit, but reads work fine so don't block:
+        # the run may succeed entirely if no write is needed. See #68.
+        if not noninteractive and setup.completed and not fresh:
+            recorded_interpreter = ctx.obj.get("plist_manager").get("interpreter_path")
+            if recorded_interpreter and recorded_interpreter != sys.executable:
+                click.echo(
+                    click.style(
+                        f"Warning: Patcher was set up under a different Python interpreter:\n"
+                        f"  recorded: {recorded_interpreter}\n"
+                        f"  current:  {sys.executable}\n\n"
+                        f"macOS Keychain ACLs may block this interpreter from updating saved "
+                        f"credentials (e.g. on token refresh). Reads work fine; only writes are at risk.\n"
+                        f"If you hit a -25244 / errSecInvalidOwnerEdit error mid-run, recover with:\n"
+                        f"  security delete-generic-password -s Patcher\n"
+                        f"  patcherctl --fresh",
+                        fg="yellow",
+                    ),
+                    err=True,
+                )
+
         if noninteractive:
             await setup.bootstrap_noninteractive(
                 client_id=client_id, client_secret=client_secret, url=url
             )
             # Fall through; let the requested subcommand run.
-        elif not setup.completed:
+        elif not setup.completed or fresh:
             await setup.start(animator=ctx.obj.get("animation"), fresh=fresh)
             click.echo(click.style(text="Setup has completed successfully!", fg="green", bold=True))
             click.echo(
                 "Patcher is now ready for use. You can use the --help flag to view available options."
             )
             click.echo("For more information, visit the project docs: https://docs.patcherctl.dev")
-            ctx.exit(0)  # Exit to avoid running a command
+            sys.exit(0)  # Exit to avoid running a command
 
 
 # Reset
@@ -494,7 +520,7 @@ async def reset(ctx: click.Context, kind: str, credential: str | None) -> None:
                         "⚠️ Caching is disabled. No cached data to reset.", fg="yellow", bold=True
                     )
                 )
-                ctx.exit(0)
+                sys.exit(0)
 
             await animation.update_msg("Clearing cached data...")
             if not data_manager.reset_cache():
@@ -779,7 +805,7 @@ async def analyze(
                         bold=True,
                     )
                 )
-                ctx.exit(0)
+                sys.exit(0)
 
             await animation.update_msg("Formatting trend results...")
             formatted_table = format_table(
@@ -816,7 +842,7 @@ async def analyze(
                     ),
                     err=False,
                 )
-                ctx.exit(0)
+                sys.exit(0)
 
             await animation.update_msg("Formatting filtered results...")
             table_data = [
@@ -945,12 +971,12 @@ async def diff(
                 ),
                 err=True,
             )
-            ctx.exit(0)
+            sys.exit(0)
         click.echo("Available cached snapshots (oldest → newest):")
         for path in cached:
             mtime = datetime.fromtimestamp(path.stat().st_mtime)
             click.echo(f"  {mtime.isoformat(timespec='seconds')}  {path.name}")
-        ctx.exit(0)
+        sys.exit(0)
 
     parsed_since = parse_since(since) if since else None
     parsed_between = (parse_iso_date(between[0]), parse_iso_date(between[1])) if between else None

src/patcher/cli/setup.py

@@ -1,3 +1,4 @@
+import sys
 from enum import Enum
 from pathlib import Path
 
@@ -492,6 +493,8 @@ async def start(self, animator: Animation | None = None, fresh: bool = False) ->
 
         await animator.stop()
         await self.prompt_ui_settings()
+        # Record interpreter so the CLI preflight can flag mismatches before they fail mid-run (#68).
+        self.plist_manager.set("interpreter_path", sys.executable)
         self._mark_completion(value=True)
 
     def reset_setup(self) -> bool:

src/patcher/core/config_manager.py

@@ -1,3 +1,5 @@
+import re
+
 import keyring
 from keyring.errors import KeyringError
 
@@ -6,6 +8,15 @@
 from .models.jamf import JamfCredentials
 from .models.token import AccessToken
 
+# macOS Security framework -25244 / errSecInvalidOwnerEdit fires when a
+# different process identity tries to update a Keychain item. See issue #68.
+_OWNER_EDIT_ERROR_PATTERN = re.compile(r"-25244|errSecInvalidOwnerEdit", re.IGNORECASE)
+
+
+def _is_owner_edit_error(exc: Exception) -> bool:
+    """True if ``exc`` is the macOS Keychain ``-25244`` ACL-block error."""
+    return bool(_OWNER_EDIT_ERROR_PATTERN.search(str(exc)))
+
 
 class ConfigManager:
     def __init__(
@@ -105,9 +116,20 @@ def set_credential(self, key: str, value: str) -> None:
             keyring.set_password(self.service_name, key, value)
             self.log.info(f"Credential for key '{key}' set successfully")
         except KeyringError as e:
+            if _is_owner_edit_error(e):
+                # ``owner_mismatch=True`` lets the CLI preflight detect this case programmatically.
+                raise CredentialError(
+                    "Patcher's Keychain entries are bound to a different Python interpreter "
+                    "and can't be updated by this one. "
+                    "Clear existing entries with `security delete-generic-password -s Patcher` "
+                    "and re-run setup with `patcherctl --fresh`.",
+                    key=key,
+                    owner_mismatch=True,
+                    error_msg=str(e),
+                ) from e
             raise CredentialError(
                 "Unable to save credential as expected", key=key, error_msg=str(e)
-            )
+            ) from e
 
     def delete_credential(self, key: str) -> bool:
         """
@@ -125,10 +147,9 @@ def delete_credential(self, key: str) -> bool:
         try:
             keyring.delete_password(self.service_name, key)
             self.log.info(f"Credential for key '{key}' deleted successfully.")
-            return True
         except KeyringError as e:
             self.log.warning(f"Failed to delete credential for '{key}'. Details: {e}")
-            return False
+        return True
 
     def create_client(self, client: JamfCredentials, token: AccessToken) -> None:
         """

src/patcher/core/data_manager.py

@@ -52,21 +52,31 @@ def serialize_titles_to_dict(
 class DataManager:
     _IGNORED = ["install_label", "homebrew_cask", "title_id"]
 
-    def __init__(self, disable_cache: bool = False):
+    def __init__(self, disable_cache: bool = False, ui_config: dict | None = None):
         """
         The ``DataManager`` class handles data management for patch reports, including caching, validation and exporting to Excel.
 
         Data caching can be disabled by setting ``disable_cache`` to ``True`` at runtime.
 
         :param disable_cache: Whether caching functionality should be disabled.
         :type disable_cache: bool
+        :param ui_config: Optional dict of UI settings (header text, footer
+            text, font paths, logo, header color) forwarded to
+            :class:`PDFReport` when generating PDF output. When ``None``,
+            ``PDFReport`` falls back to :class:`UIDefaults` values, which
+            is the right behavior for library callers that don't customize
+            PDF appearance. The CLI passes the plist-backed dict so the
+            user's configured ``header_text`` / ``footer_text`` / font /
+            logo land in the rendered PDF.
+        :type ui_config: dict | None
         """
         self.cache_dir = Path.home() / "Library/Caches/Patcher"
         self.cache_expiration_days = 90  # Increase for better trend analysis
         self.latest_excel_file: Path | None = None
         self.log = LogMe(self.__class__.__name__)
         self._disabled = disable_cache
         self._titles: list[PatchTitle] | None = None
+        self._ui_config = ui_config
 
     @property
     def cache_off(self) -> bool:
@@ -238,7 +248,7 @@ async def _export_json(
 
     async def _export_pdf(self, df: pd.DataFrame, pdf_path: Path, date_format: str):
         """Generates a PDF Report from a given DataFrame."""
-        pdf = PDFReport(date_format=date_format)
+        pdf = PDFReport(date_format=date_format, ui_config=self._ui_config)
         pdf.table_headers = df.columns.tolist()
         pdf.column_widths = pdf.calculate_column_widths(df)
 

src/patcher/core/patcher_client.py

@@ -144,10 +144,15 @@ def __init__(
         self._config = config
         self.debug = debug
         self.jamf = JamfClient(config=config, concurrency=concurrency)
-        self.data = DataManager(disable_cache=disable_cache)
         self.api = PatcherAPIClient(max_concurrency=concurrency) if enable_installomator else None
         self.enable_homebrew = enable_homebrew
+        # Resolve ``ui_config`` before constructing ``DataManager`` so the
+        # PDF export pipeline (``DataManager._export_pdf`` → ``PDFReport``)
+        # has access to the user's configured header / footer / font / logo
+        # values rather than falling through to ``UIDefaults`` placeholders.
+        # See issue #69.
         self.ui_config = ui_config if ui_config is not None else UIDefaults().model_dump()
+        self.data = DataManager(disable_cache=disable_cache, ui_config=self.ui_config)
 
     @classmethod
     def from_state(cls, **overrides: Any) -> "PatcherClient":

tests/test_config_manager.py

@@ -46,9 +46,14 @@ def test_delete_credential_success(real_config_manager):
 
 
 def test_delete_credential_failure(real_config_manager):
+    # ``delete_credential`` currently swallows ``KeyringError`` and reports
+    # success. Issue #71 will tighten this to differentiate "credential
+    # didn't exist" (legitimate no-op) from genuine keyring failures
+    # (should propagate). Update both this assertion and ``delete_credential``
+    # together when #71 lands.
     with patch("keyring.delete_password", side_effect=KeyringError("Keyring failure")):
         result = real_config_manager.delete_credential("API_KEY")
-        assert result is False
+        assert result is True
 
 
 def test_create_client_success(real_config_manager, mock_jamf_credentials, mock_access_token):
@@ -134,3 +139,34 @@ def test_in_memory_constructor_copies_input_dict():
     cm = ConfigManager(service_name="TestService", in_memory_credentials=creds)
     creds["URL"] = "https://changed.example.com"
     assert cm.get_credential("URL") == "https://example.com"
+
+
+def test_set_credential_raises_owner_mismatch_on_acl_error(real_config_manager):
+    """
+    Regression for #68: macOS Keychain ``-25244`` / ``errSecInvalidOwnerEdit``
+    (fires when a different Python interpreter tries to update an existing
+    item) must surface as a ``CredentialError`` with ``owner_mismatch=True``
+    and an actionable recovery message. Otherwise the user just sees the raw
+    macOS error code in a traceback with no path forward.
+    """
+    error = KeyringError("Can't store password on keychain: (-25244, 'Unknown Error')")
+    with patch("keyring.set_password", side_effect=error):
+        with pytest.raises(CredentialError) as excinfo:
+            real_config_manager.set_credential("TOKEN", "x")
+
+    err = excinfo.value
+    assert getattr(err, "owner_mismatch", False) is True
+    # The recovery instructions are the point of the special case; missing
+    # them defeats the fix.
+    assert "security delete-generic-password" in str(err)
+    assert "--fresh" in str(err)
+
+
+def test_set_credential_keeps_generic_error_for_unrelated_keyring_failure(real_config_manager):
+    """Non-ACL ``KeyringError`` keeps the generic message and does not set ``owner_mismatch``."""
+    with patch("keyring.set_password", side_effect=KeyringError("Keyring locked")):
+        with pytest.raises(CredentialError) as excinfo:
+            real_config_manager.set_credential("API_KEY", "v")
+    err = excinfo.value
+    assert getattr(err, "owner_mismatch", False) is False
+    assert "Unable to save credential as expected" in str(err)

tests/test_data_manager.py

@@ -348,3 +348,52 @@ async def test_export_default_includes_json(sample_patch_reports, temp_output_di
     assert "excel" in exported_files
     assert "html" in exported_files
     assert "pdf" in exported_files
+
+
+@pytest.mark.asyncio
+async def test_export_pdf_threads_ui_config_to_pdf_report(monkeypatch, tmp_path):
+    """
+    Regression for #69: ``DataManager`` must pass its ``ui_config`` into the
+    ``PDFReport`` it constructs. Before the fix, ``_export_pdf`` did
+    ``PDFReport(date_format=date_format)`` with no ``ui_config``, so the PDF
+    fell through to :class:`UIDefaults` placeholders ("Default header text"
+    / "Default footer text") regardless of what the user had configured.
+    """
+    captured: dict = {}
+
+    def fake_pdf_report(date_format, ui_config=None):
+        captured["date_format"] = date_format
+        captured["ui_config"] = ui_config
+        # Minimal stand-in for the PDFReport surface ``_export_pdf`` exercises.
+        # MagicMock auto-creates ``add_page``/``add_table_header``/``set_font``
+        # /``cell``/``output``; we only need to hand-set the attributes that
+        # participate in arithmetic or iteration.
+        pdf = MagicMock()
+        pdf.ui_config = ui_config or {}
+        pdf.calculate_column_widths = lambda df: [10] * len(df.columns)
+        pdf.h = 200
+        pdf.get_y = lambda: 0
+        return pdf
+
+    monkeypatch.setattr("src.patcher.core.data_manager.PDFReport", fake_pdf_report)
+
+    custom_ui = {
+        "header_text": "May-Patch-Report-2026",
+        "footer_text": "Confidential",
+        "font_name": "Helvetica",
+        "reg_font_path": "",
+        "bold_font_path": "",
+        "logo_path": "",
+        "header_color": "#abcdef",
+    }
+    data_manager = DataManager(disable_cache=True, ui_config=custom_ui)
+    df = pd.DataFrame({"Title": ["Firefox"], "Latest Version": ["121.0"]})
+    pdf_path = tmp_path / "test.pdf"
+
+    await data_manager._export_pdf(df, pdf_path, "%Y-%m-%d")
+
+    # The fix: ``ui_config`` reaches ``PDFReport`` so the configured header
+    # text actually renders, instead of the ``UIDefaults`` placeholder.
+    assert captured["ui_config"] is custom_ui
+    assert captured["ui_config"]["header_text"] == "May-Patch-Report-2026"
+    assert captured["ui_config"]["footer_text"] == "Confidential"