Skip to content

feat: add TTL-based file-backed caching for GitHubCLIProvider#50

Open
Achiever199 wants to merge 1 commit into
Hell1213:mainfrom
Achiever199:feature/github-cache-layer
Open

feat: add TTL-based file-backed caching for GitHubCLIProvider#50
Achiever199 wants to merge 1 commit into
Hell1213:mainfrom
Achiever199:feature/github-cache-layer

Conversation

@Achiever199
Copy link
Copy Markdown

Summary

Closes #35

Adds an optional caching layer to GitHubCLIProvider to avoid
redundant gh CLI calls within and across sessions.

Changes

New file: src/oss_dev/providers/cache.py

  • CacheEntry — stores value + expiry timestamp
  • ResponseCache — two-layer cache (memory + disk)
    • Memory layer avoids repeated disk reads in the same process
    • Disk layer (JSON files) survives process restarts
    • TTL-based expiry on all entries
    • make_key() uses SHA-256 hashing for safe filenames

Modified: src/oss_dev/providers/github/client.py

  • Added _run_gh_cached() helper for read-only calls
  • Cached methods: fetch_issue, list_issues,
    get_pr_status, get_pr_comments
  • create_pr intentionally bypasses cache (mutating operation)
  • Cache controlled via config.cache.enabled / ttl / dir

New file: tests/providers/test_cache.py

  • 22 unit tests, all passing
  • Covers: TTL expiry, disk persistence, disabled mode,
    invalidation, provider integration

Copy link
Copy Markdown
Owner

@Hell1213 Hell1213 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: TTL-based caching for GitHubCLIProvider

Hi @Achiever199, great implementation! The cache is well-structured with 22 tests covering TTL, disk persistence, invalidation, and disabled mode. Tests pass (94/94).

Issues to fix (ruff F401/F841):

  1. src/oss_dev/providers/cache.py:12 - import os is unused (the code uses pathlib instead)
  2. tests/providers/test_cache.py:13 - MagicMock imported but unused (only patch is used)
  3. tests/providers/test_cache.py:91 - Variable key assigned but never used
  4. tests/providers/test_cache.py:193,210,227,240,256,272 - Each test imports GitHubCLIProvider locally, but _make_provider() already imports it. The per-test imports are redundant and trigger F401. Remove them.

Conflict notice: PR #54 by @Jiya3177 also modifies src/oss_dev/providers/github/client.py (adds rate limit awareness). Both change _run_gh and class internals. The author who merges second will need to rebase and reconcile.

Suggestion: Consider whether you want _run_gh_cached to also respect rate limits (from PR #54) once both are merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add caching layer for GitHub API responses

2 participants