feat: add apple-photos skill for macOS Photos library#91
Conversation
Consolidates raw prototype (3 separate scripts) into a single UV script with people/query/export subcommands following repo conventions. Includes collision-safe exports, per-file error handling, shared query builder, and 38 tests (unit + integration). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dd2fd2bec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| movies=getattr(args, "movies", True), | ||
| newest_first=getattr(args, "newest_first", True), |
There was a problem hiding this comment.
Fixed in PR #107. Export now uses BooleanOptionalAction with default=True for both --movies and --newest-first, making defaults consistent with the documented "default: on" behavior while allowing opt-out via --no-movies / --no-newest-first.
There was a problem hiding this comment.
Fixed in ecff93d. Added --movies and --newest-first explicitly to the export parser with default=True, and dropped the opaque getattr(..., True) fallback. Both subcommands now use the same build_query_options path with consistent, visible defaults.
There was a problem hiding this comment.
The core bug (un-toggleable flags always sending movies=True) is fixed in PR #105 via BooleanOptionalAction. The remaining default difference (export defaults movies/newest_first to True, query to False) is intentional: export is a batch-copy-everything operation while query is a targeted preview. Users can use --no-movies on export to match query behavior.
| # Pre-register a mock osxphotos so the import doesn't fail on non-macOS/CI | ||
| _mock_osxphotos = MagicMock() | ||
| _mock_osxphotos.QueryOptions = MagicMock | ||
| sys.modules.setdefault("osxphotos", _mock_osxphotos) |
There was a problem hiding this comment.
Fixed in PR #107. Test now checks importlib.util.find_spec("osxphotos") before injecting the mock (line 21), using find_spec which does not import. The mock setup (line 28) uses setdefault to preserve the real package if already imported.
There was a problem hiding this comment.
Fixed in ecff93d. Moved availability check before mock injection using importlib.util.find_spec('osxphotos') — which inspects the package finder without importing, so it's unaffected by the mock in sys.modules. Integration tests will now skip correctly on machines without the real package.
There was a problem hiding this comment.
Incorrect — is computed on line 21 via (which doesn't import) before the mock is injected via on line 28. The comment in the code even says so explicitly: "Check real package availability BEFORE injecting the mock — find_spec doesn't import." The mock has no effect on an already-computed boolean.
There was a problem hiding this comment.
Incorrect analysis. HAS_OSXPHOTOS is computed at line 21 using importlib.util.find_spec() before the mock is injected at line 28. find_spec() checks the import system without importing, so it correctly detects whether the real package is available. The comment on line 20 even notes: "Check real package availability BEFORE injecting the mock — find_spec doesn't import."
There was a problem hiding this comment.
👎 Incorrect analysis. Line 21 (HAS_OSXPHOTOS = importlib.util.find_spec(...)) runs before the mock is injected at lines 26–28. The comment above it even says "Check real package availability BEFORE injecting the mock — find_spec doesn't import." find_spec detects installed packages without triggering an import, so the check is correct and intentional.
…etection - Add --movies and --newest-first explicitly to export parser (fixes hidden default divergence between query and export subcommands) - Use find_spec() to detect real osxphotos before injecting mock, so HAS_OSXPHOTOS is correct on machines without the package installed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ecff93d. Configure here.
| from_date=parse_date(args.after), | ||
| to_date=parse_date(args.before), | ||
| favorite=True if getattr(args, "favorite", False) else None, | ||
| edited=True if getattr(args, "edited", False) else None, |
There was a problem hiding this comment.
Fixed in PR #107. Added skip_edited_filter parameter to build_query_options(). Export uses skip_edited_filter=True to avoid DB-level filtering, then implements "prefer edited" semantics at file selection time (line 169). Non-edited photos are now included as documented.
There was a problem hiding this comment.
Great catch — confirmed bug. Fixed in PR #105 by adding edited_filter=False for the export path. build_query_options now accepts a keyword-only edited_filter param (defaults to True for query). Export passes False so QueryOptions doesn't pre-filter, letting cmd_export's source-selection logic handle the "prefer edited" intent correctly.
There was a problem hiding this comment.
| p_export.add_argument("--before", help="Photos before this date (YYYY-MM-DD)") | ||
| p_export.add_argument("--edited", action="store_true", help="Prefer edited versions") | ||
| p_export.add_argument("--movies", action="store_true", default=True, help="Include movies (default: on)") | ||
| p_export.add_argument("--newest-first", action="store_true", default=True, help="Sort newest first (default: on)") |
There was a problem hiding this comment.
Fixed in PR #107. Changed from action="store_true" with default=True to action=argparse.BooleanOptionalAction with default=True, enabling --no-movies and --no-newest-first opt-out flags.
There was a problem hiding this comment.
Real bug. Fixed in PR #105 by switching both flags to argparse.BooleanOptionalAction, which adds --no-movies / --no-newest-first and gives the "default on, opt-out possible" behavior the help text implied.
The --edited flag on export was documented as "prefer edited versions" but was incorrectly filtering out all non-edited photos via QueryOptions. This fix adds a filter_edited parameter to build_query_options() that allows export to disable the filter while still using the flag as a preference in the source selection logic. Now export includes all photos and just prefers edited versions when available, matching the documented behavior. Addresses cursor[bot] feedback on PR #91. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…#107) - --edited on export now queries all matching photos and prefers the edited file path at copy time (was incorrectly filtering to only-edited photos at the DB level, silently excluding non-edited photos) - --movies and --newest-first on export now use BooleanOptionalAction so users can pass --no-movies / --no-newest-first to opt out (was store_true + default=True, making them permanent no-ops with no opt-out path) - Bump apple-photos skill to 0.1.1 Co-authored-by: Hex Sullivan <hex@technick.ai> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>

Summary
apple-photosskill — single UV script withpeople,query, andexportsubcommands wrappingosxphotosWhat changed from the prototype
_1,_2suffixes instead of silently overwritingbuild_query_optionsused by both query and export (was duplicated)shutil.copy2failures skip with report instead of killing the runpathfield —photo_to_dictnow hasoriginal_path/edited_pathonly<NAME>placeholdersTest plan
🤖 Generated with Claude Code