diff --git a/project/ticket-131/README.md b/project/ticket-131/README.md new file mode 100644 index 0000000..050ed43 --- /dev/null +++ b/project/ticket-131/README.md @@ -0,0 +1,22 @@ +# Ticket 131: Cache explicit GitHub API reads from repository monitors + +- **ID**: ticket-131 +- **Owner**: codex +- **Status**: IN_PROGRESS +- **Workflow state**: PUBLICATION +- **Created**: 2026-10-27 +- **Authorization**: SESSION_EXECUTION_AUTHORIZATION — the user requested reusable caching of repeated API records across providers and investigation of missed call paths. + +## Goal and scope + +Reuse canonical Procache for explicit GitHub API reads from repository monitors while preserving cwd-dependent execution and zero-TTL bypass. Preserve unrelated primary integration changes. + +## Acceptance criteria + +- [x] AC-01: Explicit API and repository read commands reuse cache; implicit repository context and placeholders retain direct execution. +- [x] AC-02: Cache/monitor regressions and managed governance pass; failures never trigger duplicate transport calls. +- [ ] AC-03: Observe protected publication separately from local checks and runtime deployment. + +Local verification: 42 scoped cache and monitor tests pass, with sixteen additional subtests, against the accepted canonical Procache source. Governance and scoped lint pass. No provider request is repeated after a cache/transport failure. + +The five context regression methods also pass under unittest discovery, matching the deployed protected Monag test collector. Existing accepted Procache emits SQLite ResourceWarnings; this ticket does not hide or change that dependency behavior. diff --git a/project/ticket-131/intent.json b/project/ticket-131/intent.json new file mode 100644 index 0000000..f4fe922 --- /dev/null +++ b/project/ticket-131/intent.json @@ -0,0 +1,76 @@ +{ + "schema": "new-project.intent/v3", + "ticket": "ticket-131", + "summary": "Cache explicit GitHub API reads from repository monitors", + "workstream": "application", + "classification": { + "kind": "SERVICE", + "priority": "P2", + "origin": "health" + }, + "allowedPaths": [ + "project/ticket-131/**", + "TODO.md", + "project/TICKETS.md", + "src/monag/cache.py", + "tests/test_procache_integration.py" + ], + "forbiddenPaths": [ + "project/ticket-*/user-*.md" + ], + "stacks": [], + "dependsOn": [], + "conflictsWith": [], + "integrationTicket": null, + "delivery": { + "acceptedBaseSha": "a373f608ca484389ac820eed576043e153f7f87d", + "targetBranch": "main", + "outcome": "Reuse canonical Procache for explicit GitHub API reads from repository monitors while preserving cwd-dependent command execution and zero-TTL bypass", + "nonGoals": [ + "Dependency pin changes", + "Caching writes or authorization observations", + "Changing other writers integration files" + ], + "complexity": "S", + "estimatedMinutes": 30, + "budgets": { + "maxImplementationFiles": 2, + "maxAffectedComponents": 1, + "maxPublicInterfaceChanges": 0, + "maxRuntimeDependencies": 0 + }, + "architecture": { + "status": "accepted", + "decision": "Keep the existing canonical CachedReadCommand transport and admit only commands whose repository context is explicit", + "components": [ + { + "name": "monitor-read-cache", + "paths": [ + "src/monag/cache.py", + "tests/test_procache_integration.py" + ] + } + ], + "responsibilityChanges": false, + "interfaceChanges": [], + "dataChanges": [], + "ui": { + "impact": "none", + "states": [], + "evidence": [] + }, + "rollback": "Revert the exact implementation through protected delivery" + }, + "runtimeDependencies": [], + "validation": [ + { + "criterion": "AC-02", + "commands": [ + "python3 -B -m pytest -q tests/test_procache_integration.py tests/test_monitor.py", + "./project/governance-check.sh" + ], + "evidence": "Explicit API cache hits, cwd and placeholder bypass, zero-TTL and no duplicate failures" + } + ] + } +} diff --git a/src/monag/cache.py b/src/monag/cache.py index ab883ee..a5f9c36 100644 --- a/src/monag/cache.py +++ b/src/monag/cache.py @@ -3,6 +3,7 @@ import math import os +import re from pathlib import Path from typing import Sequence, Tuple, Optional @@ -27,9 +28,6 @@ def get_cache_runner( if not PROCACHE_AVAILABLE or os.environ.get("MONAG_DISABLE_PROCACHE") == "1": return None - if _runner is not None and ttl is None and cache_path is None: - return _runner - path = Path(cache_path) if cache_path else _cache_path if path is None: cache_root = Path(os.environ.get("XDG_CACHE_HOME", Path.home() / ".cache")) @@ -45,6 +43,13 @@ def get_cache_runner( except (TypeError, ValueError, OverflowError): effective_ttl = 30.0 + # Zero disables caching, including a runner already initialized by an + # earlier monitor refresh. Let the caller execute directly once. + if effective_ttl == 0: + return None + if _runner is not None and ttl is None and cache_path is None: + return _runner + try: cache = SQLiteResponseCache(path, namespace="github-user") runner = CachedReadCommand(cache, ttl=effective_ttl) @@ -80,8 +85,10 @@ def run_cached_gh( if runner is None or not runner.is_read(args): return None - # If cwd is set and --repo is not explicitly specified, direct execution is safer - if cwd is not None and "--repo" not in args: + # CachedReadCommand does not accept cwd. gh api endpoints are explicit, + # except placeholders that gh resolves from the working repository. + # Repository subcommands need an explicit --repo/-R value instead. + if cwd is not None and not _has_explicit_context(args): return None try: @@ -95,3 +102,16 @@ def run_cached_gh( # The provider may already have executed. Returning None would cause # monitor.command to repeat the request and bypass cooldown/failure. return "", f"gh cache: {type(error).__name__}" + + +def _has_explicit_context(args: Sequence[str]) -> bool: + if len(args) > 1 and args[1] == "api": + return not any(re.search(r"\{(?:owner|repo|branch)\}", item) for item in args[2:]) + for index, item in enumerate(args): + if item in {"--repo", "-R"}: + return index + 1 < len(args) and bool(args[index + 1]) and not args[index + 1].startswith("-") + if item.startswith("--repo=") and item.removeprefix("--repo="): + return True + if item.startswith("-R") and len(item) > 2: + return True + return False diff --git a/tests/test_procache_integration.py b/tests/test_procache_integration.py index 0738724..9ba7f94 100644 --- a/tests/test_procache_integration.py +++ b/tests/test_procache_integration.py @@ -4,6 +4,9 @@ import json import os import subprocess +import tempfile +import unittest +from pathlib import Path from unittest import mock import pytest @@ -202,3 +205,70 @@ def test_cli_main_entrypoint_execution(): assert res.returncode == 0 assert "monag" in res.stdout + + +class CacheContextTests(unittest.TestCase): + """Also collected by the protected Monag unittest discovery profile.""" + + def setUp(self): + reset_cache_runner() + temporary = tempfile.TemporaryDirectory() + self.addCleanup(temporary.cleanup) + self.root = Path(temporary.name) + environment = mock.patch.dict(os.environ, { + "XDG_CACHE_HOME": str(self.root), "MONAG_GITHUB_READ_TTL": "60", + "MONAG_DISABLE_PROCACHE": "0", + }) + environment.start() + self.addCleanup(environment.stop) + self.addCleanup(reset_cache_runner) + + def test_explicit_api_endpoint_reuses_cache_with_cwd(self): + args = ["gh", "api", "repos/semcod/example/issues/42"] + with mock.patch("subprocess.run", return_value=subprocess.CompletedProcess(args, 0, '{"id":42}', "")) as transport: + first = command(args, cwd=self.root) + second = command(args, cwd=self.root / "other") + self.assertEqual(first, ('{"id":42}', None)) + self.assertEqual(second, first) + self.assertEqual(transport.call_count, 1) + + def test_context_dependent_commands_bypass_cache(self): + from monag.cache import run_cached_gh + for args in ( + ["gh", "pr", "list"], + ["gh", "api", "repos/{owner}/{repo}/issues"], + ["gh", "api", "repos/semcod/example/branches/{branch}"], + ["gh", "api", "graphql", "-f", 'query=query { repository(owner: "{owner}", name: "{repo}") { id } }'], + ): + with self.subTest(args=args), mock.patch("subprocess.run") as transport: + self.assertIsNone(run_cached_gh(args, cwd=self.root)) + transport.assert_not_called() + + def test_explicit_repository_options_cache_with_cwd(self): + for repo_option in (["--repo", "semcod/example"], ["--repo=semcod/example"], + ["-R", "semcod/example"], ["-Rsemcod/example"]): + args = ["gh", "pr", "list", *repo_option] + with self.subTest(option=repo_option), mock.patch( + "subprocess.run", return_value=subprocess.CompletedProcess(args, 0, "[]", ""), + ) as transport: + self.assertEqual(command(args, cwd=self.root), ("[]", None)) + self.assertEqual(command(args, cwd=self.root), ("[]", None)) + self.assertEqual(transport.call_count, 1) + + def test_zero_ttl_disables_an_initialized_runner_and_executes_once(self): + self.assertIsNotNone(get_cache_runner()) + with mock.patch.dict(os.environ, {"MONAG_GITHUB_READ_TTL": "0"}): + args = ["gh", "api", "repos/semcod/example"] + self.assertIsNone(get_cache_runner()) + with mock.patch("subprocess.run", return_value=subprocess.CompletedProcess(args, 0, "fresh", "")) as transport: + self.assertEqual(command(args, cwd=self.root), ("fresh", None)) + self.assertEqual(transport.call_count, 1) + self.assertEqual(transport.call_args.kwargs["cwd"], self.root) + + def test_cached_api_transport_failure_is_not_repeated_by_monitor(self): + args = ["gh", "api", "repos/semcod/example"] + with mock.patch("subprocess.run", side_effect=subprocess.TimeoutExpired(args, 8)) as transport: + stdout, error = command(args, cwd=self.root) + self.assertEqual(stdout, "") + self.assertIn("TimeoutExpired", error) + self.assertEqual(transport.call_count, 1)