From df363a7fbc83372a3fa2eba848ea2ad73b375358 Mon Sep 17 00:00:00 2001 From: "Shane B." Date: Sun, 23 Aug 2026 13:06:42 +0200 Subject: [PATCH] Say where to report something the CLI cannot fix `UnhandledProtocolError` means Apple said something this library does not model: not the user's mistake, nothing for them to correct, and a bug report is the only useful next step. The handler asked for one - "include the output if you report this" - and never said where. `grep github.com python/exporter/cli.py` returned nothing at all. The window has named the URL in its error dialog for as long as it has had one. The headless half left somebody holding `-vv` output and no destination, which is not a hint they should have to take. Issue #140 is the worked example. `KeychainSessionError` is an `UnhandledProtocolError` by inheritance, so it lands here as "no keychain keys are held" - which reads as a broken account and is not one. That reporter found the issue tracker anyway; he also decoded protobuf by hand to write the report. The link moves to `version.py` rather than being duplicated. The CLI cannot import it from `wizard.py` without pulling in tkinter, which is the same reason `VERSION` lives there - and two copies of a URL is exactly what goes stale in one place without anybody noticing, because nothing tests a link. Asserted that importing `exporter.cli` still pulls in no tkinter. Four tests. One of them is the point of the other three: an `ExportSourceError` - a thing the user can fix, like stopping a sign-in - must *not* get the link. Sending somebody to file an issue about their own typo is wrong, and it teaches them to ignore the link on the day it matters. Verified by breaking both directions: removing the line reddens two, printing it unconditionally reddens the fourth. 512 tests pass. Co-Authored-By: Claude Opus 5 --- python/exporter/cli.py | 8 ++++- python/exporter/version.py | 9 ++++++ python/exporter/wizard.py | 3 +- python/test/test_cli.py | 61 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 76 insertions(+), 5 deletions(-) diff --git a/python/exporter/cli.py b/python/exporter/cli.py index 41e41319..683c9980 100644 --- a/python/exporter/cli.py +++ b/python/exporter/cli.py @@ -49,7 +49,7 @@ suggested_name, ) from exporter.icloud import Candidate, ExportSourceError -from exporter.version import EXPORT_VIA_CLI, VERSION +from exporter.version import EXPORT_VIA_CLI, GITHUB_ISSUES_LINK, VERSION from opentagviewer_export import ( ExportError, KeyFileError, @@ -939,6 +939,12 @@ def _run_and_return(arguments: argparse.Namespace) -> int: print("it prints about you, and it is worth reading before pasting it anywhere.", file=sys.stderr) + # **And where.** Asking somebody to report something without saying where to put it leaves + # them holding output and no destination - which is not a hint they should have to take. + # The window has said this for as long as it has had an error dialog; this one asked for a + # report and named nowhere. + print(f"\nReport it at: {GITHUB_ISSUES_LINK}", file=sys.stderr) + return 1 except KeyboardInterrupt: print("\nStopped. Nothing was written.", file=sys.stderr) diff --git a/python/exporter/version.py b/python/exporter/version.py index da5855ed..7a5364da 100644 --- a/python/exporter/version.py +++ b/python/exporter/version.py @@ -42,3 +42,12 @@ A different producer, because it is one: same version, same format, different program - and a bug report that says which is worth more than one that says "the exporter". """ + +GITHUB_ISSUES_LINK = "https://github.com/parawanderer/OpenTagViewer/issues/new" +""" +Where to report something neither the user nor this program can fix. + +Here rather than in `wizard.py`, which is where it used to live alone, so the CLI can say it too +without importing tkinter - the same reason `VERSION` is here. Two copies of a URL is exactly the +sort of thing that goes stale in one place and is never noticed, because nothing tests a link. +""" diff --git a/python/exporter/wizard.py b/python/exporter/wizard.py index e4c45a0d..9d4d7ec5 100644 --- a/python/exporter/wizard.py +++ b/python/exporter/wizard.py @@ -57,7 +57,7 @@ from findmy.keychain.recovery import RecoveryError from exporter.icloud import Candidate, ExportSourceError -from exporter.version import APP_TITLE, EXPORT_VIA_WIZARD, VERSION +from exporter.version import APP_TITLE, EXPORT_VIA_WIZARD, GITHUB_ISSUES_LINK, VERSION from opentagviewer_export import ( ExportError, KeyFileError, @@ -69,7 +69,6 @@ logger = logging.getLogger(__name__) -GITHUB_ISSUES_LINK = "https://github.com/parawanderer/OpenTagViewer/issues/new" # The "Need Help?" link. It points at the page covering both routes: the older `…-From-Mac` page # keeps its name because binaries already released open it, but a copy shipping now should send # people to the one that describes what it actually does. diff --git a/python/test/test_cli.py b/python/test/test_cli.py index 66195b8f..219a45cd 100644 --- a/python/test/test_cli.py +++ b/python/test/test_cli.py @@ -16,11 +16,12 @@ from pathlib import Path import pytest -from findmy import InvalidCredentialsError, MobileMeDelegateError +from findmy import InvalidCredentialsError, MobileMeDelegateError, UnhandledProtocolError +from findmy.keychain.session import KeychainSessionError from exporter import cli, prompts from exporter.icloud import ExportSourceError -from exporter.version import EXPORT_VIA_CLI +from exporter.version import EXPORT_VIA_CLI, GITHUB_ISSUES_LINK from opentagviewer_export import ExportBundle, generate_passcode KEY = bytes(range(28)) @@ -507,3 +508,59 @@ async def _no(*_args, **_kwargs): monkeypatch.setattr(prompts, "confirm", _no) assert asyncio.run(cli._confirm_devices([mac], or_nothing=True)) == [] + + +class TestWhereToReportSomethingUnfixable: + """ + `UnhandledProtocolError` means Apple said something this library does not model. + + There is nothing for the user to correct, so the only useful next step is a bug report - and + for years this asked for one without saying where to put it. The window has always named the + URL in its error dialog; the headless half said "if you report this" and stopped. + + Issue #140 is the worked example: a `KeychainSessionError`, which is an + `UnhandledProtocolError` by inheritance, arriving as "no keychain keys are held" - which reads + as a broken account and is not one. + """ + + def _failing_run(self, monkeypatch, error): + # `run`, not `_run_and_return` - the handlers are inside the latter, so patching it + # would test nothing and let the exception straight out. + async def _boom(_arguments): + raise error + + monkeypatch.setattr(cli, "run", _boom) + + def test_it_names_where_to_report(self, monkeypatch, capsys): + self._failing_run(monkeypatch, UnhandledProtocolError("no keychain keys are held")) + + assert cli.main([]) == 1 + assert GITHUB_ISSUES_LINK in capsys.readouterr().err + + def test_it_still_says_what_apple_did(self, monkeypatch, capsys): + # The link is an addition, not a replacement: the message is the only description of the + # actual problem anybody has. + self._failing_run(monkeypatch, UnhandledProtocolError("no keychain keys are held")) + + cli.main([]) + + assert "no keychain keys are held" in capsys.readouterr().err + + def test_a_keychain_failure_reaches_the_same_place(self, monkeypatch, capsys): + # By inheritance rather than by being listed, which is the only reason #140 got a usable + # message at all. A separate handler that forgot it would be silent here. + self._failing_run( + monkeypatch, + KeychainSessionError("No keychain keys are held, so nothing can be decrypted."), + ) + + assert cli.main([]) == 1 + assert GITHUB_ISSUES_LINK in capsys.readouterr().err + + def test_a_mistake_the_user_can_fix_does_not_ask_for_a_bug_report(self, monkeypatch, capsys): + # The distinction that makes the link worth anything. Being sent to file an issue about + # your own typo is wrong, and it teaches people to ignore the link when it matters. + self._failing_run(monkeypatch, ExportSourceError("Signing in was stopped.")) + + assert cli.main([]) == 1 + assert GITHUB_ISSUES_LINK not in capsys.readouterr().err