Matas: Minor overall refactor and fixed bugs#38
Matas: Minor overall refactor and fixed bugs#38MT-superdev wants to merge 1 commit intowarestack:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the agent architecture to use LangGraph state-based orchestration, adds rate limiting and optional authentication to the API, extends GitHub integration to support public/anonymous access, updates pre-commit tooling and configuration, and introduces new models for API requests/responses and analysis workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Request
participant API as Recommendations<br/>Endpoint
participant RateLimit as Rate Limiter
participant Auth as Auth<br/>Dependency
participant Agent as Repository<br/>Analysis Agent
participant Graph as LangGraph<br/>Workflow
participant Metadata as fetch_repository<br/>_metadata Node
participant LLM as generate_rule<br/>_recommendations Node
Client->>API: POST /rules/recommend
API->>RateLimit: rate_limiter(request, user)
RateLimit->>RateLimit: Check IP/User limits
alt Limit Exceeded
RateLimit-->>API: HTTPException 429
API-->>Client: 429 Too Many Requests
else Within Limits
RateLimit-->>API: Allow
API->>Auth: get_current_user_optional()
Auth-->>API: User | None
API->>Agent: RepositoryAnalysisAgent.execute()
Agent->>Graph: graph.invoke(initial_state)
Graph->>Metadata: fetch_repository_metadata(state)
Metadata->>Metadata: Gather file tree, languages<br/>CI, docs, CODEOWNERS
Metadata-->>Graph: Return enriched state
Graph->>LLM: generate_rule_recommendations(state)
LLM->>LLM: Construct prompt from signals
LLM->>LLM: Call LLM for rules
LLM-->>Graph: RuleRecommendation list | fallback
Graph-->>Agent: Final AnalysisState
Agent-->>API: AgentResult
API->>API: Map to AnalysisResponse
API-->>Client: 200 AnalysisResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @MT-superdev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant refactor to the repository analysis and rule recommendation system, shifting towards a more robust, AI-driven, and accessible architecture. The core agent now utilizes a graph-based workflow for enhanced flexibility and maintainability. Key changes include enabling unauthenticated access for public repositories with integrated rate limiting, streamlining the development toolchain with a consolidated linter, and improving the GitHub API client for better authentication handling. These updates aim to provide more accurate and context-aware governance rule recommendations while making the service more widely available. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a major refactoring that modernizes the development tooling, overhauls the repository analysis agent to be LLM-powered using LangGraph, and enables unauthenticated access for public repositories with rate limiting. The changes are extensive and generally improve the codebase's structure, clarity, and features. Key improvements include consolidating linting/formatting under Ruff, adopting a stateful agent architecture, and adding new API dependencies for auth and rate limiting.
However, I've identified several critical and high-severity issues that need to be addressed. These include a bug in the new rate-limiting logic, regressions in the project and test configuration (pyproject.toml), duplicated code in the GitHub API client, and a misconfiguration in the FastAPI router setup. There are also some inconsistencies and potentially misleading tests that should be reviewed.
Overall, this is a great step forward, but the identified issues should be fixed to ensure the application is stable and maintainable.
I am having trouble creating individual review comments. Click here to see my feedback.
src/api/rate_limit.py (20)
The rate_limiter dependency is not correctly wired to receive the authenticated user. It uses Depends(lambda: None), which will always result in user being None. This means the rate limit for authenticated users (AUTH_LIMIT) will never be applied, and all users will be subject to the anonymous IP-based limit. To fix this, you should depend on get_current_user_optional from src.api.dependencies.
async def rate_limiter(request: Request, user: User | None = Depends(get_current_user_optional)):
pyproject.toml (141-147)
The docs dependencies have been removed from [project.optional-dependencies]. Since the project still contains documentation files that are being updated in this PR, removing the dependencies required to build the documentation (like mkdocs) will break the doc generation process. Please re-add these dependencies if documentation is still intended to be built from this repository.
pyproject.toml (191-194)
The mypy configuration has been weakened by removing warn_redundant_casts, warn_unused_ignores, warn_no_return, and warn_unreachable. These flags help enforce stricter type checking and catch potential bugs. Was there a specific reason for removing them? If not, I recommend re-enabling them to maintain a high level of static analysis quality.
pyproject.toml (213-234)
The [tool.coverage.run] and [tool.coverage.report] sections have been completely removed. This removes important configurations, such as omit patterns for excluding test files from coverage metrics and exclude_lines for ignoring specific lines (like pragma: no cover). This is likely to result in inaccurate coverage reports. Please restore this configuration.
src/main.py (103)
The recommendations_api_router is being included with the prefix /api/rules. However, the router defined in src/api/recommendations.py already has a prefix of /rules. This will result in a duplicated path, making the final endpoint URL /api/rules/rules/recommend, which is likely not intended. Please remove the prefix from either the router definition or its inclusion here to fix the path.
app.include_router(recommendations_api_router, prefix="/api/v1", tags=["Recommendations API"])
src/integrations/github/api.py (279-291)
The logic inside get_pr_checks to fetch check runs for a commit SHA is a duplication of the get_check_runs method. To adhere to the DRY principle and improve maintainability, get_pr_checks should call get_check_runs after retrieving the head_sha, as it did in the previous version.
pyproject.toml (99-108)
The keywords, classifiers, and [project.urls] sections have been removed. This metadata is valuable for package discovery on PyPI and for providing users with important links to the homepage, documentation, and issue tracker. Was their removal intentional? If not, please consider re-adding them.
pyproject.toml (207-208)
The pytest addopts for coverage reports have been changed to only include term-missing. The html and xml report formats were removed. These formats are often crucial for CI pipelines to archive and display coverage results. Please consider if this was intentional, and if not, re-add the desired report formats.
src/api/dependencies.py (9)
The logger is initialized twice. You can remove this duplicate line.
src/main.py (93)
The CORS allow_origins is hardcoded to ["*"]. While the comment mentions this is for development, it's a security risk if this configuration makes it to production. It would be safer to make this configurable via environment variables, with a restrictive default for production environments, as was done previously.
allow_origins=config.cors.origins,
src/integrations/github/service.py (19-115)
This new GitHubService appears to be introducing inconsistencies. It uses httpx for API calls, whereas the existing GitHubClient was just refactored to consistently use aiohttp. Furthermore, the logic in analyze_repository_rules seems to be a placeholder and duplicates the purpose of the new RepositoryAnalysisAgent. Could you clarify the role of this service? If it's intended for future use, it might be better to align its implementation (e.g., use aiohttp and the shared GitHubClient) or remove it for now to avoid confusion.
tests/integration/test_recommendations.py (48-71)
The test test_authenticated_access_private_repo seems to be testing analysis of a private repository. However, the endpoint implementation currently hardcodes is_public=True when calling the agent, and the PR description indicates private repo support is a future feature. This test passes only because the GitHub API calls are mocked, but it doesn't accurately reflect the current capabilities of the application. This could lead to a false sense of security about this feature. It might be better to mark this test with @pytest.mark.skip until the feature is actually implemented.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/rules/utils/contributors.py (1)
55-55: TypeError when comparing naive and timezone-aware datetimes.
datetime.now()creates a naive datetime, but_has_recent_activityparses GitHub timestamps into timezone-aware datetimes (lines 196, 206). Comparing these raisesTypeError: can't compare offset-naive and offset-aware datetimes.🐛 Proposed fix: Use timezone-aware datetime
- cutoff_date = datetime.now() - timedelta(days=days_back) + cutoff_date = datetime.now(tz=datetime.UTC) - timedelta(days=days_back)Or import and use
timezone.utc:-from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone- cutoff_date = datetime.now() - timedelta(days=days_back) + cutoff_date = datetime.now(tz=timezone.utc) - timedelta(days=days_back)src/core/utils/caching.py (1)
129-135: Global cache TTL mutation causes unintended side effects.Modifying
_GLOBAL_CACHE.ttlaffects all cached entries, not just the one being set. If one caller setsttl=60, subsequent callers expecting the default 3600s TTL will have their entries expire prematurely.Either remove the TTL override capability from
set_cache, or use per-entry TTL tracking.Suggested fix: remove global TTL mutation
async def set_cache(key: str, value: Any, ttl: int | None = None) -> None: """ Async helper to store into the module-level cache. + Note: ttl parameter is ignored; entries use the global cache TTL. """ - if ttl and ttl != _GLOBAL_CACHE.ttl: - _GLOBAL_CACHE.ttl = ttl _GLOBAL_CACHE.set(key, value)src/integrations/providers/bedrock_provider.py (3)
202-204:with_structured_outputis a no-op stub.Returning
selfwithout implementing structured output will silently ignore any output schema passed by callers. This could lead to unexpected behavior when callers expect structured responses.Consider either implementing actual structured output support or raising
NotImplementedErrorto fail explicitly.Suggested fix: fail explicitly
def with_structured_output(self, output_model: Any) -> Any: """Add structured output support.""" - return self + raise NotImplementedError( + "Structured output is not supported for Anthropic Bedrock inference profiles. " + "Use the standard Bedrock client instead." + )
239-246: Async method calls synchronous implementation.
_ageneratedirectly calls the synchronous_generate, which performs a blocking HTTP call. This blocks the event loop and defeats the purpose of async.Suggested fix: use asyncio.to_thread for true async
async def _agenerate( self, messages: list[BaseMessage], stop: list[str] | None = None, run_manager: Any | None = None, ) -> ChatResult: """Async generate using the Anthropic client.""" - return self._generate(messages, stop, run_manager) + import asyncio + return await asyncio.to_thread(self._generate, messages, stop, run_manager)
215-222: System messages must be passed via thesystemparameter, not as user messages in the messages array.Anthropic's Bedrock API requires system messages to be passed as a top-level
systemfield in the request body (alongside themessagesarray), not as messages with a role in the messages array. The current code silently converts system messages to"user"role, which breaks system prompt functionality and produces unexpected model behavior.Update the message construction to extract system messages and pass them separately:
- Check for
msg.type == "system"and collect the content- Pass it as the
systemparameter tomessages.create()- Only include "user" and "assistant" roles in the messages array
src/api/rules.py (1)
14-27: Missing error handling for agent execution.The comment on line 19 acknowledges the agent may throw, but there's no try/except. Additionally,
result.data.get()assumesresulthas adataattribute. If the agent fails or returns an unexpected format, clients receive an opaque 500 error.Suggested fix: add error handling
`@router.post`("/rules/evaluate") async def evaluate_rule(request: RuleEvaluationRequest): # Agent: uses central config—change here affects all rule evals. agent = get_agent("feasibility") - # Async call—agent may throw if rule malformed. - result = await agent.execute(rule_description=request.rule_text) + try: + result = await agent.execute(rule_description=request.rule_text) + except Exception as e: + return { + "supported": False, + "snippet": "", + "feedback": f"Rule evaluation failed: {str(e)}", + } # Output: keep format stable for frontend. Brittle if agent changes keys. return { "supported": result.data.get("is_feasible", False), "snippet": result.data.get("yaml_content", ""), "feedback": result.message, }
🤖 Fix all issues with AI agents
In `@src/agents/repository_analysis_agent/nodes.py`:
- Around line 130-151: The structured output model RecommendationsList currently
inherits AnalysisState which requires fields like repo_full_name and causes
Pydantic validation failures; change RecommendationsList to inherit from
pydantic.BaseModel (or BaseModel alias used in the project) and define only
recommendations: list[RuleRecommendation] so
llm.with_structured_output(RecommendationsList) returns a Pydantic object that
matches the LLM payload; update any imports if necessary and keep the rest of
the flow (structured_llm, response.recommendations) unchanged.
In `@src/api/dependencies.py`:
- Around line 24-41: The get_current_user_optional function currently treats any
Bearer token as authenticated; change it so that when a Bearer token is present
it validates the token signature with the configured IdP/JWT verification (or
calls your existing token validation helper) and only returns a User when
validation succeeds, otherwise return None; alternatively, gate the existing
“fake user” behavior behind an explicit dev-only flag (e.g., a CONFIG_DEV_AUTH
or ENABLE_FAKE_AUTH env var) and ensure production defaults to real validation
and None on failure; update error handling in get_current_user_optional to catch
validation errors, log them, and return None instead of authenticating every
token.
In `@src/api/rate_limit.py`:
- Around line 20-27: The rate_limiter function can raise AttributeError when
request.client is None; update rate_limiter to safely derive the IP by checking
request.client first (use request.client.host when request.client is not None)
and otherwise fall back to a header or placeholder (e.g.,
request.headers.get("x-forwarded-for") or "unknown") before building the key
variable; ensure the key assignment (key = f"ip:{...}") and limit selection
logic remain unchanged and still use AUTH_LIMIT/ANON_LIMIT.
- Around line 20-24: The rate_limiter dependency is using Depends(lambda: None)
so it always gets None; replace that with the optional auth dependency so
authenticated users are recognized: import and use get_current_user_optional and
change the parameter in async def rate_limiter(request: Request, user: User |
None = Depends(get_current_user_optional)) so the function receives the actual
user when present (refer to rate_limiter and get_current_user_optional).
In `@src/api/recommendations.py`:
- Around line 102-106: Replace logging of raw user email in the Analysis request
log: do not use user.email directly in logger.info; instead derive a stable
non-PII identifier (e.g., user.id or a hashed/redacted token) and log that.
Update the code around repo_url_str, client_ip, user_id and the logger.info call
so user_id is set from user.id (or a sanitized/hash of user.email) and then
include that identifier in logger.info(f"Analysis requested for {repo_url_str}
by {user_id} (IP: {client_ip})").
- Around line 82-92: The rate limiter dependency currently uses a lambda
returning None which prevents authenticated detection; update the limiter in
rate_limit.py to depend on get_current_user_optional instead of the lambda
(import get_current_user_optional from src.api.dependencies at the top of
rate_limit.py) so rate_limiter can check the passed User (user and user.email)
and apply the authenticated (100 req/hr) vs anonymous (5 req/hr) limits; no
change needed to recommend_rules other than ensuring it still uses
Depends(rate_limiter).
In `@src/integrations/github/api.py`:
- Around line 168-179: The create_check_run method currently always includes
"conclusion" in the payload, which GitHub rejects unless status == "completed";
update the payload construction in create_check_run so that data = {"name":
name, "head_sha": sha, "status": status, "output": output} and only add
data["conclusion"] = conclusion when status == "completed" (use the existing
function name create_check_run and variables url, headers, data to locate the
change), preserving the early-return when headers is falsy and leaving the rest
of the request flow unchanged.
In `@src/integrations/github/service.py`:
- Around line 40-48: The rate-limit handling in the async HTTP fetch needs to
also treat HTTP 429 as a rate-limit error: inside the block using
httpx.AsyncClient() where response is checked (the existing status_code checks
and raises for GitHubResourceNotFoundError and GitHubRateLimitError), add a
condition that treats response.status_code == 429 (or response.status_code ==
403 with "rate limit" in response.text) as a GitHubRateLimitError and raise
GitHubRateLimitError accordingly; ensure this check occurs before calling
response.raise_for_status() so the custom error is raised for 429/403-rate-limit
cases.
- Around line 66-73: The loop that checks files (iterating files_to_check and
calling client.get on
f"{self.BASE_URL}/repos/{owner}/{repo}/contents/{filepath}") treats any non-200
as "not found" and thus silently masks rate limits, auth failures, and server
errors; update the logic inside that async with block to inspect
resp.status_code: if 404 add to found_files logic as before, if 401/403/429/5xx
log an informative message via the service logger (include resp.status_code,
resp.text or headers) and raise or return a distinct error so callers can handle
auth or rate-limit cases, and if 429 include retry-after handling (sleep/retry
or surface the rate-limit error); keep existing behavior only for genuine 404s.
- Around line 107-114: The _parse_url function currently splits the raw string
and can return invalid owner/repo for inputs like "https://github.com"; update
_parse_url to robustly extract and validate owner and repo by parsing the URL
path (e.g. using urllib.parse.urlparse) or by supporting plain "owner/repo"
input, split the path into segments, ensure there are at least two non-empty
segments (owner and repo), and raise a ValueError with a clear message if they
are missing or empty; adjust the return to explicitly return (owner, repo) only
after validation.
In `@src/main.py`:
- Around line 88-97: Replace the hardcoded CORS settings in the
app.add_middleware(CORSMiddleware, ...) call by importing and using the
environment-aware settings from config (e.g., config.cors.origins and
config.cors.headers) instead of ["*"], and set allow_credentials to False by
default or conditionally (e.g., only True in non-production/dev) so credentials
are not enabled in production; update the CORSMiddleware call to reference
config.cors.origins, config.cors.headers and a config-driven allow_credentials
flag.
In `@tests/conftest.py`:
- Around line 20-34: The mock_settings fixture currently calls the non-existent
pytest.helpers.mock_env and will fail at collection; replace that usage by
applying unittest.mock.patch.dict directly in the fixture (or register the local
Helpers class as a pytest plugin via pytest-helpers-namespace if you prefer
reuse). Specifically, update the mock_settings fixture to use
unittest.mock.patch.dict(os.environ, {...}, clear=False) as a context manager
around yield, and remove or re-register references to pytest.helpers.mock_env
and any direct calls to pytest.helpers in tests; if keeping the Helpers class,
ensure it is exposed to pytest (e.g., via pytest_plugins or
pytest-helpers-namespace) so code referencing pytest.helpers.mock_env resolves.
🧹 Nitpick comments (15)
src/core/config/settings.py (1)
141-150: Consider whether the"gcp"alias is too broad.The alias
"gcp"is generic and could cause confusion or conflicts if other GCP-based AI services are added in the future (e.g., Gemini API direct access vs. Vertex AI). Consider removing it or documenting why it's intentionally mapped to Vertex AI.♻️ Suggested refinement
vertex_aliases = { "vertex_ai", "garden", "vertex", "vertexai", "model_garden", - "gcp", }src/core/utils/logging.py (1)
148-149: Consider simplifying the comment.The comment is slightly imprecise—we're not using a string name for the type; we're relying on PEP 563's postponed evaluation via
__future__. A clearer comment might be:✏️ Suggested clarification
- # Callable is now only needed for typing, so it's safe to use the string name or handled by __future__ + # Callable is imported under TYPE_CHECKING; PEP 563 ensures this annotation isn't evaluated at runtimesrc/rules/utils/contributors.py (1)
247-252: Singleton ignores subsequentgithub_clientarguments.Once created,
get_contributor_analyzeralways returns the same instance regardless of thegithub_clientpassed. If different clients are used (e.g., for different authentication contexts), this could cause unexpected behavior.♻️ Consider validating client consistency or using a registry
def get_contributor_analyzer(github_client) -> ContributorAnalyzer: """Get or create the global contributor analyzer instance.""" global _contributor_analyzer - if _contributor_analyzer is None: + if _contributor_analyzer is None or _contributor_analyzer.github_client is not github_client: _contributor_analyzer = ContributorAnalyzer(github_client) return _contributor_analyzerAlternatively, document that the first client "wins" if this is intentional.
linting_commands.txt (1)
5-9: Consider includingtests/directory in linting commands.The
tests/directory exists in the project, and while pre-commit hooks already lint it viapre-commit run --all-files, the manual linting commands target onlysrc/. For consistency and to ensure tests follow the same linting standards as production code, consider extending the paths:# Run ruff linter and formatter directly (if needed) -ruff check src/ -ruff format src/ +ruff check src/ tests/ +ruff format src/ tests/ # Run mypy for type checking -mypy src/ +mypy src/ tests/src/core/utils/caching.py (1)
161-163: Confusing cache type selection based onttlparameter.The ternary selects
AsyncCachewhenttlis provided, otherwiseTTLCache. This coupling between TTL presence and cache implementation type is unclear. Both cache types support TTL, so the selection criteria seems arbitrary.Consider making the cache type selection explicit via a separate parameter or always using the same cache type.
src/tasks/task_queue.py (1)
114-127: Deduplication logic is defined but not used.
_create_event_hashis implemented (lines 47-112) butenqueuealways setsevent_hash=Noneand never calls it. Theevent_hashesdict and cleanup logic exist but serve no purpose.Either remove the dead code or complete the deduplication implementation.
Option: Enable deduplication
async def enqueue(self, event_type: str, repo_full_name: str, installation_id: int, payload: dict[str, Any]) -> str: """Enqueue a new task for background processing.""" + # Check for duplicate events + event_hash = self._create_event_hash(event_type, repo_full_name, payload) + if event_hash in self.event_hashes: + existing_task_id = self.event_hashes[event_hash] + logger.info(f"Duplicate event detected, returning existing task {existing_task_id}") + return existing_task_id + task_id = f"{event_type}_{repo_full_name}_{datetime.now().timestamp()}" task = Task( id=task_id, event_type=event_type, repo_full_name=repo_full_name, installation_id=installation_id, payload=payload, status=TaskStatus.PENDING, created_at=datetime.now(), - event_hash=None, # No deduplication for now + event_hash=event_hash, ) self.tasks[task_id] = task + self.event_hashes[event_hash] = task_id logger.info(f"Enqueued task {task_id} for {repo_full_name}") return task_idsrc/webhooks/handlers/deployment_status.py (1)
36-42: Comment is misleading - queue has no overflow protection.The comment states "may fail if queue overloaded," but the in-memory
TaskQueuehas no size limits or backpressure mechanism. Consider either:
- Removing the misleading comment, or
- Adding actual queue capacity limits to
TaskQueuepyproject.toml (1)
37-37: Consider aligning the Ruff version floor with pre-commit.The dev dependency specifies
ruff>=0.1.0, but.pre-commit-config.yamlpinsv0.3.0. Considerruff>=0.3.0to ensure local dev environments match the pre-commit behavior.src/integrations/github/service.py (1)
40-41: Consider adding request timeouts to prevent indefinite hangs.Neither
get_repo_metadatanoranalyze_repository_rulesspecifies a timeout. Network issues could cause requests to hang indefinitely.♻️ Suggested improvement
- async with httpx.AsyncClient() as client: - response = await client.get(api_url) + async with httpx.AsyncClient(timeout=30.0) as client: + response = await client.get(api_url)Alternatively, define a class-level timeout constant for consistency across methods.
Also applies to: 66-67
src/core/models.py (1)
7-18: Consider excludinggithub_tokenfrom default serialization.Storing the OAuth token in the User model is a valid pattern, but the token could be accidentally exposed in logs or API responses if the model is serialized without care. Pydantic's
Fieldsupportsrepr=Falseto hide it from string representations.♻️ Suggested improvement
# storing the token allows the service layer to make requests on behalf of the user - github_token: str | None = Field(None, description="OAuth token for GitHub API access") + github_token: str | None = Field( + None, + description="OAuth token for GitHub API access", + repr=False, # Prevent token from appearing in repr/logs + )Additionally, when returning User data in API responses, explicitly exclude the token:
user.model_dump(exclude={"github_token"})tests/conftest.py (1)
49-58: Remove the customevent_loopfixture—it conflicts withasyncio_mode = "auto".With
asyncio_mode = "auto"configured inpyproject.toml(line 133), pytest-asyncio automatically manages event loop creation and cleanup. The custom session-scopedevent_loopfixture is redundant and can cause conflicts or deprecation warnings in pytest-asyncio ≥0.21. Simply delete the fixture and let pytest-asyncio handle loop management.Removal diff
-# 4. Async Support (Essential for FastAPI) -# Note: 'asyncio_mode = "auto"' in pyproject.toml handles the loop, -# but this fixture ensures scope cleanliness if needed. -@pytest.fixture(scope="session") -def event_loop(): - import asyncio - - policy = asyncio.get_event_loop_policy() - loop = policy.new_event_loop() - yield loop - loop.close()src/api/rate_limit.py (1)
29-40: Memory leak: stale rate-limit keys are never purged.The store grows unbounded as new IPs/users make requests. While timestamps within each key are pruned, keys for inactive clients persist indefinitely.
Consider adding periodic cleanup or TTL-based key expiration. Given the PR notes Redis migration as an immediate priority, this may be acceptable short-term.
♻️ Suggested improvement: prune empty keys
timestamps = [ts for ts in timestamps if now - ts < WINDOW] if len(timestamps) >= limit: retry_after = int(WINDOW - (now - min(timestamps))) raise HTTPException( status_code=status.HTTP_429_TOO_MANY_REQUESTS, detail=f"Rate limit exceeded. Try again in {retry_after} seconds.", headers={"Retry-After": str(retry_after)}, ) timestamps.append(now) - _RATE_LIMIT_STORE[key] = timestamps + if timestamps: + _RATE_LIMIT_STORE[key] = timestamps + elif key in _RATE_LIMIT_STORE: + del _RATE_LIMIT_STORE[key]src/rules/validators.py (1)
246-260: Consider timezone handling for consistency.
WeekendConditionusesdatetime.now()(local time), whileAllowedHoursConditionsupports explicit timezones via pytz. For consistency and predictable behavior across deployment environments, consider adding timezone support here as well.src/api/dependencies.py (1)
8-9: Remove duplicate logger assignment.♻️ Proposed fix
-logger = logging.getLogger(__name__) # Logger: keep at module level for reuse. -logger = logging.getLogger(__name__) +logger = logging.getLogger(__name__) # Logger: keep at module level for reuse.src/agents/repository_analysis_agent/models.py (1)
6-16: Constrainseverity/categoryto known values.Right now any string passes validation; using
Literal/Enumhelps prevent invalid values and keeps docs aligned with actual outputs (e.g., the fallback usessystem).♻️ Suggested typing constraints
-from pydantic import BaseModel, Field +from typing import Literal +from pydantic import BaseModel, Field @@ - severity: str = Field("medium", description="low, medium, high, or critical") - category: str = Field("quality", description="security, quality, compliance, or velocity") + severity: Literal["low", "medium", "high", "critical"] = Field( + "medium", description="low, medium, high, or critical" + ) + category: Literal["security", "quality", "compliance", "velocity", "system"] = Field( + "quality", description="security, quality, compliance, velocity, or system" + )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.pre-commit-config.yamlREADME.mddocs/features.mdlinting_commands.txtpyproject.tomlsrc/__init__.pysrc/agents/repository_analysis_agent/agent.pysrc/agents/repository_analysis_agent/models.pysrc/agents/repository_analysis_agent/nodes.pysrc/agents/repository_analysis_agent/prompts.pysrc/api/dependencies.pysrc/api/rate_limit.pysrc/api/recommendations.pysrc/api/rules.pysrc/core/config/settings.pysrc/core/models.pysrc/core/utils/caching.pysrc/core/utils/logging.pysrc/integrations/github/api.pysrc/integrations/github/service.pysrc/integrations/providers/bedrock_provider.pysrc/integrations/providers/factory.pysrc/main.pysrc/rules/utils/contributors.pysrc/rules/validators.pysrc/tasks/scheduler/deployment_scheduler.pysrc/tasks/task_queue.pysrc/webhooks/auth.pysrc/webhooks/dispatcher.pysrc/webhooks/handlers/deployment.pysrc/webhooks/handlers/deployment_status.pysrc/webhooks/handlers/issue_comment.pysrc/webhooks/handlers/pull_request.pysrc/webhooks/router.pytests/conftest.pytests/integration/test_recommendations.pytests/integration/test_rules_api.py
🧰 Additional context used
🧬 Code graph analysis (15)
src/webhooks/dispatcher.py (9)
src/webhooks/handlers/deployment.py (1)
handle(17-46)src/webhooks/handlers/deployment_status.py (1)
handle(17-50)src/webhooks/handlers/issue_comment.py (2)
handle(25-147)event_type(19-20)src/webhooks/handlers/pull_request.py (1)
handle(18-45)src/webhooks/handlers/check_run.py (1)
handle(16-29)src/webhooks/handlers/push.py (1)
handle(16-29)src/webhooks/handlers/deployment_protection_rule.py (1)
handle(16-33)src/webhooks/handlers/deployment_review.py (1)
handle(16-33)src/webhooks/handlers/base.py (1)
handle(16-26)
src/integrations/github/service.py (1)
src/core/utils/caching.py (1)
get(44-66)
src/integrations/providers/factory.py (1)
src/core/config/provider_config.py (2)
get_max_tokens_for_agent(57-63)get_temperature_for_agent(65-71)
src/webhooks/router.py (2)
src/webhooks/handlers/issue_comment.py (1)
event_type(19-20)src/core/models.py (1)
EventType(24-38)
src/api/dependencies.py (2)
src/core/models.py (1)
User(7-18)src/integrations/github/service.py (1)
GitHubService(19-115)
src/agents/repository_analysis_agent/nodes.py (7)
src/agents/repository_analysis_agent/models.py (2)
AnalysisState(19-44)RuleRecommendation(6-16)src/integrations/providers/bedrock_provider.py (2)
get_chat_model(24-48)with_structured_output(202-204)src/integrations/providers/factory.py (1)
get_chat_model(97-130)src/core/utils/caching.py (1)
get(44-66)src/core/models.py (1)
repo_full_name(56-58)src/agents/repository_analysis_agent/test_agent.py (1)
agent(18-20)src/integrations/providers/vertex_ai_provider.py (1)
ainvoke(109-131)
src/api/rate_limit.py (1)
src/core/models.py (1)
User(7-18)
src/agents/repository_analysis_agent/models.py (1)
src/core/models.py (1)
repo_full_name(56-58)
tests/integration/test_recommendations.py (1)
src/core/utils/caching.py (1)
get(44-66)
src/api/recommendations.py (6)
src/agents/repository_analysis_agent/agent.py (2)
RepositoryAnalysisAgent(13-62)execute(39-62)src/agents/repository_analysis_agent/models.py (1)
RuleRecommendation(6-16)src/api/dependencies.py (1)
get_current_user_optional(24-43)src/api/rate_limit.py (1)
rate_limiter(20-40)src/core/models.py (2)
User(7-18)repo_full_name(56-58)tests/integration/test_rules_api.py (1)
client(21-23)
src/webhooks/handlers/deployment.py (2)
src/core/utils/caching.py (1)
get(44-66)src/core/models.py (1)
repo_full_name(56-58)
src/main.py (3)
src/webhooks/handlers/deployment_protection_rule.py (1)
DeploymentProtectionRuleEventHandler(10-33)src/tasks/scheduler/deployment_scheduler.py (2)
get_deployment_scheduler(364-369)stop(40-48)src/tasks/task_queue.py (1)
stop_workers(143-149)
src/tasks/task_queue.py (3)
src/webhooks/handlers/issue_comment.py (1)
event_type(19-20)src/core/models.py (1)
repo_full_name(56-58)src/core/utils/caching.py (1)
get(44-66)
src/integrations/github/api.py (2)
src/core/models.py (1)
repo_full_name(56-58)src/core/utils/caching.py (1)
get(44-66)
tests/conftest.py (1)
src/integrations/github/api.py (1)
close(163-166)
🪛 Gitleaks (8.30.0)
tests/conftest.py
[high] 28-28: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
README.md
238-238: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
244-244: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (49)
src/core/config/settings.py (1)
136-138: LGTM!The simplified Bedrock validation is clear and the comment helpfully explains why only the model ID is required (credentials come from AWS environment/IMDS).
src/core/utils/logging.py (1)
8-18: Clean modernization of type imports.Good use of PEP 563 (
from __future__ import annotations) combined with theTYPE_CHECKINGguard pattern. This correctly avoids runtime import ofCallablewhile maintaining full type checker support. Usingcollections.abc.Callableover the deprecatedtyping.Callableis the right choice.src/rules/utils/contributors.py (1)
61-65: LGTM!The refactor correctly combines nested conditionals while preserving short-circuit evaluation—
_has_recent_activityis only called when the contribution threshold is met, avoiding unnecessary API calls.src/__init__.py (1)
1-1: LGTM!The updated docstring appropriately signals the intent to keep this root package file minimal.
docs/features.md (1)
279-287: LGTM!The new documentation section clearly communicates the rate limiting behavior and authentication requirements. This aligns well with the API changes in this PR.
src/tasks/scheduler/deployment_scheduler.py (1)
2-2: LGTM!Good refactor using
contextlib.suppress()instead of a baretry/except/passblock, following the SIM105 recommendation. The explicitCancelledErrorhandling in_scheduler_loop(line 100) is correctly preserved since it has actual logic (logging and breaking the loop).Also applies to: 45-47
README.md (1)
238-245: LGTM!The new section clearly documents the rate limiting feature for users. The static analysis warnings (MD036) appear to be false positives—line 239 uses proper heading syntax (
##), and line 244 is a list item, not misused emphasis.src/integrations/providers/factory.py (1)
8-9: LGTM!Clean refactor:
- Adding
from __future__ import annotationsenables postponed annotation evaluation.- The inline conditional assignments maintain the same precedence logic (explicit params → agent config → global config) in a more concise form.
Also applies to: 78-79
src/webhooks/auth.py (1)
32-44: LGTM!The clarifying comments improve code readability without altering the secure implementation. The constant-time comparison and HMAC-SHA256 usage are correctly documented.
src/core/utils/caching.py (1)
169-175: LGTM!The ternary for key generation and unified
.get()call simplify the code while maintaining correct behavior for both cache types.src/integrations/providers/bedrock_provider.py (1)
151-167: LGTM on the profile matching refactor.The lowercase normalization and grouped provider checks improve readability and correctness. The explicit keyword lists make the matching criteria clear.
src/tasks/task_queue.py (1)
35-57: LGTM on the comment clarifications.The updated comments improve understanding of the deduplication mechanism and event-specific hash fields.
src/webhooks/handlers/deployment_status.py (1)
17-50: LGTM on the handler implementation.The handler correctly validates the installation ID, extracts deployment info, and enqueues with all required parameters. The logging provides good visibility into the event processing.
.pre-commit-config.yaml (1)
7-7: LGTM! Clean pre-commit configuration update.Good additions:
check-added-large-fileshelps prevent accidental large file commits--exit-non-zero-on-fixensures CI fails when auto-fixes are applied, enforcing clean commits- Consolidating linting and formatting under Ruff simplifies the toolchain
Also applies to: 14-14, 18-24
tests/conftest.py (1)
28-28: Gitleaks false positive — this is intentional mock data.The base64 string
bW9jay1rZXk=decodes tomock-key, and the surrounding comments clearly indicate these are dummy test values. This is not a real secret.pyproject.toml (1)
43-76: LGTM! Well-structured Ruff configuration.Good choices:
- Rule selection covers core errors, import sorting, bug detection, and modernization
B008ignore is correctly documented for FastAPI's dependency injection patternE501delegated to formatter is the right approachdocstring-code-format = trueimproves docstring readabilitysrc/core/models.py (1)
24-38: LGTM! Clean model definitions.
EventType(str, Enum)improves JSON serialization compatibilityWebhookEventproperty accessors provide safe access with sensible defaults- The efficiency comment explaining why
WebhookEventisn't a Pydantic model is helpfulAlso applies to: 41-62
src/webhooks/handlers/pull_request.py (1)
22-23: LGTM! Helpful documentation of a known fragility.The comment appropriately warns about the string-based action comparison. While this is a standard pattern for GitHub webhooks, the note helps future maintainers understand the coupling.
src/webhooks/handlers/deployment.py (1)
27-38: LGTM!The updated comments appropriately document the fragility of payload structure dependencies and the async nature of the enqueue operation. The implementation remains sound.
src/webhooks/dispatcher.py (2)
16-17: LGTM!The comment clarifies the registry's purpose for hot-swap and test injection patterns.
60-60: LGTM!The singleton comment appropriately documents the lifecycle and thread-safety considerations. For the current async FastAPI usage where handlers are registered at startup, this pattern is acceptable.
src/webhooks/handlers/issue_comment.py (3)
35-39: LGTM!The bot self-reply guard with comprehensive username variants is well-implemented. The comment clarifies the purpose.
155-169: LGTM!The pattern hierarchy (double quotes → single quotes → unquoted fallback) is sensible, and the defensive empty-reason check at line 168 prevents accidental matches from the greedy unquoted pattern.
189-194: LGTM!The
any()pattern is idiomatic and allows easy extension if more help triggers are needed.src/webhooks/router.py (3)
13-16: LGTM!The dependency injection pattern for the dispatcher improves testability and lifecycle control.
31-34: Unconventional but acceptable pattern for unsupported events.Using
HTTPExceptionwith 202 for control flow is non-standard, but the design intent is clear: accept unknown events gracefully to prevent GitHub retry storms. The comment documents this well.
58-66: LGTM!The exception handling correctly distinguishes 202 (unsupported event) from other HTTP errors, ensuring graceful degradation for unknown GitHub events while preserving error semantics for actual failures.
src/rules/validators.py (5)
85-91: LGTM!The metadata comment clarifies the purpose of these class attributes for dynamic validator selection.
238-243: Placeholder logic documented appropriately.The TODO and comment clearly indicate this is demo-only behavior. The PR description notes these as known gaps for production hardening.
622-630: LGTM!The lazy import with graceful fallback for missing pytz is a reasonable pattern for an optional dependency. The warning log helps diagnose misconfiguration.
181-187: LGTM!The conditional logic is clear: match presence for
files_match_pattern, match absence forfiles_not_match_pattern. The comment documents the intent well.
980-1007: LGTM!The validator registry provides a clean mapping from condition names to validator instances, and the
get_validator_descriptions()function enables dynamic introspection for the LLM-powered reasoning engine mentioned in the PR objectives.src/integrations/github/api.py (5)
22-27: Anonymous/public auth flow is clear and well‑scoped.Priority order is explicit and the public repo helpers consistently reuse it.
Also applies to: 36-64, 98-129, 135-142
189-228: PR retrieval and check‑run lookup via head SHA look solid.The flow is clear and correctly reuses the PR metadata for check‑runs.
Also applies to: 266-293
236-243: JWT generation and org team lookup adjustments are straightforward.Also applies to: 297-304
245-252: The code correctly assumes base64-encoded private key per documented configuration.The environment variable
PRIVATE_KEY_BASE64_GITHUBand setup documentation (LOCAL_SETUP.md, DEVELOPMENT.md) explicitly instruct users to base64-encode the.pemfile before providing it to the application. The unconditionalbase64.b64decode()is correct by design and should not include a fallback for raw PEM, as that would contradict the documented configuration contract and could mask user configuration errors.
230-234: [rewritten review comment]
[classification tag]tests/integration/test_rules_api.py (1)
28-28: Conditional toggle reads clearer with!= "true"check.Also applies to: 71-71, 106-106
tests/integration/test_recommendations.py (1)
13-71: Good coverage for anonymous vs authenticated access paths.src/main.py (3)
16-18: Lifecycle logging and handler wiring look good.Also applies to: 38-38, 65-65, 70-70, 78-78
121-125: Task status aggregation is clearer and consistent.Also applies to: 135-135
103-103: Verify router prefix alignment with recommendations routes and tests.The new prefix
/api/rulesmay change the public path; confirm it still matches/v1/rules/recommend(tests and clients). Inspect recommendations router definitions and test expectations to ensure path consistency.src/api/dependencies.py (1)
14-18: DI helper and strict auth dependency are clean and straightforward.Also applies to: 46-54
src/agents/repository_analysis_agent/prompts.py (1)
3-35: Prompts are concise and focused on JSON output.src/agents/repository_analysis_agent/agent.py (2)
22-37: Clean two‑step LangGraph flow.Node registration and edge wiring are straightforward and easy to follow.
39-58: Solid state rehydration and error handling.The model_dump → graph → rehydrate pattern is clear and keeps the happy path tight.
src/agents/repository_analysis_agent/models.py (1)
19-44: AnalysisState shape looks coherent.Inputs, signals, outputs, and metadata are clearly separated and easy to extend.
src/api/recommendations.py (1)
45-76: Good fast‑fail URL parsing.Nice touch allowing owner/repo shorthand and providing a clear validation error.
src/agents/repository_analysis_agent/nodes.py (1)
13-104: Signal collection is clear and defensive.Graceful handling of missing files and workflow parsing keeps this resilient.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # 2. Initialize LLM | ||
| # We use the factory to respect project settings (provider, temperature) | ||
| try: | ||
| llm = get_chat_model(agent="repository_analysis") | ||
|
|
||
| # Build YAML rule with proper indentation | ||
| # parameters: is at column 0, source_patterns: at column 2, list items at column 4 | ||
| source_patterns_yaml = "\n".join(f' - "{pattern}"' for pattern in source_patterns) | ||
| test_patterns_yaml = "\n".join(f' - "{pattern}"' for pattern in test_patterns) | ||
| # 3. Structured Output Enforcement | ||
| # We define a wrapper model to ensure we get a list of recommendations | ||
| # Note: LangChain's with_structured_output is preferred over raw JSON parsing | ||
| class RecommendationsList(AnalysisState): | ||
| # We strictly want the list, reusing the model definition | ||
| recommendations: list[RuleRecommendation] | ||
|
|
||
| yaml_content = f"""description: "Require tests when code changes" | ||
| enabled: true | ||
| severity: medium | ||
| event_types: | ||
| - pull_request | ||
| parameters: | ||
| source_patterns: | ||
| {source_patterns_yaml} | ||
| test_patterns: | ||
| {test_patterns_yaml} | ||
| """ | ||
| structured_llm = llm.with_structured_output(RecommendationsList) | ||
|
|
||
| confidence = 0.74 | ||
| if pr_issues.get("missing_tests", 0) > 0: | ||
| confidence = 0.85 | ||
| if state.contributing_analysis.content and state.contributing_analysis.requires_tests: | ||
| confidence = min(0.95, confidence + 0.1) | ||
|
|
||
| recommendations.append( | ||
| RuleRecommendation( | ||
| yaml_rule=yaml_content.strip(), | ||
| confidence=confidence, | ||
| reasoning=test_reasoning, | ||
| strategy_used="hybrid", | ||
| response = await structured_llm.ainvoke( | ||
| [SystemMessage(content=REPOSITORY_ANALYSIS_SYSTEM_PROMPT), HumanMessage(content=user_prompt)] | ||
| ) | ||
| ) | ||
|
|
||
| # Require description in PR body. | ||
| # Increase confidence if we detect short titles in PR history (indicator of missing context) | ||
| desc_reasoning = f"Repository analysis for {state.repository_full_name}." | ||
| if pr_issues.get("short_titles", 0) > 0: | ||
| desc_reasoning += f" Detected {pr_issues['short_titles']} PRs with very short titles (likely missing context)." | ||
| else: | ||
| desc_reasoning += " Encourages context for reviewers; lightweight default." | ||
|
|
||
| desc_confidence = 0.68 | ||
| if pr_issues.get("short_titles", 0) > 0: | ||
| desc_confidence = 0.80 | ||
|
|
||
| recommendations.append( | ||
| RuleRecommendation( | ||
| yaml_rule="""description: "Ensure PRs include context" | ||
| enabled: true | ||
| severity: low | ||
| event_types: | ||
| - pull_request | ||
| parameters: | ||
| min_description_length: 50 | ||
| """.strip(), | ||
| confidence=desc_confidence, | ||
| reasoning=desc_reasoning, | ||
| strategy_used="static", | ||
| # The response is already a Pydantic object (RecommendationsList or similar) | ||
| # We extract the list of recommendations | ||
| valid_recs = response.recommendations if hasattr(response, "recommendations") else [] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/agents/repository_analysis_agent/nodes.py | head -200Repository: warestack/watchflow
Length of output: 8012
🏁 Script executed:
cat src/agents/repository_analysis_agent/models.pyRepository: warestack/watchflow
Length of output: 1664
Structured output schema needs BaseModel instead of AnalysisState to prevent validation failures.
RecommendationsList inherits AnalysisState, which has a required repo_full_name: str field with no default. LLM responses will omit this field and other metadata, causing Pydantic validation to fail and triggering the exception handler's fallback on every successful LLM call. Use a minimal BaseModel with only the recommendations field.
🛠️ Corrected structured-output model
+from pydantic import BaseModel, Field, ConfigDict
from langchain_core.messages import HumanMessage, SystemMessage
@@
- class RecommendationsList(AnalysisState):
- # We strictly want the list, reusing the model definition
- recommendations: list[RuleRecommendation]
+ class RecommendationsList(BaseModel):
+ recommendations: list[RuleRecommendation] = Field(default_factory=list)
+ model_config = ConfigDict(extra="ignore")
@@
- valid_recs = response.recommendations if hasattr(response, "recommendations") else []
+ if isinstance(response, RecommendationsList):
+ valid_recs = response.recommendations
+ else:
+ raise ValueError("Structured output missing recommendations")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 2. Initialize LLM | |
| # We use the factory to respect project settings (provider, temperature) | |
| try: | |
| llm = get_chat_model(agent="repository_analysis") | |
| # Build YAML rule with proper indentation | |
| # parameters: is at column 0, source_patterns: at column 2, list items at column 4 | |
| source_patterns_yaml = "\n".join(f' - "{pattern}"' for pattern in source_patterns) | |
| test_patterns_yaml = "\n".join(f' - "{pattern}"' for pattern in test_patterns) | |
| # 3. Structured Output Enforcement | |
| # We define a wrapper model to ensure we get a list of recommendations | |
| # Note: LangChain's with_structured_output is preferred over raw JSON parsing | |
| class RecommendationsList(AnalysisState): | |
| # We strictly want the list, reusing the model definition | |
| recommendations: list[RuleRecommendation] | |
| yaml_content = f"""description: "Require tests when code changes" | |
| enabled: true | |
| severity: medium | |
| event_types: | |
| - pull_request | |
| parameters: | |
| source_patterns: | |
| {source_patterns_yaml} | |
| test_patterns: | |
| {test_patterns_yaml} | |
| """ | |
| structured_llm = llm.with_structured_output(RecommendationsList) | |
| confidence = 0.74 | |
| if pr_issues.get("missing_tests", 0) > 0: | |
| confidence = 0.85 | |
| if state.contributing_analysis.content and state.contributing_analysis.requires_tests: | |
| confidence = min(0.95, confidence + 0.1) | |
| recommendations.append( | |
| RuleRecommendation( | |
| yaml_rule=yaml_content.strip(), | |
| confidence=confidence, | |
| reasoning=test_reasoning, | |
| strategy_used="hybrid", | |
| response = await structured_llm.ainvoke( | |
| [SystemMessage(content=REPOSITORY_ANALYSIS_SYSTEM_PROMPT), HumanMessage(content=user_prompt)] | |
| ) | |
| ) | |
| # Require description in PR body. | |
| # Increase confidence if we detect short titles in PR history (indicator of missing context) | |
| desc_reasoning = f"Repository analysis for {state.repository_full_name}." | |
| if pr_issues.get("short_titles", 0) > 0: | |
| desc_reasoning += f" Detected {pr_issues['short_titles']} PRs with very short titles (likely missing context)." | |
| else: | |
| desc_reasoning += " Encourages context for reviewers; lightweight default." | |
| desc_confidence = 0.68 | |
| if pr_issues.get("short_titles", 0) > 0: | |
| desc_confidence = 0.80 | |
| recommendations.append( | |
| RuleRecommendation( | |
| yaml_rule="""description: "Ensure PRs include context" | |
| enabled: true | |
| severity: low | |
| event_types: | |
| - pull_request | |
| parameters: | |
| min_description_length: 50 | |
| """.strip(), | |
| confidence=desc_confidence, | |
| reasoning=desc_reasoning, | |
| strategy_used="static", | |
| # The response is already a Pydantic object (RecommendationsList or similar) | |
| # We extract the list of recommendations | |
| valid_recs = response.recommendations if hasattr(response, "recommendations") else [] | |
| # 2. Initialize LLM | |
| # We use the factory to respect project settings (provider, temperature) | |
| try: | |
| llm = get_chat_model(agent="repository_analysis") | |
| # 3. Structured Output Enforcement | |
| # We define a wrapper model to ensure we get a list of recommendations | |
| # Note: LangChain's with_structured_output is preferred over raw JSON parsing | |
| class RecommendationsList(BaseModel): | |
| recommendations: list[RuleRecommendation] = Field(default_factory=list) | |
| model_config = ConfigDict(extra="ignore") | |
| structured_llm = llm.with_structured_output(RecommendationsList) | |
| response = await structured_llm.ainvoke( | |
| [SystemMessage(content=REPOSITORY_ANALYSIS_SYSTEM_PROMPT), HumanMessage(content=user_prompt)] | |
| ) | |
| # The response is already a Pydantic object (RecommendationsList or similar) | |
| # We extract the list of recommendations | |
| if isinstance(response, RecommendationsList): | |
| valid_recs = response.recommendations | |
| else: | |
| raise ValueError("Structured output missing recommendations") |
🤖 Prompt for AI Agents
In `@src/agents/repository_analysis_agent/nodes.py` around lines 130 - 151, The
structured output model RecommendationsList currently inherits AnalysisState
which requires fields like repo_full_name and causes Pydantic validation
failures; change RecommendationsList to inherit from pydantic.BaseModel (or
BaseModel alias used in the project) and define only recommendations:
list[RuleRecommendation] so llm.with_structured_output(RecommendationsList)
returns a Pydantic object that matches the LLM payload; update any imports if
necessary and keep the rest of the flow (structured_llm,
response.recommendations) unchanged.
| async def get_current_user_optional(request: Request) -> User | None: | ||
| """ | ||
| Auth check—don't fail if missing. Critical for public repo support (Phase 1). | ||
| """ | ||
| auth_header = request.headers.get("Authorization") | ||
|
|
||
| if not auth_header: | ||
| return None | ||
|
|
||
| try: | ||
| # Token extraction—fragile if header format changes. | ||
| scheme, token = auth_header.split() | ||
| if scheme.lower() != "bearer": | ||
| return None | ||
|
|
||
| # TODO: Wire to real IdP (Supabase/Auth0). For now, fake user if token present. WARNING: Must verify signature in prod. | ||
| return User(id=123, username="authenticated_user", email="user@example.com", github_token=token) | ||
| except Exception as e: |
There was a problem hiding this comment.
Any Bearer token currently authenticates a user.
This is an auth‑bypass for any endpoint using get_current_user and allows arbitrary access to private flows. Please validate tokens (IdP/JWT signature) or gate this behavior behind an explicit dev‑only flag and return None otherwise.
🤖 Prompt for AI Agents
In `@src/api/dependencies.py` around lines 24 - 41, The get_current_user_optional
function currently treats any Bearer token as authenticated; change it so that
when a Bearer token is present it validates the token signature with the
configured IdP/JWT verification (or calls your existing token validation helper)
and only returns a User when validation succeeds, otherwise return None;
alternatively, gate the existing “fake user” behavior behind an explicit
dev-only flag (e.g., a CONFIG_DEV_AUTH or ENABLE_FAKE_AUTH env var) and ensure
production defaults to real validation and None on failure; update error
handling in get_current_user_optional to catch validation errors, log them, and
return None instead of authenticating every token.
| async def rate_limiter(request: Request, user: User | None = Depends(lambda: None)): | ||
| now = time.time() | ||
| if user and user.email: | ||
| key = f"user:{user.email}" | ||
| limit = AUTH_LIMIT | ||
| else: | ||
| key = f"ip:{request.client.host}" | ||
| limit = ANON_LIMIT |
There was a problem hiding this comment.
Potential AttributeError when request.client is None.
request.client can be None when the app runs behind certain proxies or during testing. Accessing .host directly will raise an AttributeError.
🐛 Proposed fix
if user and user.email:
key = f"user:{user.email}"
limit = AUTH_LIMIT
else:
- key = f"ip:{request.client.host}"
+ client_host = request.client.host if request.client else "unknown"
+ key = f"ip:{client_host}"
limit = ANON_LIMIT🤖 Prompt for AI Agents
In `@src/api/rate_limit.py` around lines 20 - 27, The rate_limiter function can
raise AttributeError when request.client is None; update rate_limiter to safely
derive the IP by checking request.client first (use request.client.host when
request.client is not None) and otherwise fall back to a header or placeholder
(e.g., request.headers.get("x-forwarded-for") or "unknown") before building the
key variable; ensure the key assignment (key = f"ip:{...}") and limit selection
logic remain unchanged and still use AUTH_LIMIT/ANON_LIMIT.
| async def rate_limiter(request: Request, user: User | None = Depends(lambda: None)): | ||
| now = time.time() | ||
| if user and user.email: | ||
| key = f"user:{user.email}" | ||
| limit = AUTH_LIMIT |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how rate_limiter is used in recommendations.py
rg -n -A10 'rate_limiter' src/api/recommendations.pyRepository: warestack/watchflow
Length of output: 805
🏁 Script executed:
#!/bin/bash
# Read the rate_limiter function implementation
cat -n src/api/rate_limit.pyRepository: warestack/watchflow
Length of output: 1582
Fix user dependency injection in rate_limiter.
The Depends(lambda: None) default means rate_limiter always receives None for user, regardless of whether the endpoint has authenticated the request. FastAPI does not automatically share dependencies between the route's dependencies list and the endpoint parameters. As a result, all requests are treated as anonymous and limited to 5/hour, contradicting the docstring's stated behavior. Change line 20 to use Depends(get_current_user_optional) instead:
Suggested fix
from src.core.auth import get_current_user_optional
async def rate_limiter(request: Request, user: User | None = Depends(get_current_user_optional)):🤖 Prompt for AI Agents
In `@src/api/rate_limit.py` around lines 20 - 24, The rate_limiter dependency is
using Depends(lambda: None) so it always gets None; replace that with the
optional auth dependency so authenticated users are recognized: import and use
get_current_user_optional and change the parameter in async def
rate_limiter(request: Request, user: User | None =
Depends(get_current_user_optional)) so the function receives the actual user
when present (refer to rate_limiter and get_current_user_optional).
| @router.post( | ||
| "/v1/rules/recommend/proceed-with-pr", | ||
| response_model=ProceedWithPullRequestResponse, | ||
| summary="Create a PR with generated Watchflow rules", | ||
| description="Creates a branch, commits rules.yaml, and opens a PR using either installation or user token.", | ||
| "/recommend", | ||
| response_model=AnalysisResponse, | ||
| status_code=status.HTTP_200_OK, | ||
| summary="Analyze Repository for Governance Rules", | ||
| description="Analyzes a public or private repository using AI agents. Public repos allow anonymous access.", | ||
| dependencies=[Depends(rate_limiter)], | ||
| ) | ||
| async def proceed_with_pr(request: ProceedWithPullRequestRequest) -> ProceedWithPullRequestResponse: | ||
| if not request.repository_full_name: | ||
| raise HTTPException(status_code=400, detail="repository_full_name or repository_url is required") | ||
| if not request.installation_id and not request.user_token: | ||
| raise HTTPException(status_code=400, detail="installation_id or user_token is required") | ||
|
|
||
| repo = request.repository_full_name | ||
| auth_ctx = {"installation_id": request.installation_id, "user_token": request.user_token} | ||
|
|
||
| repo_data = await github_client.get_repository(repo, **auth_ctx) | ||
| base_branch = request.base_branch or (repo_data or {}).get("default_branch", "main") | ||
|
|
||
| base_sha = await github_client.get_git_ref_sha(repo, base_branch, **auth_ctx) | ||
| if not base_sha: | ||
| log_structured( | ||
| logger, | ||
| "base_branch_resolution_failed", | ||
| operation="proceed_with_pr", | ||
| subject_ids=[repo], | ||
| base_branch=base_branch, | ||
| error="Unable to resolve base branch SHA", | ||
| ) | ||
| raise HTTPException(status_code=400, detail=f"Unable to resolve base branch '{base_branch}'") | ||
|
|
||
| # Check if branch already exists | ||
| existing_branch_sha = await github_client.get_git_ref_sha(repo, request.branch_name, **auth_ctx) | ||
| if existing_branch_sha: | ||
| # Branch exists - use it | ||
| log_structured( | ||
| logger, | ||
| "branch_already_exists", | ||
| operation="proceed_with_pr", | ||
| subject_ids=[repo], | ||
| branch=request.branch_name, | ||
| existing_sha=existing_branch_sha, | ||
| ) | ||
| # Verify the branch points to the correct base | ||
| if existing_branch_sha != base_sha: | ||
| log_structured( | ||
| logger, | ||
| "branch_sha_mismatch", | ||
| operation="proceed_with_pr", | ||
| subject_ids=[repo], | ||
| branch=request.branch_name, | ||
| existing_sha=existing_branch_sha, | ||
| expected_sha=base_sha, | ||
| warning="Branch exists but points to different SHA than base branch", | ||
| ) | ||
| else: | ||
| # Create new branch | ||
| created_ref = await github_client.create_git_ref(repo, request.branch_name, base_sha, **auth_ctx) | ||
| if not created_ref: | ||
| log_structured( | ||
| logger, | ||
| "branch_creation_failed", | ||
| operation="proceed_with_pr", | ||
| subject_ids=[repo], | ||
| branch=request.branch_name, | ||
| base_branch=base_branch, | ||
| base_sha=base_sha, | ||
| error="Failed to create branch - check logs for GitHub API error details", | ||
| ) | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=( | ||
| f"Failed to create branch '{request.branch_name}' from '{base_branch}'. " | ||
| "The branch may already exist or you may not have permission to create branches." | ||
| ), | ||
| ) | ||
| log_structured( | ||
| logger, | ||
| "branch_created", | ||
| operation="proceed_with_pr", | ||
| subject_ids=[repo], | ||
| branch=request.branch_name, | ||
| base_branch=base_branch, | ||
| new_sha=created_ref.get("object", {}).get("sha"), | ||
| ) | ||
|
|
||
| file_result = await github_client.create_or_update_file( | ||
| repo_full_name=repo, | ||
| path=request.file_path, | ||
| content=request.rules_yaml, | ||
| message=request.commit_message, | ||
| branch=request.branch_name, | ||
| **auth_ctx, | ||
| ) | ||
| if not file_result: | ||
| log_structured( | ||
| logger, | ||
| "file_creation_failed", | ||
| operation="proceed_with_pr", | ||
| subject_ids=[repo], | ||
| branch=request.branch_name, | ||
| file_path=request.file_path, | ||
| error="Failed to create or update file - check logs for GitHub API error details", | ||
| ) | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=( | ||
| f"Failed to create or update file '{request.file_path}' on branch '{request.branch_name}'. " | ||
| "Check server logs for detailed error information." | ||
| ), | ||
| ) | ||
|
|
||
| commit_sha = (file_result.get("commit") or {}).get("sha") | ||
| log_structured( | ||
| logger, | ||
| "file_created", | ||
| operation="proceed_with_pr", | ||
| subject_ids=[repo], | ||
| branch=request.branch_name, | ||
| file_path=request.file_path, | ||
| commit_sha=commit_sha, | ||
| ) | ||
| async def recommend_rules( | ||
| request: Request, payload: AnalyzeRepoRequest, user: User | None = Depends(get_current_user_optional) | ||
| ) -> AnalysisResponse: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find rate_limiter definition and usage
rg -n "def rate_limiter" -C3Repository: warestack/watchflow
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Find dependencies with rate_limiter
rg -n "Depends\(rate_limiter\)" -C2Repository: warestack/watchflow
Length of output: 451
🏁 Script executed:
#!/bin/bash
# Find get_current_user_optional definition and usage
rg -n "def get_current_user_optional" -C2Repository: warestack/watchflow
Length of output: 359
🏁 Script executed:
#!/bin/bash
# Check rate_limit.py file structure
fd -t f "rate_limit" --extension pyRepository: warestack/watchflow
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Read rate_limit.py to see current implementation
cat -n src/api/rate_limit.pyRepository: warestack/watchflow
Length of output: 1582
🏁 Script executed:
#!/bin/bash
# Read dependencies.py to see get_current_user_optional
cat -n src/api/dependencies.pyRepository: warestack/watchflow
Length of output: 2193
🏁 Script executed:
#!/bin/bash
# Search for rate_limiter function definition
rg -n "^(async )?def rate_limiter" -A 5Repository: warestack/watchflow
Length of output: 419
Authenticated requests are rate-limited as anonymous.
The route uses Depends(rate_limiter), but the limiter currently depends on lambda: None, which always returns None. This means the if user and user.email: check will never be true, and all requests get the anonymous limit (5 req/hr) instead of the authenticated limit (100 req/hr).
Change the limiter dependency to:
🛠️ Fix (rate_limit.py, line 20)
-async def rate_limiter(request: Request, user: User | None = Depends(lambda: None)):
+async def rate_limiter(request: Request, user: User | None = Depends(get_current_user_optional)):Also add the import at the top of rate_limit.py:
from src.api.dependencies import get_current_user_optional🤖 Prompt for AI Agents
In `@src/api/recommendations.py` around lines 82 - 92, The rate limiter dependency
currently uses a lambda returning None which prevents authenticated detection;
update the limiter in rate_limit.py to depend on get_current_user_optional
instead of the lambda (import get_current_user_optional from
src.api.dependencies at the top of rate_limit.py) so rate_limiter can check the
passed User (user and user.email) and apply the authenticated (100 req/hr) vs
anonymous (5 req/hr) limits; no change needed to recommend_rules other than
ensuring it still uses Depends(rate_limiter).
| async with httpx.AsyncClient() as client: | ||
| response = await client.get(api_url) | ||
|
|
||
| if response.status_code == 404: | ||
| raise GitHubResourceNotFoundError(f"Repo {owner}/{repo} not found") | ||
| if response.status_code == 403 and "rate limit" in response.text.lower(): | ||
| raise GitHubRateLimitError("GitHub API rate limit exceeded") | ||
|
|
||
| response.raise_for_status() |
There was a problem hiding this comment.
Rate limit detection misses HTTP 429 status code.
GitHub returns both 403 with rate-limit text and 429 Too Many Requests for rate limiting. The current check only handles the former.
🐛 Proposed fix
async with httpx.AsyncClient() as client:
response = await client.get(api_url)
if response.status_code == 404:
raise GitHubResourceNotFoundError(f"Repo {owner}/{repo} not found")
- if response.status_code == 403 and "rate limit" in response.text.lower():
+ if response.status_code == 429 or (
+ response.status_code == 403 and "rate limit" in response.text.lower()
+ ):
raise GitHubRateLimitError("GitHub API rate limit exceeded")
response.raise_for_status()
return response.json()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async with httpx.AsyncClient() as client: | |
| response = await client.get(api_url) | |
| if response.status_code == 404: | |
| raise GitHubResourceNotFoundError(f"Repo {owner}/{repo} not found") | |
| if response.status_code == 403 and "rate limit" in response.text.lower(): | |
| raise GitHubRateLimitError("GitHub API rate limit exceeded") | |
| response.raise_for_status() | |
| async with httpx.AsyncClient() as client: | |
| response = await client.get(api_url) | |
| if response.status_code == 404: | |
| raise GitHubResourceNotFoundError(f"Repo {owner}/{repo} not found") | |
| if response.status_code == 429 or ( | |
| response.status_code == 403 and "rate limit" in response.text.lower() | |
| ): | |
| raise GitHubRateLimitError("GitHub API rate limit exceeded") | |
| response.raise_for_status() |
🤖 Prompt for AI Agents
In `@src/integrations/github/service.py` around lines 40 - 48, The rate-limit
handling in the async HTTP fetch needs to also treat HTTP 429 as a rate-limit
error: inside the block using httpx.AsyncClient() where response is checked (the
existing status_code checks and raises for GitHubResourceNotFoundError and
GitHubRateLimitError), add a condition that treats response.status_code == 429
(or response.status_code == 403 with "rate limit" in response.text) as a
GitHubRateLimitError and raise GitHubRateLimitError accordingly; ensure this
check occurs before calling response.raise_for_status() so the custom error is
raised for 429/403-rate-limit cases.
| async with httpx.AsyncClient() as client: | ||
| for filepath in files_to_check: | ||
| # Tricky: Public repos can be read without auth, Private need auth | ||
| # We use the 'contents' API | ||
| check_url = f"{self.BASE_URL}/repos/{owner}/{repo}/contents/{filepath}" | ||
| resp = await client.get(check_url, headers=headers) | ||
| if resp.status_code == 200: | ||
| found_files.append(filepath) |
There was a problem hiding this comment.
Silent failure masks errors beyond "file not found".
All non-200 responses are treated as "file doesn't exist," including rate limits, server errors, and auth failures on private repos. This could lead to misleading recommendations.
🐛 Proposed fix: Log unexpected responses and handle rate limits
async with httpx.AsyncClient() as client:
for filepath in files_to_check:
# Tricky: Public repos can be read without auth, Private need auth
# We use the 'contents' API
check_url = f"{self.BASE_URL}/repos/{owner}/{repo}/contents/{filepath}"
resp = await client.get(check_url, headers=headers)
if resp.status_code == 200:
found_files.append(filepath)
+ elif resp.status_code == 429 or (
+ resp.status_code == 403 and "rate limit" in resp.text.lower()
+ ):
+ raise GitHubRateLimitError("GitHub API rate limit exceeded")
+ elif resp.status_code not in (404, 403):
+ # 404 = file not found, 403 = private repo without auth
+ logger.warning(f"Unexpected response {resp.status_code} for {filepath}")🤖 Prompt for AI Agents
In `@src/integrations/github/service.py` around lines 66 - 73, The loop that
checks files (iterating files_to_check and calling client.get on
f"{self.BASE_URL}/repos/{owner}/{repo}/contents/{filepath}") treats any non-200
as "not found" and thus silently masks rate limits, auth failures, and server
errors; update the logic inside that async with block to inspect
resp.status_code: if 404 add to found_files logic as before, if 401/403/429/5xx
log an informative message via the service logger (include resp.status_code,
resp.text or headers) and raise or return a distinct error so callers can handle
auth or rate-limit cases, and if 429 include retry-after handling (sleep/retry
or surface the rate-limit error); keep existing behavior only for genuine 404s.
| def _parse_url(self, url: str) -> tuple[str, str]: | ||
| """ | ||
| Extracts owner and repo from https://github.com/owner/repo | ||
| """ | ||
| clean_url = str(url).rstrip("/") | ||
| parts = clean_url.split("/") | ||
| if len(parts) < 2: | ||
| raise ValueError("Invalid GitHub URL") |
There was a problem hiding this comment.
URL parsing is fragile and accepts invalid inputs.
The validation len(parts) < 2 passes for malformed URLs like "https://github.com" (parts: ['https:', '', 'github.com']), returning invalid owner/repo values.
🐛 Proposed fix: Validate extracted values
def _parse_url(self, url: str) -> tuple[str, str]:
"""
Extracts owner and repo from https://github.com/owner/repo
"""
clean_url = str(url).rstrip("/")
parts = clean_url.split("/")
- if len(parts) < 2:
+ if len(parts) < 2 or not parts[-2] or not parts[-1]:
raise ValueError("Invalid GitHub URL")
+ owner, repo = parts[-2], parts[-1]
+ # Reject URL fragments that aren't valid GitHub identifiers
+ if owner in ("github.com", "") or repo == "":
+ raise ValueError("Invalid GitHub URL: could not extract owner/repo")
- return parts[-2], parts[-1]
+ return owner, repo🤖 Prompt for AI Agents
In `@src/integrations/github/service.py` around lines 107 - 114, The _parse_url
function currently splits the raw string and can return invalid owner/repo for
inputs like "https://github.com"; update _parse_url to robustly extract and
validate owner and repo by parsing the URL path (e.g. using
urllib.parse.urlparse) or by supporting plain "owner/repo" input, split the path
into segments, ensure there are at least two non-empty segments (owner and
repo), and raise a ValueError with a clear message if they are missing or empty;
adjust the return to explicitly return (owner, repo) only after validation.
| # We explicitly allow all origins ("*") to prevent the browser from blocking requests | ||
| # from your local file system or different localhost ports. | ||
|
|
||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=config.cors.origins, | ||
| allow_origins=["*"], # Explicitly allow all for development | ||
| allow_credentials=True, | ||
| allow_methods=["*"], | ||
| allow_headers=config.cors.headers, | ||
| allow_headers=["*"], | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the full src/main.py file to understand the context
wc -l src/main.pyRepository: warestack/watchflow
Length of output: 78
🏁 Script executed:
# Read the src/main.py file
cat -n src/main.pyRepository: warestack/watchflow
Length of output: 6387
🏁 Script executed:
# Search for environment-related configurations and CORS-related code
rg -i "environ|cors|allow_credentials|allow_origins" --type py -A 3 -B 3Repository: warestack/watchflow
Length of output: 33632
🏁 Script executed:
# Check if there are config files or environment setup
fd -t f -e "env" -e "config" -e "yml" -e "yaml" -e "toml" -e "ini"Repository: warestack/watchflow
Length of output: 510
🏁 Script executed:
# Look for any development/production distinctions
rg -i "development|production|is_prod|debug|env\.get" --type pyRepository: warestack/watchflow
Length of output: 4977
🏁 Script executed:
# Search for usage of settings.cors or CORSConfig
rg "settings\.cors|from.*cors_config|CORSConfig" --type py -B 2 -A 2Repository: warestack/watchflow
Length of output: 1249
🏁 Script executed:
# Check if there are any other CORS-related configurations or middleware additions
rg "add_middleware.*CORS|CORSMiddleware" --type py -B 2 -A 5Repository: warestack/watchflow
Length of output: 749
🏁 Script executed:
# Look at the cors_config.py file directly
cat -n src/core/config/cors_config.pyRepository: warestack/watchflow
Length of output: 324
🏁 Script executed:
# Search for any usage of settings.cors in the application code
rg "settings\.cors|config\.cors" --type py -B 3 -A 3Repository: warestack/watchflow
Length of output: 515
🏁 Script executed:
# Search for where the Settings class is imported and instantiated
rg "from.*settings import|Settings\(\)" --type py -B 2 -A 2Repository: warestack/watchflow
Length of output: 272
🏁 Script executed:
# Get full context of settings.py CORS configuration
sed -n '1,150p' src/core/config/settings.py | tail -80Repository: warestack/watchflow
Length of output: 3099
🏁 Script executed:
# Check if the config is instantiated globally
cat -n src/core/config/__init__.pyRepository: warestack/watchflow
Length of output: 377
🏁 Script executed:
# Search for any usage of config in main.py or app initialization
grep -n "config\|settings\|import.*core" src/main.pyRepository: warestack/watchflow
Length of output: 45
🏁 Script executed:
# Check if config is imported anywhere in main.py or if there's a reference to it
head -30 src/main.py | grep -i "config\|import"Repository: warestack/watchflow
Length of output: 1229
🏁 Script executed:
# Search the entire codebase for where the hardcoded CORS should be replaced
rg "allow_origins.*\[" --type py | head -20Repository: warestack/watchflow
Length of output: 139
🏁 Script executed:
# Check if there's a main() function or if config gets initialized elsewhere
cat src/main.py | grep -A 5 "if __name__"Repository: warestack/watchflow
Length of output: 45
🏁 Script executed:
# Check if there are any environment variables set in docker-compose or helm charts
grep -i "cors\|environment\|debug" docker-compose.yaml helm-chart/values.yaml helm-chart/templates/deployment.yamlRepository: warestack/watchflow
Length of output: 99
🏁 Script executed:
# Look for any production/staging configuration references
rg "production|staging|deploy" --type yaml -B 2 -A 2 | head -40Repository: warestack/watchflow
Length of output: 1581
🏁 Script executed:
# Check README or deployment documentation
find . -name "README*" -o -name "DEPLOY*" -o -name "*.md" | xargs grep -l -i "cors\|environment\|config" 2>/dev/null | head -5Repository: warestack/watchflow
Length of output: 152
🏁 Script executed:
# Final verification: confirm the default origins are safe (not wildcard)
cat -n src/core/config/settings.py | grep -A 12 "CORS_ORIGINS"Repository: warestack/watchflow
Length of output: 669
Import and use the environment-aware CORS configuration instead of hardcoding values.
The hardcoded allow_origins=["*"] with allow_credentials=True violates the CORS specification and will be active in production. A proper, environment-aware configuration system already exists in src/core/config/settings.py with safe defaults (localhost + warestack domains only), but it's being bypassed. Import config and use config.cors.origins and config.cors.headers. Additionally, set allow_credentials=False by default or make it conditional on the environment (disable for production). Credentials should only be enabled if explicitly needed.
🤖 Prompt for AI Agents
In `@src/main.py` around lines 88 - 97, Replace the hardcoded CORS settings in the
app.add_middleware(CORSMiddleware, ...) call by importing and using the
environment-aware settings from config (e.g., config.cors.origins and
config.cors.headers) instead of ["*"], and set allow_credentials to False by
default or conditionally (e.g., only True in non-production/dev) so credentials
are not enabled in production; update the CORSMiddleware call to reference
config.cors.origins, config.cors.headers and a config-driven allow_credentials
flag.
| @pytest.fixture(autouse=True) | ||
| def mock_settings(): | ||
| """Forces the test environment to use dummy values.""" | ||
| with pytest.helpers.mock_env( | ||
| { | ||
| "APP_CLIENT_ID_GITHUB": "mock-client-id", | ||
| "APP_CLIENT_SECRET_GITHUB": "mock-client-secret", | ||
| "WEBHOOK_SECRET_GITHUB": "mock-webhook-secret", | ||
| "PRIVATE_KEY_BASE64_GITHUB": "bW9jay1rZXk=", # "mock-key" in base64 | ||
| "AI_PROVIDER": "openai", | ||
| "OPENAI_API_KEY": "sk-mock-key", | ||
| "ENVIRONMENT": "test", | ||
| } | ||
| ): | ||
| yield |
There was a problem hiding this comment.
Critical: pytest.helpers does not exist — this will fail at test collection.
The mock_settings fixture references pytest.helpers.mock_env(), but pytest has no built-in helpers attribute. The Helpers class is defined below but not registered as a pytest plugin.
🐛 Proposed fix
Use unittest.mock.patch.dict directly instead of the undefined pytest.helpers:
+from unittest.mock import patch
+
+
# 2. Mock Environment Variables (Security First)
# We do this BEFORE importing app code to ensure no real secrets are read
`@pytest.fixture`(autouse=True)
def mock_settings():
"""Forces the test environment to use dummy values."""
- with pytest.helpers.mock_env(
- {
+ with patch.dict(
+ os.environ,
+ {
"APP_CLIENT_ID_GITHUB": "mock-client-id",
"APP_CLIENT_SECRET_GITHUB": "mock-client-secret",
"WEBHOOK_SECRET_GITHUB": "mock-webhook-secret",
"PRIVATE_KEY_BASE64_GITHUB": "bW9jay1rZXk=", # "mock-key" in base64
"AI_PROVIDER": "openai",
"OPENAI_API_KEY": "sk-mock-key",
"ENVIRONMENT": "test",
}
):
yieldIf you want to keep the Helpers class for reuse, consider using the pytest-helpers-namespace plugin or simply import the helper directly.
Also applies to: 38-41
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 28-28: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In `@tests/conftest.py` around lines 20 - 34, The mock_settings fixture currently
calls the non-existent pytest.helpers.mock_env and will fail at collection;
replace that usage by applying unittest.mock.patch.dict directly in the fixture
(or register the local Helpers class as a pytest plugin via
pytest-helpers-namespace if you prefer reuse). Specifically, update the
mock_settings fixture to use unittest.mock.patch.dict(os.environ, {...},
clear=False) as a context manager around yield, and remove or re-register
references to pytest.helpers.mock_env and any direct calls to pytest.helpers in
tests; if keeping the Helpers class, ensure it is exposed to pytest (e.g., via
pytest_plugins or pytest-helpers-namespace) so code referencing
pytest.helpers.mock_env resolves.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.