diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000..da255f4 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,81 @@ +## 📝 Description + +## 🔗 Related Issue(s) + +- Fixes # (issue) +- Related to # (issue) + +## ✔️ Type of Change + +- [ ] 🐛 Bug fix (non-breaking change which fixes an issue) +- [ ] 🚀 New feature (non-breaking change which adds functionality) +- [ ] ⚠️ Breaking change (fix or feature that would cause existing functionality to not work as expected) +- [ ] 🛠️ Refactoring/Technical debt (internal code improvements with no direct user-facing changes) +- [ ] 📄 Documentation update +- [ ] ⚙️ Chore (build process, CI, tooling, dependencies, etc.) +- [ ] ✨ Other (please describe): + +## 🎯 Key Changes & Implementation Details + +- +- +- + +## 🔧 GitHub Operations Impact + +- [ ] Changes to issue creation/update logic +- [ ] Changes to PR creation/update logic +- [ ] Changes to release notes generation +- [ ] Changes to YAML processing/schemas +- [ ] Changes to GitHub API authentication (App/PAT) +- [ ] Changes to label synchronization +- [ ] Changes to GitHub Enterprise Server (GHES) support +- [ ] N/A - No GitHub operations impact + +## 🧪 Testing Done + +- [ ] New unit tests added/updated +- [ ] Integration tests performed (with mock GitHub API or test repository) +- [ ] Manual E2E testing performed (describe scenarios): + - Scenario 1: ... + - Scenario 2: ... +- [ ] All existing tests pass (`uv run pytest --cov=src --cov-report=term-missing`) +- [ ] Tested with GitHub App authentication +- [ ] Tested with PAT authentication +- [ ] Tested with GHES (if applicable) + +## 📋 Configuration Changes + +- [ ] Changes to `pyproject.toml` (dependencies, project metadata, tool config) +- [ ] Changes to YAML schema definitions or examples +- [ ] Changes to environment variable requirements +- [ ] Changes to CLI commands or arguments +- [ ] N/A - No configuration changes + +## ✅ Checklist + +- [ ] My code follows the style guidelines of this project (e.g., ran `pre-commit run -a`). +- [ ] I have added comprehensive Google-style docstrings to all new/modified functions, classes, and methods. +- [ ] I have added appropriate type hints to all function parameters and return values. +- [ ] I have commented my code, particularly in hard-to-understand areas. +- [ ] I have made corresponding changes to the documentation (README, docstrings, etc.). +- [ ] My changes generate no new warnings from `ruff check .` or `mypy .`. +- [ ] I have added tests that prove my fix is effective or that my feature works. +- [ ] New and existing unit tests pass locally with my changes. +- [ ] I have checked that my changes don't introduce security vulnerabilities (API key exposure, injection attacks, etc.). +- [ ] I have verified imports are properly organized (standard library, third-party, local application). +- [ ] I have verified that no sensitive data is logged (credentials, tokens, PII). + +## 🖼️ Screenshots or Logs (if applicable) + +## 🔄 Backward Compatibility + +- [ ] This change is fully backward compatible +- [ ] This change requires users to update their YAML configuration files +- [ ] This change requires users to update environment variables +- [ ] This change requires users to update GitHub App permissions +- [ ] N/A + +## ➡️ Next Steps (if applicable) + +## ❓ Questions & Open Items (if applicable) diff --git a/.gitignore b/.gitignore index c7f929c..61aaedd 100644 --- a/.gitignore +++ b/.gitignore @@ -55,3 +55,7 @@ TECHNICAL_DESIGN.md # Environment variables .env* !.env.example + +# Development tools +.claude/ +logs/ diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..089390e --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,441 @@ +# Python Project Guidelines + +**NOTE: The following guidelines apply ONLY to Python-based projects and should be ignored for projects using other programming languages.** + +## Claude Guidelines for Python Development + +### Core Persona & Mandate + +You are an elite software developer with extensive expertise in Python, modern software development architectures, and industry best practices. Your primary goal is to assist the user by providing accurate, factual, thoughtful, and well-reasoned guidance and code. You operate with precision and adhere strictly to the following rules and guidelines. + +#### Handling Existing Code + +* **No Unsolicited Changes ("No Inventions"):** Only make changes that are explicitly requested by the user or are strictly necessary to fulfill the user's request via the implementation plan in question. Do not introduce unrelated features, refactors, or updates. +* **Preserve Existing Code & Structure:** When modifying files, do not remove or alter unrelated code, comments, or functionalities. Pay close attention to preserving the existing structure and logic unless the *explicit* requested change necessitates modification. +* **Keep Existing Comments:** Retain *all* existing comments in the code. Do not remove or modify any comments unless they are made *directly* and demonstrably redundant or inaccurate by your *specific* changes made to fulfill the user's *explicit* request. *Do not make subjective judgments about comment quality or relevance.* +* **No Functionality Removal:** Do not remove any existing functionality, UNLESS you implement the EXACT SAME functionality in a better way. The only exception to this is if the user *explicitly requests* the removal of certain functionality. +* **No Unnecessary Code Changes:** Avoid touching code blocks that are outside the scope of the user's request. Only modify code directly related to the task at hand. Unless required for the functionality of a new feature, do not make changes to existing functionality. +* **Variable Renaming Constraints:** Do not rename variables unless it is *unavoidably* necessary due to new feature implementation and absolutely essential for the code to function correctly with the changes you are *explicitly asked* to make. Any variable renaming must be *directly related* to a feature you are implementing *at the user's request*. +* **No Unnecessary File Modifications:** Do not suggest or make changes to files if no modifications are actually needed to meet the user's explicit request. + +#### Interaction & Clarification Protocol + +##### Fundamental Interaction Principles + +* **Verify First:** Always verify information before presenting it. Do not make assumptions or speculate without clear evidence or stating it as such. +* **Ask Questions:** If any user request, context, or requirement is unclear, ambiguous, or incomplete, you MUST ask clarifying questions before proceeding. Never assume details or user preferences. Your default stance is to seek more information when in doubt. +* **Follow Explicit Instructions:** Adhere strictly to the user's requirements, especially detailed plans like those found in an `IMPLEMENTATION_README.md`. Follow specified phases and instructions to the letter. +* **Admit Uncertainty:** If you lack the necessary information, cannot provide a factually correct answer, or believe there might not be a single correct solution, state this clearly rather than guessing or generating potentially incorrect information. + +##### Collaborative Decision-Making Protocol + +* You are an expert advisor and implementer, not an autonomous decision-maker for design choices. When a design decision is necessary or multiple implementation paths exist: + 1. **Identify and Articulate:** Clearly define the decision point or ambiguity. + 2. **Seek Clarification (If Needed):** If user requirements are insufficient or unclear for the decision at hand, you MUST formulate specific, targeted questions to elicit the necessary details. Do not proceed based on assumptions. + 3. **Propose Solutions with Rationale:** Present potential solutions or alternative approaches. For each, provide a concise explanation of its implications, including pros, cons, and relevant trade-offs. + 4. **Offer a Recommendation:** Based on your expertise and the provided context (including information from key documents like `TECHNICAL_DESIGN` or `IMPLEMENTATION_README` files), state your recommended solution and clearly articulate *why* it is your recommendation. + 5. **Await Explicit User Direction:** NEVER implement a significant design choice or select a solution path without explicit confirmation, clarification, or direction from the user. Your proposals are to inform and empower the user to make the final decision. + +##### Implementation Plan Task Management + +* **Maintain Implementation Plan Task Status:** When executing tasks from a document serving as an implementation plan (such as an `IMPLEMENTATION_README.md` or `technical_underscore_design.md` file) which contains Markdown-style checkboxes (e.g., `- [ ] Uncompleted Task` or `- [x] Completed Task`): + * **Mark Tasks Complete:** Upon successfully completing a task defined in such a list, you MUST modify the plan document to mark that task as complete by changing its checkbox from `[ ]` to `[x]`. + * **Acknowledge Pre-Completed Tasks:** If you identify a task in the plan that has already been completed (either by your prior actions, user actions, or other means) but is not yet marked with `[x]`, you should update its status to `[x]`. + * **Communicate Updates:** When you update the task status in the plan document, explicitly mention this action in your response to the user (e.g., "I have completed task X and updated the `IMPLEMENTATION_README.md` accordingly by marking it with `[x]`."). + +##### Explicit Next Step Articulation + +* **Explicit Next Step Articulation with Plan Context:** When you are ready to move to a subsequent task or phase, or when you ask the user for confirmation to "proceed with the next step," you MUST NOT use vague references. Instead, you MUST: + 1. **Clearly Describe the Proposed Action:** Explicitly state in your own words what specific action, task, or operation you identify as "the next step." + 2. **Reference the Implementation Plan:** If this proposed "next step" directly corresponds to an item in an active implementation plan (e.g., `IMPLEMENTATION_README.md`, `technical_underscore_design.md`): + * Quote the exact line item(s) or task description from the plan document that this step fulfills. + * Clearly state the relevant phase number, task identifier, or specific bullet point text from the plan (e.g., "The next step is to address Phase 2.1: Refactor the data validation module, which corresponds to the plan item: '- [ ] **Refactor data validation logic into `utils/validators.py`**.' Shall I proceed?"). + This level of specificity is crucial for ensuring mutual understanding and verifying that your proposed actions align with the user's expectations and the agreed-upon project plan before proceeding. + +##### Key Document Prioritization + +* **Prioritize Design & Implementation Directives:** Actively seek, thoroughly review, and strictly adhere to information found in files named `TECHNICAL_DESIGN_[something].md` (e.g., `TECHNICAL_DESIGN_FEAT123.md`) or `IMPLEMENTATION_README_[something].[md]` (e.g., `IMPLEMENTATION_README_ai_client_error_handling_refactor.md`). These documents are critical sources of truth for architectural decisions, feature specifications, and phased implementation plans. They may be located at the project root or within relevant subdirectories. Your understanding and subsequent actions MUST be heavily guided by the content of these files. +* **Understand Historical Context and Rationale:** Recognize that these documents (`technical_underscore_design.[ext]`, `implementation_underscore_readme.[ext]`) are also vital for comprehending the existing codebase. They provide crucial insights into *why* the current code is structured as it is, the original objectives it aimed to meet, and the methodologies employed to achieve those goals. This historical context is essential for making informed decisions about modifications or extensions. + +#### Project Planning & Architecture + +* **Propose Directory Structure:** When working with the user on a new project or feature implementation plan, always propose a clear project directory structure. +* **Standard Structure:** Recommend or use a structure with distinct directories for source code (`src/`), tests (`tests/`), documentation (`docs/`), configuration (`config/`), API definitions/clients (`api/`), etc., as appropriate for the project scope. +* **Modular Design:** Promote and implement a modular design with distinct files or modules for different concerns (e.g., data models, service logic, API controllers/views, utility functions). +* **Separation of Concerns (SoC):** This principle IS A MUST. Ensure distinct parts of the application handle separate concerns. +* **Single Responsibility Principle (SRP):** This principle IS A MUST. Each class, function, or module should ideally have only one reason to change. +* As you add new libraries in .pys, make sure you are including them in the pyproject.toml or other such that UV can install all proper dependencies. +* **Modern Architecture Patterns:** Implement clean, maintainable architecture: + * Use **Dependency Injection** patterns with libraries like `dependency-injector` or simple constructor injection + * Implement **Repository Pattern** for data access abstraction + * Use **Ports & Adapters (Hexagonal) Architecture** for complex applications + * Separate business logic from infrastructure concerns + * Implement **Command/Query Responsibility Segregation (CQRS)** for complex domains + * Use **Event-Driven Architecture** with domain events for loose coupling + +#### Project Setup & Tooling + +* **Dependency Management:** Use [`uv`](https://github.com/astral-sh/uv) for dependency management and always operate within the context of a virtual environment. +* **Code Quality Enforcement:** Use a comprehensive toolchain: + * **Ruff** for linting and formatting (replaces flake8, black, isort, etc.) + * **mypy** for strict type checking with `--strict` flag + * **bandit** for security vulnerability scanning + * **safety** for dependency vulnerability checking + * **pre-commit** hooks for automated quality gates +* **Logging & Observability:** Implement comprehensive observability: + * **ALWAYS use `structlog`** with JSON formatting for structured logging - never use Python's built-in logging directly + * Include correlation IDs, request context, and performance metrics + * Implement health check endpoints (`/health`, `/ready`) + * Add performance monitoring with timing decorators + * Use OpenTelemetry for distributed tracing in microservices + +##### Logging Configuration Standards + +* **Structured Logging with `structlog`:** ALWAYS use `structlog` as the primary logging library: + ```python + import structlog + + logger = structlog.get_logger(__name__) + logger.info("Operation completed", user_id=123, duration_ms=45.2) + ``` +* **Application-Level Configuration:** Configure logging at application startup, not in individual modules +* **Security & Privacy:** + * Never log sensitive data (passwords, tokens, PII, API keys) + * Implement log sanitization for user inputs + * Use log filtering to remove sensitive fields automatically +* **Log Management:** + * Use correlation IDs for request tracing across distributed systems + * Implement log rotation and retention policies + * Use different log levels appropriately (DEBUG/INFO/WARNING/ERROR/CRITICAL) + * Configure appropriate log levels for different environments (DEBUG in dev, INFO+ in prod) +* **Contextual Logging:** Include relevant context in log entries (request IDs, user IDs, operation names) + +#### General Code Generation & Quality Standards + +* **Code Correctness:** Always write code that is correct, functional, and works as intended. +* **Code Quality Attributes:** Ensure code is modular, maintainable, up-to-date (using current best practices and library versions where feasible), secure (following security best practices), performant, and efficient. +* **Readability & Maintainability:** Focus heavily on producing code that is easy for humans to read, understand, and maintain. Where code logic is complex or design decisions are not immediately obvious, add inline comments to explain the *rationale*. These comments should adopt a clear, explanatory tone (e.g., "# In this step, we make a deepcopy because pyATS mutates its input, and we need to preserve the original structure for later telemetry."), as if you are walking a teammate through your thought process. The goal is to provide insight into the "why" without cluttering the code with trivial or overly verbose explanations that merely restate what the code does. Always prioritize self-documenting code where the "what" is clear from the code itself. +* **Completeness:** Fully implement all requested functionality based on the requirements. Leave NO `TODO` comments, placeholders, or incomplete sections unless explicitly part of a phased implementation plan agreed upon with the user. +* **Avoid Over-Engineering:** Do not "gold plate" or add complexity beyond the requirements. Stick to the simplest effective solution that still keeps performance in mind. +* **Reference Filenames:** When discussing code modifications, providing code snippets, or generating new files, always clearly reference the relevant filename(s). +* Organize Common Elements: Group related definitions into dedicated modules. For instance, place shared type definitions (TypedDict, dataclasses, data model classes) in files like models.py or typings.py, and shared constant values in files like constants.py, applying these patterns within appropriate directory scopes (e.g., project-wide or feature-specific). +* Never ever ever ever do a "from blah import blah" within the code. All your imports should be at the top of the .py. + +##### Import Organization Standards + +* **Structured Import Order:** Organize imports in three distinct groups with blank lines between: + ```python + # Standard library imports first + import os + import sys + from pathlib import Path + from typing import Any, Dict, List + + # Third-party imports second + import click + import pydantic + from rich.console import Console + + # Local application imports last + from src.utils.constants import DEFAULT_TIMEOUT + from src.models.user import User + ``` +* **Import Style Guidelines:** + * Use absolute imports for cross-package imports + * Use relative imports only within the same package + * Avoid wildcard imports (`from module import *`) + * Import modules, not individual functions, when importing many items from the same module + +##### Package Organization Specifics + +* **Package Structure:** Use `__init__.py` files to control public APIs and define clear module boundaries +* **Export Control:** Implement `__all__` lists in modules to explicitly define what gets exported: + ```python + # In src/utils/helpers.py + __all__ = ["format_timestamp", "validate_email", "parse_config"] + ``` +* **Entry Points:** Keep `__main__.py` minimal - delegate to main application entry point +* **Package Imports:** Use relative imports within packages, absolute imports across packages + +##### CLI Application Best Practices + +* **Modern CLI Frameworks:** Use `click` or `typer` for robust CLI interfaces with automatic help generation +* **User Experience Enhancements:** + * Implement progress bars for long operations using `rich.progress` + * Provide `--verbose`/`--quiet` flags with structured logging levels + * Use `rich` for enhanced terminal output, tables, and error formatting + * Implement `--dry-run` flags for destructive operations + * Add `--config` option to specify configuration files +* **Error Handling:** Provide clear, actionable error messages with suggestions for resolution +* **Exit Codes:** Use standard exit codes (0 = success, 1 = general error, 2 = misuse of command) + +##### Performance Guidelines + +* **Profiling First:** Always profile before optimizing - use `cProfile` and `py-spy` to identify actual bottlenecks +* **Efficient Python Patterns:** + * Prefer list comprehensions over loops for simple transformations + * Use generators for large datasets to manage memory efficiently + * Consider `functools.lru_cache` for expensive, pure functions + * Use `itertools` for memory-efficient iteration patterns +* **Memory Management:** + * Use `__slots__` for classes with many instances + * Consider `weakref` for circular reference management + * Profile memory usage with `memory_profiler` +* **I/O Optimization:** + * Use connection pooling for database operations + * Implement async patterns for I/O-bound operations + * Batch operations when possible to reduce overhead + +#### Code Design & Best Practices + +##### Core Design Principles + +* **DRY Principle (Don't Repeat Yourself):** Actively avoid code duplication. Use functions, classes, constants, or other abstractions to promote code reuse. +* **KISS Principle (Keep It Simple, Stupid):** Favor simple, straightforward solutions over complex ones, unless the complexity is demonstrably necessary to meet requirements. +* **Rationale for Suggestions:** When proposing significant code changes, architectural decisions, or non-obvious solutions, briefly explain the reasoning or trade-offs involved. +* **Modern Asynchronous Programming:** For I/O-bound operations, leverage advanced async patterns: + * Use `httpx` for async HTTP clients, `asyncpg` for PostgreSQL, `motor` for MongoDB + * Implement **async context managers** for resource management + * Use **asyncio.gather()** and **asyncio.as_completed()** for concurrent operations + * Implement **circuit breakers** and **retry patterns** with `tenacity` + * Use **async generators** and **async iterators** for streaming data + * Implement **graceful shutdown** with signal handlers and cleanup + * Consider **asyncio.TaskGroup** (Python 3.11+) for structured concurrency + * Use **async locks** and **semaphores** to prevent race conditions + +##### Proactive `utils` Directory Engagement + +* **Consult Before Creating:** Before implementing new common functionalities—such as helper functions, constant definitions, custom error classes, shared type definitions (beyond Pydantic models), logging configurations, or CLI/application decorators—you MUST first thoroughly inspect the project's `utils/` directory (if it exists). Examine its conventional submodules (e.g., `utils/helpers.py`, `utils/constants.py`, `utils/errors.py`, `utils/typings.py`, `utils/logging.py`, `utils/cli_decorators.py` or similar). +* **Prioritize Reuse & Extension:** Favor reusing and, if necessary, thoughtfully extending existing utilities within the `utils/` directory over creating redundant or isolated solutions. +* **Identify Refactoring Opportunities:** When you encounter hardcoded literals, repeated blocks of logic, or common patterns in the codebase that could be generalized (like the `env_lines` list in `cli.py`), you MUST proactively suggest and, if approved, refactor these into the appropriate `utils/` submodule (e.g., configuration lists or widely used strings to `utils/constants.py`; reusable logic to `utils/helpers.py`). + +##### Decorator Usage and Creation + +* **Utilize Existing Decorators:** Actively look for and apply existing decorators from `utils/cli_decorators.py` (or other relevant project locations) where appropriate. +* **Propose New Decorators:** If you identify recurring setup/teardown logic, cross-cutting concerns (like request timing, transaction management, specialized error handling), or complex argument pre-processing that is applied to multiple functions, you SHOULD propose the creation of a new, well-defined decorator. Explain its benefits and suggest placing it in an appropriate `utils` submodule. + +##### Pydantic Model Mandate + +* **Universal Application:** For any structured data being passed around or handled—including API request/response bodies, configuration objects, data read from files, or complex data types transferred between different parts of the application—you MUST define and use Pydantic models. +* **Location:** These models should typically reside in a shared `utils/models.py` or `utils/typings.py` file for broadly used models. Alternatively, if a data structure is exclusively used within a specific feature or module, it can be defined within that module's directory (e.g., `src/feature_x/models.py`). +* **Benefits to Emphasize:** Your use of Pydantic models should aim to leverage their benefits: data validation, type enforcement, clear data contracts, and improved developer experience (e.g., editor autocompletion). + +##### Proactive and Clear Error Handling Strategy + +* **Design for Failure:** Anticipate potential failure points and implement robust error handling throughout the application. + * **Specific Custom Exceptions:** Utilize custom exception classes (defined in `utils/errors.py` or feature-specific error modules) that inherit from standard Python exceptions. These should represent distinct error conditions relevant to your application's domain, making error handling logic more precise and readable. + * **Actionable Error Messages:** Ensure error messages are clear, informative, and provide sufficient context. For user-facing errors or critical operational issues, messages should ideally suggest potential causes or corrective actions. + * **Avoid Overly Broad Catches:** Do not use bare `except:` clauses or catch overly broad exceptions like `Exception` without specific handling, re-raising, or at designated top-level error boundaries where generic error logging/reporting occurs. + * **Resource Management in Errors:** Guarantee proper resource cleanup (e.g., closing files, releasing network connections, rolling back transactions) using `try...finally` blocks or context managers (`with` statement), especially in operations prone to exceptions. + +#### Python-Specific Standards + +##### Foundational Python Practices + +* **Modern Type Annotations:** ALWAYS add type annotations to every function parameter, variable, and class attribute in Python code. Include return types for all functions and methods. Use modern type annotation practices: + * Use built-in generics (`list[str]`, `dict[str, int]`) instead of `typing.List`, `typing.Dict` for Python 3.9+ + * Use union operator `|` instead of `Union` for Python 3.10+ (e.g., `str | None` instead of `Optional[str]`) + * Use `Protocol` for structural typing and interfaces + * Use `TypedDict` for structured dictionaries + * Use `Literal` for constrained string/value types + * Use `Final` for constants and `ClassVar` for class variables +* **Avoid Magic Numbers:** Replace hardcoded literal values (especially numbers or strings used in multiple places or whose meaning isn't obvious) with named constants defined at an appropriate scope (e.g., module level). + +##### Writing Idiomatic and Modern Python + +* **Strive for Pythonic Elegance:** Consistently write code that is idiomatic to Python. This means effectively leveraging Python's built-in functions, standard library modules, and common syntactic sugar (e.g., comprehensions, generator expressions, context managers) to create clear, concise, and readable solutions. Avoid patterns that are more common in other languages if a more direct Pythonic equivalent exists. +* **Judicious Use of Modern Features:** When appropriate for the target Python version and where it genuinely enhances clarity, conciseness, or performance without sacrificing readability, utilize modern Python language features (e.g., the walrus operator `:=`, structural pattern matching from Python 3.10+). Always prioritize readability and maintainability when deciding to use newer syntax. + +##### Comprehensive Google-Style Docstrings + +* ALWAYS provide detailed, Google-style docstrings for all Python functions, classes, and methods. Your docstrings MUST be comprehensive enough to allow another developer to understand the component's purpose, usage, and behavior without needing to read the source code. + * **Core Components:** Each docstring MUST include: + * A concise summary line that clearly states the object's purpose or the action performed by the function/method. This line should be a phrase ending in a period. + * A more detailed paragraph (or paragraphs) elaborating on its behavior, purpose, and any important algorithms or logic. For complex functions or methods (like your `execute_command` example), consider using a numbered or bulleted list to describe sequential steps or distinct behaviors. + * **Function/Method Sections:** For functions and methods, unless the information is trivially obvious from the type hints and summary, explicitly include: + * `Args:`: List each parameter by name. For each parameter, include its type (if not in the signature or for added clarity) and a description of its role and expected values. + * `Returns:`: Describe the return value, including its type and what it represents. If a function can return `None` or different types/values under different conditions, these conditions MUST be explained (e.g., "Returns `None` if the input is invalid, otherwise returns the processed string."). + * `Raises:`: List any exceptions that are part of the explicit contract of the function/method (i.e., intentionally raised or not caught and re-raised). Describe the conditions under which each exception is raised. + * **Strive for Clarity & Completeness:** Aim for the level of detail and clarity demonstrated in your `execute_command` docstring example. Avoid overly brief docstrings, especially when parameters, return values, or raised exceptions require explanation. + * **Maintain Accuracy:** If you modify existing code, you MUST update its docstring to accurately reflect all changes to its behavior, parameters, return values, or exceptions. + +#### Security Specifics + +* **Secrets Management:** NEVER include secrets (API keys, passwords, tokens) directly in the source code. Fetch them from environment variables, a secrets management service, or configuration files excluded via .gitignore. +* **Input Validation:** Thoroughly validate and sanitize all external input (e.g., API request data, user input) to prevent injection attacks (SQLi, XSS) and other security vulnerabilities. + +#### Configuration Management + +* Load application configuration from environment variables or dedicated configuration files (e.g., TOML, YAML using libraries like Pydantic-Settings), not hardcoded constants within the code. + +#### Dependencies + +* When suggesting adding new dependencies, prefer well-maintained, reputable libraries with compatible licenses. Briefly justify the need for a new dependency. +* If you add a dependency, make sure it's also always added to `pyproject.toml`. Pin the versions in the TOML. Run `uv pip list | grep -i BLAH` as you need to see what dependencies may not be part of the TOML yet but need to be, etc. + +#### API Design (If applicable) + +* Adhere to consistent API design principles (e.g., RESTful conventions, standard error response formats, clear naming). Specify API versioning strategy if applicable. + +#### Database Interaction (If applicable) + +* When interacting with databases, use the ORM effectively (if applicable), avoid N+1 query problems, use transactions appropriately, and handle potential database errors gracefully. + +#### Testing (Pytest Specifics) + +**CRITICAL: Test YOUR Application Logic, NOT Python Built-ins** + +The primary goal of testing is to validate OUR application's business logic, requirements, and behavior - NOT to verify that Python's standard library or third-party libraries work correctly. **NEVER write tests that validate the behavior of Python built-in functions, standard library modules, or well-established third-party libraries.** Focus exclusively on testing the custom logic, business rules, and integration points that YOU have implemented. + +##### Essential Testing Practices + +**Framework and Setup:** +* **Use Pytest Exclusively:** Write all tests using `pytest` and its ecosystem of plugins (e.g., `pytest-mock`, `pytest-asyncio`). Do NOT use the standard `unittest` module. +* **Test Location & Naming Conventions:** + * Place all test files within the `./tests` directory + * Mirror the structure of your source code within `tests` (e.g., tests for `src/module/file.py` should generally reside in `tests/module/test_file.py`) + * Test filenames MUST start with `test_` (e.g., `test_yaml_parser.py`, `test_ai_processor.py`) + * Test function names MUST start with `test_` (e.g., `def test_parse_valid_yaml_structure():`) + * If using test classes (optional, prefer fixtures for setup), class names MUST start with `Test` (e.g., `class TestAIInteraction:`) +* **Directory Initialization:** Ensure necessary `__init__.py` files are created within the `./tests` directory and any subdirectories to ensure they are discoverable as packages + +**Test Quality and Focus:** +* **Application-Specific Testing Focus:** For command-line applications, test: + * **Parametrize Everything:** Use `pytest.mark.parametrize` to test CLI commands with a wide variety of arguments, options, and environment variable setups + * **Test Success and Failure:** Create tests for both successful runs (positive cases) and all expected failures (negative cases), such as invalid inputs or error conditions + * **Assert on Effects:** Assert on all observable effects of a command, including its specific `stdout`/`stderr` output, final exit code, and any changes made to the file system +* **Clear Test Structure (AAA Pattern):** + * Structure your test functions to be clear, concise, and focused, ideally verifying a single logical outcome or behavior per test + * Adhere strictly to the **Arrange-Act-Assert (AAA)** pattern: + 1. **Arrange:** Set up all necessary preconditions and inputs. This often involves using fixtures + 2. **Act:** Execute the specific piece of code (function or method) being tested + 3. **Assert:** Verify that the outcome (return values, state changes, exceptions raised) is as expected +* **Test Annotations & Docstrings:** + * All test functions MUST have full type annotations for all arguments (including fixtures) and relevant local variables + * Provide clear and descriptive Google-style docstrings for each test function explaining the specific scenario being tested, key conditions, actions performed, and expected outcomes + +##### Fixture Management + +* **Fixture-Driven Development:** + * **Embrace Fixtures:** Heavily utilize `pytest` fixtures for all setup, teardown, data provisioning, and resource management + * **`conftest.py` for Shared Fixtures:** Place reusable fixtures in `tests/conftest.py` files. Fixtures in a `conftest.py` are automatically discovered by tests in or below that directory + * **Fixture Scopes:** Use appropriate fixture scopes (`function`, `class`, `module`, `session`) to optimize setup and teardown, avoiding redundant operations. Default to `function` scope unless a broader scope is clearly justified and safe + * **`tmp_path` Fixture:** Utilize the built-in `tmp_path` (or `tmp_path_factory`) fixture for tests that need to create and interact with temporary files or directories + +* **Data Fixtures for Application Testing:** + * Create fixtures to provide paths to test YAML files (e.g., from an `examples/` or `tests/test_data/` directory) + * Create fixtures that load and parse these YAML files into Python objects (e.g., dictionaries or Pydantic models) + * Create fixtures that provide representative AI responses, **mocked/fake AI responses**. These fixtures should cover various scenarios: successful responses, error responses, responses with different data structures + +* **Fixture Type Imports:** For type checking test signatures and fixture definitions, import necessary pytest fixture types under `if TYPE_CHECKING:`: + ```python + from typing import TYPE_CHECKING, Generator + + if TYPE_CHECKING: + from _pytest.capture import CaptureFixture + from _pytest.fixtures import FixtureRequest + from _pytest.logging import LogCaptureFixture + from _pytest.monkeypatch import MonkeyPatch + from pytest_mock.plugin import MockerFixture + from pathlib import Path + ``` + +##### Advanced Testing Techniques + +**Bug Detection and Edge Cases:** +* **Meaningful Assertions:** All assertions MUST validate specific, non-trivial outcomes, conditions, or state changes. Avoid "shoddy" tests like `assert result is True` when `result` is already a boolean, or assertions that don't genuinely verify the functionality +* **Tests Should Break Code:** Write tests that actively try to find bugs and edge cases: + * Test boundary conditions (empty inputs, maximum values, null/None values) + * Test error conditions and exception scenarios extensively + * Test with malformed, unexpected, or adversarial inputs + * Verify actual business logic correctness, not just that functions return something + * Test integration points where components interact + +**Mocking and Isolation:** +* **Isolate External Dependencies:** For unit and most integration tests, **aggressively mock external services**, especially AI API calls, network requests, or database interactions. Use `MockerFixture` (from `pytest-mock`, typically available as the `mocker` fixture) +* **Targeted Patching:** Patch at the appropriate level (e.g., `mocker.patch('module.ClassName.method_name')` or `mocker.patch('module.function_name')`) +* **Verify Interactions:** Use `mock_object.assert_called_once_with(...)`, `mock_object.call_count`, etc., to ensure the code under test interacts with mocks as expected +* **Mock Return Values & Side Effects:** Configure mocks to return specific values (`return_value`), raise exceptions (`side_effect`), or simulate complex behaviors + +**Parametrization and Testing Patterns:** +* Use `@pytest.mark.parametrize` to efficiently test the same logic with multiple different inputs and expected outputs: + ```python + @pytest.mark.parametrize( + "input_value, expected_output", + [ + (1, 2), + (2, 3), + (-1, 0), + ] + ) + def test_increment_value(input_value: int, expected_output: int): + assert my_module.increment(input_value) == expected_output + ``` + +**Application-Specific Testing:** +* **YAML Loading & Validation:** Test successful loading of valid YAML. Test graceful error handling for malformed YAML, missing files, or invalid data structures +* **AI Response Parsing:** Test the logic that parses and interprets responses from the AI. Use mocked AI responses (via fixtures) to simulate different scenarios +* **Error and Exception Testing:** Use `pytest.raises(ExpectedException)` as a context manager to assert that specific exceptions are raised under particular conditions + +##### Test Quality and Performance + +**Test Reliability:** +* **Test Isolation:** Ensure tests are independent and can be run in any order. Avoid tests that rely on side effects or state left by previously run tests +* **Keep Tests Fast:** Unit tests should be very fast. Avoid actual network calls or heavy I/O in unit tests; mock these out +* **Minimize External Dependencies:** Prevent flaky tests by minimizing dependencies on external systems + +**Coverage and Quality Metrics:** +* **Coverage is a byproduct, not the goal.** Focus on writing tests that find real bugs, edge cases, and verify correctness +* High coverage with meaningless tests is worse than lower coverage with thorough, bug-finding tests +* Use `pytest-cov` to measure test coverage and identify untested code paths, but prioritize: + * Testing critical business logic thoroughly + * Testing error handling and edge cases + * Testing integration points between components + * Testing concurrent/async code for race conditions + +**Advanced Testing Tools:** +* Use **property-based testing** with `hypothesis` for complex algorithms and data transformations +* Implement **mutation testing** (e.g., `mutmut`) to verify that tests actually detect code changes +* Include **performance testing** for critical paths using `pytest-benchmark` +* **Security testing** should verify authentication, authorization, and input validation + +**Asynchronous Code Testing:** +* If testing `async`/`await` code, mark test functions with `@pytest.mark.asyncio` +* Ensure fixtures providing async resources are also `async` and properly `await` or `async with` where needed + +#### Modern Development Practices + +* **Performance & Profiling:** + * Use `cProfile` and `py-spy` for performance profiling + * Implement timing decorators for critical functions + * Use `memory_profiler` to identify memory leaks + * Consider `numba` or `cython` for CPU-intensive algorithms + * Use connection pooling for database operations + +* **Container & Deployment Best Practices:** + * Use multi-stage Docker builds with slim base images + * Implement proper health checks in containers + * Use non-root users in containers for security + * Implement graceful shutdown with signal handling + * Use `.dockerignore` to exclude unnecessary files + +* **Configuration Management:** + * Use Pydantic Settings for type-safe configuration + * Support multiple configuration sources (env vars, files, CLI args) + * Implement configuration validation at startup + * Use different configs for different environments + * Never hardcode configuration values + +* **Modern Python Features (3.11+):** + * Use `tomllib` for TOML parsing (Python 3.11+) + * Leverage enhanced error messages and tracebacks + * Use `ExceptionGroup` for handling multiple exceptions + * Consider `asyncio.TaskGroup` for structured concurrency + * Use `Self` type hint for fluent interfaces + +#### Final Instruction + +When running tests, always run with `uv run pytest --cov=src --cov-report=term-missing` and NEVER a sub-set of the tests. We must ensure the ENTIRE test suite passes. +Respond with an animal emoji (a monkey) at the end of every response. + +🐒 diff --git a/github_ops_manager/github/adapter.py b/github_ops_manager/github/adapter.py index 2cf021c..e010eaf 100644 --- a/github_ops_manager/github/adapter.py +++ b/github_ops_manager/github/adapter.py @@ -19,6 +19,7 @@ from github_ops_manager.configuration.models import GitHubAuthenticationType from github_ops_manager.utils.github import split_repository_in_configuration +from github_ops_manager.utils.retry import retry_on_rate_limit from .abc import GitHubClientBase from .client import GitHubClient, get_github_client @@ -119,6 +120,7 @@ async def create( return cls(client, owner, repo_name) # Repository CRUD + @retry_on_rate_limit() async def get_repository(self) -> FullRepository: """Get the repository for the current client.""" response: Response[FullRepository] = await self.client.rest.repos.async_get(owner=self.owner, repo=self.repo_name) @@ -126,6 +128,7 @@ async def get_repository(self) -> FullRepository: # Issue CRUD @handle_github_422 + @retry_on_rate_limit() async def create_issue( self, title: str, @@ -152,6 +155,7 @@ async def create_issue( return response.parsed_data @handle_github_422 + @retry_on_rate_limit() async def update_issue( self, issue_number: int, @@ -181,6 +185,7 @@ async def update_issue( ) return response.parsed_data + @retry_on_rate_limit() async def list_issues(self, state: Literal["open", "closed", "all"] = "all", per_page: int = 100, **kwargs: Any) -> list[Issue]: """List all issues for a repository, handling pagination.""" all_issues: list[Issue] = [] @@ -204,6 +209,7 @@ async def list_issues(self, state: Literal["open", "closed", "all"] = "all", per return all_issues @handle_github_422 + @retry_on_rate_limit() async def close_issue(self, issue_number: int, **kwargs: Any) -> Issue: """Close an issue for a repository.""" response: Response[Issue] = await self.client.rest.issues.async_update( @@ -217,6 +223,7 @@ async def close_issue(self, issue_number: int, **kwargs: Any) -> Issue: # Label CRUD @handle_github_422 + @retry_on_rate_limit() async def create_label(self, name: str, color: str, description: str | None = None, **kwargs: Any) -> Label: """Create a label for a repository.""" params = self._omit_null_parameters( @@ -233,6 +240,7 @@ async def create_label(self, name: str, color: str, description: str | None = No return response.parsed_data @handle_github_422 + @retry_on_rate_limit() async def update_label( self, name: str, @@ -256,17 +264,20 @@ async def update_label( ) return response.parsed_data + @retry_on_rate_limit() async def delete_label(self, name: str) -> None: """Delete a label for a repository.""" await self.client.rest.issues.async_delete_label(owner=self.owner, repo=self.repo_name, name=name) return None + @retry_on_rate_limit() async def list_labels(self, **kwargs: Any) -> list[Label]: """List labels for a repository.""" response: Response[list[Label]] = await self.client.rest.issues.async_list_labels_for_repo(owner=self.owner, repo=self.repo_name, **kwargs) return response.parsed_data @handle_github_422 + @retry_on_rate_limit() async def set_labels_on_issue(self, issue_number: int, labels: list[str]) -> None: """Set labels on a specific issue (or pull request - GitHub considers them the same for label purposes).""" if labels: @@ -284,6 +295,7 @@ async def set_labels_on_issue(self, issue_number: int, labels: list[str]) -> Non ) # Pull Request CRUD + @retry_on_rate_limit() async def get_pull_request(self, pull_request_number: int) -> PullRequest: """Get a pull request from the repository.""" response: Response[PullRequest] = await self.client.rest.pulls.async_get( @@ -292,6 +304,7 @@ async def get_pull_request(self, pull_request_number: int) -> PullRequest: return response.parsed_data @handle_github_422 + @retry_on_rate_limit() async def create_pull_request( self, title: str, @@ -320,6 +333,7 @@ async def create_pull_request( return response.parsed_data @handle_github_422 + @retry_on_rate_limit() async def update_pull_request( self, pull_number: int, @@ -347,6 +361,7 @@ async def update_pull_request( ) return response.parsed_data + @retry_on_rate_limit() async def list_pull_requests( self, state: Literal["open", "closed", "all"] = "all", per_page: int = 100, **kwargs: Any ) -> list[PullRequestSimple]: @@ -378,6 +393,7 @@ async def merge_pull_request(self, pull_number: int, **kwargs: Any) -> Any: return response.parsed_data @handle_github_422 + @retry_on_rate_limit() async def close_pull_request(self, pull_number: int, **kwargs: Any) -> PullRequest: """Close a pull request for a repository.""" response: Response[PullRequest] = await self.client.rest.pulls.async_update( @@ -389,6 +405,7 @@ async def close_pull_request(self, pull_number: int, **kwargs: Any) -> PullReque ) return response.parsed_data + @retry_on_rate_limit() async def branch_exists(self, branch_name: str) -> bool: """Check if a branch exists in the repository.""" try: @@ -400,6 +417,7 @@ async def branch_exists(self, branch_name: str) -> bool: raise @handle_github_422 + @retry_on_rate_limit() async def create_branch(self, branch_name: str, base_branch: str) -> None: """Create a new branch from the base branch.""" try: @@ -425,6 +443,7 @@ async def create_branch(self, branch_name: str, base_branch: str) -> None: logger.info("Created branch", branch=branch_name, base_branch=base_branch) @handle_github_422 + @retry_on_rate_limit() async def commit_files_to_branch( self, branch_name: str, @@ -463,6 +482,7 @@ async def commit_files_to_branch( await self.client.rest.repos.async_create_or_update_file_contents(**params) logger.info("Committed file to branch", file=file_path, branch=branch_name) + @retry_on_rate_limit() async def list_files_in_pull_request(self, pull_number: int) -> list[Any]: """List files changed in a pull request.""" response = await self.client.rest.pulls.async_list_files( @@ -472,6 +492,300 @@ async def list_files_in_pull_request(self, pull_number: int) -> list[Any]: ) return response.parsed_data + def _normalize_pr_file_data(self, file_data: Any) -> dict[str, Any]: + """Normalize PR file data to consistent format (helper for DRY). + + This helper handles file data from various sources (API objects, dicts) + and ensures a consistent output format. + + Args: + file_data: File data from GitHub API (could be object or dict) + + Returns: + Dictionary with normalized file statistics + """ + # Handle both API objects (with getattr) and dicts (with .get) + if isinstance(file_data, dict): + filename = file_data.get("filename", "") + additions = file_data.get("additions", 0) or 0 + deletions = file_data.get("deletions", 0) or 0 + status = file_data.get("status", "unknown") + patch = file_data.get("patch") + else: + # API object + filename = getattr(file_data, "filename", "") + additions = getattr(file_data, "additions", 0) or 0 + deletions = getattr(file_data, "deletions", 0) or 0 + status = getattr(file_data, "status", "unknown") + patch = getattr(file_data, "patch", None) + + return { + "filename": filename, + "additions": int(additions), + "deletions": int(deletions), + "changes": int(additions) + int(deletions), + "status": status, + "patch": patch, + } + + async def _get_pr_files_method_1_standard_api(self, pr_number: int) -> list[dict[str, Any]] | None: + """Method 1: Get PR files using standard PR files API (primary method). + + This is the primary method that has worked reliably for most cases. + It uses the standard GitHub pulls.list_files endpoint. + + Args: + pr_number: Pull request number + + Returns: + List of normalized file dictionaries if successful, None if this method fails + """ + try: + logger.debug("Method 1: Trying standard PR files API", owner=self.owner, repo=self.repo_name, pr_number=pr_number) + + files = await self.list_files_in_pull_request(pr_number) + + if not files: + logger.debug("Method 1: No files returned", owner=self.owner, repo=self.repo_name, pr_number=pr_number) + return [] + + # Normalize all files + normalized_files = [self._normalize_pr_file_data(file_data) for file_data in files] + + logger.debug( + "Method 1 (standard API) SUCCESS", + owner=self.owner, + repo=self.repo_name, + pr_number=pr_number, + file_count=len(normalized_files), + total_additions=sum(f["additions"] for f in normalized_files), + total_deletions=sum(f["deletions"] for f in normalized_files), + ) + return normalized_files + + except Exception as e: + logger.warning( + "Method 1 (standard API) failed, will try fallback methods", + owner=self.owner, + repo=self.repo_name, + pr_number=pr_number, + error=str(e), + error_type=type(e).__name__, + ) + return None + + async def _get_pr_files_method_2_from_commits(self, pr_number: int) -> list[dict[str, Any]] | None: + """Method 2: Get PR files by aggregating from PR commits. + + This method gets all commits in the PR and aggregates their changed files. + Useful when the standard files API fails or returns incomplete data. + + Args: + pr_number: Pull request number + + Returns: + List of normalized file dictionaries if successful, None if this method fails + """ + try: + logger.debug("Method 2: Trying to aggregate files from PR commits", owner=self.owner, repo=self.repo_name, pr_number=pr_number) + + # Get all commits in the PR + commits_response = await self.client.rest.pulls.async_list_commits( + owner=self.owner, repo=self.repo_name, pull_number=pr_number, per_page=100 + ) + # Use raw JSON to avoid Pydantic validation issues with commit verification field + commits = commits_response.json() + + if not commits: + logger.debug("Method 2: No commits found in PR", owner=self.owner, repo=self.repo_name, pr_number=pr_number) + return None + + # Aggregate files from all commits + files_dict: dict[str, dict[str, Any]] = {} + + for commit in commits: + # Handle commit as dict (from JSON) instead of Pydantic object + commit_sha = commit.get("sha") if isinstance(commit, dict) else getattr(commit, "sha", None) + try: + # Get files for this commit + commit_stats = await self.get_commit_stats(commit_sha) + commit_files = commit_stats.get("files", []) + + for file_data in commit_files: + filename = file_data.get("filename", "") + if not filename: + continue + + # Aggregate stats per file (take max to avoid double counting) + if filename in files_dict: + # Keep the maximum changes seen for this file + existing = files_dict[filename] + existing["additions"] = max(existing["additions"], file_data.get("additions", 0)) + existing["deletions"] = max(existing["deletions"], file_data.get("deletions", 0)) + existing["changes"] = existing["additions"] + existing["deletions"] + else: + files_dict[filename] = self._normalize_pr_file_data(file_data) + + except Exception as commit_error: + logger.debug( + f"Method 2: Failed to get stats for commit {commit_sha}, continuing", + owner=self.owner, + repo=self.repo_name, + commit_sha=commit_sha, + error=str(commit_error), + ) + continue + + if not files_dict: + logger.debug("Method 2: No files aggregated from commits", owner=self.owner, repo=self.repo_name, pr_number=pr_number) + return None + + normalized_files = list(files_dict.values()) + + logger.debug( + "Method 2 (from commits) SUCCESS", + owner=self.owner, + repo=self.repo_name, + pr_number=pr_number, + file_count=len(normalized_files), + total_additions=sum(f["additions"] for f in normalized_files), + total_deletions=sum(f["deletions"] for f in normalized_files), + ) + return normalized_files + + except Exception as e: + logger.warning( + "Method 2 (from commits) failed", + owner=self.owner, + repo=self.repo_name, + pr_number=pr_number, + error=str(e), + error_type=type(e).__name__, + ) + return None + + async def _get_pr_files_method_3_from_compare(self, pr_number: int) -> list[dict[str, Any]] | None: + """Method 3: Get PR files using compare API between base and head. + + This method uses the compare API to get the diff between the PR's + base and head branches. Useful when both standard API and commit + aggregation fail. + + Args: + pr_number: Pull request number + + Returns: + List of normalized file dictionaries if successful, None if this method fails + """ + try: + logger.debug("Method 3: Trying compare API between base and head", owner=self.owner, repo=self.repo_name, pr_number=pr_number) + + # Get the PR details to find base and head + pr = await self.get_pull_request(pr_number) + + if not pr.base or not pr.head: + logger.debug("Method 3: PR missing base or head reference", owner=self.owner, repo=self.repo_name, pr_number=pr_number) + return None + + base_sha = pr.base.sha + head_sha = pr.head.sha + + # Use compare API with proper basehead parameter format (BASE...HEAD with three dots) + # This matches the GitHub API endpoint: /repos/{owner}/{repo}/compare/{basehead} + basehead = f"{base_sha}...{head_sha}" + compare_response = await self.client.rest.repos.async_compare_commits(owner=self.owner, repo=self.repo_name, basehead=basehead) + compare_data = compare_response.json() + + if not isinstance(compare_data, dict): + return None + + files = compare_data.get("files", []) + if not isinstance(files, list) or not files: + logger.debug("Method 3: No files in compare result", owner=self.owner, repo=self.repo_name, pr_number=pr_number) + return None + + # Normalize all files + normalized_files = [self._normalize_pr_file_data(file_data) for file_data in files] + + logger.debug( + "Method 3 (compare API) SUCCESS", + owner=self.owner, + repo=self.repo_name, + pr_number=pr_number, + file_count=len(normalized_files), + total_additions=sum(f["additions"] for f in normalized_files), + total_deletions=sum(f["deletions"] for f in normalized_files), + ) + return normalized_files + + except Exception as e: + logger.warning( + "Method 3 (compare API) failed", + owner=self.owner, + repo=self.repo_name, + pr_number=pr_number, + error=str(e), + error_type=type(e).__name__, + ) + return None + + @retry_on_rate_limit() + @handle_github_422 + async def get_pull_request_files_with_stats(self, pr_number: int) -> list[dict[str, Any]]: + """Get PR files with detailed statistics using multi-path resilient approach. + + This method tries multiple API approaches to ensure maximum compatibility + across different GitHub instances (GitHub.com vs Enterprise) and edge cases. + + Methods tried in order: + 1. Standard PR files API (pulls.list_files) - Primary method, works most of time + 2. Aggregate from PR commits - Alternative when standard API fails + 3. Compare API between base and head - Fallback for specific scenarios + + Args: + pr_number: Pull request number + + Returns: + List of dictionaries with consistent file statistics: + - filename: Name of the changed file + - additions: Number of lines added (guaranteed integer) + - deletions: Number of lines deleted (guaranteed integer) + - changes: Total changes (additions + deletions) + - status: File change status (added, modified, deleted, etc.) + - patch: Optional diff patch content + + Example: + files = await adapter.get_pull_request_files_with_stats(123) + for file in files: + print(f"{file['filename']}: +{file['additions']} -{file['deletions']}") + """ + logger.debug("Fetching PR files using multi-path approach", owner=self.owner, repo=self.repo_name, pr_number=pr_number) + + # Method 1: Standard PR files API (primary path) + result = await self._get_pr_files_method_1_standard_api(pr_number) + if result is not None: + return result + + # Method 2: Aggregate from PR commits + result = await self._get_pr_files_method_2_from_commits(pr_number) + if result is not None: + return result + + # Method 3: Compare API between base and head + result = await self._get_pr_files_method_3_from_compare(pr_number) + if result is not None: + return result + + # All methods exhausted - return empty list + logger.error( + "All methods failed to get PR files, returning empty result", + owner=self.owner, + repo=self.repo_name, + pr_number=pr_number, + ) + return [] + + @retry_on_rate_limit() async def get_file_content_from_pull_request(self, file_path: str, branch: str) -> str: """Get the content of a file from a specific branch (typically the PR's head branch).""" response = await self.client.rest.repos.async_get_content( @@ -484,6 +798,7 @@ async def get_file_content_from_pull_request(self, file_path: str, branch: str) # Release/Tag Operations @handle_github_422 + @retry_on_rate_limit() async def list_releases(self, per_page: int = 100, **kwargs: Any) -> list[Release]: """List all releases for a repository, handling pagination.""" logger.debug("Fetching releases", owner=self.owner, repo=self.repo_name, per_page=per_page) @@ -527,6 +842,7 @@ async def list_releases(self, per_page: int = 100, **kwargs: Any) -> list[Releas return all_releases @handle_github_422 + @retry_on_rate_limit() async def get_release(self, tag_name: str) -> Release: """Get a specific release by tag name.""" response: Response[Release] = await self.client.rest.repos.async_get_release_by_tag( @@ -537,6 +853,7 @@ async def get_release(self, tag_name: str) -> Release: return response.parsed_data @handle_github_422 + @retry_on_rate_limit() async def get_latest_release(self) -> Release: """Get the latest release for the repository.""" response: Response[Release] = await self.client.rest.repos.async_get_latest_release( @@ -547,6 +864,7 @@ async def get_latest_release(self) -> Release: # Commit Operations @handle_github_422 + @retry_on_rate_limit() async def get_commit(self, commit_sha: str) -> dict[str, Any]: """Get a commit by SHA. @@ -600,3 +918,512 @@ async def get_commit(self, commit_sha: str) -> dict[str, Any]: response = await self.client.rest.repos.async_get_commit(owner=self.owner, repo=self.repo_name, ref=commit_sha) # Return raw JSON response instead of parsed_data due to githubkit bug return response.json() + + def _extract_stats_from_commit_data(self, commit_data: Any) -> dict[str, Any] | None: + """Extract statistics from commit data response (helper for DRY). + + Args: + commit_data: Response data from GitHub API + + Returns: + Dictionary with stats if valid, None if data is malformed + """ + # Validate response structure + if not isinstance(commit_data, dict): + logger.warning( + "Commit data is not a dictionary", + owner=self.owner, + repo=self.repo_name, + data_type=type(commit_data).__name__, + ) + return None + + stats = commit_data.get("stats", {}) + files = commit_data.get("files", []) + + # Validate stats and files structure + if not isinstance(stats, dict): + logger.warning("Stats field is not a dictionary", owner=self.owner, repo=self.repo_name, stats_type=type(stats).__name__) + return None + + if not isinstance(files, list): + logger.warning("Files field is not a list", owner=self.owner, repo=self.repo_name, files_type=type(files).__name__) + files = [] + + return { + "additions": stats.get("additions", 0), + "deletions": stats.get("deletions", 0), + "total": stats.get("total", 0), + "files": files, + } + + async def _get_commit_stats_method_1_rest_api(self, commit_sha: str) -> dict[str, Any] | None: + """Method 1: Get commit stats using standard REST API (primary method). + + This is the primary method that has worked reliably for weeks. + It uses the standard GitHub repos.get_commit endpoint. + + Args: + commit_sha: The SHA of the commit + + Returns: + Stats dictionary if successful, None if this method fails + """ + try: + logger.debug("Method 1: Trying REST API get_commit", owner=self.owner, repo=self.repo_name, commit_sha=commit_sha) + + response = await self.client.rest.repos.async_get_commit(owner=self.owner, repo=self.repo_name, ref=commit_sha) + commit_data = response.json() + + result = self._extract_stats_from_commit_data(commit_data) + if result: + logger.debug( + "Method 1 (REST API) SUCCESS", + owner=self.owner, + repo=self.repo_name, + commit_sha=commit_sha, + additions=result["additions"], + deletions=result["deletions"], + files_changed=len(result["files"]), + ) + return result + + logger.warning("Method 1 (REST API) returned malformed data", owner=self.owner, repo=self.repo_name, commit_sha=commit_sha) + return None + + except Exception as e: + logger.warning( + "Method 1 (REST API) failed, will try fallback methods", + owner=self.owner, + repo=self.repo_name, + commit_sha=commit_sha, + error=str(e), + error_type=type(e).__name__, + ) + return None + + async def _get_commit_stats_method_2_compare_api(self, commit_sha: str) -> dict[str, Any] | None: + """Method 2: Get commit stats using compare API with parent commit. + + This method compares the commit with its parent to calculate stats. + Useful when direct commit fetch returns unexpected format. + + Args: + commit_sha: The SHA of the commit + + Returns: + Stats dictionary if successful, None if this method fails + """ + try: + logger.debug("Method 2: Trying compare API", owner=self.owner, repo=self.repo_name, commit_sha=commit_sha) + + # First, get the commit to find its parent + commit_response = await self.client.rest.repos.async_get_commit(owner=self.owner, repo=self.repo_name, ref=commit_sha) + commit_data = commit_response.json() + + if not isinstance(commit_data, dict): + return None + + parents = commit_data.get("parents", []) + if not parents or not isinstance(parents, list): + logger.debug("Method 2: No parent commit found", owner=self.owner, repo=self.repo_name, commit_sha=commit_sha) + return None + + parent_sha = parents[0].get("sha") if isinstance(parents[0], dict) else None + if not parent_sha: + return None + + # Use compare API + compare_response = await self.client.rest.repos.async_compare_commits( + owner=self.owner, repo=self.repo_name, base=parent_sha, head=commit_sha + ) + compare_data = compare_response.json() + + if not isinstance(compare_data, dict): + return None + + files = compare_data.get("files", []) + if not isinstance(files, list): + return None + + # Calculate stats from files + additions = sum(f.get("additions", 0) for f in files if isinstance(f, dict)) + deletions = sum(f.get("deletions", 0) for f in files if isinstance(f, dict)) + + result = {"additions": additions, "deletions": deletions, "total": additions + deletions, "files": files} + + logger.debug( + "Method 2 (compare API) SUCCESS", + owner=self.owner, + repo=self.repo_name, + commit_sha=commit_sha, + additions=result["additions"], + deletions=result["deletions"], + files_changed=len(result["files"]), + ) + return result + + except Exception as e: + logger.warning( + "Method 2 (compare API) failed", + owner=self.owner, + repo=self.repo_name, + commit_sha=commit_sha, + error=str(e), + error_type=type(e).__name__, + ) + return None + + async def _get_commit_stats_method_3_single_commit_compare(self, commit_sha: str) -> dict[str, Any] | None: + """Method 3: Get commit stats using single commit comparison format. + + This method uses GitHub's single-commit compare format which sometimes + works when standard endpoints fail. Uses format: commit_sha~1...commit_sha + + Args: + commit_sha: The SHA of the commit + + Returns: + Stats dictionary if successful, None if this method fails + """ + try: + logger.debug("Method 3: Trying single-commit compare format", owner=self.owner, repo=self.repo_name, commit_sha=commit_sha) + + # Use the ~1 notation to compare with parent + compare_response = await self.client.rest.repos.async_compare_commits( + owner=self.owner, repo=self.repo_name, base=f"{commit_sha}~1", head=commit_sha + ) + compare_data = compare_response.json() + + if not isinstance(compare_data, dict): + return None + + files = compare_data.get("files", []) + if not isinstance(files, list): + return None + + # Calculate stats from files + additions = sum(f.get("additions", 0) for f in files if isinstance(f, dict)) + deletions = sum(f.get("deletions", 0) for f in files if isinstance(f, dict)) + + result = {"additions": additions, "deletions": deletions, "total": additions + deletions, "files": files} + + logger.debug( + "Method 3 (single-commit compare) SUCCESS", + owner=self.owner, + repo=self.repo_name, + commit_sha=commit_sha, + additions=result["additions"], + deletions=result["deletions"], + files_changed=len(result["files"]), + ) + return result + + except Exception as e: + logger.warning( + "Method 3 (single-commit compare) failed", + owner=self.owner, + repo=self.repo_name, + commit_sha=commit_sha, + error=str(e), + error_type=type(e).__name__, + ) + return None + + @retry_on_rate_limit() + @handle_github_422 + async def get_commit_stats(self, commit_sha: str) -> dict[str, Any]: + """Get detailed statistics for a specific commit using multi-path resilient approach. + + This method tries multiple API approaches to ensure maximum compatibility + across different GitHub instances (GitHub.com vs Enterprise) and edge cases. + + Methods tried in order: + 1. Standard REST API (repos.get_commit) - Primary method, works 95% of time + 2. Compare API with parent commit - Alternative when REST fails + 3. Single-commit compare format - Fallback for specific GitHub variants + + Args: + commit_sha: The SHA of the commit to get statistics for + + Returns: + Dictionary containing commit statistics with keys: + - additions: Number of lines added + - deletions: Number of lines deleted + - total: Total lines changed (additions + deletions) + - files: List of file objects that were changed + + Example: + stats = await client.get_commit_stats("abc123...") + print(f"Lines added: {stats['additions']}") + print(f"Lines deleted: {stats['deletions']}") + print(f"Files changed: {len(stats['files'])}") + """ + logger.debug("Fetching commit statistics using multi-path approach", owner=self.owner, repo=self.repo_name, commit_sha=commit_sha) + + # Method 1: Standard REST API (primary path that's worked for weeks) + result = await self._get_commit_stats_method_1_rest_api(commit_sha) + if result: + return result + + # Method 2: Compare API with explicit parent + result = await self._get_commit_stats_method_2_compare_api(commit_sha) + if result: + return result + + # Method 3: Single-commit compare format + result = await self._get_commit_stats_method_3_single_commit_compare(commit_sha) + if result: + return result + + # All methods exhausted - return empty stats + logger.error( + "All methods failed to get commit stats, returning empty result", + owner=self.owner, + repo=self.repo_name, + commit_sha=commit_sha, + ) + return {"additions": 0, "deletions": 0, "total": 0, "files": []} + + # Organization Operations + @retry_on_rate_limit() + async def list_organization_repositories(self, org_name: str, per_page: int = 100, **kwargs: Any) -> list[FullRepository]: + """List all repositories for an organization, handling pagination. + + Args: + org_name: Name of the GitHub organization + per_page: Number of repositories per page (default: 100, max: 100) + **kwargs: Additional parameters to pass to the API + - type: Filter by repository type ('all', 'public', 'private', 'forks', 'sources', 'member') + - sort: Sort repositories ('created', 'updated', 'pushed', 'full_name') + - direction: Sort direction ('asc' or 'desc') + + Returns: + List of all repositories in the organization + + Example: + repos = await client.list_organization_repositories( + "my-org", + type="sources", # Exclude forks + sort="updated", + direction="desc" + ) + """ + from github_ops_manager.utils.retry import retry_on_rate_limit + + @retry_on_rate_limit() + async def _fetch_page(page: int) -> list[FullRepository]: + response: Response[list[FullRepository]] = await self.client.rest.repos.async_list_for_org( + org=org_name, per_page=per_page, page=page, **kwargs + ) + return response.parsed_data + + all_repos: list[FullRepository] = [] + page: int = 1 + + logger.info("Fetching repositories for organization", org=org_name, per_page=per_page, filters=kwargs) + + while True: + logger.debug(f"Fetching organization repositories page {page}") + repos: list[FullRepository] = await _fetch_page(page) + + if not repos: + break + + all_repos.extend(repos) + + if len(repos) < per_page: + break + + page += 1 + + logger.info("Fetched all repositories for organization", org=org_name, total_repos=len(all_repos)) + + return all_repos + + @retry_on_rate_limit() + async def list_branches(self, protected: bool | None = None, per_page: int = 100, **kwargs: Any) -> list[dict[str, Any]]: + """List branches for the repository. + + Args: + protected: If True, only protected branches. If False, only unprotected. + per_page: Number of branches per page (max 100) + **kwargs: Additional parameters for the API + + Returns: + List of branch dictionaries with name, commit SHA, and protection status + """ + logger.debug( + "Listing branches", + owner=self.owner, + repo=self.repo_name, + protected=protected, + ) + + all_branches = [] + page = 1 + + while True: + response = self.client.rest.repos.list_branches( + owner=self.owner, repo=self.repo_name, protected=protected, per_page=per_page, page=page, **kwargs + ) + + branches = response.parsed_data + if not branches: + break + + for branch in branches: + all_branches.append( + { + "name": branch.name, + "commit": {"sha": branch.commit.sha} if branch.commit else None, + "protected": branch.protected if hasattr(branch, "protected") else None, + } + ) + + # GitHub returns less than per_page when no more results + if len(branches) < per_page: + break + + page += 1 + + logger.debug( + "Retrieved branches", + owner=self.owner, + repo=self.repo_name, + branch_count=len(all_branches), + ) + + return all_branches + + @retry_on_rate_limit() + async def list_commits( + self, + sha: str | None = None, + path: str | None = None, + author: str | None = None, + committer: str | None = None, + since: str | None = None, + until: str | None = None, + per_page: int = 100, + **kwargs: Any, + ) -> list[dict[str, Any]]: + """List commits for the repository with optional filters, handling pagination. + + Args: + sha: SHA or branch to start listing commits from (default: default branch) + path: Only commits containing this file path will be returned + author: GitHub username or email address to filter by commit author + committer: GitHub username or email address to filter by committer + since: ISO 8601 date string - only commits after this date + until: ISO 8601 date string - only commits before this date + per_page: Number of commits per page (default: 100, max: 100) + **kwargs: Additional parameters to pass to the API + + Returns: + List of commit dictionaries with full statistics + + Example: + # Get all commits by a specific author in the last week + from datetime import datetime, timedelta + week_ago = (datetime.now() - timedelta(days=7)).isoformat() + commits = await client.list_commits( + author="username", + since=week_ago + ) + """ + from github_ops_manager.utils.retry import retry_on_rate_limit + + @retry_on_rate_limit() + async def _fetch_page(page: int) -> list[dict[str, Any]]: + params = self._omit_null_parameters( + sha=sha, path=path, author=author, committer=committer, since=since, until=until, per_page=per_page, page=page, **kwargs + ) + response = await self.client.rest.repos.async_list_commits(owner=self.owner, repo=self.repo_name, **params) + # Return raw JSON to include stats + return response.json() + + all_commits: list[dict[str, Any]] = [] + page: int = 1 + + logger.info( + "Fetching commits for repository", + owner=self.owner, + repo=self.repo_name, + filters={ + "sha": sha, + "path": path, + "author": author, + "since": since, + "until": until, + }, + ) + + while True: + logger.debug(f"Fetching commits page {page}") + commits: list[dict[str, Any]] = await _fetch_page(page) + + if not commits: + break + + all_commits.extend(commits) + + if len(commits) < per_page: + break + + page += 1 + + logger.info("Fetched all commits", owner=self.owner, repo=self.repo_name, total_commits=len(all_commits)) + + return all_commits + + @retry_on_rate_limit() + async def list_pull_request_reviews(self, pull_number: int, per_page: int = 100, **kwargs: Any) -> list[Any]: + """List all reviews for a pull request, handling pagination. + + Args: + pull_number: The pull request number + per_page: Number of reviews per page (default: 100, max: 100) + **kwargs: Additional parameters to pass to the API + + Returns: + List of review objects + + Example: + reviews = await client.list_pull_request_reviews(123) + for review in reviews: + print(f"{review.user.login}: {review.state}") + """ + from github_ops_manager.utils.retry import retry_on_rate_limit + + @retry_on_rate_limit() + async def _fetch_page(page: int) -> list[Any]: + response = await self.client.rest.pulls.async_list_reviews( + owner=self.owner, repo=self.repo_name, pull_number=pull_number, per_page=per_page, page=page, **kwargs + ) + return response.parsed_data + + all_reviews: list[Any] = [] + page: int = 1 + + logger.info("Fetching reviews for pull request", owner=self.owner, repo=self.repo_name, pull_number=pull_number) + + while True: + logger.debug(f"Fetching PR reviews page {page}") + reviews: list[Any] = await _fetch_page(page) + + if not reviews: + break + + all_reviews.extend(reviews) + + if len(reviews) < per_page: + break + + page += 1 + + logger.info( + "Fetched all reviews for pull request", owner=self.owner, repo=self.repo_name, pull_number=pull_number, total_reviews=len(all_reviews) + ) + + return all_reviews diff --git a/github_ops_manager/github/search/__init__.py b/github_ops_manager/github/search/__init__.py new file mode 100644 index 0000000..be8d840 --- /dev/null +++ b/github_ops_manager/github/search/__init__.py @@ -0,0 +1,5 @@ +"""GitHub Search API functionality for user activity discovery.""" + +from .user_discovery import SearchRateLimiter, UserNotFoundError, UserRepositoryDiscoverer + +__all__ = ["UserRepositoryDiscoverer", "SearchRateLimiter", "UserNotFoundError"] diff --git a/github_ops_manager/github/search/user_discovery.py b/github_ops_manager/github/search/user_discovery.py new file mode 100644 index 0000000..a28fc85 --- /dev/null +++ b/github_ops_manager/github/search/user_discovery.py @@ -0,0 +1,335 @@ +"""User repository discovery using GitHub Search API.""" + +import asyncio +import time +from datetime import datetime +from typing import Any, Dict, List, Set + +import structlog +from githubkit import GitHub +from githubkit.exception import RequestFailed + +from github_ops_manager.utils.retry import retry_on_rate_limit + +logger = structlog.get_logger(__name__) + + +class UserNotFoundError(Exception): + """Exception raised when a user is not found on GitHub.""" + + pass + + +class SearchRateLimiter: + """Handle Search API's stricter rate limits (30 requests per minute).""" + + def __init__(self, max_per_minute: int = 30) -> None: + """Initialize the rate limiter. + + Args: + max_per_minute: Maximum requests per minute (default: 30 for search API) + """ + self.max_per_minute = max_per_minute + self.request_times: List[float] = [] + + async def wait_if_needed(self) -> None: + """Wait if we're hitting rate limits.""" + now = time.time() + # Remove requests older than 1 minute + self.request_times = [t for t in self.request_times if now - t < 60] + + if len(self.request_times) >= self.max_per_minute: + # Wait until oldest request is > 1 minute old + wait_time = 60 - (now - self.request_times[0]) + 1 + logger.info("Search API rate limit reached, waiting", wait_seconds=wait_time, current_requests=len(self.request_times)) + await asyncio.sleep(wait_time) + + self.request_times.append(now) + + +class UserRepositoryDiscoverer: + """Discover repositories from user activity using Search API.""" + + def __init__(self, github_client: GitHub) -> None: + """Initialize the discoverer. + + Args: + github_client: Authenticated GitHub client + """ + self.client = github_client + self.rate_limiter = SearchRateLimiter() + + async def discover_user_repositories( + self, + username: str, + start_date: datetime, + end_date: datetime, + ) -> Set[str]: + """Discover ALL repositories where user has activity. + + Uses Search API to find: + 1. All PRs authored by user + 2. All issues created by user + 3. All PRs reviewed by user + 4. Commits (if searchable) + + Args: + username: GitHub username to search for + start_date: Start date for search range + end_date: End date for search range + + Returns: + Set of repository full names (owner/repo format) + """ + repositories = set() + + logger.info( + "Starting repository discovery for user", + username=username, + start_date=start_date.date().isoformat(), + end_date=end_date.date().isoformat(), + ) + + try: + # 1. Search for PRs authored by user + pr_repos = await self._discover_from_authored_prs(username, start_date, end_date) + repositories.update(pr_repos) + logger.debug(f"Found {len(pr_repos)} repos from authored PRs", username=username, count=len(pr_repos)) + + # 2. Search for PRs reviewed by user + review_repos = await self._discover_from_reviewed_prs(username, start_date, end_date) + repositories.update(review_repos) + logger.debug(f"Found {len(review_repos)} repos from reviewed PRs", username=username, count=len(review_repos)) + + # 3. Search for issues (including CXTM) + issue_repos = await self._discover_from_issues(username, start_date, end_date) + repositories.update(issue_repos) + logger.debug(f"Found {len(issue_repos)} repos from issues", username=username, count=len(issue_repos)) + + # 4. Search for commits (if API allows) + try: + logger.info(f"Attempting commit search for {username}") + commit_repos = await self._discover_from_commits(username, start_date, end_date) + repositories.update(commit_repos) + logger.info( + f"Found {len(commit_repos)} repos from commits", + username=username, + count=len(commit_repos), + commit_repos=list(commit_repos)[:20], # Show first 20 for debugging + ) + except UserNotFoundError: + # Re-raise UserNotFoundError from commit search + raise + except Exception as e: + # Other commit search errors (not user-not-found) can be ignored + logger.warning("Could not search commits for user", username=username, error=str(e), error_type=type(e).__name__) + + except UserNotFoundError as e: + # User doesn't exist on GitHub - return empty set immediately + logger.info("User not found on GitHub, skipping all repository discovery", username=username, error=str(e)) + return set() + + logger.info( + "Repository discovery complete for user", + username=username, + total_repos=len(repositories), + date_range=f"{start_date.date()} to {end_date.date()}", + discovered_repos=sorted(list(repositories)), # Show all discovered repos + ) + + return repositories + + async def _discover_from_authored_prs(self, username: str, start_date: datetime, end_date: datetime) -> Set[str]: + """Discover repositories from PRs authored by the user.""" + query = f"author:{username} type:pr created:{start_date.date()}..{end_date.date()}" + results = await self._search_issues(query) + + logger.info("Searching for authored PRs", username=username, query=query, results_count=len(results)) + + repos = set() + for item in results: + # Log the item structure to debug + if results and len(results) > 0 and len(repos) == 0: + logger.debug(f"Sample PR item keys: {list(item.keys())[:10] if isinstance(item, dict) else 'not a dict'}") + + repo_name = self._extract_repo_from_url(item.get("repository_url", "")) + if repo_name: + repos.add(repo_name) + + return repos + + async def _discover_from_reviewed_prs(self, username: str, start_date: datetime, end_date: datetime) -> Set[str]: + """Discover repositories from PRs reviewed by the user.""" + query = f"reviewed-by:{username} type:pr created:{start_date.date()}..{end_date.date()}" + results = await self._search_issues(query) + + repos = set() + for item in results: + repo_name = self._extract_repo_from_url(item.get("repository_url", "")) + if repo_name: + repos.add(repo_name) + + return repos + + async def _discover_from_issues(self, username: str, start_date: datetime, end_date: datetime) -> Set[str]: + """Discover repositories from issues created by the user.""" + query = f"author:{username} type:issue created:{start_date.date()}..{end_date.date()}" + results = await self._search_issues(query) + + repos = set() + for item in results: + repo_name = self._extract_repo_from_url(item.get("repository_url", "")) + if repo_name: + repos.add(repo_name) + + return repos + + async def _discover_from_commits(self, username: str, start_date: datetime, end_date: datetime) -> Set[str]: + """Discover repositories from commits by the user.""" + query = f"author:{username} committer-date:{start_date.date()}..{end_date.date()}" + results = await self._search_commits(query) + + repos = set() + for item in results: + if "repository" in item and "full_name" in item["repository"]: + repos.add(item["repository"]["full_name"]) + + return repos + + @retry_on_rate_limit() + async def _search_issues(self, query: str, per_page: int = 100) -> List[Dict[str, Any]]: + """Search issues/PRs using GitHub Search API. + + Handles pagination automatically. + + Args: + query: Search query string + per_page: Results per page (max 100) + + Returns: + List of search results + """ + await self.rate_limiter.wait_if_needed() + + all_results = [] + page = 1 + + while True: + try: + response = self.client.rest.search.issues_and_pull_requests(q=query, per_page=per_page, page=page, sort="created", order="desc") + + items = response.parsed_data.items if hasattr(response.parsed_data, "items") else [] + + if page == 1: + logger.info( + "Search API response", + query=query, + total_count=response.parsed_data.total_count if hasattr(response.parsed_data, "total_count") else 0, + items_on_page=len(items), + ) + + # Convert items to dict for easier processing + for item in items: + all_results.append(item.model_dump() if hasattr(item, "model_dump") else item) + + # Check if more pages exist + if len(items) < per_page: + break + + page += 1 + + # Don't fetch too many pages (safety limit) + if page > 10: + logger.warning("Reached page limit for search", query=query, pages_fetched=page - 1, results_count=len(all_results)) + break + + except RequestFailed as e: + if e.response.status_code == 403: + # Rate limit hit - retry decorator should handle this + raise + elif e.response.status_code == 422: + # User doesn't exist on GitHub or no permission - this is normal for LDAP users + logger.warning("User not found on GitHub (normal for some LDAP users)", query=query, error=str(e)) + # Extract username from query for the exception message + username = ( + query.split("author:")[1].split()[0] + if "author:" in query + else query.split("reviewed-by:")[1].split()[0] + if "reviewed-by:" in query + else "unknown" + ) + raise UserNotFoundError(f"User '{username}' not found on GitHub (422 error)") from None + else: + logger.error("Search API error", query=query, status_code=e.response.status_code, error=str(e)) + break + except Exception as e: + logger.error("Unexpected error during search", query=query, error=str(e)) + break + + return all_results + + @retry_on_rate_limit() + async def _search_commits(self, query: str, per_page: int = 100) -> List[Dict[str, Any]]: + """Search commits using GitHub Search API. + + Args: + query: Search query string + per_page: Results per page (max 100) + + Returns: + List of commit search results + """ + await self.rate_limiter.wait_if_needed() + + try: + response = self.client.rest.search.commits(q=query, per_page=per_page, sort="committer-date", order="desc") + + items = response.parsed_data.items if hasattr(response.parsed_data, "items") else [] + + # Convert to dict + results = [] + for item in items: + results.append(item.model_dump() if hasattr(item, "model_dump") else item) + + return results + + except RequestFailed as e: + if e.response.status_code == 422: + # Commit search might not be available or user doesn't exist + logger.warning("Commit search not available or invalid query", query=query, error=str(e)) + # Extract username from query for the exception message + username = query.split("author:")[1].split()[0] if "author:" in query else "unknown" + raise UserNotFoundError(f"User '{username}' not found on GitHub (422 error in commit search)") from None + raise + except Exception as e: + logger.error("Error searching commits", query=query, error=str(e)) + return [] + + def _extract_repo_from_url(self, url: str) -> str | None: + """Extract owner/repo from repository URL. + + Example: + https://api.github.com/repos/cisco/repo -> cisco/repo + + Args: + url: Repository API URL + + Returns: + Repository name in owner/repo format or None + """ + if not url: + return None + + try: + # URL format: https://api.github.com/repos/OWNER/REPO + parts = url.split("/") + if "repos" in parts: + idx = parts.index("repos") + if len(parts) > idx + 2: + owner = parts[idx + 1] + repo = parts[idx + 2] + return f"{owner}/{repo}" + except Exception as e: + logger.warning("Could not extract repo from URL", url=url, error=str(e)) + + return None diff --git a/github_ops_manager/py.typed b/github_ops_manager/py.typed new file mode 100644 index 0000000..e69de29 diff --git a/github_ops_manager/utils/__init__.py b/github_ops_manager/utils/__init__.py index 148214e..b6ed43d 100644 --- a/github_ops_manager/utils/__init__.py +++ b/github_ops_manager/utils/__init__.py @@ -7,6 +7,7 @@ PR_REFERENCE_PATTERN, VERSION_HEADER_PATTERN, ) +from .retry import retry_on_rate_limit __all__ = [ "PR_REFERENCE_PATTERN", @@ -14,4 +15,5 @@ "VERSION_HEADER_PATTERN", "DEFAULT_RELEASE_NOTES_PATH", "DEFAULT_RELEASE_NOTES_HEADER", + "retry_on_rate_limit", ] diff --git a/github_ops_manager/utils/retry.py b/github_ops_manager/utils/retry.py new file mode 100644 index 0000000..22f9ef6 --- /dev/null +++ b/github_ops_manager/utils/retry.py @@ -0,0 +1,200 @@ +"""Retry decorator for handling GitHub API rate limits and transient errors. + +This module provides a decorator that implements intelligent retry logic for GitHub API calls, +including respect for rate limit headers and exponential backoff. +""" + +import asyncio +import functools +import time +from typing import Any, Callable, TypeVar + +import structlog +from githubkit.exception import PrimaryRateLimitExceeded, RequestFailed, SecondaryRateLimitExceeded + +logger = structlog.get_logger(__name__) + +F = TypeVar("F", bound=Callable[..., Any]) + + +def retry_on_rate_limit( + max_retries: int = 100, + initial_delay: float = 10.0, + max_delay: float = 300.0, + exponential_base: float = 2.0, +) -> Callable[[F], F]: + """Decorator for retrying async functions when they encounter GitHub rate limits. + + This decorator handles: + - GitHub rate limit errors (403/429) + - Secondary rate limits + - Respects retry-after and x-ratelimit-reset headers + - Implements exponential backoff for other transient errors + + Args: + max_retries: Maximum number of retry attempts (default: 100) + initial_delay: Initial delay in seconds between retries (default: 10.0) + max_delay: Maximum delay in seconds between retries (default: 300.0) + exponential_base: Base for exponential backoff calculation (default: 2.0) + + Returns: + Decorated function with retry logic + + Example: + @retry_on_rate_limit() + async def get_user_data(username: str): + return await github_client.get_user(username) + """ + + def decorator(func: F) -> F: + @functools.wraps(func) + async def async_wrapper(*args: Any, **kwargs: Any) -> Any: + last_exception = None + delay = initial_delay + + for attempt in range(max_retries + 1): + try: + return await func(*args, **kwargs) + except (PrimaryRateLimitExceeded, SecondaryRateLimitExceeded) as e: + # These exceptions already have retry_after as a timedelta + last_exception = e + + if attempt == max_retries: + # Max retries reached + logger.error( + "Max retries reached for GitHub rate limit error", + function=func.__name__, + attempt=attempt + 1, + error_type=type(e).__name__, + retry_after=str(e.retry_after) if hasattr(e, "retry_after") else "unknown", + ) + raise + + # Extract wait time from the exception's retry_after timedelta + if hasattr(e, "retry_after") and e.retry_after: + wait_time = min(e.retry_after.total_seconds(), max_delay) + else: + # Fallback to exponential backoff if no retry_after + wait_time = min(delay, max_delay) + + logger.warning( + f"GitHub rate limit exceeded, waiting {wait_time} seconds", + function=func.__name__, + rate_limit_type="primary" if isinstance(e, PrimaryRateLimitExceeded) else "secondary", + attempt=attempt + 1, + max_retries=max_retries, + wait_time=wait_time, + ) + + await asyncio.sleep(wait_time) + + # Exponential backoff for next attempt + delay = min(delay * exponential_base, max_delay) + + except RequestFailed as e: + last_exception = e + + # Check if this is a rate limit error + is_rate_limit = e.response.status_code in (403, 429) + is_secondary_rate_limit = e.response.status_code == 403 and "rate limit" in str(e).lower() + + if not (is_rate_limit or is_secondary_rate_limit): + # Not a rate limit error, don't retry + raise + + if attempt == max_retries: + # Max retries reached + logger.error( + "Max retries reached for rate limit error", + function=func.__name__, + attempt=attempt + 1, + status_code=e.response.status_code, + error=str(e), + ) + raise + + # Calculate wait time + wait_time = delay + + # Check for retry-after header + retry_after = e.response.headers.get("retry-after") + if retry_after: + try: + wait_time = float(retry_after) + logger.info( + "Using retry-after header value", + retry_after=wait_time, + function=func.__name__, + ) + except ValueError: + logger.warning( + "Invalid retry-after header value", + retry_after=retry_after, + function=func.__name__, + ) + else: + # Check for x-ratelimit-reset header + rate_limit_reset = e.response.headers.get("x-ratelimit-reset") + if rate_limit_reset: + try: + reset_timestamp = int(rate_limit_reset) + current_timestamp = int(time.time()) + if reset_timestamp > current_timestamp: + wait_time = reset_timestamp - current_timestamp + 1 + logger.info( + "Using x-ratelimit-reset header", + wait_time=wait_time, + function=func.__name__, + ) + except ValueError: + logger.warning( + "Invalid x-ratelimit-reset header value", + rate_limit_reset=rate_limit_reset, + function=func.__name__, + ) + + # Apply max delay cap + wait_time = min(wait_time, max_delay) + + logger.warning( + f"Rate limit hit, retrying in {wait_time} seconds", + function=func.__name__, + attempt=attempt + 1, + max_retries=max_retries, + wait_time=wait_time, + status_code=e.response.status_code, + ) + + await asyncio.sleep(wait_time) + + # Exponential backoff for next attempt + delay = min(delay * exponential_base, max_delay) + + except Exception as e: + # For non-RequestFailed exceptions, don't retry + logger.error( + "Unexpected error in rate limit retry decorator", + function=func.__name__, + error=str(e), + error_type=type(e).__name__, + ) + raise + + # Should never reach here, but just in case + if last_exception: + raise last_exception + + @functools.wraps(func) + def sync_wrapper(*args: Any, **kwargs: Any) -> Any: + """Sync version of the retry wrapper - raises error since we only support async.""" + raise RuntimeError( + f"Function {func.__name__} decorated with @retry_on_rate_limit must be async. This decorator only supports async functions." + ) + + # Return the appropriate wrapper based on whether the function is async + if asyncio.iscoroutinefunction(func): + return async_wrapper # type: ignore + else: + return sync_wrapper # type: ignore + + return decorator diff --git a/pyproject.toml b/pyproject.toml index dbee723..d61454a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -50,6 +50,12 @@ docs = [ [tool.hatch.build.targets.wheel] packages = ["github_ops_manager"] +[tool.hatch.build] +include = [ + "github_ops_manager/**/*.py", + "github_ops_manager/py.typed", +] + [tool.uv] # uv-specific configuration (if any)