[WIP]feat: FastAPI server to integrate github events with orchestrator graph #171
Conversation
WalkthroughThis update introduces a new FastAPI-based microservice for handling GitHub webhook events, focusing on issue assignments and bot mentions. It adds configuration, authentication, event processing, handler modules, and a server implementation. Supporting scripts, documentation, and comprehensive tests are included for setup, local testing, and validation of the event listener's functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub as GitHub
participant EventListener as FastAPI Event Listener
participant HandlerRegistry as Handler Registry
participant Handler as Event Handler
participant Orchestrator as Orchestrator
GitHub->>EventListener: POST /github/events (webhook)
EventListener->>HandlerRegistry: handle_event(event_type, event_data)
HandlerRegistry->>Handler: can_handle(event)
Handler->>HandlerRegistry: Yes/No
HandlerRegistry->>Handler: handle(event) (if capable)
Handler->>Orchestrator: invoke_orchestrator(state, thread_id)
Orchestrator-->>Handler: result/status
Handler-->>HandlerRegistry: result
HandlerRegistry-->>EventListener: result
EventListener-->>GitHub: JSON response (status, message)
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (13)
src/event_listener_tests/test_auth.py (2)
36-48: Clarify the “1-second” expiry – the math is brittle
expires_in=61coupled with the internalexpires_at - 60logic means the token is actually valid for ~1 second.
That works, but a literal1would be far clearer and avoids any accidental coupling to implementation details.- await cache.set("test_key", "test_token", expires_in=61) # 61 - 60 = 1 second + await cache.set("test_key", "test_token", expires_in=1)
129-138: Remove unnecessary patching ofauth.clientBecause the token is already cached,
get_installation_token()returns early and never touchesself.client.
Patching it adds noise and hides real regressions (the test will still pass even if the method suddenly does hit the network).- with patch.object(auth, "client") as mock_client: - token = await auth.get_installation_token() - - assert token == "cached_token" - mock_client.post.assert_not_called() + token = await auth.get_installation_token() + assert token == "cached_token"src/event_listener/config.py (1)
38-48: Private-key loader silently swallows I/O errors
os.path.exists()covers non-existence, butopen()can still fail (permission error, directory, etc.) and you re-raise that
wrapped inValueError. Consider catchingOSErrorexplicitly and including the err message so users immediately know what went wrong.scripts/setup_github_app.py (1)
1-3: Silence RuffT201for this interactive scriptInteractive CLI utilities legitimately rely on
Add a file-level directive so CI stops failing:# ruff: noqa: T201,E722src/event_listener_tests/test_processor.py (1)
24-26: Specify encoding when reading fixture filesThis avoids platform-dependent defaults and Unicode surprises.
- with open(fixture_path) as f: + with open(fixture_path, encoding="utf-8") as f:src/event_listener/event_processor.py (1)
135-137: Consider documenting the lazy import pattern.The lazy imports are used to avoid circular dependencies. Consider adding a comment explaining this design choice for future maintainers.
# Lazy imports to avoid loading orchestrator at module level +# This prevents circular dependencies and reduces startup time from common.state import Project from orchestrator.state import Statesrc/event_listener/handlers.py (2)
199-200: Fix typo in class docstringThe docstring appears to have a typo or incomplete text.
Apply this diff to fix the documentation:
- """Handles issue comment it shouldn with AI bot trigger phrases.""" + """Handles issue comment events with AI bot trigger phrases."""
247-250: Fix docstring formattingOne-line docstring should fit on a single line.
Apply this diff to fix the formatting:
- def __init__(self): - """Initialize handler registry. - """ + def __init__(self): + """Initialize handler registry."""🧰 Tools
🪛 Ruff (0.11.9)
248-249: One-line docstring should fit on one line
Reformat to one line
(D200)
🪛 GitHub Actions: Checks
[error] 248-249: D200: One-line docstring should fit on one line. Reformat docstring.
src/event_listener/server.py (1)
107-108: Use imperative mood for function docstringFunction docstrings should start with an imperative verb.
Apply this diff to fix the docstring:
-async def health_check(): - """Basic health check endpoint.""" +async def health_check(): + """Provide basic health check endpoint."""🧰 Tools
🪛 Ruff (0.11.9)
108-108: First line of docstring should be in imperative mood: "Basic health check endpoint."
(D401)
🪛 GitHub Actions: Checks
[error] 108-108: D401: First line of docstring should be in imperative mood.
src/event_listener/README.md (2)
108-115: Replace emphasis with proper headings to satisfy markdownlint MD036
**Option A: …**and**Option B: …**are rendered as bold text, but markdownlint flags them because they are used in place of headings.
Convert them to level-4 headings (or another appropriate level) so tooling and screen readers recognise the document structure.-**Option A: Interactive Setup (Recommended)** +#### Option A: Interactive setup (Recommended) … -**Option B: Manual Setup** +#### Option B: Manual setup🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
108-108: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
113-113: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
115-115: Bare URL used
null(MD034, no-bare-urls)
115-120: Avoid bare URLs (markdownlint MD034)Lines 115–120 contain plain URLs. Wrap them in Markdown link syntax so the rendered text is cleaner and you stay lint-clean.
-1. Create GitHub App at https://github.com/settings/apps/new +1. Create a GitHub App at [github.com/settings/apps/new](https://github.com/settings/apps/new)Apply the same treatment to the other bare URLs in this block.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
115-115: Bare URL used
null(MD034, no-bare-urls)
118-118: Bare URL used
null(MD034, no-bare-urls)
119-119: Bare URL used
null(MD034, no-bare-urls)
scripts/test_webhooks.py (2)
19-32:run_curlloses the HTTP status code
curl’s exit status (captured viareturncode) only signals transport-layer success, not the HTTP status.
Add-w '%{http_code}'and capture it, or switch torequestsfor simplicity.- cmd = [ + cmd = [ "curl", "-sS", # silent, show errors "-o", "-", # output response body to stdout + "-w", "\\nHTTP_STATUS:%{http_code}", # append status markerThen parse the marker to show the actual 2xx/4xx code.
39-55: Usecheck=True(or explicit handling) when invokingsubprocess.runCurrently failures (e.g., network down) are silent. Pass
check=Trueor inspectresult.returncodeand abort early.- result = subprocess.run(cmd, capture_output=True, text=True) + result = subprocess.run(cmd, capture_output=True, text=True, check=False) + if result.returncode != 0: + raise RuntimeError(f"curl exited with {result.returncode}: {result.stderr}")🧰 Tools
🪛 Ruff (0.11.9)
41-41:
Remove
(T201)
45-45:
Remove
(T201)
47-47:
Remove
(T201)
50-50:
Remove
(T201)
52-52:
Remove
(T201)
55-55:
Remove
(T201)
🪛 GitHub Actions: Checks
[error] 41-52: Ruff T201:
[error] 55-55: Ruff T201:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.env.example(1 hunks)pyproject.toml(3 hunks)scripts/setup_github_app.py(1 hunks)scripts/test_webhooks.py(1 hunks)src/event_listener/README.md(1 hunks)src/event_listener/__init__.py(1 hunks)src/event_listener/config.py(1 hunks)src/event_listener/event_processor.py(1 hunks)src/event_listener/github_auth.py(1 hunks)src/event_listener/handlers.py(1 hunks)src/event_listener/server.py(1 hunks)src/event_listener_tests/__init__.py(1 hunks)src/event_listener_tests/fixtures/issue_assigned.json(1 hunks)src/event_listener_tests/fixtures/pull_request_opened.json(1 hunks)src/event_listener_tests/test_auth.py(1 hunks)src/event_listener_tests/test_handlers.py(1 hunks)src/event_listener_tests/test_processor.py(1 hunks)src/event_listener_tests/test_server.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
scripts/setup_github_app.py (2)
src/event_listener/config.py (1)
validate_private_key(40-48)scripts/test_webhooks.py (1)
main(276-340)
src/event_listener/config.py (1)
scripts/setup_github_app.py (1)
validate_private_key(79-95)
src/event_listener/github_auth.py (2)
src/event_listener_tests/test_server.py (1)
client(16-18)src/event_listener/config.py (1)
github_headers(68-73)
src/event_listener/handlers.py (2)
src/event_listener/event_processor.py (3)
get_event_processor(226-231)should_process_event(174-219)process_event(125-172)src/orchestrator/graph.py (1)
OrchestratorGraph(102-275)
src/event_listener/server.py (3)
src/event_listener/github_auth.py (4)
cleanup_auth(192-197)get_github_auth(184-189)get(22-30)get_rate_limit(149-163)src/event_listener/handlers.py (4)
cleanup_handlers(312-315)get_handler_registry(303-309)orchestrator(29-33)handle_event(269-296)src/event_listener_tests/test_handlers.py (4)
registry(264-266)handler(120-122)handler(200-202)handler(228-230)
🪛 Ruff (0.11.9)
scripts/setup_github_app.py
14-14: print found
Remove print
(T201)
15-15: print found
Remove print
(T201)
16-16: print found
Remove print
(T201)
21-21: print found
Remove print
(T201)
22-22: print found
Remove print
(T201)
32-32: print found
Remove print
(T201)
35-35: print found
Remove print
(T201)
83-83: print found
Remove print
(T201)
90-90: print found
Remove print
(T201)
94-94: print found
Remove print
(T201)
105-105: print found
Remove print
(T201)
108-108: print found
Remove print
(T201)
110-110: Do not use bare except
(E722)
111-111: print found
Remove print
(T201)
112-112: print found
Remove print
(T201)
117-117: First line of docstring should be in imperative mood: "Main setup flow."
(D401)
126-126: print found
Remove print
(T201)
129-129: print found
Remove print
(T201)
130-130: print found
Remove print
(T201)
131-131: print found
Remove print
(T201)
134-134: print found
Remove print
(T201)
140-140: print found
Remove print
(T201)
141-141: print found
Remove print
(T201)
142-142: print found
Remove print
(T201)
143-143: print found
Remove print
(T201)
144-144: print found
Remove print
(T201)
145-145: print found
Remove print
(T201)
146-146: print found
Remove print
(T201)
147-147: print found
Remove print
(T201)
148-148: print found
Remove print
(T201)
149-149: print found
Remove print
(T201)
150-150: print found
Remove print
(T201)
151-151: print found
Remove print
(T201)
152-152: print found
Remove print
(T201)
153-153: print found
Remove print
(T201)
154-154: print found
Remove print
(T201)
155-155: print found
Remove print
(T201)
156-156: print found
Remove print
(T201)
157-157: print found
Remove print
(T201)
158-158: print found
Remove print
(T201)
159-159: print found
Remove print
(T201)
168-168: print found
Remove print
(T201)
172-172: print found
Remove print
(T201)
173-173: print found
Remove print
(T201)
174-174: print found
Remove print
(T201)
175-175: print found
Remove print
(T201)
176-176: print found
Remove print
(T201)
177-177: print found
Remove print
(T201)
178-178: print found
Remove print
(T201)
183-183: print found
Remove print
(T201)
187-187: print found
Remove print
(T201)
192-192: print found
Remove print
(T201)
196-196: print found
Remove print
(T201)
197-197: print found
Remove print
(T201)
198-198: print found
Remove print
(T201)
199-199: print found
Remove print
(T201)
200-200: print found
Remove print
(T201)
211-211: print found
Remove print
(T201)
215-215: print found
Remove print
(T201)
216-216: print found
Remove print
(T201)
217-217: print found
Remove print
(T201)
218-218: print found
Remove print
(T201)
219-219: print found
Remove print
(T201)
220-220: print found
Remove print
(T201)
221-221: print found
Remove print
(T201)
222-222: print found
Remove print
(T201)
223-223: print found
Remove print
(T201)
226-226: print found
Remove print
(T201)
230-230: print found
Remove print
(T201)
231-231: print found
Remove print
(T201)
232-232: print found
Remove print
(T201)
233-233: print found
Remove print
(T201)
234-234: print found
Remove print
(T201)
src/event_listener_tests/test_auth.py
237-237: Local variable auth is assigned to but never used
Remove assignment to unused variable auth
(F841)
src/event_listener/github_auth.py
18-18: Missing docstring in __init__
(D107)
48-48: Missing docstring in __init__
(D107)
scripts/test_webhooks.py
34-34: print found
Remove print
(T201)
35-35: print found
Remove print
(T201)
36-36: print found
Remove print
(T201)
41-41: print found
Remove print
(T201)
45-45: print found
Remove print
(T201)
47-47: print found
Remove print
(T201)
50-50: print found
Remove print
(T201)
52-52: print found
Remove print
(T201)
55-55: print found
Remove print
(T201)
254-254: print found
Remove print
(T201)
255-255: print found
Remove print
(T201)
265-265: print found
Remove print
(T201)
267-267: print found
Remove print
(T201)
270-270: print found
Remove print
(T201)
273-273: print found
Remove print
(T201)
277-277: First line of docstring should be in imperative mood: "Main test function."
(D401)
278-278: print found
Remove print
(T201)
279-279: print found
Remove print
(T201)
280-280: print found
Remove print
(T201)
281-281: print found
Remove print
(T201)
282-282: print found
Remove print
(T201)
283-283: print found
Remove print
(T201)
312-312: print found
Remove print
(T201)
313-313: print found
Remove print
(T201)
315-315: print found
Remove print
(T201)
316-316: print found
Remove print
(T201)
317-317: print found
Remove print
(T201)
318-318: print found
Remove print
(T201)
319-319: print found
Remove print
(T201)
322-322: print found
Remove print
(T201)
325-325: print found
Remove print
(T201)
328-328: print found
Remove print
(T201)
331-331: print found
Remove print
(T201)
332-332: print found
Remove print
(T201)
333-333: print found
Remove print
(T201)
334-334: print found
Remove print
(T201)
335-335: print found
Remove print
(T201)
338-338: print found
Remove print
(T201)
src/event_listener/handlers.py
88-88: print found
Remove print
(T201)
89-89: print found
Remove print
(T201)
248-249: One-line docstring should fit on one line
Reformat to one line
(D200)
src/event_listener/server.py
78-78: Local variable auth is assigned to but never used
Remove assignment to unused variable auth
(F841)
79-79: Local variable registry is assigned to but never used
Remove assignment to unused variable registry
(F841)
108-108: First line of docstring should be in imperative mood: "Basic health check endpoint."
(D401)
🪛 Pylint (3.3.7)
scripts/setup_github_app.py
[refactor] 104-109: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 116-116: Too many statements (85/50)
(R0915)
src/event_listener_tests/test_server.py
[refactor] 213-213: Too few public methods (1/2)
(R0903)
src/event_listener_tests/test_auth.py
[error] 150-150: Instance of 'GitHubAuth' has no 'verify_webhook_signature' member
(E1101)
[error] 161-161: Instance of 'GitHubAuth' has no 'verify_webhook_signature' member
(E1101)
[error] 172-172: Instance of 'GitHubAuth' has no 'verify_webhook_signature' member
(E1101)
src/event_listener_tests/test_processor.py
[refactor] 276-276: Too few public methods (1/2)
(R0903)
src/event_listener_tests/test_handlers.py
[error] 8-18: No name 'IssueCommentHandler' in module 'event_listener.handlers'
(E0611)
[error] 8-18: No name 'IssuesHandler' in module 'event_listener.handlers'
(E0611)
[error] 8-18: No name 'PullRequestHandler' in module 'event_listener.handlers'
(E0611)
[error] 8-18: No name 'PullRequestReviewHandler' in module 'event_listener.handlers'
(E0611)
[error] 8-18: No name 'PushHandler' in module 'event_listener.handlers'
(E0611)
[error] 8-18: No name 'get_orchestrator' in module 'event_listener.handlers'
(E0611)
[error] 266-266: Too many positional arguments for constructor call
(E1121)
[error] 352-356: No name '_orchestrator_instance' in module 'event_listener.handlers'
(E0611)
[error] 367-370: No name '_orchestrator_instance' in module 'event_listener.handlers'
(E0611)
src/event_listener/handlers.py
[refactor] 146-153: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
src/event_listener/server.py
[refactor] 31-31: Too few public methods (0/2)
(R0903)
[refactor] 40-40: Too few public methods (0/2)
(R0903)
[refactor] 48-48: Too few public methods (0/2)
(R0903)
🪛 GitHub Actions: Checks
scripts/setup_github_app.py
[error] 14-16: Ruff T201: print statement found. Remove print usage.
[error] 21-22: Ruff T201: print statement found. Remove print usage.
[error] 32-35: Ruff T201: print statement found. Remove print usage.
[error] 83-94: Ruff T201: print statement found. Remove print usage.
[error] 105-112: Ruff T201: print statement found and E722: bare except used. Remove print and avoid bare except.
[error] 117-136: Ruff T201: Multiple print statements found. Remove print usage.
[error] 140-160: Ruff T201: Multiple print statements found. Remove print usage.
[error] 168-183: Ruff T201: print statements found. Remove print usage.
[error] 187-192: Ruff T201: print statements found. Remove print usage.
[error] 196-201: Ruff T201: print statements found. Remove print usage.
[error] 211-234: Ruff T201: Multiple print statements found. Remove print usage.
src/event_listener_tests/test_auth.py
[error] 237-237: F841: Local variable auth assigned but never used. Remove unused assignment.
src/event_listener/github_auth.py
[error] 18-18: D107: Missing docstring in __init__ method.
[error] 48-48: D107: Missing docstring in __init__ method.
scripts/test_webhooks.py
[error] 34-36: Ruff T201: print statements found. Remove print usage.
[error] 41-52: Ruff T201: print statements found. Remove print usage.
[error] 55-55: Ruff T201: print statement found. Remove print usage.
[error] 254-255: Ruff T201: print statements found. Remove print usage.
[error] 265-270: Ruff T201: print statements found. Remove print usage.
[error] 273-273: Ruff T201: print statement found. Remove print usage.
[error] 277-280: Ruff T201: print statements found. Remove print usage.
[error] 281-283: Ruff T201: print statements found. Remove print usage.
[error] 312-315: Ruff T201: print statements found. Remove print usage.
[error] 316-338: Ruff T201: Multiple print statements found. Remove print usage.
src/event_listener/handlers.py
[error] 88-89: Ruff T201: print statements found. Remove print usage.
[error] 248-249: D200: One-line docstring should fit on one line. Reformat docstring.
src/event_listener/server.py
[error] 78-79: F841: Local variables auth and registry assigned but never used. Remove unused assignments.
[error] 108-108: D401: First line of docstring should be in imperative mood.
🪛 markdownlint-cli2 (0.17.2)
src/event_listener/README.md
108-108: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
113-113: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
115-115: Bare URL used
null
(MD034, no-bare-urls)
118-118: Bare URL used
null
(MD034, no-bare-urls)
119-119: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (10)
src/event_listener_tests/__init__.py (1)
1-1: Package initializer with descriptive docstringThe module-level docstring clearly states the purpose of this test package and marks the directory as a Python package.
src/event_listener/__init__.py (1)
1-3: Docstring and version declaration look good.The module initializes the
event_listenerpackage with a clear docstring and matching__version__.pyproject.toml (2)
20-24: FastAPI microservice dependencies added.The new dependencies (
fastapi,uvicorn[standard],httpx,PyJWT,pydantic-settings) are appropriate for the event listener service. Ensure that the version constraints (~=) align with your support matrix.
57-59: Registerevent_listenerpackage in setuptools.The addition of
"event_listener"under[tool.setuptools.packages]and its mapping in[tool.setuptools.package-dir]correctly exposes the new module for packaging..env.example (1)
15-19: Add environment variables for the event listener service.New placeholders (
GITHUB_INSTALLATION_ID,EVENT_LISTENER_HOST,EVENT_LISTENER_PORT,ORCHESTRATOR_TIMEOUT,MAX_CONCURRENT_EVENTS) align with FastAPI configuration. Please verify these names and defaults match the schema insrc/event_listener/config.py.src/event_listener_tests/fixtures/issue_assigned.json (1)
1-63: Approve issue assigned event fixture.The JSON structure closely follows GitHub’s issues event schema for
"assigned"actions and includes all relevant fields for testing processors and handlers.src/event_listener_tests/fixtures/pull_request_opened.json (1)
1-46: Approve pull request opened event fixture.This fixture accurately mirrors GitHub’s pull_request event payload for
"opened"actions and will support end-to-end testing of the webhook listener.src/event_listener_tests/test_auth.py (1)
141-174: Static-analysis flags indicateverify_webhook_signaturemay be missing
pylintreportsGitHubAuthhas no attributeverify_webhook_signature. If the implementation exists in
event_listener/github_auth.pyyou can ignore this warning, otherwise these three tests will fail at runtime.
Please double-check the production code.🧰 Tools
🪛 Pylint (3.3.7)
[error] 150-150: Instance of 'GitHubAuth' has no 'verify_webhook_signature' member
(E1101)
[error] 161-161: Instance of 'GitHubAuth' has no 'verify_webhook_signature' member
(E1101)
[error] 172-172: Instance of 'GitHubAuth' has no 'verify_webhook_signature' member
(E1101)
src/event_listener_tests/test_handlers.py (1)
95-102:datetimepatching won’t work as intended
datetime.utcnow()returns adatetimeobject — patchingtimestamp.return_value
on the mocked object has no effect becausetimestamp()is called on the return value,
not onutcnowitself. Usefreezegunortime.time()patching instead.with patch("event_listener.handlers.time.time", return_value=1234567890): thread_id = handler.generate_thread_id(event)src/event_listener/github_auth.py (1)
165-178: Well-implemented context manager for authenticated requests.The context manager properly handles client lifecycle and ensures cleanup, which is excellent for preventing resource leaks.
| from event_listener.github_auth import _auth_instance, cleanup_auth | ||
|
|
||
| # Create instance | ||
| auth = get_github_auth() | ||
| assert _auth_instance is not None |
There was a problem hiding this comment.
Drop unused auth variable to silence F841
The instance creation side-effect is all that’s needed for the assertion; the variable itself is never used.
- auth = get_github_auth()
+ _ = get_github_auth()📝 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.
| from event_listener.github_auth import _auth_instance, cleanup_auth | |
| # Create instance | |
| auth = get_github_auth() | |
| assert _auth_instance is not None | |
| from event_listener.github_auth import _auth_instance, cleanup_auth | |
| # Create instance | |
| _ = get_github_auth() | |
| assert _auth_instance is not None |
🧰 Tools
🪛 Ruff (0.11.9)
237-237: Local variable auth is assigned to but never used
Remove assignment to unused variable auth
(F841)
🪛 GitHub Actions: Checks
[error] 237-237: F841: Local variable auth assigned but never used. Remove unused assignment.
🤖 Prompt for AI Agents
In src/event_listener_tests/test_auth.py around lines 234 to 238, the variable
'auth' is assigned the result of get_github_auth() but never used, causing an
unused variable warning (F841). Remove the assignment to 'auth' and just call
get_github_auth() directly to trigger the instance creation side-effect needed
for the assertion.
| # Optional GitHub Enterprise Configuration | ||
| github_api_url: str = "https://api.github.com" | ||
|
|
||
| # Security | ||
| allowed_github_ips: Optional[str] = None | ||
|
|
||
| @field_validator("github_app_private_key") | ||
| @classmethod | ||
| def validate_private_key(cls, v): | ||
| """Ensure private key is properly formatted.""" | ||
| if not v.startswith("-----BEGIN"): | ||
| # Try to load from file if it's a path | ||
| if os.path.exists(v): | ||
| with open(v) as f: | ||
| return f.read() | ||
| raise ValueError("Private key must be PEM formatted or a valid file path") | ||
| return v | ||
|
|
||
| @field_validator("allowed_github_ips") | ||
| @classmethod | ||
| def parse_allowed_ips(cls, v): | ||
| """Parse comma-separated IP list.""" | ||
| if v: | ||
| return [ip.strip() for ip in v.split(",")] | ||
| return None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
allowed_github_ips type mismatch
The field is annotated as Optional[str] but the validator converts it to list[str].
Either update the annotation or return a string to keep typing (and IDE autocomplete) correct.
- allowed_github_ips: Optional[str] = None
+ from typing import Optional, List
+ allowed_github_ips: Optional[List[str]] = NoneCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/event_listener/config.py between lines 32 and 57, the allowed_github_ips
field is annotated as Optional[str] but the validator returns a list of strings,
causing a type mismatch. To fix this, update the type annotation of
allowed_github_ips to Optional[list[str]] to match the validator's output,
ensuring consistency between the declared type and the returned value.
| def test_webhook_endpoint(): | ||
| """Test if the webhook endpoint is accessible.""" | ||
| try: | ||
| import requests | ||
|
|
||
| response = requests.get("http://localhost:8000/health", timeout=2) | ||
| if response.status_code == 200: | ||
| print("✓ Event Listener service is running") | ||
| return True | ||
| else: | ||
| print("✗ Event Listener service returned error") | ||
| return False | ||
| except: | ||
| print("✗ Event Listener service is not running") | ||
| print(" Start it with: uvicorn src.event_listener.server:app --reload") | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace bare except with explicit Exception and remove dead else branch
The current construct hides unexpected errors and violates Ruff E722.
- try:
- import requests
- response = requests.get("http://localhost:8000/health", timeout=2)
- if response.status_code == 200:
- print("✓ Event Listener service is running")
- return True
- else:
- print("✗ Event Listener service returned error")
- return False
- except:
+ try:
+ import requests
+ response = requests.get("http://localhost:8000/health", timeout=2)
+ if response.status_code == 200:
+ print("✓ Event Listener service is running")
+ return True
+ print("✗ Event Listener service returned error")
+ return False
+ except Exception as exc:
print("✗ Event Listener service is not running")
print(" Start it with: uvicorn src.event_listener.server:app --reload")
- return False
+ return False📝 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.
| def test_webhook_endpoint(): | |
| """Test if the webhook endpoint is accessible.""" | |
| try: | |
| import requests | |
| response = requests.get("http://localhost:8000/health", timeout=2) | |
| if response.status_code == 200: | |
| print("✓ Event Listener service is running") | |
| return True | |
| else: | |
| print("✗ Event Listener service returned error") | |
| return False | |
| except: | |
| print("✗ Event Listener service is not running") | |
| print(" Start it with: uvicorn src.event_listener.server:app --reload") | |
| return False | |
| def test_webhook_endpoint(): | |
| """Test if the webhook endpoint is accessible.""" | |
| try: | |
| import requests | |
| response = requests.get("http://localhost:8000/health", timeout=2) | |
| if response.status_code == 200: | |
| print("✓ Event Listener service is running") | |
| return True | |
| print("✗ Event Listener service returned error") | |
| return False | |
| except Exception as exc: | |
| print("✗ Event Listener service is not running") | |
| print(" Start it with: uvicorn src.event_listener.server:app --reload") | |
| return False |
🧰 Tools
🪛 Ruff (0.11.9)
105-105: print found
Remove print
(T201)
108-108: print found
Remove print
(T201)
110-110: Do not use bare except
(E722)
111-111: print found
Remove print
(T201)
112-112: print found
Remove print
(T201)
🪛 Pylint (3.3.7)
[refactor] 104-109: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🪛 GitHub Actions: Checks
[error] 105-112: Ruff T201: print statement found and E722: bare except used. Remove print and avoid bare except.
🤖 Prompt for AI Agents
In scripts/setup_github_app.py around lines 98 to 113, replace the bare except
clause with except Exception to avoid catching system-exiting exceptions and
improve error handling clarity. Also, remove the else branch after the if
checking response.status_code == 200, as it is unnecessary; instead, handle the
non-200 case directly without an else. This will make the code cleaner and
comply with Ruff E722.
| if not env_path.exists(): | ||
| if env_example_path.exists(): | ||
| print("Creating .env file from .env.example...") | ||
| env_example_path.rename(env_path) | ||
| else: | ||
| print("ERROR: Neither .env nor .env.example found!") |
There was a problem hiding this comment.
Copy .env.example instead of moving it
Path.rename deletes the template, making subsequent bootstrap steps and documentation harder.
Use shutil.copy so the example stays intact.
- env_example_path.rename(env_path)
+ import shutil
+ shutil.copy(env_example_path, env_path)📝 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.
| if not env_path.exists(): | |
| if env_example_path.exists(): | |
| print("Creating .env file from .env.example...") | |
| env_example_path.rename(env_path) | |
| else: | |
| print("ERROR: Neither .env nor .env.example found!") | |
| if not env_path.exists(): | |
| if env_example_path.exists(): | |
| print("Creating .env file from .env.example...") | |
| import shutil | |
| shutil.copy(env_example_path, env_path) | |
| else: | |
| print("ERROR: Neither .env nor .env.example found!") |
🧰 Tools
🪛 Ruff (0.11.9)
32-32: print found
Remove print
(T201)
35-35: print found
Remove print
(T201)
🪛 GitHub Actions: Checks
[error] 32-35: Ruff T201: print statement found. Remove print usage.
🤖 Prompt for AI Agents
In scripts/setup_github_app.py around lines 30 to 35, the code uses Path.rename
to create the .env file from .env.example, which removes the original
.env.example file. Replace Path.rename with shutil.copy to copy the .env.example
file to .env instead, preserving the original template for future use and
documentation.
| "action": "opened", | ||
| "pull_request": {"number": 42}, | ||
| "repository": {"id": 12345, "full_name": "test/repo"}, | ||
| } | ||
|
|
||
| # Mock the first handler to handle the event | ||
| mock_handler = registry._handlers[0] | ||
| mock_handler.can_handle = AsyncMock(return_value=True) | ||
| mock_handler.handle = AsyncMock(return_value={"status": "handled"}) | ||
|
|
||
| result = await registry.handle_event("pull_request", event_data) | ||
|
|
||
| assert result["status"] == "handled" | ||
| assert event_data["X-GitHub-Event"] == "pull_request" | ||
|
|
There was a problem hiding this comment.
X-GitHub-Event assertion may raise KeyError
event_data is initialised without the header, but the test later asserts its presence.
Either include the key up-front or assert on the returned value instead of mutating the input.
event_data = {
"X-GitHub-Event": "pull_request",
"action": "opened",
...
}📝 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.
| "action": "opened", | |
| "pull_request": {"number": 42}, | |
| "repository": {"id": 12345, "full_name": "test/repo"}, | |
| } | |
| # Mock the first handler to handle the event | |
| mock_handler = registry._handlers[0] | |
| mock_handler.can_handle = AsyncMock(return_value=True) | |
| mock_handler.handle = AsyncMock(return_value={"status": "handled"}) | |
| result = await registry.handle_event("pull_request", event_data) | |
| assert result["status"] == "handled" | |
| assert event_data["X-GitHub-Event"] == "pull_request" | |
| event_data = { | |
| "X-GitHub-Event": "pull_request", | |
| "action": "opened", | |
| "pull_request": {"number": 42}, | |
| "repository": {"id": 12345, "full_name": "test/repo"}, | |
| } | |
| # Mock the first handler to handle the event | |
| mock_handler = registry._handlers[0] | |
| mock_handler.can_handle = AsyncMock(return_value=True) | |
| mock_handler.handle = AsyncMock(return_value={"status": "handled"}) | |
| result = await registry.handle_event("pull_request", event_data) | |
| assert result["status"] == "handled" | |
| assert event_data["X-GitHub-Event"] == "pull_request" |
🤖 Prompt for AI Agents
In src/event_listener_tests/test_handlers.py around lines 300 to 314, the test
asserts the presence of the "X-GitHub-Event" key in event_data, but this key is
not initially set, which can cause a KeyError. To fix this, either add the
"X-GitHub-Event" key with the appropriate value to event_data before the
assertion or modify the test to assert on the returned result or another
structure that includes this key without mutating the input event_data.
| print(f"Invoking orchestrator with state: {state}") | ||
| print(f"Thread ID: {thread_id}") | ||
|
|
There was a problem hiding this comment.
Remove debug print statements
Replace the print statements with proper logging to avoid console output in production.
Apply this diff to fix the issue:
- print(f"Invoking orchestrator with state: {state}")
- print(f"Thread ID: {thread_id}")
+ logger.debug(f"Invoking orchestrator with state: {state}")
+ logger.debug(f"Thread ID: {thread_id}")📝 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.
| print(f"Invoking orchestrator with state: {state}") | |
| print(f"Thread ID: {thread_id}") | |
| logger.debug(f"Invoking orchestrator with state: {state}") | |
| logger.debug(f"Thread ID: {thread_id}") |
🧰 Tools
🪛 Ruff (0.11.9)
88-88: print found
Remove print
(T201)
89-89: print found
Remove print
(T201)
🪛 GitHub Actions: Checks
[error] 88-89: Ruff T201: print statements found. Remove print usage.
🤖 Prompt for AI Agents
In src/event_listener/handlers.py around lines 88 to 90, replace the debug print
statements with proper logging calls using the configured logger. Use
appropriate log levels such as info or debug to record the state and thread ID
instead of printing directly to the console. This will prevent unwanted console
output in production environments.
| auth = get_github_auth() | ||
| registry = get_handler_registry() |
There was a problem hiding this comment.
Remove unused variable assignments
The auth and registry variables are assigned but never used.
Apply this diff to remove the unused assignments:
- # Initialize components
- auth = get_github_auth()
- registry = get_handler_registry()
+ # Initialize components
+ get_github_auth()
+ get_handler_registry()📝 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.
| auth = get_github_auth() | |
| registry = get_handler_registry() | |
| # Initialize components | |
| get_github_auth() | |
| get_handler_registry() |
🧰 Tools
🪛 Ruff (0.11.9)
78-78: Local variable auth is assigned to but never used
Remove assignment to unused variable auth
(F841)
79-79: Local variable registry is assigned to but never used
Remove assignment to unused variable registry
(F841)
🪛 GitHub Actions: Checks
[error] 78-79: F841: Local variables auth and registry assigned but never used. Remove unused assignments.
🤖 Prompt for AI Agents
In src/event_listener/server.py around lines 78 to 79, the variables 'auth' and
'registry' are assigned values but never used afterward. Remove these two
variable assignments entirely to clean up the code and avoid unused variable
warnings.
| @app.post("/github/events", response_model=EventResponse) | ||
| async def github_webhook( | ||
| request: Request, | ||
| x_github_event: str = Header(None), | ||
| x_github_delivery: str = Header(None), | ||
| ): | ||
| """Receive and process GitHub webhook events. | ||
|
|
||
| Args: | ||
| request: FastAPI request object | ||
| x_github_event: GitHub event type header | ||
| x_github_delivery: GitHub delivery ID header | ||
|
|
||
| Returns: | ||
| EventResponse: Processing response | ||
| """ | ||
| # Validate headers | ||
| if not x_github_event: | ||
| raise HTTPException(status_code=400, detail="Missing X-GitHub-Event header") | ||
|
|
||
| if not x_github_delivery: | ||
| raise HTTPException(status_code=400, detail="Missing X-GitHub-Delivery header") | ||
|
|
||
| # Get raw payload | ||
| payload = await request.body() | ||
|
|
||
| # Parse JSON payload | ||
| try: | ||
| event_data = json.loads(payload) | ||
| except json.JSONDecodeError as e: | ||
| logger.error(f"Invalid JSON payload: {str(e)}") | ||
| raise HTTPException(status_code=400, detail="Invalid JSON payload") | ||
|
|
||
| # Log event receipt | ||
| repo_name = event_data.get("repository", {}).get("full_name", "unknown") | ||
| logger.info( | ||
| f"Received {x_github_event} event for {repo_name} " | ||
| f"(delivery: {x_github_delivery})" | ||
| ) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add webhook signature verification for security
The webhook endpoint is missing signature verification, which is a critical security requirement for GitHub webhooks. This allows anyone to send fake webhook events to your service.
You need to verify the webhook signature before processing the event. The GitHubAuth class should have a method for this.
Let me check if the verification method exists:
Add signature verification after getting the payload:
# Get raw payload
payload = await request.body()
+ # Verify webhook signature
+ auth = get_github_auth()
+ signature = request.headers.get("X-Hub-Signature-256")
+ if not signature:
+ raise HTTPException(status_code=401, detail="Missing webhook signature")
+
+ if not auth.verify_webhook_signature(payload, signature):
+ raise HTTPException(status_code=401, detail="Invalid webhook signature")
+
# Parse JSON payload🏁 Script executed:
#!/bin/bash
# Search for webhook signature verification method in GitHubAuth
ast-grep --pattern 'def verify_webhook_signature($$$)'Length of output: 56
I’ll locate the GitHubAuth class and list its available methods to confirm if a signature-verification function exists:
🏁 Script executed:
#!/bin/bash
set -e
# Find the file defining GitHubAuth
class_file=$(rg -l "class GitHubAuth" .)
echo "GitHubAuth class defined in: $class_file"
# List all methods in that class file
rg -n "^def " "$class_file"Length of output: 319
I’ll list all methods (including indented ones) in the GitHubAuth file and look for any “signature” references to confirm whether a verification function exists:
🏁 Script executed:
#!/bin/bash
set -e
# List all function definitions in GitHubAuth (including nested)
rg -n "^\s*def " src/event_listener/github_auth.py || echo "No methods found"
# Search for any signature-related code
rg -n "signature" src/event_listener/github_auth.py || echo "No signature references found"Length of output: 361
Add webhook signature verification to prevent unauthorized requests
The GitHub webhook endpoint currently accepts any payload without verifying its HMAC signature—a critical security risk. Since there’s no existing verify_webhook_signature (or similar) method in GitHubAuth, you must:
• Implement a signature‐verification helper in GitHubAuth (or a new module) that uses your GITHUB_WEBHOOK_SECRET to compute an HMAC SHA-256 and safely compare it against the X-Hub-Signature-256 header.
• Invoke this helper immediately after reading the raw payload in github_webhook()—before JSON parsing or any business logic.
Example additions:
—in src/event_listener/github_auth.py
import os, hmac, hashlib
class GitHubAuth:
...
+ def verify_webhook_signature(self, payload: bytes, signature_header: str) -> bool:
+ secret = os.getenv("GITHUB_WEBHOOK_SECRET", "")
+ mac = hmac.new(secret.encode(), payload, hashlib.sha256)
+ expected = "sha256=" + mac.hexdigest()
+ return hmac.compare_digest(expected, signature_header)—in src/event_listener/server.py (inside github_webhook)
# Get raw payload
payload = await request.body()
+ # Verify webhook signature
+ signature = request.headers.get("X-Hub-Signature-256")
+ if not signature:
+ raise HTTPException(status_code=400, detail="Missing X-Hub-Signature-256 header")
+ auth = get_github_auth()
+ if not auth.verify_webhook_signature(payload, signature):
+ raise HTTPException(status_code=401, detail="Invalid webhook signature")
+
# Parse JSON payloadThis ensures only GitHub-signed events are accepted.
🤖 Prompt for AI Agents
In src/event_listener/server.py lines 202 to 241, the github_webhook endpoint
lacks verification of the GitHub webhook signature, which is a security risk. To
fix this, first implement a helper method in GitHubAuth (or a new module) that
uses the GITHUB_WEBHOOK_SECRET to compute the HMAC SHA-256 of the raw payload
and safely compare it to the X-Hub-Signature-256 header. Then, in the
github_webhook function, immediately after reading the raw payload but before
JSON parsing, retrieve the X-Hub-Signature-256 header and call the verification
helper. If verification fails, raise an HTTP 401 error to reject unauthorized
requests. This ensures only valid GitHub-signed events are processed.
| GITHUB_APP_ID=123456 | ||
| GITHUB_INSTALLATION_ID=12345678 | ||
| GITHUB_APP_PRIVATE_KEY="-----BEGIN RSA PRIVATE KEY----- | ||
| ...your key content... | ||
| -----END RSA PRIVATE KEY-----" | ||
| ``` |
There was a problem hiding this comment.
Do not embed raw private keys directly in .env examples
Copy-pasting the PEM into an environment file is error-prone (escaping newlines) and encourages storing secrets in source control.
Recommend either:
- Pointing to a file on disk
- Reading from a secrets manager
-GITHUB_APP_PRIVATE_KEY="-----BEGIN RSA PRIVATE KEY-----
-...your key content...
-----END RSA PRIVATE KEY-----"
+# Recommended: path to the PEM file
+GITHUB_APP_PRIVATE_KEY_PATH=/run/secrets/github_app.pem
+
+# Or, if you really need the inline value, document that it **must never** be
+# committed and that newlines must be preserved exactly.📝 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.
| GITHUB_APP_ID=123456 | |
| GITHUB_INSTALLATION_ID=12345678 | |
| GITHUB_APP_PRIVATE_KEY="-----BEGIN RSA PRIVATE KEY----- | |
| ...your key content... | |
| -----END RSA PRIVATE KEY-----" | |
| ``` | |
| GITHUB_APP_ID=123456 | |
| GITHUB_INSTALLATION_ID=12345678 | |
| # Recommended: path to the PEM file | |
| GITHUB_APP_PRIVATE_KEY_PATH=/run/secrets/github_app.pem | |
| # Or, if you really need the inline value, document that it **must never** be | |
| # committed and that newlines must be preserved exactly. |
🤖 Prompt for AI Agents
In src/event_listener/README.md around lines 133 to 138, the example shows
embedding a raw private key directly in the .env file, which is error-prone and
insecure. Update the example to remove the raw PEM key and instead demonstrate
setting an environment variable that points to a file path containing the
private key or instructions to read it from a secrets manager. Add guidance in
the README to avoid storing raw keys in source control and recommend secure key
management practices.
| #!/usr/bin/env python3 | ||
| """Test script for GitHub Event Listener webhook endpoints. | ||
|
|
||
| This script provides easy testing of various webhook events using curl commands. | ||
| """ | ||
|
|
||
| import json |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Silence Ruff T201 by replacing print statements with logging or file-level ignore
The cascade of CI failures is due to the many print calls.
Fastest unblock:
#!/usr/bin/env python3
+# ruff: noqa: T201Better long-term:
import logging
logger = logging.getLogger(__name__)
…
logger.info("🚀 Testing %s event...", event_type)Choose one and keep CI green.
🤖 Prompt for AI Agents
In scripts/test_webhooks.py at the beginning of the file (lines 1 to 7), replace
all print statements with logging calls to silence Ruff T201 warnings and
prevent CI failures. Import the logging module, configure a logger for the
module, and change print calls to logger.info or an appropriate logging level.
Alternatively, add a file-level ignore comment for Ruff T201 if you prefer to
keep print statements, but using logging is the recommended long-term solution.
| try: | ||
| with open(path) as f: | ||
| content = f.read() | ||
| if "BEGIN RSA PRIVATE KEY" not in content: |
There was a problem hiding this comment.
Probably shouldn't assume RSA here
| try: | ||
| import requests | ||
|
|
||
| response = requests.get("http://localhost:8000/health", timeout=2) |
There was a problem hiding this comment.
Probably shouldn't assume port 8000 (or even localhost...?)
| # Step 1: Create GitHub App | ||
| print_step(1, "Create a GitHub App") | ||
| print("Go to: https://github.com/settings/apps/new") | ||
| print("\nUse these settings:") | ||
| print(" Name: AI Nexus Event Listener (or your choice)") | ||
| print(" Homepage URL: http://localhost:8000") | ||
| print(" Webhook URL: http://your-domain:8000/github/events") | ||
| print(" (Use ngrok for testing: ngrok http 8000)") | ||
| print(" Webhook secret: Generate a random string") | ||
| print("\nPermissions needed:") | ||
| print(" Repository permissions:") | ||
| print(" - Contents: Read") | ||
| print(" - Issues: Read & Write") | ||
| print(" - Pull requests: Read & Write") | ||
| print(" - Actions: Read (optional)") | ||
| print(" Subscribe to events:") | ||
| print(" - Issues") | ||
| print(" - Issue comment") | ||
| print(" - Pull request") | ||
| print(" - Pull request review") | ||
| print(" - Pull request review comment") |
There was a problem hiding this comment.
I'd rather see this in a MD and maybe just cat it here
There was a problem hiding this comment.
...ok, this is all interactive, so that might justify the way it's done here. But I could see wanting to review this documentation in a different context
| "curl", | ||
| "-X", | ||
| "POST", | ||
| "http://localhost:8000/github/events", |
There was a problem hiding this comment.
Hard-coded port 8000 again
|
|
||
| **Request Comment:** | ||
| {comment_body} | ||
| **Comment URL:** {comment_url} |
There was a problem hiding this comment.
There are tools for things like querying or posting a comment. Github returns a lot of URLs like this for convenience (?), but I think it's going to be confusing if we mix using tools (which go through an SDK) and URLs which do the equivalent thing.
It's likely more efficient to hit a URL directly than to go through a tool+SDK, so maybe there's a good argument for paying attention to these (it could likely lead to fewer LLM invocations and fewer API queries).
But I doubt giving the LLM a URL is the way to go about this. It's probably confusing, and the LLM would need to do things like add auth headers, use the right HTTP verb, etc. So I suggest we not pass URLs as part of LLM prompts; if we use them, we should do so directly in code where we can be most efficient about it.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores