Harden portable Redmine CLI runtime and cache writes

redmine_tickets.py

@@ -10,6 +10,8 @@ from __future__ import annotations
 
 import argparse
 import json
+import copy
+from redmine_cache_io import CacheError, cache_lock, read_json, atomic_write_json, update_manifest_atomic
 import os
 import re
 import sys
@@ -1062,31 +1064,50 @@ class ManifestTicketStateStore:
         self.project = project
         self.manifest_path = cache_dir / "manifest.json"
         self.manifest = self.load_manifest()
+        self._baseline = copy.deepcopy(self.manifest)
 
     def load_manifest(self) -> dict[str, Any]:
         if not self.manifest_path.is_file():
             return {"version": 1, "tickets": {}}
-        with self.manifest_path.open("r", encoding="utf-8") as handle:
-            data = json.load(handle)
+        data = read_json(self.manifest_path)
         data.setdefault("version", 1)
         data.setdefault("tickets", {})
         return data
 
     def save(self) -> None:
-        self.cache_dir.mkdir(parents=True, exist_ok=True)
-        with self.manifest_path.open("w", encoding="utf-8") as handle:
-            json.dump(self.manifest, handle, indent=2, sort_keys=True)
-            handle.write("\n")
+        # Apply only locally changed leaves to current disk state. This preserves
+        # updates made by another long-lived cache instance since our last read.
+        def delta(current, before, after):
+            for key in before.keys() - after.keys():
+                current.pop(key, None)
+            for key, value in after.items():
+                if key not in before:
+                    current[key] = copy.deepcopy(value)
+                elif isinstance(value, dict) and isinstance(before[key], dict):
+                    target = current.setdefault(key, {})
+                    if not isinstance(target, dict):
+                        target = current[key] = {}
+                    delta(target, before[key], value)
+                elif value != before[key]:
+                    current[key] = copy.deepcopy(value)
+        self.manifest = update_manifest_atomic(
+            self.manifest_path, lambda current: delta(current, self._baseline, self.manifest)
+        )
+        self._baseline = copy.deepcopy(self.manifest)
 
     def get_ticket_state(self, issue_id: int | str) -> dict[str, Any] | None:
-        return self.manifest.get("tickets", {}).get(str(issue_id))
+        # Readers see atomic snapshots and need no writer lock.
+        return self.load_manifest().get("tickets", {}).get(str(issue_id))
 
     def upsert_ticket_state(self, issue_id: int | str, fields: dict[str, Any]) -> None:
-        ticket_entry = self.manifest.setdefault("tickets", {}).setdefault(str(issue_id), {})
-        ticket_entry.setdefault("project", self.project)
-        ticket_entry.setdefault("actions", {})
-        ticket_entry.update(fields)
-        self.save()
+        with cache_lock(self.manifest_path):
+            self.manifest = self.load_manifest()
+            self._baseline = copy.deepcopy(self.manifest)
+            ticket_entry = self.manifest.setdefault("tickets", {}).setdefault(str(issue_id), {})
+            ticket_entry.setdefault("project", self.project)
+            ticket_entry.setdefault("actions", {})
+            ticket_entry.update(fields)
+            self.save()
 
     def has_action(self, issue_id: int | str, action_name: str) -> bool:
         entry = self.get_ticket_state(issue_id) or {}
@@ -1099,15 +1120,18 @@ class ManifestTicketStateStore:
         result: str,
         metadata: dict[str, Any] | None = None,
     ) -> None:
-        ticket_entry = self.manifest.setdefault("tickets", {}).setdefault(str(issue_id), {})
-        ticket_entry.setdefault("project", self.project)
-        actions = ticket_entry.setdefault("actions", {})
-        actions[action_name] = {
-            "last_run": now_iso(),
-            "result": result,
-            "metadata": metadata or {},
-        }
-        self.save()
+        with cache_lock(self.manifest_path):
+            self.manifest = self.load_manifest()
+            self._baseline = copy.deepcopy(self.manifest)
+            ticket_entry = self.manifest.setdefault("tickets", {}).setdefault(str(issue_id), {})
+            ticket_entry.setdefault("project", self.project)
+            actions = ticket_entry.setdefault("actions", {})
+            actions[action_name] = {
+                "last_run": now_iso(),
+                "result": result,
+                "metadata": metadata or {},
+            }
+            self.save()
 
 
 class TicketCache:
@@ -1146,19 +1170,20 @@ class TicketCache:
 
     def write_issue(self, issue: dict[str, Any]) -> None:
         issue_id = issue["id"]
