Matas/fix/fix actions #42
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR introduces a graph-based repository analysis system replacing procedural workflows, adds rate limiting and optional authentication, refactors webhook processing to use async task queues, standardizes logging to structlog across the codebase, and expands the GitHub integration with new API methods and GraphQL support. Changes
Sequence Diagram(s)sequenceDiagram
actor Client as Client (HTTP)
participant API as /rules/recommend<br/>Endpoint
participant Agent as Repository<br/>Analysis Agent
participant Graph as LangGraph<br/>State Machine
participant GitHub as GitHub<br/>API
participant LLM as OpenAI<br/>Chat API
Client->>API: POST /rules/recommend<br/>(repo_url, force_refresh)
API->>API: Validate & Parse URL<br/>Rate Limit Check<br/>Extract Auth
API->>Agent: execute(**kwargs)
Agent->>Agent: _build_graph()
Agent->>Graph: Run graph.invoke()<br/>(AnalysisState)
Graph->>Graph: fetch_metadata node
Graph->>GitHub: GET repo metadata<br/>list files, detect languages
GitHub-->>Graph: repo data
Graph->>Graph: fetch_pr_signals node
Graph->>GitHub: GraphQL PR hygiene data
GitHub-->>Graph: PRs with metrics
Graph->>Graph: generate_rules node
Graph->>LLM: POST chat completion<br/>(system prompt + signals)
LLM-->>Graph: recommendations
Graph->>Graph: AnalysisState complete
Agent-->>API: AgentResult<br/>(recommendations)
API->>API: Map to AnalysisResponse<br/>Generate YAML & PR plan
API-->>Client: 200 OK<br/>(rules_yaml, pr_plan,<br/>analysis_summary)
sequenceDiagram
actor GitHub as GitHub<br/>Webhook
participant Router as /webhooks<br/>github
participant Auth as Signature<br/>Verification
participant Dispatcher as Webhook<br/>Dispatcher
participant Queue as Task<br/>Queue
participant Workers as Background<br/>Workers
participant Handler as Event<br/>Handler
GitHub->>Router: POST webhook event<br/>(PR, Push, Deployment)
Router->>Auth: verify_github_signature()
Auth-->>Router: Valid ✓
Router->>Router: Parse & validate payload<br/>(GitHubEventModel)
Router->>Dispatcher: dispatch(event)
Dispatcher->>Queue: enqueue(handler_func,<br/>event_type, payload)
Queue->>Queue: Generate task_id<br/>Check deduplication
Queue-->>Dispatcher: bool (enqueued?)
Dispatcher-->>Router: WebhookResponse<br/>(status)
Router-->>GitHub: 202 Accepted
Workers->>Queue: Dequeue task
Workers->>Handler: handler.handle(event)
Handler->>Handler: Process event<br/>(log, update state)
Handler-->>Workers: WebhookResponse
Workers->>Queue: task_done()
sequenceDiagram
actor User as Client<br/>(Anonymous/Auth)
participant RateLimit as Rate Limiter<br/>Middleware
participant API as API<br/>Endpoint
participant Auth as Auth<br/>Dependencies
participant Service as GitHub<br/>Service
User->>RateLimit: Request
RateLimit->>Auth: Extract user ID or IP
Auth-->>RateLimit: User | None
RateLimit->>RateLimit: Determine limit<br/>(authenticated: 100/hr<br/>anonymous: 5/hr)
RateLimit->>RateLimit: Check timestamp store<br/>Prune 1-hr window
alt Within Limit
RateLimit->>API: Allow request
API->>Service: Process request
Service-->>API: Result
API-->>User: 200 OK
RateLimit->>RateLimit: Record timestamp
else Exceeds Limit
RateLimit-->>User: 429 Too Many<br/>Requests<br/>(Retry-After header)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (40)
✏️ Tip: You can disable this entire section by setting 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 |
|
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (57.8%) is below the target coverage (80.0%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #42 +/- ##
========================================
+ Coverage 32.8% 43.4% +10.5%
========================================
Files 85 108 +23
Lines 5162 6374 +1212
========================================
+ Hits 1698 2771 +1073
- Misses 3464 3603 +139 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/integrations/providers/bedrock_provider.py (4)
202-204:with_structured_outputis a no-op stub.Returning
selfwithout modification means structured output isn't actually implemented. Callers expecting schema-validated responses will silently receive unstructured text. This could cause downstream parsing failures or data integrity issues.Consider either implementing proper structured output support (e.g., using tool calls or JSON mode) or raising
NotImplementedErrorto fail fast.🔧 Minimal fix to 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 AnthropicBedrockWrapper. " + "Use the standard Bedrock client instead." + )
239-246: Async method blocks the event loop.
_ageneratecalls the synchronous_generatemethod directly, which will block the event loop during API calls. This defeats the purpose of async and can cause performance issues in concurrent workloads.Consider using
asyncio.to_threadto run the sync call in a thread pool, or implement true async using the Anthropic client's async methods if available.🔧 Suggested fix using asyncio.to_thread
+ import asyncio + 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) + return await asyncio.to_thread(self._generate, messages, stop, run_manager)
219-220: System messages lose their semantics when mapped to "user" role.Anthropic's API handles system messages via a separate
systemparameter in themessages.create()call, not as a message with role "user". Mapping system messages to user role may produce unexpected model behavior.🔧 Suggested approach to handle system messages
def _generate( self, messages: list[BaseMessage], stop: list[str] | None = None, run_manager: Any | None = None, ) -> ChatResult: """Generate a response using the Anthropic client.""" anthropic_messages = [] + system_content = None for msg in messages: if msg.type == "human": role = "user" elif msg.type == "ai": role = "assistant" elif msg.type == "system": - role = "user" + system_content = msg.content + continue else: role = "user" anthropic_messages.append({"role": role, "content": msg.content}) + create_kwargs = { + "model": self.model_id, + "max_tokens": self.max_tokens, + "temperature": self.temperature, + "messages": anthropic_messages, + } + if system_content: + create_kwargs["system"] = system_content + - response = self.anthropic_client.messages.create( - model=self.model_id, - max_tokens=self.max_tokens, - temperature=self.temperature, - messages=anthropic_messages, - ) + response = self.anthropic_client.messages.create(**create_kwargs)
50-52:supports_structured_outputmay return incorrect value for wrapped clients.This returns
Trueunconditionally, but theAnthropicBedrockWrappercreated in the fallback path (lines 44-45) doesn't actually support structured output. This inconsistency could cause callers to expect functionality that won't work.Consider making this method aware of which client type is in use, or ensure all client types actually support structured output.
src/tasks/task_queue.py (1)
89-110: Avoid logging issue comment bodies at info level.
User-generated comment content can include sensitive data; emitting it in info logs is a compliance/privacy risk. Consider logging only length/hash or gating behind a debug-level redaction policy.🔒 Suggested redaction
- logger.info(f" Comment body: {event_data.get('comment_body', '')[:50]}...") + logger.info(f" Comment body length: {len(event_data.get('comment_body') or '')}")
🤖 Fix all issues with AI agents
In @.pre-commit-config.yaml:
- Around line 6-29: The pinned ruff-pre-commit rev is outdated; update the rev
for the repo "https://github.com/astral-sh/ruff-pre-commit" (the hooks with id:
ruff and id: ruff-format) from v0.3.0 to v0.14.14 in .pre-commit-config.yaml so
the pre-commit config uses the current ruff release.
In `@src/agents/base.py`:
- Around line 114-118: The subclass execute signatures are incompatible with the
BaseAgent.async def execute(self, **kwargs: Any) -> AgentResult; update
RuleFeasibilityAgent.execute, RuleEngineAgent.execute, and
AcknowledgmentAgent.execute to accept **kwargs: Any (matching BaseAgent) and
refactor their bodies to pull needed values from kwargs (e.g., rule_description
= kwargs.get("rule_description"), event_type = kwargs.get("event_type"),
event_data = kwargs.get("event_data"), rules = kwargs.get("rules")) before use
so MyPy substitutability is preserved while behavior remains the same.
In `@src/agents/repository_analysis_agent/models.py`:
- Around line 119-165: Duplicate HygieneMetrics class definitions (one in the
agents module and one in rules) cause maintenance drift; extract a single
canonical HygieneMetrics model into a shared module (e.g., a core/shared models
module) and update both callers to import that single definition, reconciling
field differences by either merging the fields (ci_skip_rate,
new_code_test_coverage, test_coverage_delta_avg, ai_generated_rate) into a
consistent schema with clear optional defaults or, if divergence is intentional,
replace the duplicate with a small variant class that inherits from the shared
HygieneMetrics and add a comment explaining the purposeful differences so the
canonical source remains authoritative (reference class name HygieneMetrics and
update import sites accordingly).
In `@src/agents/repository_analysis_agent/nodes.py`:
- Around line 212-221: The code sets state.hygiene_summary using a field name
test_coverage_delta_avg that doesn't exist on the HygieneMetrics model and will
raise a Pydantic ValidationError; update the initializer for
state.hygiene_summary in nodes.py (the block that constructs HygieneMetrics) to
use the correct field name new_code_test_coverage (or otherwise provide the
actual field names defined on HygieneMetrics) and ensure all keys (e.g.,
unlinked_issue_rate, average_pr_size, first_time_contributor_count,
issue_diff_mismatch_rate, ghost_contributor_rate, new_code_test_coverage,
codeowner_bypass_rate, ai_generated_rate) match the HygieneMetrics definition.
- Around line 366-375: The fallback initialization of state.hygiene_summary uses
the wrong field name test_coverage_delta_avg; update the HygieneMetrics
instantiation to use new_code_test_coverage instead (i.e., replace
test_coverage_delta_avg=0.0 with new_code_test_coverage=0.0) so the keys match
the HygieneMetrics model and avoid runtime attribute/serialization errors when
creating state.hygiene_summary.
In `@src/api/dependencies.py`:
- Around line 33-40: The current get_current_user implementation accepts any
Bearer token and returns a fake User (User(..., github_token=token)),
effectively bypassing auth; change it to gate this mock behavior behind an
explicit demo flag (e.g., DEMO_MODE or a config value) or add minimal token
validation before returning a User. Specifically, update get_current_user to: 1)
check a config/demo flag (referencing get_current_user and auth_header) and only
return the fake User when the flag is enabled; 2) otherwise perform basic
validation (e.g., reject empty token or call a verify_token helper that will be
wired to the IdP later) and return None on failure; and 3) keep the returned
object as User(...) only when validation/demo allows it so protected endpoints
are not bypassed.
In `@src/api/rate_limit.py`:
- Around line 21-28: The rate_limiter function can raise AttributeError when
request.client is None or when user.email is missing; update rate_limiter to
safely derive the key by first checking user and user.email (use AUTH_LIMIT when
user and user.email are present) and otherwise fall back to an IP extracted from
request.client.host only if request.client is not None, else try
request.headers.get("x-forwarded-for") or request.client.peername or use a
constant fallback like "unknown" before forming key (e.g.,
"ip:{ip_or_fallback}"), and ensure limit is set to AUTH_LIMIT or ANON_LIMIT
accordingly so key and limit are always defined without accessing attributes on
None.
In `@src/api/recommendations.py`:
- Around line 68-107: The endpoint recommend_rules currently ignores the
injected user and always calls RepositoryAnalysisAgent.execute with
is_public=True, preventing authenticated/private repo analysis; update
recommend_rules to set is_public based on whether user is present (e.g.,
is_public = False when user is authenticated) and pass the user's
auth/installation context into the agent (either via
RepositoryAnalysisAgent(...) constructor or as parameters to
RepositoryAnalysisAgent.execute) so the agent can perform authenticated GitHub
requests; ensure you use the existing get_current_user_optional injection,
adjust the call sites for RepositoryAnalysisAgent.execute to accept and forward
the token/installation info, and keep parse_repo_from_url and existing error
handling unchanged.
In `@src/integrations/github/api.py`:
- Around line 982-1005: The List PR handler currently sums additions/deletions
from the list response (variables additions, deletions, lines_changed inside the
merged_prs append) but GitHub's List PR endpoint omits those fields; update the
logic so lines_changed is computed from a reliable source: for each PR in the
list call either GET /repos/{owner}/{repo}/pulls/{pull_number} to read
additions/deletions from the full PR object, or call GET
/repos/{owner}/{repo}/pulls/{pull_number}/files and sum file['additions'] and
file['deletions']; replace the current additions/deletions/lines_changed
assignment inside the merged_prs construction with values returned by the per-PR
request and ensure this per-PR fetch is done in the loop that builds merged_prs
(keep _detect_issue_references usage as-is).
In `@src/integrations/github/graphql_client.py`:
- Around line 77-81: The HTTP request in the GraphQL client uses
httpx.Client().post without a timeout which can hang indefinitely; update the
call to include a timeout (either set on the client or per-request) — e.g., pass
timeout=10 (or a configurable constant) to httpx.Client(timeout=...) or
client.post(..., timeout=10) in the method that posts to self.endpoint, so
response.raise_for_status() and response.json() cannot block forever; ensure any
existing tests/configuration read the timeout constant if you make it
configurable.
- Around line 89-96: The commit list comprehension that constructs Commit(...)
assumes node["commit"]["author"]["name"] always exists and can raise KeyError
for commits with null authors; update the extraction in the commits
comprehension (the code that builds Commit instances from
pr_data["commits"]["nodes"]) to safely access the author name using .get() and a
fallback (e.g., author = node["commit"].get("author", {}) and name =
author.get("name") or a sensible default like None or an empty string), matching
how reviews/comments are handled, so Commit(...) receives a safe author value.
In `@src/integrations/github/service.py`:
- Around line 86-109: After each client.get in the block that iterates
files_to_check, explicitly handle non-200 responses so auth/server errors are
surfaced: check resp.status_code — if 200 append filepath to found_files; if 404
treat as legitimately missing and continue; otherwise log the error (use logger
with repo, status_code and response.text) and then call resp.raise_for_status()
(or re-raise) so the existing httpx.HTTPStatusError except block runs; this
change involves the variables/methods shown (files_to_check, resp, found_files,
BASE_URL, logger) and ensures non-404 failures are not silently treated as
missing files.
In `@src/main.py`:
- Around line 111-119: The CORS setup uses allow_origins=["*"] together with
allow_credentials=True which is invalid for credentialed browser requests;
update the app.add_middleware(CORSMiddleware, ...) configuration so
allow_credentials=True only when allow_origins is a specific list of origins
(not "*"). Replace the wildcard with an explicit origins list (e.g., load from
an env var like FRONTEND_ORIGINS and parse into a list) and pass that list to
allow_origins; keep allow_methods and allow_headers as-is and ensure the code
paths that set allow_credentials consult whether the origins list contains
specific entries rather than "*".
In `@src/rules/models.py`:
- Around line 30-74: HygieneMetrics in rules.models is dead code; remove the
entire HygieneMetrics class definition from that module and any leftover imports
referring to it, ensuring the package export list (__all__) in rules.__init__
does not reference HygieneMetrics and that the active HygieneMetrics in
agents.repository_analysis_agent.models remains the canonical model used by
nodes.py and tests.
In `@src/rules/validators.py`:
- Around line 239-243: The has_min_approvals validator currently always returns
True; replace the hardcoded return with real approval logic: read min_approvals
from parameters (e.g., parameters.get("min_approvals", 1)), obtain the PR review
list from the validator context or payload used elsewhere (similar to how
MinApprovalsCondition accesses PR reviews), count reviews with state=="APPROVED"
(and optionally dedupe by reviewer) and return True only if approvals_count >=
min_approvals; if you cannot access reviews in this validator, remove or
feature-flag has_min_approvals to avoid silently passing rules.
In `@src/webhooks/dispatcher.py`:
- Around line 3-9: Dispatcher uses f-string interpolation in structlog calls;
change each logger.* call in dispatcher.py that currently does
logger.info(f"...{var}...") / logger.error(f"...") to structured logging by
passing a plain message string and the variables as keyword context (e.g.,
logger.info("dispatching webhook event", event_type=event.type,
webhook_id=webhook.id, handler=name)), and for exceptions include exc_info or
pass the exception as a context key (e.g., logger.exception("handler failed",
handler=handler_name, error=exc)). Update all such calls related to EventType,
WebhookEvent, and EventHandler usage so message and metadata are separated.
In `@tests/conftest.py`:
- Around line 32-49: The test hardcodes a mock API key (OPENAI_API_KEY) inside
the mock_settings fixture causing a gitleaks false positive; update the test to
avoid embedding a string that looks like a real key by either adding this
specific test value to the gitleaks allowlist configuration or moving the mock
secrets into a gitleaks-ignored test env file and loading them from there, and
ensure the change targets the mock_settings fixture and the OPENAI_API_KEY entry
so CI stops flagging it.
In `@tests/integration/test_recommendations.py`:
- Around line 35-70: The test is mocking httpx calls with respx but
fetch_repository_metadata (used via github_client in
src/integrations/github/api.py) uses aiohttp so those mocks are never applied;
update the test to inject an httpx-backed client for fetch_repository_metadata
(or replace the respx mocks with aioresponses mocks) so the GitHub API stubs
cover both fetch_repository_metadata and fetch_pr_signals (GitHubClient in
src/integrations/github/client.py); additionally update nodes.py to stop
catching only httpx.HTTPStatusError for errors from the aiohttp-based
client—either catch aiohttp.ClientResponseError or use a broader exception (or
normalize exceptions in the injected client) so error handling matches the
actual client implementation.
🟡 Minor comments (11)
src/api/rules.py-9-11 (1)
9-11:event_datafield is defined but never used.The
event_dataparameter is added to the request model but is not passed toagent.execute(). Looking at theRuleFeasibilityAgent.execute()signature, it only acceptsrule_description. Either remove this unused field or implement its usage.Option 1: Remove unused field
class RuleEvaluationRequest(BaseModel): rule_text: str - event_data: dict | None = None # Advanced: pass extra event data for edge cases.src/webhooks/handlers/issue_comment.py-35-39 (1)
35-39: Substring matching may cause false positives.The
incheck matches any username containing a bot name substring (e.g.,"mywatchflowhelper"or"watchflowfan"would be treated as bots). Consider exact matching or word-boundary checks if legitimate users could have these substrings in their names.Suggested fix using exact matching
- if commenter and any(bot_name.lower() in commenter.lower() for bot_name in bot_usernames): + if commenter and commenter.lower() in [b.lower() for b in bot_usernames]:docs/benchmarks.md-9-9 (1)
9-9: Inconsistent formatting with "70+" references.Line 9 uses "70 +" (with spaces) while line 58 uses "70+" (no spaces). This appears to be an unintentional whitespace change that creates inconsistency within the document.
Proposed fix — use consistent formatting
-Our analysis of 70 + enterprise policies from major tech companies revealed a critical insight: **85% of real-world governance policies require context** and cannot be effectively enforced with traditional static rules. +Our analysis of 70+ enterprise policies from major tech companies revealed a critical insight: **85% of real-world governance policies require context** and cannot be effectively enforced with traditional static rules.src/agents/feasibility_agent/agent.py-2-2 (1)
2-2: Typo: extra space in docstring.There's a double space between "handling" and "and" in the module docstring.
Proposed fix
-Rule Feasibility Agent implementation with error handling and retry logic. +Rule Feasibility Agent implementation with error handling and retry logic.docs/features.md-279-287 (1)
279-287: Minor wording adjustment needed — rate limit values are correct.The rate limit values (5 requests/hour for anonymous, 100 for authenticated) match the implementation in
src/api/rate_limit.py. However, change "100 repos/hour" to "100 requests/hour" for authenticated users, as the rate limit is applied per request, not per repository analyzed. The 429 response with Retry-After header is correctly implemented.src/integrations/github/graphql_client.py-116-120 (1)
116-120:exc_infoshould beTrue, not the exception object.With structlog, passing
exc_info=Truecaptures the current exception's traceback. Passing the exception object directly may not produce the expected output.🐛 Proposed fix
except httpx.HTTPStatusError as e: - logger.error("HTTP error fetching PR context", exc_info=e) + logger.error("HTTP error fetching PR context", status_code=e.response.status_code, exc_info=True) raise except Exception as e: - logger.error("An unexpected error occurred", exc_info=e) + logger.error("Unexpected error fetching PR context", exc_info=True) raisesrc/agents/repository_analysis_agent/nodes.py-286-293 (1)
286-293: PotentialKeyErrorif review author isNone.When iterating reviews,
review["author"]["login"]will raiseKeyErrorifauthorisNone(e.g., deleted user accounts).🐛 Proposed defensive access
- approved = any(review["state"] == "APPROVED" and review["author"]["login"] != author for review in reviews) + approved = any( + review["state"] == "APPROVED" + and review.get("author", {}).get("login") != author + for review in reviews + )src/integrations/github/schemas.py-16-18 (1)
16-18: Avoid defaultingdefault_branchto"main"when the field is missing.If upstream payloads omit
default_branch(e.g., partial GraphQL selections), defaulting to"main"can misroute downstream calls for repos using"master"or custom branches. Prefer making this optional so absence is explicit.💡 Suggested adjustment
- default_branch: str = Field(default="main") + default_branch: str | None = Nonesrc/api/recommendations.py-46-62 (1)
46-62: Tighten GitHub host validation.
"github.com" in p.hostwill acceptevilgithub.com. Prefer an exact match (or a strict allowlist) so non‑GitHub domains don’t pass validation.🔧 Suggested fix
- if not p.valid or not p.owner or not p.repo or "github.com" not in p.host: + if not p.valid or not p.owner or not p.repo or p.host not in {"github.com", "www.github.com"}:src/integrations/github/api.py-951-1042 (1)
951-1042: Remove httpx import and catch aiohttp errors instead.This method uses aiohttp (via
self._get_session()), sohttpx.HTTPStatusErrorwill never fire. Network failures will be classified as "unknown_error" instead of "network_error". Replace withaiohttp.ClientError:🔧 Suggested fix
- import httpx - logger = structlog.get_logger() @@ - except httpx.HTTPStatusError as e: - logger.error( - "pr_fetch_http_error", - repo=repo_full_name, - status_code=e.response.status_code, - error_type="network_error", - error=str(e), - ) - return [] + except aiohttp.ClientError as e: + logger.error( + "pr_fetch_http_error", + repo=repo_full_name, + error_type="network_error", + error=str(e), + ) + return []src/integrations/github/client.py-38-45 (1)
38-45: Do not send an empty Authorization header to GitHub API.When no token is provided, the client currently sends
Authorization: "", which GitHub treats as an authentication attempt with invalid credentials. This triggers a401 Unauthorizedresponse and can cause repeated requests to hit GitHub's failed login limit, resulting in temporary403 Forbiddenresponses. The correct approach is to omit the header entirely when no token is available, which allows requests to use GitHub's unauthenticated rate limit (60 req/hour per IP).Suggested fix
- self.headers = { - "Authorization": f"Bearer {self.token}" if self.token else "", - "Accept": "application/vnd.github.v3+json", - } + self.headers = {"Accept": "application/vnd.github.v3+json"} + if self.token: + self.headers["Authorization"] = f"Bearer {self.token}"
🧹 Nitpick comments (20)
src/integrations/providers/bedrock_provider.py (1)
156-159: Overly broad keywords may cause incorrect profile matching.The keywords
"general"and"default"in theis_anthropiccheck could match profiles that aren't actually Anthropic profiles. A profile named "general-purpose-inference" or "default-model-profile" would incorrectly match for any Anthropic model request.Consider restricting the keywords to provider-specific identifiers only, or verifying the profile's underlying model configuration.
♻️ Suggested fix
- is_anthropic = any(k in profile_name for k in ["claude", "anthropic", "general", "default"]) + is_anthropic = any(k in profile_name for k in ["claude", "anthropic"])src/core/utils/caching.py (1)
161-163: Cache initialization logic looks correct, butttl=0edge case silently defaults to 3600.When
ttl=0is passed, the falsy check causes fallback toTTLCachewithttl=3600. Whilettl=0is unlikely in practice, consider using an explicitNonecheck if you want to distinguish between "no TTL provided" and "TTL explicitly set to 0":cache = AsyncCache(maxsize=maxsize, ttl=ttl) if ttl is not None else TTLCache(maxsize=maxsize, ttl=3600)The lint comment (
SIM108) can be removed now that the refactor is applied—it documents why the change was made, not what the code does.src/webhooks/handlers/issue_comment.py (1)
54-58: Extract duplicated PR number resolution into a helper method.This logic is repeated identically at lines 100-104 and 122-126. A helper method would reduce duplication and ensure consistent behavior.
Suggested helper method
def _get_pr_or_issue_number(self, event: WebhookEvent) -> int | None: """Extract PR or issue number from event payload.""" return ( event.payload.get("issue", {}).get("number") or event.payload.get("pull_request", {}).get("number") or event.payload.get("number") )Then replace each occurrence:
- pr_number = ( - event.payload.get("issue", {}).get("number") - or event.payload.get("pull_request", {}).get("number") - or event.payload.get("number") - ) + pr_number = self._get_pr_or_issue_number(event)src/webhooks/handlers/deployment_status.py (1)
36-42: Consider adding error handling for queue failures.The updated comment correctly notes that enqueue "may fail if queue overloaded," but there's no try/except around the
task_queue.enqueue()call. If the queue fails, the exception propagates and the webhook returns an error to GitHub, which may trigger retries.Consider wrapping this in error handling to return a graceful degradation response or implement backpressure signaling.
Proposed enhancement
# Enqueue: async, may fail if queue overloaded. - task_id = await task_queue.enqueue( - event_type="deployment_status", - repo_full_name=repo_full_name, - installation_id=installation_id, - payload=payload, - ) + try: + task_id = await task_queue.enqueue( + event_type="deployment_status", + repo_full_name=repo_full_name, + installation_id=installation_id, + payload=payload, + ) + except Exception as e: + logger.error(f"Failed to enqueue deployment status event for {repo_full_name}: {e}") + return {"status": "error", "message": "Queue temporarily unavailable", "retry": True}linting_commands.txt (1)
1-9: Consider consolidating into a Makefile or documenting in CONTRIBUTING.md.These commands are useful, but a standalone
.txtfile may be easily overlooked. Consider:
- Adding a
Makefilewith targets likemake lint,make format,make typecheck- Including these commands in
CONTRIBUTING.mdfor better discoverability- If keeping as a reference file, rename to something like
scripts/lint.shand make it executablesrc/core/config/settings.py (2)
114-119: Consider removing demo-specific comments from production code.The comment "CRITICAL: USE_MOCK_DATA must be False for CEO demo (Phase 5)" is a transient operational note that doesn't belong in the codebase. Consider documenting deployment requirements in a runbook or deployment checklist instead.
148-155: Vertex AI aliases are duplicated withPROVIDER_MAPin factory.py.This
vertex_aliasesset mirrors the aliases defined insrc/integrations/providers/factory.py(lines 24-28). If aliases are added or removed, both locations must be updated, risking divergence.Consider extracting a shared constant or reusing
PROVIDER_MAPto derive the canonical provider name.♻️ Example: Extract shared aliases
Create a shared constant in a common location (e.g.,
provider_config.py):VERTEX_AI_ALIASES = {"vertex_ai", "garden", "vertex", "vertexai", "model_garden", "gcp"}Then import and use it in both
factory.pyandsettings.py.src/integrations/github/graphql_client.py (2)
35-35: Consider making this method async for consistency with the codebase.This method uses a synchronous
httpx.Client, but the rest of the codebase (e.g.,contributors.py) uses async patterns. Calling this synchronous method from an async context will block the event loop.♻️ Async alternative
- def fetch_pr_context(self, owner: str, repo: str, pr_number: int) -> PRContext: + async def fetch_pr_context(self, owner: str, repo: str, pr_number: int) -> PRContext: """ Fetches the context of a pull request from GitHub's GraphQL API. """ # ... query definition ... try: - with httpx.Client() as client: - response = client.post(self.endpoint, json={"query": query, "variables": variables}, headers=headers) + async with httpx.AsyncClient(timeout=30.0) as client: + response = await client.post( + self.endpoint, + json={"query": query, "variables": variables}, + headers=headers, + )
83-85: Prefer a specific exception type over genericException.Raising a bare
Exceptionmakes it harder for callers to distinguish GraphQL errors from other failures. Consider defining a custom exception or including error details.♻️ Proposed improvement
+class GitHubGraphQLError(Exception): + """Raised when a GitHub GraphQL query fails.""" + def __init__(self, errors: list): + self.errors = errors + super().__init__(f"GraphQL query failed: {errors}") # In fetch_pr_context: if "errors" in data: logger.error("GraphQL query failed", errors=data["errors"]) - raise Exception("GraphQL query failed") + raise GitHubGraphQLError(data["errors"])src/core/models.py (1)
7-18: Consider protectinggithub_tokenfrom accidental serialization/logging.The
github_tokenfield stores a sensitive OAuth token. Pydantic's default serialization behavior will include this field when the model is converted to dict/JSON, potentially exposing it in logs or API responses.Consider using
SecretStror excluding it from serialization:♻️ Option 1: Use SecretStr for automatic masking
-from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, SecretStr class User(BaseModel): ... - github_token: str | None = Field(None, description="OAuth token for GitHub API access") + github_token: SecretStr | None = Field(None, description="OAuth token for GitHub API access")Access the raw value via
user.github_token.get_secret_value()when needed.♻️ Option 2: Exclude from default serialization
- 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", exclude=True)src/api/rate_limit.py (1)
13-14: In-memory rate limiting won't persist across restarts or scale horizontally.The in-memory
_RATE_LIMIT_STOREis appropriate for single-instance deployments but will reset on process restart and won't synchronize across multiple instances (e.g., in Kubernetes).This is acceptable for an MVP, but consider Redis-backed rate limiting for production multi-instance deployments.
pyproject.toml (1)
119-119: Duplicate dependency:giturlparseis already a main dependency.
giturlparse>=0.1.0is listed in both the maindependencies(line 28) and[tool.uv] dev-dependencies(line 119). The dev-dependencies entry is redundant.♻️ Remove duplicate
[tool.uv] dev-dependencies = [ "pytest>=7.4.0", "pytest-asyncio>=0.21.0", "pytest-cov>=4.1.0", "mypy>=1.7.0", "pre-commit>=3.5.0", "ruff>=0.1.0", - "giturlparse>=0.1.0", ]src/agents/repository_analysis_agent/models.py (2)
6-17: Consider validating the parsed identifier format.
parse_github_repo_identifierdoesn't validate that the result matchesowner/repoformat. Inputs like"https://github.com/"or"///"would return malformed strings.♻️ Proposed validation
def parse_github_repo_identifier(identifier: str) -> str: """ Normalizes various GitHub identifiers into 'owner/repo' format. Used by tests to verify repository strings. """ # Remove protocol and domain clean_id = identifier.replace("https://github.com/", "").replace("http://github.com/", "") # Remove .git suffix and trailing slashes clean_id = clean_id.replace(".git", "").strip("/") + # Validate format + if "/" not in clean_id or clean_id.count("/") != 1: + raise ValueError(f"Invalid repository identifier: {identifier}") + return clean_id
132-165: Missing range validation for percentage fields.Fields like
unlinked_issue_rate,ci_skip_rate, etc. are documented as(0.0-1.0)percentages but lack validation constraints. Invalid values (negative or >1.0) would be accepted silently.♻️ Add field validators
from pydantic import field_validator `@field_validator`( "unlinked_issue_rate", "ci_skip_rate", "codeowner_bypass_rate", "new_code_test_coverage", "issue_diff_mismatch_rate", "ghost_contributor_rate" ) `@classmethod` def validate_rate(cls, v: float) -> float: if not 0.0 <= v <= 1.0: raise ValueError("Rate must be between 0.0 and 1.0") return vtests/unit/agents/test_repository_analysis_models.py (1)
19-28: Tests verify basic instantiation but lack edge case coverage.Consider adding tests for:
- Boundary values (
unlinked_issue_rate=0.0,unlinked_issue_rate=1.0)- Invalid inputs if validation is added (negative rates, rates > 1.0)
- Default field values verification
src/agents/repository_analysis_agent/nodes.py (1)
36-45: Duplicate AI keywords list.The same keyword list appears in
_map_github_pr_to_signal(lines 36-45) andfetch_pr_signals(lines 244-253). Extract to a module-level constant for DRY compliance.♻️ Extract constant
+AI_DETECTION_KEYWORDS = [ + "generated by claude", + "cursor", + "copilot", + "chatgpt", + "ai-generated", + "llm", + "i am an ai", + "as an ai", +] + def _map_github_pr_to_signal(pr_data: dict[str, Any]) -> PRSignal: # ... - ai_keywords = [ - "generated by claude", - # ... - ] - is_ai_generated = any(keyword in body or keyword in title for keyword in ai_keywords) + is_ai_generated = any(keyword in body or keyword in title for keyword in AI_DETECTION_KEYWORDS)Also applies to: 244-253
src/webhooks/handlers/check_run.py (1)
18-18: Prefer structured logging over f-strings with structlog.Using f-strings defeats the purpose of structured logging. Structlog works best with keyword arguments for searchable, parseable log fields.
♻️ Use keyword arguments
- logger.info(f"🔄 Enqueuing check run event for {event.repo_full_name}") + logger.info("enqueuing_check_run_event", repo=event.repo_full_name) # ... - logger.info(f"✅ Check run event enqueued with task ID: {task_id}") + logger.info("check_run_event_enqueued", task_id=task_id)Also applies to: 27-27
src/api/dependencies.py (1)
8-9: Remove duplicate logger initialization.The second assignment is redundant.
🧹 Suggested cleanup
-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/main.py (1)
30-53: Align structlog filtering level with configured log level.
make_filtering_bound_logger(logging.INFO)hardcodes INFO even ifconfig.logging.levelis DEBUG, so debug logs are dropped. Use the configured level consistently (with a safe default).🔧 Suggested adjustment
-# Configure structlog for JSON logging (Phase 5: Observability) -structlog.configure( +log_level = getattr(logging, config.logging.level, logging.INFO) +# Configure structlog for JSON logging (Phase 5: Observability) +structlog.configure( processors=[ structlog.contextvars.merge_contextvars, structlog.processors.add_log_level, structlog.processors.TimeStamper(fmt="iso"), structlog.processors.JSONRenderer(), ], - wrapper_class=structlog.make_filtering_bound_logger(logging.INFO), + wrapper_class=structlog.make_filtering_bound_logger(log_level), context_class=dict, logger_factory=structlog.PrintLoggerFactory(), cache_logger_on_first_use=False, ) @@ -logging.basicConfig( - level=getattr(logging, config.logging.level), +logging.basicConfig( + level=log_level, format="%(message)s", # structlog handles formatting )src/integrations/github/api.py (1)
232-258: Avoid relying on aiohttp’s private_loopattribute.Accessing
_loopis fragile across aiohttp versions. Consider tracking the loop yourself when you create the session and compare to the current loop.♻️ Example refactor
- if self._session is None or self._session.closed: - self._session = aiohttp.ClientSession() + if self._session is None or self._session.closed: + self._session = aiohttp.ClientSession() + self._session_loop = asyncio.get_running_loop() else: # Check if we're in a different event loop (avoid deprecated .loop property) try: current_loop = asyncio.get_running_loop() - # Try to access session's internal loop to check if it's the same - # If the session's loop is closed, this will fail - if self._session._loop != current_loop or self._session._loop.is_closed(): + session_loop = getattr(self, "_session_loop", None) + if session_loop != current_loop or current_loop.is_closed(): await self._session.close() self._session = aiohttp.ClientSession() + self._session_loop = current_loop except RuntimeError: # No running loop or loop is closed, recreate session - self._session = aiohttp.ClientSession() + self._session = aiohttp.ClientSession() + self._session_loop = asyncio.get_running_loop() except Exception: # Fallback: ensure we have a valid session - self._session = aiohttp.ClientSession() + self._session = aiohttp.ClientSession() + self._session_loop = asyncio.get_running_loop()
📜 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 (55)
.github/workflows/docs.yaml.pre-commit-config.yamlREADME.mddocs/benchmarks.mddocs/features.mdlinting_commands.txtpyproject.tomlsrc/__init__.pysrc/agents/base.pysrc/agents/feasibility_agent/agent.pysrc/agents/repository_analysis_agent/__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/agents/repository_analysis_agent/test_agent.pysrc/api/__init__.pysrc/api/dependencies.pysrc/api/rate_limit.pysrc/api/recommendations.pysrc/api/rules.pysrc/core/config/settings.pysrc/core/errors.pysrc/core/models.pysrc/core/utils/caching.pysrc/core/utils/logging.pysrc/integrations/github/api.pysrc/integrations/github/client.pysrc/integrations/github/graphql_client.pysrc/integrations/github/schemas.pysrc/integrations/github/service.pysrc/integrations/providers/bedrock_provider.pysrc/integrations/providers/factory.pysrc/main.pysrc/rules/__init__.pysrc/rules/models.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/check_run.pysrc/webhooks/handlers/deployment.pysrc/webhooks/handlers/deployment_protection_rule.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_repo_analysis.pytests/integration/test_rules_api.pytests/unit/agents/test_repository_analysis_models.pytests/unit/api/test_recommendations.py
💤 Files with no reviewable changes (1)
- src/agents/repository_analysis_agent/test_agent.py
🧰 Additional context used
🧬 Code graph analysis (24)
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/rules/__init__.py (1)
src/rules/models.py (5)
Rule(91-100)RuleAction(84-88)RuleCategory(20-27)RuleCondition(77-81)RuleSeverity(9-17)
src/api/rate_limit.py (2)
src/core/models.py (1)
User(7-18)tests/integration/test_rules_api.py (1)
client(21-23)
src/rules/models.py (1)
src/agents/repository_analysis_agent/models.py (1)
HygieneMetrics(119-165)
src/webhooks/handlers/deployment_protection_rule.py (2)
src/core/models.py (1)
WebhookEvent(41-63)src/webhooks/handlers/base.py (1)
EventHandler(7-26)
src/webhooks/dispatcher.py (4)
src/core/models.py (2)
EventType(24-38)WebhookEvent(41-63)src/webhooks/handlers/base.py (2)
EventHandler(7-26)handle(16-26)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/agents/repository_analysis_agent/nodes.py (2)
src/agents/repository_analysis_agent/models.py (3)
PRSignal(94-116)RuleRecommendation(81-91)HygieneMetrics(119-165)src/core/models.py (1)
repo_full_name(56-58)
src/webhooks/handlers/deployment.py (3)
src/core/models.py (3)
EventType(24-38)WebhookEvent(41-63)repo_full_name(56-58)src/webhooks/handlers/base.py (1)
EventHandler(7-26)src/core/utils/caching.py (1)
get(44-66)
src/api/__init__.py (1)
src/api/recommendations.py (3)
AnalysisResponse(33-40)AnalyzeRepoRequest(22-30)parse_repo_from_url(46-62)
tests/integration/test_recommendations.py (1)
src/core/utils/caching.py (1)
get(44-66)
src/api/dependencies.py (2)
src/core/models.py (1)
User(7-18)src/integrations/github/service.py (1)
GitHubService(15-166)
tests/integration/test_repo_analysis.py (3)
src/agents/repository_analysis_agent/agent.py (1)
RepositoryAnalysisAgent(14-76)src/agents/repository_analysis_agent/models.py (2)
AnalysisState(168-195)HygieneMetrics(119-165)src/rules/models.py (1)
HygieneMetrics(30-74)
src/api/rules.py (3)
src/agents/factory.py (1)
get_agent(20-52)src/agents/base.py (1)
execute(114-119)src/agents/feasibility_agent/agent.py (1)
execute(57-123)
src/integrations/github/client.py (2)
src/core/errors.py (2)
GitHubGraphQLError(6-11)RepositoryNotFoundError(14-17)src/integrations/github/schemas.py (1)
GitHubRepository(6-26)
src/agents/repository_analysis_agent/agent.py (3)
src/agents/base.py (5)
AgentResult(16-29)BaseAgent(32-119)_build_graph(55-60)execute(114-119)_execute_with_timeout(93-111)src/agents/repository_analysis_agent/nodes.py (3)
fetch_repository_metadata(58-175)fetch_pr_signals(178-376)generate_rule_recommendations(379-454)src/core/models.py (1)
repo_full_name(56-58)
src/api/recommendations.py (5)
src/agents/repository_analysis_agent/agent.py (2)
RepositoryAnalysisAgent(14-76)execute(42-76)src/api/dependencies.py (1)
get_current_user_optional(24-43)src/api/rate_limit.py (1)
rate_limiter(21-41)src/core/models.py (2)
User(7-18)repo_full_name(56-58)src/agents/base.py (1)
execute(114-119)
src/integrations/github/service.py (1)
src/core/errors.py (2)
GitHubRateLimitError(20-23)GitHubResourceNotFoundError(26-29)
src/webhooks/handlers/check_run.py (2)
src/core/models.py (1)
WebhookEvent(41-63)src/webhooks/handlers/base.py (1)
EventHandler(7-26)
src/agents/base.py (4)
src/agents/repository_analysis_agent/agent.py (2)
_build_graph(22-40)execute(42-76)src/agents/feasibility_agent/agent.py (2)
_build_graph(34-55)execute(57-123)src/agents/acknowledgment_agent/agent.py (2)
_build_graph(36-52)execute(212-217)src/agents/engine_agent/agent.py (2)
_build_graph(54-73)execute(75-173)
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/unit/api/test_recommendations.py (1)
src/api/recommendations.py (1)
parse_repo_from_url(46-62)
src/integrations/providers/factory.py (2)
src/core/config/provider_config.py (2)
get_max_tokens_for_agent(57-63)get_temperature_for_agent(65-71)tests/unit/test_feasibility_agent.py (1)
agent(18-31)
src/agents/repository_analysis_agent/models.py (2)
src/rules/models.py (1)
HygieneMetrics(30-74)src/core/models.py (1)
repo_full_name(56-58)
tests/conftest.py (1)
src/integrations/github/api.py (1)
close(165-168)
🪛 Gitleaks (8.30.0)
src/integrations/github/api.py
[high] 105-105: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 121-121: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
tests/conftest.py
[high] 42-42: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v5.0.0 | ||
| rev: v6.0.0 | ||
| hooks: | ||
| - id: trailing-whitespace | ||
| # Removes any trailing whitespace from lines in text files | ||
| - id: end-of-file-fixer | ||
| # Ensures files end with a newline | ||
| - id: check-yaml | ||
| exclude: '^helm-chart/templates.*\.yaml$|^mkdocs\.yml$' | ||
| # Checks YAML files for syntax errors | ||
| - id: check-json | ||
| # Checks JSON files for syntax errors | ||
| - id: check-ast | ||
| name: "Check Python syntax (AST validation)" | ||
| # Validates that Python files have valid syntax | ||
| - id: check-added-large-files | ||
|
|
||
| # Ruff hooks for Python linting and formatting | ||
| # Ruff: The One Tool to Rule Them All | ||
| - repo: https://github.com/astral-sh/ruff-pre-commit | ||
| rev: v0.9.9 | ||
| rev: v0.3.0 | ||
| hooks: | ||
| # First, run ruff format - won't change imports but will format them | ||
| - id: ruff-format | ||
| name: "Format code (without changing imports)" | ||
|
|
||
| # Then, use a single ruff pass for both import sorting and linting | ||
| # Linter (Fixes imports, modernizes syntax, checks bugs) | ||
| - id: ruff | ||
| name: "Linting and import sorting" | ||
| args: ["--fix"] | ||
|
|
||
| # Pyupgrade: Check and fix Python version incompatibilities and outdated syntax | ||
| - repo: https://github.com/asottile/pyupgrade | ||
| rev: v3.15.2 | ||
| hooks: | ||
| - id: pyupgrade | ||
| name: "Upgrade syntax for Python 3.12+" | ||
| args: [--py312-plus] | ||
| # Auto-fixes outdated syntax to Python 3.12+ compatible code | ||
| args: [--fix, --exit-non-zero-on-fix] | ||
| # Formatter (Replaces Black) | ||
| - id: ruff-format | ||
|
|
||
| # Conventional pre-commit hooks for commit messages | ||
| # Convention Enforcement | ||
| - repo: https://github.com/compilerla/conventional-pre-commit | ||
| rev: v4.0.0 | ||
| rev: v4.3.0 | ||
| hooks: |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pre-commit-hooks latest release 2025 2026
💡 Result:
The latest release of pre-commit-hooks (pre-commit/pre-commit-hooks) is v6.0.0, released on August 9, 2025 (and it is still the latest as of January 23, 2026). [1], [2]
Where it’s published:
- GitHub Releases: v6.0.0 (Aug 9, 2025) [1]
- PyPI package
pre-commit-hooks: 6.0.0 (uploaded Aug 9, 2025) [2]
🌐 Web query:
ruff-pre-commit latest release 2025 2026
💡 Result:
- Latest
astral-sh/ruff-pre-commitrelease (as of Jan 23, 2026): v0.14.14, released Jan 22, 2026. [1] - Last release in 2025: v0.14.10, released Dec 18, 2025. [1]
Sources:
[1] GitHub Releases – astral-sh/ruff-pre-commit: https://github.com/astral-sh/ruff-pre-commit/releases
🌐 Web query:
conventional-pre-commit latest release 2025 2026
💡 Result:
As of January 23, 2026, the latest released version of conventional-pre-commit is 4.3.0, released on October 10, 2025. No newer (2026) release is listed in the PyPI release history. [1] [2]
Recent releases:
- 4.3.0 — Oct 10, 2025 [1]
- 4.2.0 — May 5, 2025 [1]
- 4.1.0 — Apr 29, 2025 [1]
Sources: [1] [2]
Update ruff-pre-commit to v0.14.14.
The pinned version v0.3.0 is significantly outdated; the latest release is v0.14.14 (Jan 22, 2026). The other hooks are current: pre-commit-hooks v6.0.0 (Aug 2025) and conventional-pre-commit v4.3.0 (Oct 2025).
🤖 Prompt for AI Agents
In @.pre-commit-config.yaml around lines 6 - 29, The pinned ruff-pre-commit rev
is outdated; update the rev for the repo
"https://github.com/astral-sh/ruff-pre-commit" (the hooks with id: ruff and id:
ruff-format) from v0.3.0 to v0.14.14 in .pre-commit-config.yaml so the
pre-commit config uses the current ruff release.
| async def execute(self, **kwargs: Any) -> AgentResult: | ||
| """ | ||
| Execute the agent with given parameters. | ||
| Explicitly typed **kwargs to satisfy MyPy strict mode. | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all BaseAgent subclasses
echo "=== BaseAgent subclasses ==="
rg -n --type=py 'class .*\(.*BaseAgent.*\):' -C2
echo ""
echo "=== Execute method signatures in agents ==="
rg -n --type=py 'def execute\(' -C2 src/agentsRepository: warestack/watchflow
Length of output: 3468
Fix execute method signatures in RuleFeasibilityAgent, RuleEngineAgent, and AcknowledgmentAgent to match the BaseAgent abstract signature.
Three subclasses have execute method signatures incompatible with the base class:
RuleFeasibilityAgent(line 57 ofsrc/agents/feasibility_agent/agent.py): usesrule_description: strRuleEngineAgent(line 75 ofsrc/agents/engine_agent/agent.py): usesevent_type: str, event_data: dict[str, Any], rules: list[dict[str, Any]]AcknowledgmentAgent(line 212 ofsrc/agents/acknowledgment_agent/agent.py): usesevent_type: str, event_data: dict[str, Any], rules: list[dict[str, Any]]
These violate substitutability and will trigger MyPy "incompatible override" errors under strict mode. Update all subclass execute methods to use **kwargs: Any to match the BaseAgent signature.
🤖 Prompt for AI Agents
In `@src/agents/base.py` around lines 114 - 118, The subclass execute signatures
are incompatible with the BaseAgent.async def execute(self, **kwargs: Any) ->
AgentResult; update RuleFeasibilityAgent.execute, RuleEngineAgent.execute, and
AcknowledgmentAgent.execute to accept **kwargs: Any (matching BaseAgent) and
refactor their bodies to pull needed values from kwargs (e.g., rule_description
= kwargs.get("rule_description"), event_type = kwargs.get("event_type"),
event_data = kwargs.get("event_data"), rules = kwargs.get("rules")) before use
so MyPy substitutability is preserved while behavior remains the same.
| # Unused: min_approvals—left for future logic. | ||
| # min_approvals = parameters.get("min_approvals", 1) | ||
|
|
||
| # Placeholder logic - in production, this would check the actual PR reviews | ||
| # TODO: Check actual PR reviews. Always returns True—demo only. | ||
| return True |
There was a problem hiding this comment.
has_min_approvals always returns True—rule is ineffective.
If this validator is registered, it silently disables approval checks. Either implement it or remove/feature-flag it to avoid false “pass” results.
✅ Suggested implementation (aligns with MinApprovalsCondition)
- # Unused: min_approvals—left for future logic.
- # min_approvals = parameters.get("min_approvals", 1)
-
- # TODO: Check actual PR reviews. Always returns True—demo only.
- return True
+ min_approvals = parameters.get("min_approvals", 1)
+ reviews = event.get("reviews", [])
+ approved_count = sum(1 for review in reviews if review.get("state") == "APPROVED")
+ logger.debug(
+ f"ApprovalCountCondition: PR has {approved_count} approvals, requires {min_approvals}"
+ )
+ return approved_count >= min_approvals📝 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.
| # Unused: min_approvals—left for future logic. | |
| # min_approvals = parameters.get("min_approvals", 1) | |
| # Placeholder logic - in production, this would check the actual PR reviews | |
| # TODO: Check actual PR reviews. Always returns True—demo only. | |
| return True | |
| min_approvals = parameters.get("min_approvals", 1) | |
| reviews = event.get("reviews", []) | |
| approved_count = sum(1 for review in reviews if review.get("state") == "APPROVED") | |
| logger.debug( | |
| f"ApprovalCountCondition: PR has {approved_count} approvals, requires {min_approvals}" | |
| ) | |
| return approved_count >= min_approvals |
🤖 Prompt for AI Agents
In `@src/rules/validators.py` around lines 239 - 243, The has_min_approvals
validator currently always returns True; replace the hardcoded return with real
approval logic: read min_approvals from parameters (e.g.,
parameters.get("min_approvals", 1)), obtain the PR review list from the
validator context or payload used elsewhere (similar to how
MinApprovalsCondition accesses PR reviews), count reviews with state=="APPROVED"
(and optionally dedupe by reviewer) and return True only if approvals_count >=
min_approvals; if you cannot access reviews in this validator, remove or
feature-flag has_min_approvals to avoid silently passing rules.
…stration in Acknowledgment and Engine Agents
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.