-        self.project_dir.mkdir(parents=True, exist_ok=True)
-        with self.issue_path(issue_id).open("w", encoding="utf-8") as handle:
-            json.dump(issue, handle, indent=2, sort_keys=True)
-            handle.write("\n")
-        self.state_store.upsert_ticket_state(
-            issue_id,
-            {
-                "project": self.project,
-                "updated_on": issue.get("updated_on"),
-                "last_synced": now_iso(),
-                "path": str(self.issue_path(issue_id)),
-            },
-        )
+        # Use one lock for a record + manifest transaction. Alternate stores keep
+        # their own state transaction semantics, but JSON still writes atomically.
+        lock_target = self.manifest_path or (self.cache_dir / "manifest.json")
+        with cache_lock(lock_target):
+            atomic_write_json(self.issue_path(issue_id), issue)
+            self.state_store.upsert_ticket_state(
+                issue_id,
+                {
+                    "project": self.project,
+                    "updated_on": issue.get("updated_on"),
+                    "last_synced": now_iso(),
+                    "path": str(self.issue_path(issue_id)),
+                },
+            )
 
     def iter_cached_issues(self) -> Iterable[dict[str, Any]]:
         if not self.project_dir.is_dir():
@@ -2322,10 +2347,14 @@ def run_walk_session(issues: list[dict[str, Any]], cache: TicketCache, pager: st
 
 def client_from_args(args: argparse.Namespace) -> TicketRedmineClient:
     api_key = os.getenv("REDMINE_API_KEY")
-    if not api_key and getattr(args, "api_key_file", None):
-        key_path = Path(args.api_key_file)
-        if key_path.is_file():
+    if getattr(args, "api_key_file", None):
+        key_path = Path(args.api_key_file).expanduser()
+        try:
             api_key = key_path.read_text(encoding="utf-8").strip()
+        except OSError as exc:
+            raise RedmineError(f"Cannot read API key file {key_path}: {exc.strerror}") from exc
+        # The internal contacts client and attachment/OCR helpers share this key.
+        os.environ["REDMINE_API_KEY"] = api_key
     if not api_key:
         raise RedmineError("Set REDMINE_API_KEY in the environment or supply --api-key-file.")
     return TicketRedmineClient(base_url=args.base_url, api_key=api_key, project=args.project)
@@ -3265,7 +3294,7 @@ KNOWN_COMMANDS = frozenset(
 
 # Parent-parser options that take a following value. Used when injecting the
 # default `sync` subcommand so sync-only flags like --updated-since parse correctly.
-_PARENT_VALUE_OPTIONS = frozenset({"--base-url", "--project", "--cache-dir", "--config", "--allow-prod-writes"})
+_PARENT_VALUE_OPTIONS = frozenset({"--base-url", "--project", "--cache-dir", "--config", "--api-key-file", "--allow-prod-writes"})
 
 
 def inject_default_sync_command(argv: list[str]) -> list[str]:
@@ -3311,6 +3340,16 @@ def main(argv: list[str] | None = None) -> int:
     normalized_argv = inject_default_sync_command(raw_argv)
     parser = build_parser()
     args = parser.parse_args(normalized_argv)
+    if getattr(args, "api_key_file", None):
+        try:
+            key = Path(args.api_key_file).expanduser().read_text(encoding="utf-8").strip()
+        except OSError as exc:
+            print(f"error: Cannot read API key file {args.api_key_file}: {exc.strerror}", file=sys.stderr)
+            return 2
+        if not key:
+            print("error: API key file is empty; supply REDMINE_API_KEY or a nonempty --api-key-file.", file=sys.stderr)
+            return 2
+        os.environ["REDMINE_API_KEY"] = key
 
     # Post-process common globals so subparsers (tui/browse/...) that re-declare them
     # (to support "cmd --allow-prod-writes ...") don't leave them as None.
@@ -3763,11 +3802,8 @@ def main(argv: list[str] | None = None) -> int:
             print(f"contact_id={cid}")
             print(f"handoff_hint=contact-show --contact-id {cid}")
             print(f"handoff_hint=contact-set --contact-id {cid} --add-tag maybe-customer")
-            print(
-                "handoff_hint_full_crm="
-                f"redmine_contacts.py --base-url {args.base_url} --project {args.project} "
-                f"show --contact-id {cid}"
-            )
+            from redmine_cli_runtime import contacts_handoff
+            print("handoff_hint_full_crm=" + contacts_handoff(args, cid))
         # --fetch implies show; alone it used to silently do nothing (bug).
         want_show = bool(getattr(args, "show", False) or getattr(args, "fetch", False))
         want_fetch = bool(getattr(args, "fetch", False))
@@ -4925,4 +4961,9 @@ def main(argv: list[str] | None = None) -> int:
 
 
 if __name__ == "__main__":
-    raise SystemExit(main())
+    try:
+        raise SystemExit(main())
+    except (CacheError, RedmineError) as exc:
+        label = "Cache error" if isinstance(exc, CacheError) else "error"
+        print(f"{label}: {exc}", file=sys.stderr)
+        raise SystemExit(75 if isinstance(exc, CacheError) else 2)