feat(logging): migrate to structured JSON logging with context tracking #968
Conversation
Migrate from text-based logging with regex parsing to structured JSON logging. New components: - webhook_server/utils/context.py: ContextVar-based WebhookContext for request tracking - webhook_server/utils/structured_logger.py: JSON log writer with structured output - webhook_server/tests/test_context.py: Tests for context system - webhook_server/tests/test_structured_logger.py: Tests for structured logger - webhook_server/tests/test_log_viewer.py: Tests for log viewer Modified components: - app.py: Integrate WebhookContext and structured logging - github_api.py: Use structured logger throughout - All handlers: Migrated to structured logging - log_parser.py: Updated to parse JSON logs - log_viewer.py: Enhanced to handle structured logs - app_utils.py, helpers.py: Logging improvements - All test files: Updated for structured logging Benefits: - Eliminates regex-based log parsing - Improved log searchability and filtering - Better structured data extraction - Simplified log analysis
|
Warning Rate limit exceeded@myakove has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughCreates a ContextVar-backed WebhookContext and JSONL structured logging; integrates ctx into GitHub API and handlers to track per-step lifecycle and errors; converts log viewer to async/JSON-first streaming; app ensures summary/log write and context clearing for every webhook run. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
CRITICAL: Verify ContextVar isolation across concurrent async tasks to prevent cross-request context leakage. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Fix all issues with AI Agents 🤖
In @webhook_server/libs/handlers/owners_files_handler.py:
- Around line 17-18: The list_changed_files function currently swallows failures
and asyncio.CancelledError by using a broad except and returning an empty list;
change it to let CancelledError propagate (don’t catch asyncio.CancelledError),
and on any git run_command failure (success is False) or unexpected exception,
raise a clear exception instead of logging and returning [] so callers/CI fail
fast; use run_command’s result to include detailed error info in the raised
exception or log (e.g., include stdout/stderr/return code) and only catch and
re-raise non-cancellation exceptions if you need to add context.
In @webhook_server/libs/handlers/pull_request_review_handler.py:
- Around line 9-18: The class uses WebhookContext in a runtime-evaluated
annotation and __init__ lacks a return type; update
PullRequestReviewHandler.__init__ to annotate the ctx field with a string
literal (e.g., self.ctx: "WebhookContext" | None = ...) or enable postponed
evaluation via from __future__ import annotations, and add the explicit return
annotation -> None on __init__; optionally wrap the body of
process_pull_request_review_webhook_data in a try/finally so
complete_step("pr_review_handler") always runs when self.ctx exists.
In @webhook_server/libs/handlers/push_handler.py:
- Around line 29-55: process_push_webhook_data currently always calls
self.ctx.complete_step("push_handler") even when upload_to_pypi or
runner_handler.run_build_container raise; update the exception handlers so they
call self.ctx.fail_step("push_handler", ex, traceback) (or a new secondary step
like "push_handler.pypi" / "push_handler.container" with start/complete/fail
calls) instead of swallowing errors—specifically modify the except blocks around
await self.upload_to_pypi(...) and await
self.runner_handler.run_build_container(...) to capture the exception and
traceback and call ctx.fail_step with those details (or create and manage
per-operation steps) before re-raising or returning, so the JSON workflow
reflects failures accurately.
In @webhook_server/libs/log_parser.py:
- Around line 400-422: Change the two exception handlers in parse_json_log_file
to call self.logger.exception(...) instead of self.logger.error(...);
specifically replace the self.logger.error(f"Failed to read JSON log file
{file_path}: {e}") in the OSError except block and the
self.logger.error(f"Failed to decode JSON log file {file_path}: {e}") in the
UnicodeDecodeError except block with self.logger.exception(...) calls that
include the same descriptive message so the traceback is automatically logged.
In @webhook_server/tests/test_context.py:
- Around line 19-32: Add a concrete return type to the inner mock function so
mypy strict accepts it: change the signature of mock_now to include parameter
and return types (e.g. def mock_now(tz: Optional[datetime.tzinfo] = None) ->
datetime:) and import Optional and tzinfo/ datetime as needed; keep the function
body unchanged and ensure the patched target
(patch("webhook_server.utils.context.datetime")) still uses mock_now as before.
In @webhook_server/tests/test_github_api.py:
- Around line 1498-1500: Reuse the existing info_calls list instead of creating
a new info_completion_calls: remove the redundant assignment that builds
info_completion_calls from mock_logger.info.call_args_list and use the
previously created info_calls variable in the assertion that checks for
"deletion event (skipped)". Update the assertion to iterate over info_calls
(e.g., assert any("deletion event (skipped)" in call.lower() for call in
info_calls)) so the test uses the single shared collection of info log call
strings.
In @webhook_server/tests/test_log_parser.py:
- Around line 1256-1367: Test helpers use naive datetimes, missing type hints,
missing return type, and perform blocking I/O; update all datetime.datetime(...)
calls in these tests to include tzinfo=datetime.UTC, add annotations to the
collect_entries helper (async_gen: AsyncIterator[LogEntry], max_entries: int)
and its return type -> None, and replace the blocking file append using
open(...) with an asyncio.to_thread call (e.g., offload current_log.write_text
or the append operation) so monitor_log_directory and collect_entries operate
with UTC-aware timestamps, proper AsyncIterator typing, explicit None return,
and non-blocking file writes.
In @webhook_server/tests/test_log_viewer.py:
- Around line 18-31: The test test_stream_json_log_entries_no_log_directory has
an unused tmp_path parameter causing ARG002; to fix, make a trivial reference to
tmp_path (e.g., assert tmp_path is not None or add a no-op comment like
"tmp_path # noqa: ARG002") inside that test so the parameter is used and
linting passes, keeping the rest of the test logic unchanged.
In @webhook_server/tests/test_structured_logger.py:
- Around line 15-405: The lint issues are minor: replace positional boolean
patches with explicit keyword usage (e.g., in test_write_log_without_fcntl and
test_write_log_uses_file_locking change @patch("...HAS_FCNTL", False) to use
explicit new= parameter) and mark the unused tmp_path in
test_different_dates_create_different_files as intentionally used (e.g., assign
_ = tmp_path) so the fixture name remains but Ruff won’t flag ARG002; locate
these in the tests named test_write_log_without_fcntl,
test_write_log_uses_file_locking, and
test_different_dates_create_different_files and apply the two small edits.
In @webhook_server/utils/structured_logger.py:
- Around line 92-117: StructuredLogWriter.write_log currently always uses
datetime.now(UTC) for completed_at which can drift from
WebhookContext.completed_at set elsewhere; change write_log to prefer
context.completed_at (from WebhookContext) as the source of truth and only fall
back to datetime.now(UTC) if context.completed_at is None, then compute
duration_ms using that chosen completed_at and context.started_at, and write
those values into context_dict["timing"] before serializing; update references
to context.to_dict(), WebhookContext.completed_at, write_webhook_log, and
log_webhook_summary to ensure consistency without mutating the original context.
In @webhook_server/web/log_viewer.py:
- Around line 814-857: The current _stream_json_log_entries reads each file with
readlines(), which loads the entire file into memory; change it to stream lines
into a bounded collections.deque sized by the remaining entries needed
(remaining = max_entries - total_yielded) while iterating the file line-by-line,
then iterate reversed(deque) to process newest-first and call
self.log_parser.get_raw_json_entry on each line; update total_yielded and break
out when reaching max_entries, and keep the existing exception handling around
the file open/processing loop.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
webhook_server/libs/handlers/issue_comment_handler.py (1)
41-63: Context step tracking is hooked in correctly; consider marking failures explicitlySeverity: LOW.
Wiringself.ctxfromgithub_webhook.ctxand bracketingprocess_comment_webhook_datawithstart_step/complete_step("issue_comment_handler")(including the edited/deleted and welcome-message early exits) is consistent with the WebhookContext API and avoids leaking an “open” step. If you want failures inuser_commandsto be visible at the workflow level, a follow-up enhancement could be to callctx.fail_step("issue_comment_handler", ...)when any gathered command raises instead of always marking the step as completed, but that’s optional and doesn’t block this PR.Also applies to: 65-85, 114-116
webhook_server/app.py (1)
378-455: HIGH: Re-raiseasyncio.CancelledErrorexplicitly inprocess_with_error_handlingThe inner async function catches cancellation signals via the broad
except Exception as ex:handler at line 424. This violates the repo guideline: "Always re-raiseasyncio.CancelledError- never suppress it in async Python exception handlers."In Python 3.10 and earlier,
CancelledErrorinherits fromException, so it gets caught and logged as a generic failure instead of propagating cancellation. In 3.11+, it's aBaseExceptionsubclass, so it passes through—but explicit handling prevents confusion and ensures correct shutdown semantics across versions.Add an explicit
except asyncio.CancelledError:handler before the genericExceptioncatch, log it (optional), and re-raise:Example fix
except RepositoryNotFoundInConfigError: # Repository-specific error - not exceptional, log as error not exception _logger.error(f"{_log_context} Repository not found in configuration") ctx.success = False ctx.error = { "type": "RepositoryNotFoundInConfigError", "message": "Repository not found in configuration", "traceback": "", } except asyncio.CancelledError: # Preserve cooperative cancellation semantics _logger.info(f"{_log_context} Webhook processing cancelled") raise except (httpx.ConnectError, httpx.RequestError, requests.exceptions.ConnectionError) as ex: # Network/connection errors - can be transient _logger.exception(f"{_log_context} API connection error - check network connectivity") ctx.success = False ctx.error = { "type": type(ex).__name__, "message": str(ex), "traceback": traceback.format_exc(), } except Exception as ex: # Catch-all for unexpected errors _logger.exception(f"{_log_context} Unexpected error in background webhook processing") ctx.success = False ctx.error = { "type": type(ex).__name__, "message": str(ex), "traceback": traceback.format_exc(), }webhook_server/libs/handlers/pull_request_handler.py (1)
805-907: HIGH: Re-raiseasyncio.CancelledErrorincheck_if_can_be_mergedto preserve task cancellation semanticsThe broad
except Exception as exat line 897 will swallowasyncio.CancelledError, violating the explicit repo guideline that cancellation must never be suppressed in async handlers. SuppressingCancelledErrorbreaks cooperative cancellation semantics and prevents the framework from cleanly shutting down tasks—this can mask resource leaks and cause hung connections.Separate the
CancelledErrorhandler (re-raise immediately after optional context logging) from the generic exception handler (log, cleanup, and record failure context). Also switch tologger.exception()to capture the traceback automatically.Corrected error handling pattern
try: self.logger.info(f"{self.log_prefix} Check if {CAN_BE_MERGED_STR}.") await self.check_run_handler.set_merge_check_in_progress() # ... existing logic ... - except Exception as ex: - self.logger.error( - f"{self.log_prefix} Failed to check if can be merged, set check run to {FAILURE_STR} {ex}" - ) - _err = "Failed to check if can be merged, check logs" - output["text"] = _err - await self.labels_handler._remove_label(pull_request=pull_request, label=CAN_BE_MERGED_STR) - await self.check_run_handler.set_merge_check_failure(output=output) - if self.ctx: - self.ctx.fail_step("check_merge_eligibility", ex, traceback.format_exc()) + except asyncio.CancelledError: + if self.ctx: + self.ctx.fail_step( + "check_merge_eligibility", + asyncio.CancelledError(), + traceback.format_exc(), + ) + raise + except Exception as ex: + self.logger.exception( + f"{self.log_prefix} Failed to check if can be merged, set check run to {FAILURE_STR}" + ) + _err = "Failed to check if can be merged, check logs" + output["text"] = _err + await self.labels_handler._remove_label(pull_request=pull_request, label=CAN_BE_MERGED_STR) + await self.check_run_handler.set_merge_check_failure(output=output) + if self.ctx: + self.ctx.fail_step("check_merge_eligibility", ex, traceback.format_exc())(
asyncioandtracebackare already imported.)
Enhance structured logging with pretty-printed JSON format and improve log viewer file ordering for better user experience. Changes: - structured_logger.py: Pretty-print JSON logs with 2-space indentation and blank line separators between entries for improved readability - log_viewer.py: Prioritize JSON webhook files over internal logs in file listing to show primary data source first
Updated test_structured_logger.py to handle pretty-printed JSON format with blank line separators instead of JSONL. Fixed test_log_api.py to properly mock file operations and stat() method for error handling test.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webhook_server/tests/test_log_api.py (1)
443-470: MEDIUM: Test name no longer reflects the actual failure mode being tested.The test
test_stream_log_entries_parse_errornow simulates anopen()failure rather than a parse error. The name is misleading since the test exercises I/O exception handling, not JSON/log parsing failures.Consider renaming to
test_stream_log_entries_file_read_errorortest_stream_log_entries_io_errorto accurately describe the failure path being tested.Suggested rename
- def test_stream_log_entries_parse_error(self, controller): - """Test log entries loading with parse error.""" + def test_stream_log_entries_file_read_error(self, controller): + """Test log entries loading with file read error."""
♻️ Duplicate comments (3)
webhook_server/tests/test_structured_logger.py (1)
15-672: Comprehensive test coverage for structured logging.The test suite thoroughly covers
StructuredLogWriterandwrite_webhook_log:
- Initialization and log directory creation
- Date-based log file paths
- JSON serialization with timing, workflow steps, PR details, and errors
- File locking behavior on platforms with/without
fcntl- Error handling and graceful recovery
- Unicode content handling
- Temp file cleanup scenarios
The previous review already noted the minor lint issues (boolean positional in
@patchand unusedtmp_pathfixture). Those are cosmetic and don't affect test correctness.webhook_server/web/log_viewer.py (1)
813-855: Memory concern withreadlines()already noted in prior review.The
readlines()call at line 845 loads the entire file into memory before reversing, which can spike memory for large JSON log files. This was flagged in a previous review - consider using a boundeddequesimilar to_stream_log_entriesto maintain consistent memory-efficient streaming behavior.Memory-efficient alternative
try: with open(log_file, encoding="utf-8") as f: - # Read lines in reverse for newest-first ordering - lines = f.readlines() - for line in reversed(lines): + remaining_capacity = max_entries - total_yielded + buffer: deque[dict[str, Any]] = deque(maxlen=remaining_capacity) + + for line in f: if total_yielded >= max_entries: break - data = self.log_parser.get_raw_json_entry(line) if data: - yield data - total_yielded += 1 + buffer.append(data) + + for data in reversed(buffer): + if total_yielded >= max_entries: + break + yield data + total_yielded += 1webhook_server/utils/structured_logger.py (1)
93-115: Timing source-of-truth concern previously noted.The implementation always calculates
completed_atlocally at line 107, even ifcontext.completed_atwas already set by the caller. This can cause minor timing drift between the JSON log and other logging outputs. The previous review suggested preferringcontext.completed_atwhen set.This is LOW severity since the drift is typically milliseconds, but worth addressing for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
webhook_server/tests/test_log_api.pywebhook_server/tests/test_structured_logger.pywebhook_server/utils/structured_logger.pywebhook_server/web/log_viewer.py
🧰 Additional context used
📓 Path-based instructions (4)
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Never use defensive checks (hasattr, if object:, if value is not None) on required parameters passed to__init__()- required parameters are always provided in this self-contained server application
Defensive checks ARE acceptable in destructors (__del__) because they may be called during failed initialization
Defensive checks ARE acceptable for lazy initialization where attributes explicitly start asNone- check is legitimate for uninitialized attributes
Never use defensive checks on known library versions controlled inpyproject.toml- e.g., PyGithub >=2.4.0 methods are guaranteed to exist
Never use defensive checks on webhook payload fields guaranteed by GitHub webhook specification - e.g.,user.node_id,user.type,senderalways exist in webhook payloads
Never use hasattr() for type discrimination - useisinstance()instead for type checking
Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
ALL PyGithub method calls MUST be wrapped withasyncio.to_thread()to avoid blocking the FastAPI event loop - including.get_*(),.create_*(),.edit(),.add_to_*(),.remove_from_*()
ALL PyGithub property accesses that may trigger API calls MUST be wrapped withasyncio.to_thread()- properties like.draft,.mergeable,.state,.committer,.permissions,.labels,.assigneesare NOT safe to access directly
PyGithub PaginatedList iteration MUST be wrapped inasyncio.to_thread()with list conversion - never iterate directly to avoid blocking the event loop
ALL imports must be at the top of files - no imports in the middle of functions or try/except blocks except for TYPE_CHECKING conditional imports
ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated
Code coverage of 90% or higher is ...
Files:
webhook_server/utils/structured_logger.pywebhook_server/tests/test_structured_logger.pywebhook_server/tests/test_log_api.pywebhook_server/web/log_viewer.py
webhook_server/**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Defensive checks ARE acceptable for optional parameters explicitly typed as
Type | None- check is legitimate for parameters allowing None
Files:
webhook_server/utils/structured_logger.pywebhook_server/tests/test_structured_logger.pywebhook_server/tests/test_log_api.pywebhook_server/web/log_viewer.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test files MUST use
TEST_GITHUB_TOKEN = 'ghp_test1234...'with# pragma: allowlist secretcomment to avoid security warnings in test tokens
Files:
webhook_server/tests/test_structured_logger.pywebhook_server/tests/test_log_api.py
webhook_server/web/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
FastAPI endpoint handlers must be async (
async def) to maintain non-blocking operation of the event loop
Files:
webhook_server/web/log_viewer.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use structured logging with contextual parameters via `get_logger_with_params()` including repository, hook_id, and component-specific identifiers for webhook correlation
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification to ensure compatibility with GitHub API changes
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use structured logging with contextual parameters via `get_logger_with_params()` including repository, hook_id, and component-specific identifiers for webhook correlation
Applied to files:
webhook_server/utils/structured_logger.pywebhook_server/tests/test_structured_logger.pywebhook_server/web/log_viewer.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/**/*.py : Internal methods in `webhook_server/libs/` can change freely without backward compatibility requirements - return types, method signatures, and implementations may be modified without deprecation warnings
Applied to files:
webhook_server/utils/structured_logger.pywebhook_server/tests/test_log_api.pywebhook_server/web/log_viewer.py
📚 Learning: 2025-12-29T13:09:56.231Z
Learnt from: rnetser
Repo: myk-org/github-webhook-server PR: 959
File: webhook_server/libs/handlers/pull_request_handler.py:887-887
Timestamp: 2025-12-29T13:09:56.231Z
Learning: In the myk-org/github-webhook-server repository, logging statements in the webhook_server codebase should use f-strings for interpolation. Do not suggest changing to lazy formatting with % (e.g., logger.debug('msg %s', var)) since that is an established style choice. When adding new log statements, prefer f"...{var}..." for readability and performance, and avoid string concatenation or format-style logging unless the project explicitly adopts a different convention.
Applied to files:
webhook_server/utils/structured_logger.pywebhook_server/tests/test_structured_logger.pywebhook_server/tests/test_log_api.pywebhook_server/web/log_viewer.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/tests/**/*.py : Test files MUST use `TEST_GITHUB_TOKEN = 'ghp_test1234...'` with `# pragma: allowlist secret` comment to avoid security warnings in test tokens
Applied to files:
webhook_server/tests/test_structured_logger.pywebhook_server/tests/test_log_api.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Code coverage of 90% or higher is MANDATORY - measured via `pytest --cov=webhook_server` and enforced in CI
Applied to files:
webhook_server/tests/test_structured_logger.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/**/*.py : Eliminate unnecessary defensive checks for return type changes and method signature modifications in internal APIs - fail-fast principle is preferred over compatibility
Applied to files:
webhook_server/tests/test_log_api.pywebhook_server/web/log_viewer.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
Applied to files:
webhook_server/tests/test_log_api.py
🧬 Code graph analysis (4)
webhook_server/utils/structured_logger.py (2)
webhook_server/libs/config.py (1)
Config(11-142)webhook_server/utils/context.py (3)
WebhookContext(49-232)get_context(274-284)to_dict(195-232)
webhook_server/tests/test_structured_logger.py (3)
webhook_server/libs/config.py (2)
Config(11-142)exists(24-26)webhook_server/utils/context.py (4)
WebhookContext(49-232)start_step(116-133)complete_step(135-157)to_dict(195-232)webhook_server/utils/structured_logger.py (5)
StructuredLogWriter(53-263)write_webhook_log(266-290)_get_log_file_path(79-91)write_log(93-179)write_error_log(181-263)
webhook_server/tests/test_log_api.py (1)
webhook_server/web/log_viewer.py (1)
_stream_log_entries(734-811)
webhook_server/web/log_viewer.py (1)
webhook_server/libs/log_parser.py (3)
parse_json_log_entry(313-363)parse_log_entry(129-183)get_raw_json_entry(424-439)
🪛 Ruff (0.14.10)
webhook_server/utils/structured_logger.py
171-172: Logging statement uses f-string
(G004)
177-178: Logging statement uses f-string
(G004)
257-257: Logging statement uses f-string
(G004)
262-262: Logging statement uses f-string
(G004)
282-282: Avoid specifying long messages outside the exception class
(TRY003)
webhook_server/tests/test_structured_logger.py
303-303: Boolean positional value in function call
(FBT003)
320-320: Boolean positional value in function call
(FBT003)
437-437: Boolean positional value in function call
(FBT003)
633-633: Unused method argument: tmp_path
(ARG002)
webhook_server/web/log_viewer.py
570-570: Abstract raise to an inner function
(TRY301)
570-570: Avoid specifying long messages outside the exception class
(TRY003)
854-854: Do not catch blind exception: Exception
(BLE001)
855-855: Logging statement uses f-string
(G004)
🔇 Additional comments (7)
webhook_server/web/log_viewer.py (3)
536-573: LGTM - Clean JSON workflow step retrieval.The
get_workflow_steps_jsonmethod provides efficient access to structured workflow data directly from JSON logs. The return structure includes all relevant fields (hook_id, event_type, timing, steps, etc.) and the 404 handling via HTTPException is appropriate.One minor note: the static analyzer flags TRY301/TRY003 for raising
ValueErrorwith a long message at line 570, but this pattern is idiomatic for this codebase since the message is immediately caught and converted to HTTPException at line 573.
588-593: Good fallback strategy for backward compatibility.The JSON-first approach with graceful fallback to text log parsing ensures newer JSON logs are used when available while maintaining compatibility with existing text-based logs. This is the right pattern for migration.
758-771: Well-structured file prioritization for JSON logs.The sort key elegantly prioritizes JSON webhook files (
webhooks_*.json) over text logs while maintaining newest-first ordering within each category. This ensures the richer structured data is consumed first when available.webhook_server/utils/structured_logger.py (4)
1-51: Well-documented module with clear architecture.The module docstring provides excellent documentation of:
- JSON format (pretty-printed, blank line separators)
- File naming and rotation strategy
- Atomic write and concurrency patterns
- Usage examples
The cross-platform
fcntlhandling is correctly implemented with a feature flag.
123-179: Solid atomic write pattern with proper locking.The implementation correctly:
- Creates temp file in same directory (ensures atomic rename on same filesystem)
- Acquires exclusive lock on temp file during write
- Acquires exclusive lock on target file during append
- Properly releases locks in finally blocks
- Cleans up temp file even on failure
- Logs errors without re-raising (correct for non-critical logging operations)
The f-string usage in logging statements follows the established codebase convention. Based on learnings, this is the preferred style.
181-263: Comprehensive error logging for early failures.The
write_error_logmethod handles two important scenarios:
- With partial context: Marks the existing context as failed and preserves any existing error details (line 206 check prevents overwriting)
- Without context: Creates a minimal error entry matching the
WebhookContext.to_dict()structure for schema consistencyThis ensures webhook failures are logged even when processing fails before context is fully populated.
266-290: Clean convenience API for webhook log writing.The
write_webhook_logfunction provides a simple interface that:
- Automatically retrieves context from ContextVar when not explicitly provided
- Raises a clear
ValueErrorif no context is available (helpful for debugging misconfigured callers)- Handles Config and StructuredLogWriter instantiation internally
The static analyzer flags TRY003 for the long exception message at line 282, but clear error messages are valuable for debugging. This is acceptable.
CodeRabbit Review Comments - AddressedAll 15 CodeRabbit review comments have been addressed: Implemented Changes (14)
Analyzed - No Changes Needed (1)
✅ All 1077 tests pass with 90.45% coverage |
CodeRabbit Review Response (Review #3626307511)Addressed (3)
Notes:
Automated response from CodeRabbit Review Handler |
This PR contains the following updates: | Update | Change | |---|---| | lockFileMaintenance | All locks refreshed | 🔧 This Pull Request updates lock files to use the latest dependency versions. --- ### Configuration 📅 **Schedule**: Branch creation - Between 12:00 AM and 03:59 AM, only on Monday ( * 0-3 * * 1 ) (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://redirect.github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/myk-org/github-webhook-server). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0Mi42OS4xIiwidXBkYXRlZEluVmVyIjoiNDIuNjkuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
- Fix CancelledError handling in handlers (re-raise instead of catching) - Add fail_step() with traceback in exception handlers - Use logger.exception() instead of logger.error() for automatic traceback - Add from __future__ import annotations for type annotation fixes - Fix test mocks for OwnersFileHandler.initialize - Rename test_stream_log_entries_parse_error to test_stream_log_entries_file_read_error - Memory-efficient streaming with collections.deque in log_viewer - Prefer context.completed_at as timing source of truth
- CLAUDE.md: Add section on WebhookContext system and step tracking - README.md: Add section on JSON webhook logs with format and access methods
…feature/structured-json-logging
There was a problem hiding this comment.
Actionable comments posted: 6
Fix all issues with AI Agents 🤖
In @webhook_server/app.py:
- Around line 6-10: The finally block in process_with_error_handling currently
calls write_webhook_log(ctx) directly which can raise and prevent
clear_context() from running; wrap the write_webhook_log(ctx) call in its own
try/except that catches Exception, logs the failure with the repository-scoped
logger (use _logger.exception and include _log_context in the message), and then
ensure clear_context() runs in a finally clause; keep log_webhook_summary(ctx,
_logger, _log_context) before the write and do not suppress the original
processing exception.
In @webhook_server/libs/handlers/issue_comment_handler.py:
- Around line 54-55: The issue: process_comment_webhook_data collects exceptions
from asyncio.gather and then logs a traceback using traceback.format_exc()
outside an except block (producing "NoneType: None") and also wraps
asyncio.CancelledError in a RuntimeError so cancellation is suppressed. Fix by
formatting the traceback from the actual exception object (use
traceback.format_exception on the first_exception and pass that string to
ctx.fail_step in process_comment_webhook_data) and by special‑casing
asyncio.CancelledError when iterating over results from asyncio.gather (re‑raise
CancelledError immediately instead of adding it to failed_commands), plus add a
top‑level except asyncio.CancelledError: raise to let cancellation propagate;
update references in the code around process_comment_webhook_data, the results
loop handling the gather call, and the ctx.fail_step call to use the
exception-derived traceback string.
In @webhook_server/tests/test_log_viewer.py:
- Around line 383-406: The test
test_stream_json_log_entries_handles_file_read_errors currently uses a no-op
assertion; update it to actually validate behavior by replacing assert
len(entries) >= 0 with a meaningful check: either assert isinstance(entries,
list) to confirm the generator returned a list without raising, or assert that
the valid entry from sample_json_webhook_data is present (e.g., check
any(e.get("hook_id") == "test-hook-123" for e in entries)) so
controller._stream_json_log_entries(max_files=10, max_entries=100) is verified
to skip the unreadable bad_file and still yield the expected JSON entry.
In @webhook_server/web/log_viewer.py:
- Around line 536-573: get_workflow_steps_json always misses pretty‑printed
multi‑line JSON because _stream_json_log_entries and _stream_log_entries treat
each physical line as a full JSON entry; update the readers to accumulate lines
into multi‑line JSON blocks delimited by blank lines (or EOF) before calling
log_parser.get_raw_json_entry / parse_json_log_entry so each yielded item is a
whole JSON object, and/or make StructuredLogWriter produce JSON‑lines to match
the current line‑based parsing; specifically modify _stream_json_log_entries and
the branch in _stream_log_entries that processes webhooks_*.json to buffer lines
into entries (using a rolling buffer/deque to cap memory to max_entries) and
only call get_raw_json_entry / parse_json_log_entry on the joined block,
ensuring you still respect max_files and max_entries and log warnings on parsing
errors.
♻️ Duplicate comments (1)
webhook_server/tests/test_structured_logger.py (1)
437-451: Use keywordnew=in boolean@patchfor lint clarity (LOW)Severity: LOW. To satisfy FBT003 and keep tests consistent with other patches in this file, prefer an explicit keyword for the boolean:
- @patch("webhook_server.utils.structured_logger.HAS_FCNTL", True) + @patch("webhook_server.utils.structured_logger.HAS_FCNTL", new=True) @patch("fcntl.flock") def test_write_error_log_uses_file_locking(self, mock_flock: Mock, log_writer: StructuredLogWriter) -> None:This makes the intent explicit and avoids positional-boolean warnings without changing behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.gitignoreCLAUDE.mdREADME.mdwebhook_server/app.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/libs/log_parser.pywebhook_server/tests/test_context.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_log_api.pywebhook_server/tests/test_log_parser.pywebhook_server/tests/test_log_viewer.pywebhook_server/tests/test_structured_logger.pywebhook_server/utils/context.pywebhook_server/utils/structured_logger.pywebhook_server/web/log_viewer.py
💤 Files with no reviewable changes (1)
- .gitignore
🧰 Additional context used
📓 Path-based instructions (6)
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Never use defensive checks (hasattr, if object:, if value is not None) on required parameters passed to__init__()- required parameters are always provided in this self-contained server application
Defensive checks ARE acceptable in destructors (__del__) because they may be called during failed initialization
Defensive checks ARE acceptable for lazy initialization where attributes explicitly start asNone- check is legitimate for uninitialized attributes
Never use defensive checks on known library versions controlled inpyproject.toml- e.g., PyGithub >=2.4.0 methods are guaranteed to exist
Never use defensive checks on webhook payload fields guaranteed by GitHub webhook specification - e.g.,user.node_id,user.type,senderalways exist in webhook payloads
Never use hasattr() for type discrimination - useisinstance()instead for type checking
Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
ALL PyGithub method calls MUST be wrapped withasyncio.to_thread()to avoid blocking the FastAPI event loop - including.get_*(),.create_*(),.edit(),.add_to_*(),.remove_from_*()
ALL PyGithub property accesses that may trigger API calls MUST be wrapped withasyncio.to_thread()- properties like.draft,.mergeable,.state,.committer,.permissions,.labels,.assigneesare NOT safe to access directly
PyGithub PaginatedList iteration MUST be wrapped inasyncio.to_thread()with list conversion - never iterate directly to avoid blocking the event loop
ALL imports must be at the top of files - no imports in the middle of functions or try/except blocks except for TYPE_CHECKING conditional imports
ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated
Code coverage of 90% or higher is ...
Files:
webhook_server/tests/test_structured_logger.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_log_parser.pywebhook_server/web/log_viewer.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/utils/structured_logger.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/tests/test_context.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.pywebhook_server/utils/context.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_log_api.pywebhook_server/libs/log_parser.pywebhook_server/tests/test_log_viewer.pywebhook_server/app.py
webhook_server/**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Defensive checks ARE acceptable for optional parameters explicitly typed as
Type | None- check is legitimate for parameters allowing None
Files:
webhook_server/tests/test_structured_logger.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_log_parser.pywebhook_server/web/log_viewer.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/utils/structured_logger.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/tests/test_context.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.pywebhook_server/utils/context.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_log_api.pywebhook_server/libs/log_parser.pywebhook_server/tests/test_log_viewer.pywebhook_server/app.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test files MUST use
TEST_GITHUB_TOKEN = 'ghp_test1234...'with# pragma: allowlist secretcomment to avoid security warnings in test tokens
Files:
webhook_server/tests/test_structured_logger.pywebhook_server/tests/test_log_parser.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_context.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_log_api.pywebhook_server/tests/test_log_viewer.py
webhook_server/libs/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/**/*.py: Internal methods inwebhook_server/libs/can change freely without backward compatibility requirements - return types, method signatures, and implementations may be modified without deprecation warnings
Eliminate unnecessary defensive checks for return type changes and method signature modifications in internal APIs - fail-fast principle is preferred over compatibility
Files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/libs/log_parser.py
webhook_server/libs/handlers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/libs/handlers/**/*.py: Architecture guarantee:repository_datais ALWAYS set before handlers instantiate inGithubWebhook.process()with fail-fast exceptions - no defensive checks needed
Useawait asyncio.gather(*tasks, return_exceptions=True)for concurrent non-blocking PyGithub operations to enable parallel processing of multiple GitHub API calls
Use structured logging with contextual parameters viaget_logger_with_params()including repository, hook_id, and component-specific identifiers for webhook correlation
Configuration access viaConfig(repository='org/repo-name')- repository parameter is required for per-repository overrides via.github-webhook-server.yaml
Handler pattern: implement__init__(self, github_webhook: GithubWebhook, ...)andprocess_event(event_data: dict) -> Nonefor all GitHub event handlers
Useself.github_webhook.unified_apifor all GitHub API operations in handlers - never access GitHub API directly
Files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/libs/handlers/issue_comment_handler.py
webhook_server/web/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
FastAPI endpoint handlers must be async (
async def) to maintain non-blocking operation of the event loop
Files:
webhook_server/web/log_viewer.py
🧠 Learnings (43)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use structured logging with contextual parameters via `get_logger_with_params()` including repository, hook_id, and component-specific identifiers for webhook correlation
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification to ensure compatibility with GitHub API changes
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use structured logging with contextual parameters via `get_logger_with_params()` including repository, hook_id, and component-specific identifiers for webhook correlation
Applied to files:
README.mdwebhook_server/tests/test_structured_logger.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/web/log_viewer.pywebhook_server/utils/structured_logger.pywebhook_server/libs/handlers/push_handler.pyCLAUDE.mdwebhook_server/utils/context.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/app.py
📚 Learning: 2025-08-05T21:15:07.060Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 840
File: webhook_server/docs/pr-flow-api.md:17-26
Timestamp: 2025-08-05T21:15:07.060Z
Learning: The PR Flow API endpoint (/logs/api/pr-flow/{hook_id}) in webhook_server/docs/pr-flow-api.md has no built-in authentication and is designed to be deployed in trusted networks or behind a reverse proxy for security, rather than using API-level authentication mechanisms.
Applied to files:
README.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/tests/**/*.py : Test files MUST use `TEST_GITHUB_TOKEN = 'ghp_test1234...'` with `# pragma: allowlist secret` comment to avoid security warnings in test tokens
Applied to files:
webhook_server/tests/test_structured_logger.pywebhook_server/tests/test_log_parser.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/tests/test_context.pywebhook_server/tests/test_github_api.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Code coverage of 90% or higher is MANDATORY - measured via `pytest --cov=webhook_server` and enforced in CI
Applied to files:
webhook_server/tests/test_structured_logger.pywebhook_server/tests/test_context.py
📚 Learning: 2025-12-29T13:09:56.231Z
Learnt from: rnetser
Repo: myk-org/github-webhook-server PR: 959
File: webhook_server/libs/handlers/pull_request_handler.py:887-887
Timestamp: 2025-12-29T13:09:56.231Z
Learning: In the myk-org/github-webhook-server repository, logging statements in the webhook_server codebase should use f-strings for interpolation. Do not suggest changing to lazy formatting with % (e.g., logger.debug('msg %s', var)) since that is an established style choice. When adding new log statements, prefer f"...{var}..." for readability and performance, and avoid string concatenation or format-style logging unless the project explicitly adopts a different convention.
Applied to files:
webhook_server/tests/test_structured_logger.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_log_parser.pywebhook_server/web/log_viewer.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/utils/structured_logger.pywebhook_server/tests/test_issue_comment_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/tests/test_context.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.pywebhook_server/utils/context.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_log_api.pywebhook_server/libs/log_parser.pywebhook_server/tests/test_log_viewer.pywebhook_server/app.py
📚 Learning: 2024-10-08T09:19:56.185Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/**/*.py : Eliminate unnecessary defensive checks for return type changes and method signature modifications in internal APIs - fail-fast principle is preferred over compatibility
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/web/log_viewer.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.pywebhook_server/tests/test_log_api.pywebhook_server/app.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL imports must be at the top of files - no imports in the middle of functions or try/except blocks except for TYPE_CHECKING conditional imports
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/app.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_log_parser.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/utils/structured_logger.pywebhook_server/libs/handlers/push_handler.pywebhook_server/tests/test_context.pywebhook_server/utils/context.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/app.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Never use defensive checks on webhook payload fields guaranteed by GitHub webhook specification - e.g., `user.node_id`, `user.type`, `sender` always exist in webhook payloads
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.pywebhook_server/libs/handlers/issue_comment_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Handler pattern: implement `__init__(self, github_webhook: GithubWebhook, ...)` and `process_event(event_data: dict) -> None` for all GitHub event handlers
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/tests/test_github_api.pyCLAUDE.mdwebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/app.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Never use defensive checks on known library versions controlled in `pyproject.toml` - e.g., PyGithub >=2.4.0 methods are guaranteed to exist
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.pywebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/app.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL PyGithub property accesses that may trigger API calls MUST be wrapped with `asyncio.to_thread()` - properties like `.draft`, `.mergeable`, `.state`, `.committer`, `.permissions`, `.labels`, `.assignees` are NOT safe to access directly
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.pywebhook_server/utils/context.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Architecture guarantee: `repository_data` is ALWAYS set before handlers instantiate in `GithubWebhook.process()` with fail-fast exceptions - no defensive checks needed
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.pyCLAUDE.mdwebhook_server/libs/handlers/issue_comment_handler.pywebhook_server/app.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use `await asyncio.gather(*tasks, return_exceptions=True)` for concurrent non-blocking PyGithub operations to enable parallel processing of multiple GitHub API calls
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/tests/test_github_api.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/*check_run*.py : Repository cloning for check_run webhook events MUST be optimized by skipping clone when action != 'completed' or when check name is 'can-be-merged' with non-success conclusion
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/tests/test_github_api.py
📚 Learning: 2024-10-29T10:42:50.163Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:925-926
Timestamp: 2024-10-29T10:42:50.163Z
Learning: In `webhook_server_container/libs/github_api.py`, the method `self._keep_approved_by_approvers_after_rebase()` must be called after removing labels when synchronizing a pull request. Therefore, it should be placed outside the `ThreadPoolExecutor` to ensure it runs sequentially after label removal.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/owners_files_handler.py
📚 Learning: 2024-10-29T08:09:57.157Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Applied to files:
webhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/tests/test_github_api.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/**/*.py : Internal methods in `webhook_server/libs/` can change freely without backward compatibility requirements - return types, method signatures, and implementations may be modified without deprecation warnings
Applied to files:
webhook_server/web/log_viewer.pywebhook_server/utils/structured_logger.pywebhook_server/libs/handlers/push_handler.pywebhook_server/tests/test_github_api.pywebhook_server/utils/context.pywebhook_server/app.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use `self.github_webhook.unified_api` for all GitHub API operations in handlers - never access GitHub API directly
Applied to files:
webhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/push_handler.pywebhook_server/tests/test_github_api.pyCLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.{py,ts,tsx} : Defensive checks ARE acceptable for optional parameters explicitly typed as `Type | None` - check is legitimate for parameters allowing None
Applied to files:
webhook_server/libs/handlers/pull_request_review_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Never use defensive checks (hasattr, if object:, if value is not None) on required parameters passed to `__init__()` - required parameters are always provided in this self-contained server application
Applied to files:
webhook_server/libs/handlers/pull_request_review_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Defensive checks ARE acceptable for lazy initialization where attributes explicitly start as `None` - check is legitimate for uninitialized attributes
Applied to files:
webhook_server/libs/handlers/pull_request_review_handler.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the GitHub webhook server codebase, `root_approvers` and `root_reviewers` are properties of the GithubWebhook class, not methods, so they can be used with `.copy()` without parentheses.
Applied to files:
webhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.py
📚 Learning: 2025-05-10T22:07:53.544Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 773
File: webhook_server/libs/pull_request_handler.py:553-557
Timestamp: 2025-05-10T22:07:53.544Z
Learning: In the webhook server codebase, `root_approvers` and `root_reviewers` are defined as properties in the `GithubWebhook` class, not methods, so they can be accessed without parentheses and used with methods like `.copy()` directly.
Applied to files:
webhook_server/libs/handlers/pull_request_review_handler.py
📚 Learning: 2024-10-14T14:12:28.924Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1638-1639
Timestamp: 2024-10-14T14:12:28.924Z
Learning: In `webhook_server_container/libs/github_api.py`, it is acceptable for `self.repository.owner.email` to be `None` since internal commands can handle this case.
Applied to files:
webhook_server/libs/handlers/pull_request_review_handler.pywebhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.py
📚 Learning: 2024-10-15T10:37:45.791Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 598
File: webhook_server_container/libs/github_api.py:1860-1874
Timestamp: 2024-10-15T10:37:45.791Z
Learning: In the `process_retest_command` method in `webhook_server_container/libs/github_api.py`, `_target_tests` is defined before use.
Applied to files:
webhook_server/tests/test_issue_comment_handler.py
📚 Learning: 2025-05-13T12:06:27.297Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 778
File: webhook_server/libs/pull_request_handler.py:327-330
Timestamp: 2025-05-13T12:06:27.297Z
Learning: In the GitHub webhook server, synchronous GitHub API calls (like create_issue_comment, add_to_assignees, etc.) in async methods should be awaited using asyncio.to_thread or loop.run_in_executor to prevent blocking the event loop.
Applied to files:
webhook_server/tests/test_issue_comment_handler.pywebhook_server/libs/handlers/issue_comment_handler.py
📚 Learning: 2025-10-28T13:04:00.466Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/handlers/runner_handler.py:491-571
Timestamp: 2025-10-28T13:04:00.466Z
Learning: In webhook_server/libs/handlers/runner_handler.py, the run_build_container method is designed with the pattern that push=True is always called with set_check=False in production code, so no check-run status needs to be finalized after push operations.
Applied to files:
webhook_server/libs/handlers/push_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification to ensure compatibility with GitHub API changes
Applied to files:
webhook_server/libs/handlers/push_handler.pywebhook_server/tests/test_github_api.pywebhook_server/app.py
📚 Learning: 2024-10-14T14:13:21.316Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 588
File: webhook_server_container/libs/github_api.py:1632-1637
Timestamp: 2024-10-14T14:13:21.316Z
Learning: In the `ProcessGithubWehook` class in `webhook_server_container/libs/github_api.py`, avoid using environment variables to pass tokens because multiple commands with multiple tokens can run at the same time.
Applied to files:
webhook_server/libs/handlers/push_handler.py
📚 Learning: 2024-10-15T13:18:38.590Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 602
File: webhook_server_container/libs/github_api.py:2053-2062
Timestamp: 2024-10-15T13:18:38.590Z
Learning: In the `run_podman_command` method in `webhook_server_container/libs/github_api.py`, the `rc` variable is a boolean that returns `True` when the command succeeds and `False` when it fails.
Applied to files:
webhook_server/libs/handlers/push_handler.py
📚 Learning: 2024-11-21T13:34:45.218Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 624
File: webhook_server_container/libs/github_api.py:596-604
Timestamp: 2024-11-21T13:34:45.218Z
Learning: In the codebase, aggregation of reviewers and approvers from all OWNERS files is handled within the `assign_reviewers` method.
Applied to files:
webhook_server/libs/handlers/owners_files_handler.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Always re-raise `asyncio.CancelledError` - never suppress it in async Python exception handlers
Applied to files:
webhook_server/libs/handlers/owners_files_handler.pyCLAUDE.mdwebhook_server/app.py
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
webhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Configuration access via `Config(repository='org/repo-name')` - repository parameter is required for per-repository overrides via `.github-webhook-server.yaml`
Applied to files:
webhook_server/libs/handlers/owners_files_handler.pywebhook_server/tests/test_github_api.py
📚 Learning: 2024-11-21T16:10:01.777Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 624
File: webhook_server_container/libs/github_api.py:2067-2101
Timestamp: 2024-11-21T16:10:01.777Z
Learning: In `webhook_server_container/libs/github_api.py`, keep the `max_owners_files` limit as a hardcoded value rather than making it configurable through the config.
Applied to files:
webhook_server/tests/test_github_api.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Use `logger.exception()` for logging exceptions (automatic traceback) instead of `logger.error(..., exc_info=True)` for cleaner error logging
Applied to files:
CLAUDE.mdwebhook_server/tests/test_log_api.pywebhook_server/libs/log_parser.pywebhook_server/app.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Defensive checks ARE acceptable in destructors (`__del__`) because they may be called during failed initialization
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : PyGithub PaginatedList iteration MUST be wrapped in `asyncio.to_thread()` with list conversion - never iterate directly to avoid blocking the event loop
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL PyGithub method calls MUST be wrapped with `asyncio.to_thread()` to avoid blocking the FastAPI event loop - including `.get_*()`, `.create_*()`, `.edit()`, `.add_to_*()`, `.remove_from_*()`
Applied to files:
CLAUDE.mdwebhook_server/utils/context.py
🧬 Code graph analysis (10)
webhook_server/libs/handlers/pull_request_handler.py (1)
webhook_server/utils/context.py (4)
WebhookContext(82-372)start_step(149-166)complete_step(168-222)fail_step(261-295)
webhook_server/tests/test_log_parser.py (1)
webhook_server/libs/log_parser.py (9)
parse_json_log_entry(313-363)parse_json_log_file(400-422)get_raw_json_entry(424-439)parse_log_entry(129-183)LogFilter(507-620)LogEntry(16-49)get_unique_values(585-601)get_entry_count_by_field(603-620)monitor_log_directory(473-504)
webhook_server/web/log_viewer.py (1)
webhook_server/libs/log_parser.py (2)
parse_json_log_entry(313-363)get_raw_json_entry(424-439)
webhook_server/libs/handlers/pull_request_review_handler.py (2)
webhook_server/utils/context.py (3)
WebhookContext(82-372)start_step(149-166)complete_step(168-222)webhook_server/libs/github_api.py (1)
GithubWebhook(78-787)
webhook_server/tests/test_issue_comment_handler.py (1)
webhook_server/libs/handlers/issue_comment_handler.py (1)
process_comment_webhook_data(66-136)
webhook_server/libs/handlers/push_handler.py (5)
webhook_server/libs/handlers/check_run_handler.py (1)
CheckRunHandler(33-484)webhook_server/utils/helpers.py (1)
run_command(266-417)webhook_server/utils/notification_utils.py (1)
send_slack_message(11-35)webhook_server/libs/github_api.py (1)
GithubWebhook(78-787)webhook_server/utils/context.py (4)
WebhookContext(82-372)start_step(149-166)fail_step(261-295)complete_step(168-222)
webhook_server/tests/test_context.py (1)
webhook_server/utils/context.py (9)
WebhookContext(82-372)clear_context(427-433)create_context(375-411)get_context(414-424)start_step(149-166)complete_step(168-222)fail_step(261-295)to_dict(334-372)_build_summary(297-332)
webhook_server/libs/handlers/owners_files_handler.py (1)
webhook_server/utils/helpers.py (1)
run_command(266-417)
webhook_server/tests/test_github_api.py (1)
webhook_server/libs/handlers/owners_files_handler.py (1)
OwnersFileHandler(23-534)
webhook_server/tests/test_log_api.py (1)
webhook_server/web/log_viewer.py (1)
_stream_log_entries(734-811)
🪛 LanguageTool
CLAUDE.md
[uncategorized] ~971-~971: The official name of this software platform is spelled with a capital “H”.
Context: ...Pattern:** Handlers access context via github_webhook.ctx: ```python class PullRequ...
(GITHUB)
🪛 Ruff (0.14.10)
webhook_server/tests/test_structured_logger.py
437-437: Boolean positional value in function call
(FBT003)
webhook_server/libs/handlers/pull_request_handler.py
809-809: Logging statement uses f-string
(G004)
890-890: Logging statement uses f-string
(G004)
898-898: Logging statement uses f-string
(G004)
901-901: Logging statement uses f-string
(G004)
webhook_server/web/log_viewer.py
570-570: Abstract raise to an inner function
(TRY301)
570-570: Avoid specifying long messages outside the exception class
(TRY003)
858-858: Do not catch blind exception: Exception
(BLE001)
859-859: Logging statement uses f-string
(G004)
webhook_server/libs/handlers/pull_request_review_handler.py
43-44: Logging statement uses f-string
(G004)
55-55: Logging statement uses f-string
(G004)
webhook_server/utils/structured_logger.py
171-172: Logging statement uses f-string
(G004)
177-178: Logging statement uses f-string
(G004)
257-257: Logging statement uses f-string
(G004)
262-262: Logging statement uses f-string
(G004)
282-282: Avoid specifying long messages outside the exception class
(TRY003)
webhook_server/libs/handlers/push_handler.py
37-37: Logging statement uses f-string
(G004)
38-38: Logging statement uses f-string
(G004)
40-40: Logging statement uses f-string
(G004)
44-44: Logging statement uses f-string
(G004)
50-50: Logging statement uses f-string
(G004)
55-55: Logging statement uses f-string
(G004)
55-55: Redundant exception object included in logging.exception call
(TRY401)
webhook_server/tests/test_context.py
33-33: datetime.datetime() called without a tzinfo argument
(DTZ001)
145-145: Unused method argument: mock_datetime
(ARG002)
193-193: Unused method argument: mock_datetime
(ARG002)
257-257: Unused method argument: mock_datetime
(ARG002)
331-331: Unused method argument: mock_datetime
(ARG002)
350-350: Unused method argument: mock_datetime
(ARG002)
378-378: Unused method argument: mock_datetime
(ARG002)
webhook_server/libs/handlers/owners_files_handler.py
129-129: Logging statement uses f-string
(G004)
130-130: Abstract raise to an inner function
(TRY301)
135-135: Logging statement uses f-string
(G004)
136-136: Consider moving this statement to an else block
(TRY300)
149-149: Logging statement uses f-string
(G004)
webhook_server/utils/context.py
149-149: Dynamically typed expressions (typing.Any) are disallowed in **data
(ANN401)
172-172: Dynamically typed expressions (typing.Any) are disallowed in **data
(ANN401)
261-261: Dynamically typed expressions (typing.Any) are disallowed in **data
(ANN401)
webhook_server/libs/handlers/issue_comment_handler.py
74-74: Logging statement uses f-string
(G004)
79-79: Logging statement uses f-string
(G004)
85-85: Logging statement uses f-string
(G004)
117-117: Logging statement uses f-string
(G004)
127-127: Abstract raise to an inner function
(TRY301)
webhook_server/libs/log_parser.py
418-418: Logging statement uses f-string
(G004)
418-418: Redundant exception object included in logging.exception call
(TRY401)
420-420: Logging statement uses f-string
(G004)
420-420: Redundant exception object included in logging.exception call
(TRY401)
webhook_server/app.py
430-430: Logging statement uses f-string
(G004)
439-439: Logging statement uses f-string
(G004)
443-443: Logging statement uses f-string
(G004)
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @webhook_server/tests/test_edge_cases_validation.py:
- Around line 890-902: The helper function signatures use bare dict which fails
mypy strict checks; update user_request(controller: LogViewerController,
filters: dict) -> dict to use explicit types (e.g., filters: dict[str, Any] and
return type -> dict[str, Any]) and ensure you import Any from typing (or from
collections.abc if using Python 3.10+ style). Also update any related local
helper async_iter_wrapper annotations if needed (e.g., parameter:
list[LogEntry]) so all function parameters and return types are fully annotated.
In @webhook_server/tests/test_log_viewer.py:
- Around line 1098-1104: Add explicit return type annotations to the async test
helper functions so their signatures reflect they return HTML strings; update
the mock helper(s) named mock_get_html used as the side effect for
controller._get_log_viewer_html to declare async def mock_get_html() -> str:
(apply the same change to the second occurrence around controller.get_log_page
tests), keeping the body unchanged so the function still returns the HTML
string.
- Around line 1042-1046: Add full type hints to the async mock close functions:
annotate their parameters (e.g., code: int, reason: str) and the return type as
-> None so they satisfy mypy strict mode; update the async def mock_close(...)
used to set ws1.close and ws2.close and the other analogous mock close function
later in the file to include these parameter and return type annotations.
In @webhook_server/utils/app_utils.py:
- Around line 192-193: The summary step building code calls
format_duration(step_duration_ms) without ensuring step_duration_ms is not None;
update the validation around step_data to check both presence and non-None
(e.g., if "duration_ms" in step_data and step_data["duration_ms"] is not None)
before calling format_duration, or alternatively change format_duration(ms) to
accept None (int | None) and return a placeholder like "unknown" when ms is
None; update references in the steps_summary construction (variable
step_duration_ms and call to format_duration) and tests accordingly.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webhook_server/tests/test_log_parser.py (2)
271-278: HIGH: Add complete type hints to_collect_entrieshelper methodThis method violates the mandatory coding guideline requiring complete type hints for ALL functions. Parameters
async_gen,entries_list, andmax_entrieslack type annotations, and the return type is missing.As per coding guidelines: "ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated."
The correct pattern is already demonstrated at line 1336 in the
collect_entrieslocal function within the same file.🔎 Add type hints matching the pattern at line 1336
- async def _collect_entries(self, async_gen, entries_list, max_entries=10): + async def _collect_entries( + self, + async_gen: AsyncIterator[LogEntry], + entries_list: list[LogEntry], + max_entries: int = 10, + ) -> None: """Helper to collect entries from async generator with a limit."""Based on coding guidelines, which mandate mypy-strict-compatible type hints for all functions.
252-256: HIGH: Replace blocking file I/O with async-compatible wrapperThis code uses blocking
open()and file operations directly in an async function, which violates the mandatory coding guideline requiring all file operations be wrapped withasyncio.to_thread().As per coding guidelines: "ALL PyGithub method calls MUST be wrapped with
asyncio.to_thread()to avoid blocking the FastAPI event loop" - this extends to all blocking I/O operations including file writes.The correct pattern is demonstrated at lines 1353-1358 in the same file where identical file operations are properly wrapped.
🔎 Wrap file operations using the pattern from lines 1353-1358
# Give the tail a moment to start await asyncio.sleep(0.1) - # Add new content to the file - with open(f.name, "a") as append_f: - append_f.write("\n2025-07-31T10:01:00.000000 main DEBUG New entry 1") - append_f.write("\n2025-07-31T10:02:00.000000 main ERROR New entry 2") - append_f.flush() + # Add new content to the file - non-blocking + def _append_log() -> None: + with open(f.name, "a") as append_f: + append_f.write("\n2025-07-31T10:01:00.000000 main DEBUG New entry 1") + append_f.write("\n2025-07-31T10:02:00.000000 main ERROR New entry 2") + append_f.flush() + + await asyncio.to_thread(_append_log) # Wait for the tail to collect entries with timeoutBased on coding guidelines, which prohibit blocking I/O in async functions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
webhook_server/tests/test_edge_cases_validation.pywebhook_server/tests/test_log_parser.pywebhook_server/tests/test_log_viewer.pywebhook_server/utils/app_utils.pywebhook_server/web/log_viewer.py
🧰 Additional context used
📓 Path-based instructions (4)
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Never use defensive checks (hasattr, if object:, if value is not None) on required parameters passed to__init__()- required parameters are always provided in this self-contained server application
Defensive checks ARE acceptable in destructors (__del__) because they may be called during failed initialization
Defensive checks ARE acceptable for lazy initialization where attributes explicitly start asNone- check is legitimate for uninitialized attributes
Never use defensive checks on known library versions controlled inpyproject.toml- e.g., PyGithub >=2.4.0 methods are guaranteed to exist
Never use defensive checks on webhook payload fields guaranteed by GitHub webhook specification - e.g.,user.node_id,user.type,senderalways exist in webhook payloads
Never use hasattr() for type discrimination - useisinstance()instead for type checking
Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
ALL PyGithub method calls MUST be wrapped withasyncio.to_thread()to avoid blocking the FastAPI event loop - including.get_*(),.create_*(),.edit(),.add_to_*(),.remove_from_*()
ALL PyGithub property accesses that may trigger API calls MUST be wrapped withasyncio.to_thread()- properties like.draft,.mergeable,.state,.committer,.permissions,.labels,.assigneesare NOT safe to access directly
PyGithub PaginatedList iteration MUST be wrapped inasyncio.to_thread()with list conversion - never iterate directly to avoid blocking the event loop
ALL imports must be at the top of files - no imports in the middle of functions or try/except blocks except for TYPE_CHECKING conditional imports
ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated
Code coverage of 90% or higher is ...
Files:
webhook_server/utils/app_utils.pywebhook_server/tests/test_log_viewer.pywebhook_server/tests/test_log_parser.pywebhook_server/web/log_viewer.pywebhook_server/tests/test_edge_cases_validation.py
webhook_server/**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Defensive checks ARE acceptable for optional parameters explicitly typed as
Type | None- check is legitimate for parameters allowing None
Files:
webhook_server/utils/app_utils.pywebhook_server/tests/test_log_viewer.pywebhook_server/tests/test_log_parser.pywebhook_server/web/log_viewer.pywebhook_server/tests/test_edge_cases_validation.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test files MUST use
TEST_GITHUB_TOKEN = 'ghp_test1234...'with# pragma: allowlist secretcomment to avoid security warnings in test tokens
Files:
webhook_server/tests/test_log_viewer.pywebhook_server/tests/test_log_parser.pywebhook_server/tests/test_edge_cases_validation.py
webhook_server/web/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
FastAPI endpoint handlers must be async (
async def) to maintain non-blocking operation of the event loop
Files:
webhook_server/web/log_viewer.py
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use structured logging with contextual parameters via `get_logger_with_params()` including repository, hook_id, and component-specific identifiers for webhook correlation
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/**/*.py : Internal methods in `webhook_server/libs/` can change freely without backward compatibility requirements - return types, method signatures, and implementations may be modified without deprecation warnings
Applied to files:
webhook_server/utils/app_utils.pywebhook_server/web/log_viewer.pywebhook_server/tests/test_edge_cases_validation.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use structured logging with contextual parameters via `get_logger_with_params()` including repository, hook_id, and component-specific identifiers for webhook correlation
Applied to files:
webhook_server/utils/app_utils.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
Applied to files:
webhook_server/utils/app_utils.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated
Applied to files:
webhook_server/utils/app_utils.pywebhook_server/tests/test_log_viewer.pywebhook_server/tests/test_log_parser.py
📚 Learning: 2025-12-29T13:09:56.231Z
Learnt from: rnetser
Repo: myk-org/github-webhook-server PR: 959
File: webhook_server/libs/handlers/pull_request_handler.py:887-887
Timestamp: 2025-12-29T13:09:56.231Z
Learning: In the myk-org/github-webhook-server repository, logging statements in the webhook_server codebase should use f-strings for interpolation. Do not suggest changing to lazy formatting with % (e.g., logger.debug('msg %s', var)) since that is an established style choice. When adding new log statements, prefer f"...{var}..." for readability and performance, and avoid string concatenation or format-style logging unless the project explicitly adopts a different convention.
Applied to files:
webhook_server/utils/app_utils.pywebhook_server/tests/test_log_viewer.pywebhook_server/tests/test_log_parser.pywebhook_server/web/log_viewer.pywebhook_server/tests/test_edge_cases_validation.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/tests/**/*.py : Test files MUST use `TEST_GITHUB_TOKEN = 'ghp_test1234...'` with `# pragma: allowlist secret` comment to avoid security warnings in test tokens
Applied to files:
webhook_server/tests/test_log_parser.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/web/**/*.py : FastAPI endpoint handlers must be async (`async def`) to maintain non-blocking operation of the event loop
Applied to files:
webhook_server/web/log_viewer.pywebhook_server/tests/test_edge_cases_validation.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/**/*.py : Eliminate unnecessary defensive checks for return type changes and method signature modifications in internal APIs - fail-fast principle is preferred over compatibility
Applied to files:
webhook_server/web/log_viewer.pywebhook_server/tests/test_edge_cases_validation.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : PyGithub PaginatedList iteration MUST be wrapped in `asyncio.to_thread()` with list conversion - never iterate directly to avoid blocking the event loop
Applied to files:
webhook_server/web/log_viewer.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL PyGithub method calls MUST be wrapped with `asyncio.to_thread()` to avoid blocking the FastAPI event loop - including `.get_*()`, `.create_*()`, `.edit()`, `.add_to_*()`, `.remove_from_*()`
Applied to files:
webhook_server/web/log_viewer.pywebhook_server/tests/test_edge_cases_validation.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL PyGithub property accesses that may trigger API calls MUST be wrapped with `asyncio.to_thread()` - properties like `.draft`, `.mergeable`, `.state`, `.committer`, `.permissions`, `.labels`, `.assignees` are NOT safe to access directly
Applied to files:
webhook_server/web/log_viewer.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use `await asyncio.gather(*tasks, return_exceptions=True)` for concurrent non-blocking PyGithub operations to enable parallel processing of multiple GitHub API calls
Applied to files:
webhook_server/web/log_viewer.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Use `logger.exception()` for logging exceptions (automatic traceback) instead of `logger.error(..., exc_info=True)` for cleaner error logging
Applied to files:
webhook_server/web/log_viewer.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Always re-raise `asyncio.CancelledError` - never suppress it in async Python exception handlers
Applied to files:
webhook_server/web/log_viewer.py
📚 Learning: 2025-08-05T21:15:07.060Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 840
File: webhook_server/docs/pr-flow-api.md:17-26
Timestamp: 2025-08-05T21:15:07.060Z
Learning: The PR Flow API endpoint (/logs/api/pr-flow/{hook_id}) in webhook_server/docs/pr-flow-api.md has no built-in authentication and is designed to be deployed in trusted networks or behind a reverse proxy for security, rather than using API-level authentication mechanisms.
Applied to files:
webhook_server/web/log_viewer.py
🧬 Code graph analysis (4)
webhook_server/utils/app_utils.py (3)
webhook_server/utils/context.py (1)
WebhookContext(82-372)webhook_server/utils/helpers.py (1)
get_logger_with_params(31-92)webhook_server/web/log_viewer.py (1)
LogViewerController(22-1130)
webhook_server/tests/test_log_viewer.py (1)
webhook_server/web/log_viewer.py (8)
_stream_json_log_entries(835-890)get_workflow_steps_json(547-584)get_workflow_steps(586-697)_stream_log_entries(748-833)get_log_entries(91-224)_estimate_total_log_count(1083-1130)_build_log_prefix_from_context(516-545)_generate_json_export(989-1008)
webhook_server/web/log_viewer.py (1)
webhook_server/libs/log_parser.py (3)
LogEntry(16-49)parse_json_log_entry(313-374)parse_log_entry(129-183)
webhook_server/tests/test_edge_cases_validation.py (1)
webhook_server/web/log_viewer.py (3)
LogViewerController(22-1130)get_log_entries(91-224)export_logs(271-385)
🪛 Ruff (0.14.10)
webhook_server/utils/app_utils.py
176-176: Avoid specifying long messages outside the exception class
(TRY003)
183-185: Avoid specifying long messages outside the exception class
(TRY003)
187-190: Avoid specifying long messages outside the exception class
(TRY003)
203-204: Logging statement uses f-string
(G004)
webhook_server/tests/test_log_viewer.py
200-200: Async functions should not open files with blocking methods like open
(ASYNC230)
375-375: Async functions should not open files with blocking methods like open
(ASYNC230)
507-507: Async functions should not open files with blocking methods like open
(ASYNC230)
528-528: Async functions should not open files with blocking methods like open
(ASYNC230)
554-554: Async functions should not open files with blocking methods like open
(ASYNC230)
590-590: Async functions should not open files with blocking methods like open
(ASYNC230)
634-634: Async functions should not open files with blocking methods like open
(ASYNC230)
658-658: Async functions should not open files with blocking methods like open
(ASYNC230)
678-678: Async functions should not open files with blocking methods like open
(ASYNC230)
703-703: Async functions should not open files with blocking methods like open
(ASYNC230)
759-759: Async functions should not open files with blocking methods like open
(ASYNC230)
798-798: Async functions should not open files with blocking methods like open
(ASYNC230)
843-843: Async functions should not open files with blocking methods like open
(ASYNC230)
864-864: Async functions should not open files with blocking methods like open
(ASYNC230)
910-910: Async functions should not open files with blocking methods like open
(ASYNC230)
933-933: Unused function argument: args
(ARG001)
933-933: Unused function argument: kwargs
(ARG001)
934-934: Avoid specifying long messages outside the exception class
(TRY003)
969-969: Async functions should not open files with blocking methods like open
(ASYNC230)
1003-1003: Async functions should not open files with blocking methods like open
(ASYNC230)
1042-1042: Missing return type annotation for private function mock_close
Add return type annotation: None
(ANN202)
1063-1063: Missing return type annotation for private function mock_close_error
Add return type annotation: Never
(ANN202)
1063-1063: Unused function argument: code
(ARG001)
1063-1063: Unused function argument: reason
(ARG001)
1064-1064: Create your own exception
(TRY002)
1064-1064: Avoid specifying long messages outside the exception class
(TRY003)
1098-1098: Missing return type annotation for private function mock_get_html
Add return type annotation: str
(ANN202)
1110-1110: Missing return type annotation for private function mock_get_html
(ANN202)
webhook_server/web/log_viewer.py
581-581: Abstract raise to an inner function
(TRY301)
581-581: Avoid specifying long messages outside the exception class
(TRY003)
832-832: Do not catch blind exception: Exception
(BLE001)
833-833: Logging statement uses f-string
(G004)
889-889: Do not catch blind exception: Exception
(BLE001)
890-890: Logging statement uses f-string
(G004)
1112-1112: Logging statement uses f-string
(G004)
🔇 Additional comments (16)
webhook_server/utils/app_utils.py (3)
126-156: LGTM: Clean duration formatting with proper edge case handling.The implementation correctly handles the progression from milliseconds through hours. The logic is clear and the format strings are user-friendly.
One minor observation: if
msis negative (which shouldn't happen in normal operation), the function will return something like-500ms. Consider whether negative durations are meaningful or indicate a bug upstream.
159-205: Explicit validation aligns with fail-fast principles.The explicit field validation on lines 182-190 correctly implements the coding guideline to raise exceptions rather than masking missing data with defaults. This was addressed from a previous review comment.
The f-string logging on lines 203-204 is acceptable per project conventions (f-strings are the established style for logging interpolation).
Regarding static analysis TRY003 hints about long exception messages outside classes: these are informational messages that help developers debug instrumentation gaps. The verbosity is justified here since the context ("ensure complete_step() or fail_step() was called") is actionable.
208-221: Simple passthrough wrapper is appropriate.The
get_workflow_steps_corefunction provides a clean abstraction layer between the utility module and the controller. Type hints are complete and the async signature is correct.webhook_server/tests/test_log_viewer.py (5)
111-126: Blockingopen()in test fixtures is acceptable.Severity: LOW (false positive from static analysis).
Ruff flags ASYNC230 for using blocking
open()inside async test methods. However, this is test fixture setup code that creates synthetic log files before exercising async controller methods. The blocking I/O doesn't affect test correctness since:
- File creation is a one-time setup operation
- Tests run sequentially, not concurrently
- The actual controller methods under test use
aiofilesproperlyNo change needed.
84-109: Fixture timing schema now matches production.The
sample_json_webhook_datafixture correctly usescompleted_atandduration_ms: 5000to match the production schema fromWebhookContext.to_dict()andStructuredLogWriter. This was addressed from a previous review comment.
933-935: Unused mock arguments are intentional for interface matching.Severity: LOW (false positive).
The
*argsand**kwargsinmock_stream_errorare required to match the signature of_stream_log_entries. Ruff's ARG001 warning is a false positive here since mock functions often need to accept arguments they don't use.The
yieldafterraise(line 935) is a clever pattern to make Python recognize this as an async generator while ensuring it always raises before yielding.
128-148: Good use of shallow copy for independent test entries.The test correctly uses
.copy()for simple dictionary mutations. Note that for nested structures likeentry2["pr"]["number"] = 789on line 138, this modifies the shared nested dict. This works here becausesample_json_webhook_datais a fixture that returns a fresh dict each call.
471-499: Correct use ofcopy.deepcopyfor nested structure independence.Unlike the earlier tests that used shallow
.copy(), this test correctly usescopy.deepcopy()to ensure modifications to nestedprdictionaries don't affect other entries. Good attention to detail.webhook_server/web/log_viewer.py (8)
547-584: JSON-first workflow retrieval with proper fallback.The
get_workflow_steps_jsonmethod efficiently searches JSONL logs and returns structured data. The fallback to text parsing inget_workflow_steps(lines 599-604) maintains backward compatibility.One note: The
stepsfield defaults to{}(line 575) which is correct sinceworkflow_stepsis a dictionary mapping step names to their data, not a list. This was clarified in a previous review exchange.
806-819: Async file I/O with proper JSONL parsing.The switch to
aiofiles.open()correctly makes file reading non-blocking. The parser branch based on file suffix (.jsonvs text logs) is clean.The JSONL assumption (one JSON object per line) now matches the
StructuredLogWriterformat change that writes compact JSON. This resolves the previous critical issue about multi-line JSON incompatibility.
835-890: Memory-bounded JSON streaming with proper cancellation handling.The
_stream_json_log_entriesmethod correctly:
- Uses
deque(maxlen=remaining)to bound memory (addresses previous review concern)- Re-raises
asyncio.CancelledErrorper coding guidelines- Processes JSONL format line-by-line
- Yields entries in reverse order (newest first)
The broad
Exceptioncatch on line 889 is acceptable here since file streaming should be resilient to individual file errors while continuing to process other files.
213-224: Consistentasyncio.CancelledErrorhandling throughout.All async methods now properly catch and re-raise
CancelledErrorbefore the generalExceptionhandler. This follows the coding guideline: "Always re-raise asyncio.CancelledError - never suppress it in async Python exception handlers."The pattern is correct:
except asyncio.CancelledError: self.logger.debug("Operation cancelled") raise # Always re-raise CancelledErrorAlso applies to: 373-385, 502-514, 685-697, 829-833
916-936: Async HTML template loading with graceful fallback.The
_get_log_viewer_htmlmethod correctly usesaiofilesfor non-blocking template loading. The fallback HTML onFileNotFoundErrororOSErrorensures the log viewer degrades gracefully rather than crashing.
1102-1113: Narrowed exception handling in log estimation.The exception handling was narrowed from broad
Exceptionto(OSError, PermissionError)per a previous review suggestion. The debug-level logging is appropriate for this non-critical estimation code.
778-788: JSON webhook files prioritized in sort order.The sort key function correctly prioritizes
webhooks_*.jsonfiles (primary data source) over text logs. The tuple(0 if is_json_webhook else 1, -mtime)ensures JSON files appear first, then sorted by modification time within each category.
581-581: Ruff TRY003/TRY301 hints are acceptable here.Severity: LOW (style preference).
Ruff suggests abstracting the
raise ValueError(...)to an inner function and avoiding long messages. However:
- The message is concise and actionable
- This is an internal method, not a public API
- Creating a custom exception class for this single use case would be over-engineering
No change needed.
- Add explicit dict[str, Any] type hints in test_edge_cases_validation.py - Add return type annotations to mock functions in test_log_viewer.py - Add None check for duration_ms to prevent type errors in app_utils.py - Add type hints to _collect_entries helper in test_log_parser.py - Replace blocking file I/O with asyncio.to_thread in test_log_parser.py
Reduce file size from 40.2k to 18.3k chars by: - Merging duplicate PyGithub asyncio.to_thread sections - Reducing before/after examples - Removing redundant decision tree - Condensing verbose explanations
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @CLAUDE.md:
- Line 37: Fix MD022/MD031 violations in CLAUDE.md by adding blank lines above
all headings (except the very first line of the file) and adding a blank line
both above and below every code block fence. Locate each heading and fenced code
block (triple backticks) in the document (e.g., the Python block starting with
```python and all other ``` fences) and insert the required blank lines so
headings are preceded by an empty line and code fences have an empty line before
and after; run markdownlint or the pre-commit hooks to validate no MD022/MD031
errors remain.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webhook_server/tests/test_edge_cases_validation.py (1)
857-920: Excellent concurrent user test with precise type hintsAll previously flagged type hint issues have been addressed:
user_filtersexplicitly typed aslist[dict[str, Any]](line 883)user_requesthas complete parameter and return type annotations (line 891)async_iter_wrapperincludesAsyncIterator[LogEntry]return type (line 894)- All async operations properly awaited
The test effectively validates concurrent multi-user scenarios with different filter combinations.
Optional observation:
async_iter_wrapperis defined in multiple test methods (lines 767-772, 894-899). This duplication provides test isolation and clarity, which is acceptable. If you later want to reduce duplication, consider extracting to a module-level test helper.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
CLAUDE.mdwebhook_server/tests/test_edge_cases_validation.pywebhook_server/tests/test_log_parser.pywebhook_server/tests/test_log_viewer.pywebhook_server/utils/app_utils.py
🧰 Additional context used
📓 Path-based instructions (3)
webhook_server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
webhook_server/**/*.py: Never use defensive checks (hasattr, if object:, if value is not None) on required parameters passed to__init__()- required parameters are always provided in this self-contained server application
Defensive checks ARE acceptable in destructors (__del__) because they may be called during failed initialization
Defensive checks ARE acceptable for lazy initialization where attributes explicitly start asNone- check is legitimate for uninitialized attributes
Never use defensive checks on known library versions controlled inpyproject.toml- e.g., PyGithub >=2.4.0 methods are guaranteed to exist
Never use defensive checks on webhook payload fields guaranteed by GitHub webhook specification - e.g.,user.node_id,user.type,senderalways exist in webhook payloads
Never use hasattr() for type discrimination - useisinstance()instead for type checking
Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
ALL PyGithub method calls MUST be wrapped withasyncio.to_thread()to avoid blocking the FastAPI event loop - including.get_*(),.create_*(),.edit(),.add_to_*(),.remove_from_*()
ALL PyGithub property accesses that may trigger API calls MUST be wrapped withasyncio.to_thread()- properties like.draft,.mergeable,.state,.committer,.permissions,.labels,.assigneesare NOT safe to access directly
PyGithub PaginatedList iteration MUST be wrapped inasyncio.to_thread()with list conversion - never iterate directly to avoid blocking the event loop
ALL imports must be at the top of files - no imports in the middle of functions or try/except blocks except for TYPE_CHECKING conditional imports
ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated
Code coverage of 90% or higher is ...
Files:
webhook_server/utils/app_utils.pywebhook_server/tests/test_log_viewer.pywebhook_server/tests/test_log_parser.pywebhook_server/tests/test_edge_cases_validation.py
webhook_server/**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Defensive checks ARE acceptable for optional parameters explicitly typed as
Type | None- check is legitimate for parameters allowing None
Files:
webhook_server/utils/app_utils.pywebhook_server/tests/test_log_viewer.pywebhook_server/tests/test_log_parser.pywebhook_server/tests/test_edge_cases_validation.py
webhook_server/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test files MUST use
TEST_GITHUB_TOKEN = 'ghp_test1234...'with# pragma: allowlist secretcomment to avoid security warnings in test tokens
Files:
webhook_server/tests/test_log_viewer.pywebhook_server/tests/test_log_parser.pywebhook_server/tests/test_edge_cases_validation.py
🧠 Learnings (34)
📓 Common learnings
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use structured logging with contextual parameters via `get_logger_with_params()` including repository, hook_id, and component-specific identifiers for webhook correlation
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/**/*.py : Internal methods in `webhook_server/libs/` can change freely without backward compatibility requirements - return types, method signatures, and implementations may be modified without deprecation warnings
Applied to files:
webhook_server/utils/app_utils.pywebhook_server/tests/test_edge_cases_validation.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use structured logging with contextual parameters via `get_logger_with_params()` including repository, hook_id, and component-specific identifiers for webhook correlation
Applied to files:
webhook_server/utils/app_utils.pyCLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging
Applied to files:
webhook_server/utils/app_utils.pyCLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.{py,ts,tsx} : Defensive checks ARE acceptable for optional parameters explicitly typed as `Type | None` - check is legitimate for parameters allowing None
Applied to files:
webhook_server/utils/app_utils.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL functions must have complete type hints compatible with mypy strict mode - parameter types, return types, and complex object types must be explicitly annotated
Applied to files:
webhook_server/utils/app_utils.pywebhook_server/tests/test_log_viewer.pyCLAUDE.mdwebhook_server/tests/test_log_parser.pywebhook_server/tests/test_edge_cases_validation.py
📚 Learning: 2025-12-29T13:09:56.231Z
Learnt from: rnetser
Repo: myk-org/github-webhook-server PR: 959
File: webhook_server/libs/handlers/pull_request_handler.py:887-887
Timestamp: 2025-12-29T13:09:56.231Z
Learning: In the myk-org/github-webhook-server repository, logging statements in the webhook_server codebase should use f-strings for interpolation. Do not suggest changing to lazy formatting with % (e.g., logger.debug('msg %s', var)) since that is an established style choice. When adding new log statements, prefer f"...{var}..." for readability and performance, and avoid string concatenation or format-style logging unless the project explicitly adopts a different convention.
Applied to files:
webhook_server/utils/app_utils.pywebhook_server/tests/test_log_viewer.pywebhook_server/tests/test_log_parser.pywebhook_server/tests/test_edge_cases_validation.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Architecture guarantee: `repository_data` is ALWAYS set before handlers instantiate in `GithubWebhook.process()` with fail-fast exceptions - no defensive checks needed
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Handler pattern: implement `__init__(self, github_webhook: GithubWebhook, ...)` and `process_event(event_data: dict) -> None` for all GitHub event handlers
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Never use defensive checks (hasattr, if object:, if value is not None) on required parameters passed to `__init__()` - required parameters are always provided in this self-contained server application
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Never use defensive checks on webhook payload fields guaranteed by GitHub webhook specification - e.g., `user.node_id`, `user.type`, `sender` always exist in webhook payloads
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Never use defensive checks on known library versions controlled in `pyproject.toml` - e.g., PyGithub >=2.4.0 methods are guaranteed to exist
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use `self.github_webhook.unified_api` for all GitHub API operations in handlers - never access GitHub API directly
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL PyGithub method calls MUST be wrapped with `asyncio.to_thread()` to avoid blocking the FastAPI event loop - including `.get_*()`, `.create_*()`, `.edit()`, `.add_to_*()`, `.remove_from_*()`
Applied to files:
CLAUDE.mdwebhook_server/tests/test_edge_cases_validation.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Defensive checks ARE acceptable in destructors (`__del__`) because they may be called during failed initialization
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL PyGithub property accesses that may trigger API calls MUST be wrapped with `asyncio.to_thread()` - properties like `.draft`, `.mergeable`, `.state`, `.committer`, `.permissions`, `.labels`, `.assignees` are NOT safe to access directly
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/**/*.py : Eliminate unnecessary defensive checks for return type changes and method signature modifications in internal APIs - fail-fast principle is preferred over compatibility
Applied to files:
CLAUDE.mdwebhook_server/tests/test_edge_cases_validation.py
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/*check_run*.py : Repository cloning for check_run webhook events MUST be optimized by skipping clone when action != 'completed' or when check name is 'can-be-merged' with non-success conclusion
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Configuration access via `Config(repository='org/repo-name')` - repository parameter is required for per-repository overrides via `.github-webhook-server.yaml`
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : PyGithub PaginatedList iteration MUST be wrapped in `asyncio.to_thread()` with list conversion - never iterate directly to avoid blocking the event loop
Applied to files:
CLAUDE.md
📚 Learning: 2025-10-30T00:18:06.176Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 878
File: webhook_server/libs/github_api.py:111-118
Timestamp: 2025-10-30T00:18:06.176Z
Learning: In webhook_server/libs/github_api.py, when creating temporary directories or performing operations that need repository names, prefer using self.repository_name (from webhook payload, always available) over dereferencing self.repository.name or self.repository_by_github_app.name, which may be None. This avoids AttributeError and keeps the code simple and reliable.
Applied to files:
CLAUDE.md
📚 Learning: 2025-05-13T12:06:27.297Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 778
File: webhook_server/libs/pull_request_handler.py:327-330
Timestamp: 2025-05-13T12:06:27.297Z
Learning: In the GitHub webhook server, synchronous GitHub API calls (like create_issue_comment, add_to_assignees, etc.) in async methods should be awaited using asyncio.to_thread or loop.run_in_executor to prevent blocking the event loop.
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Use `logger.exception()` for logging exceptions (automatic traceback) instead of `logger.error(..., exc_info=True)` for cleaner error logging
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : Code coverage of 90% or higher is MANDATORY - measured via `pytest --cov=webhook_server` and enforced in CI
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/**/*.py : ALL imports must be at the top of files - no imports in the middle of functions or try/except blocks except for TYPE_CHECKING conditional imports
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/tests/**/*.py : Test files MUST use `TEST_GITHUB_TOKEN = 'ghp_test1234...'` with `# pragma: allowlist secret` comment to avoid security warnings in test tokens
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Run code quality checks via `uv run ruff format && uv run ruff check --fix && uv run mypy webhook_server/` before committing - enforced by pre-commit hooks
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Log viewer endpoints (`/logs/*`) are unauthenticated by design - deploy only on trusted networks and never expose to public internet as logs contain sensitive data
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Always mask sensitive data in logs using `mask-sensitive-data: true` configuration - apply masking to tokens, webhook payloads, and user information
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-05T21:15:07.060Z
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 840
File: webhook_server/docs/pr-flow-api.md:17-26
Timestamp: 2025-08-05T21:15:07.060Z
Learning: The PR Flow API endpoint (/logs/api/pr-flow/{hook_id}) in webhook_server/docs/pr-flow-api.md has no built-in authentication and is designed to be deployed in trusted networks or behind a reverse proxy for security, rather than using API-level authentication mechanisms.
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/libs/handlers/**/*.py : Use `await asyncio.gather(*tasks, return_exceptions=True)` for concurrent non-blocking PyGithub operations to enable parallel processing of multiple GitHub API calls
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/config/schema.yaml : Configuration schema must be defined in `webhook_server/config/schema.yaml` with validation logic in `webhook_server/libs/config.py`
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification to ensure compatibility with GitHub API changes
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T12:39:43.079Z
Learnt from: CR
Repo: myk-org/github-webhook-server PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:39:43.079Z
Learning: Applies to webhook_server/web/**/*.py : FastAPI endpoint handlers must be async (`async def`) to maintain non-blocking operation of the event loop
Applied to files:
webhook_server/tests/test_edge_cases_validation.py
🧬 Code graph analysis (3)
webhook_server/utils/app_utils.py (3)
webhook_server/utils/context.py (1)
WebhookContext(82-372)webhook_server/utils/helpers.py (1)
get_logger_with_params(31-92)webhook_server/web/log_viewer.py (2)
LogViewerController(22-1130)get_workflow_steps(586-697)
webhook_server/tests/test_log_parser.py (1)
webhook_server/libs/log_parser.py (4)
LogEntry(16-49)parse_log_entry(129-183)extract_token_spend(261-280)monitor_log_directory(504-535)
webhook_server/tests/test_edge_cases_validation.py (2)
webhook_server/libs/log_parser.py (1)
LogEntry(16-49)webhook_server/web/log_viewer.py (2)
get_log_entries(91-224)export_logs(271-385)
🪛 LanguageTool
CLAUDE.md
[uncategorized] ~190-~190: The official name of this software platform is spelled with a capital “H”.
Context: ... main FastAPI app (app.py) - Pattern: __init__(github_webhook, ...) → `process_event(event_d...
(GITHUB)
[typographical] ~329-~329: Consider using an en dash here instead of a hyphen.
Context: ... is synchronous - each operation blocks 100ms-2 seconds - Blocking = frozen server (no ...
(QB_NEW_EN_DASH_RULE_EN)
[uncategorized] ~372-~372: The official name of this software platform is spelled with a capital “H”.
Context: ...ion for check_run Events Location: webhook_server/libs/github_api.py lines 534-570 **Early exit con...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
37-37: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
43-43: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
48-48: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
53-53: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
64-64: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
65-65: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
77-77: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
85-85: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
87-87: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
95-95: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
97-97: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
105-105: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
106-106: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
119-119: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
129-129: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
159-159: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
166-166: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
173-173: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
211-211: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
212-212: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
217-217: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
218-218: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
226-226: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
227-227: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
433-433: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
447-447: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
462-462: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
480-480: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
523-523: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
525-525: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
533-533: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 Ruff (0.14.10)
webhook_server/utils/app_utils.py
176-176: Avoid specifying long messages outside the exception class
(TRY003)
183-185: Avoid specifying long messages outside the exception class
(TRY003)
187-190: Avoid specifying long messages outside the exception class
(TRY003)
203-204: Logging statement uses f-string
(G004)
webhook_server/tests/test_log_viewer.py
200-200: Async functions should not open files with blocking methods like open
(ASYNC230)
375-375: Async functions should not open files with blocking methods like open
(ASYNC230)
507-507: Async functions should not open files with blocking methods like open
(ASYNC230)
528-528: Async functions should not open files with blocking methods like open
(ASYNC230)
554-554: Async functions should not open files with blocking methods like open
(ASYNC230)
590-590: Async functions should not open files with blocking methods like open
(ASYNC230)
634-634: Async functions should not open files with blocking methods like open
(ASYNC230)
658-658: Async functions should not open files with blocking methods like open
(ASYNC230)
678-678: Async functions should not open files with blocking methods like open
(ASYNC230)
703-703: Async functions should not open files with blocking methods like open
(ASYNC230)
759-759: Async functions should not open files with blocking methods like open
(ASYNC230)
798-798: Async functions should not open files with blocking methods like open
(ASYNC230)
843-843: Async functions should not open files with blocking methods like open
(ASYNC230)
864-864: Async functions should not open files with blocking methods like open
(ASYNC230)
910-910: Async functions should not open files with blocking methods like open
(ASYNC230)
933-933: Unused function argument: args
(ARG001)
933-933: Unused function argument: kwargs
(ARG001)
934-934: Avoid specifying long messages outside the exception class
(TRY003)
969-969: Async functions should not open files with blocking methods like open
(ASYNC230)
1003-1003: Async functions should not open files with blocking methods like open
(ASYNC230)
1063-1063: Unused function argument: code
(ARG001)
1063-1063: Unused function argument: reason
(ARG001)
1064-1064: Create your own exception
(TRY002)
1064-1064: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (10)
CLAUDE.md (4)
428-492: Verify structured logging section details against actual implementation.The new structured logging section (lines 428-492) introduces context creation, step tracking, and log format documentation. Ensure the following match the actual implementation:
- Context creation parameters (line 436-443):
hook_id,event_type,repository,action,sender,api_user— verify these matchWebhookContextdataclass signature- Step tracking methods (line 451-459):
start_step(),complete_step(),fail_step()withexceptionandtraceback_strparameters — verify method signatures- Context access pattern (line 450):
get_context()— verify this correctly retrieves the ContextVar-backed context- Log file path format (line 475):
{config.data_dir}/logs/webhooks_YYYY-MM-DD.json— verify this matchesStructuredLoggerimplementation- JSON structure (lines 480-492): Field names and nesting (
pr,timing,workflow_steps,token_spend,success) — verify against actual outputThis documentation sets expectations for developers, so accuracy is critical. If any parameter names, method signatures, or file paths differ from the implementation, update the documentation to match.
265-327: Verify PyGithub property access list (line 275) for completeness and accuracy.The non-blocking guidance section lists properties that trigger API calls:
.draft,.mergeable,.state,.committer,.author,.permissions,.labels,.assignees. Ensure this list is:
- Complete — Are there other commonly-used properties that also block and should be listed?
- Accurate — Do all listed properties actually trigger API calls, or are some cached from webhook payload?
- Consistent — Does the guidance match what's verified in test cases and actual handler code?
This list drives developer behavior, so accuracy is critical.
1-600: Document reflects PR migration well; address formatting and verify technical details.Strengths:
- Comprehensive coverage of structured JSON logging patterns with clear examples
- New non-blocking PyGithub guidance with decision tree is well-explained and critical for async correctness
- Anti-defensive programming section expanded with clear violations and acceptable patterns
- Architecture guarantees section provides helpful context for when defensive checks are unnecessary
- Alignment with PR objectives: ContextVar-based context, structured logging, fail-fast principle
Action items (before merge):
- Fix 20+ markdown formatting violations (blank lines around headings and code blocks)
- Correct "GitHub" capitalization (should be capitalized, not "github")
- Verify technical accuracy of structured logging section (context parameters, method signatures, file paths) — see companion comment above
- Verify PyGithub property access list is complete and accurate — see companion comment above
These are fixable issues that don't indicate deeper problems with the content.
190-190: Capitalize GitHub (proper noun) consistently.Lines 190 and 372 should use "GitHub" (capitalized) instead of "github". GitHub is a proper noun and must always be capitalized for consistency with official branding and documentation standards.
Also applies to: 372-372
⛔ Skipped due to learnings
Learnt from: myakove Repo: myk-org/github-webhook-server PR: 586 File: webhook_server_container/libs/github_api.py:1947-1956 Timestamp: 2024-10-08T09:19:56.185Z Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.Learnt from: CR Repo: myk-org/github-webhook-server PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-25T12:39:43.079Z Learning: Applies to webhook_server/libs/handlers/**/*.py : Handler pattern: implement `__init__(self, github_webhook: GithubWebhook, ...)` and `process_event(event_data: dict) -> None` for all GitHub event handlersLearnt from: CR Repo: myk-org/github-webhook-server PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-25T12:39:43.079Z Learning: Applies to webhook_server/libs/handlers/**/*.py : Architecture guarantee: `repository_data` is ALWAYS set before handlers instantiate in `GithubWebhook.process()` with fail-fast exceptions - no defensive checks neededLearnt from: CR Repo: myk-org/github-webhook-server PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-25T12:39:43.079Z Learning: Maintain backward compatibility for webhook payload handling - must follow GitHub webhook specification to ensure compatibility with GitHub API changesLearnt from: CR Repo: myk-org/github-webhook-server PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-25T12:39:43.079Z Learning: Applies to webhook_server/libs/handlers/**/*.py : Use `self.github_webhook.unified_api` for all GitHub API operations in handlers - never access GitHub API directlyLearnt from: myakove Repo: myk-org/github-webhook-server PR: 588 File: webhook_server_container/libs/github_api.py:1632-1637 Timestamp: 2024-10-14T14:13:21.316Z Learning: In the `ProcessGithubWehook` class in `webhook_server_container/libs/github_api.py`, avoid using environment variables to pass tokens because multiple commands with multiple tokens can run at the same time.webhook_server/tests/test_edge_cases_validation.py (3)
9-11: LGTM - Proper imports for async type hintsThe added imports (
AsyncIterator,Any) correctly support the async generator type hints and generic dict annotations required for mypy strict mode compatibility.
712-747: Excellent async conversion with complete type hintsThe test has been properly converted to async with all the previously flagged issues addressed:
async_iter_empty()has explicitAsyncIterator[LogEntry]return type- Uses
side_effect=lambda *_, **__: async_iter_empty()to create fresh generators per call- All controller method calls properly awaited
- Type hints fully compatible with mypy strict mode
The
if False: yieldpattern is intentional to create an explicit empty async generator, as discussed in previous reviews.
749-794: Well-structured large dataset test with proper async patternsThe test correctly:
- Converts to async with complete type hints (
AsyncIterator[LogEntry])- Uses fresh generator instances via
side_effect=lambda- Awaits all async controller methods
- Tests appropriate edge cases: large datasets (100k entries), pagination limits, and export size constraints
- Handles
HTTPExceptionwith status 413 for oversized exports per the controller's contractwebhook_server/utils/app_utils.py (1)
126-221: LGTM! Explicit validation correctly implements fail-fast principle.The implementation properly validates workflow step data before formatting:
- Lines 182-190: Explicit checks for
"status"and"duration_ms"presence, plusNonecheck forduration_ms, raisingValueErrorwith clear messages if validation fails.- Lines 191-193: Direct field access after validation ensures
format_duration(step_duration_ms)never receivesNone.This aligns perfectly with the coding guideline: "Never return fake default values ('', 0, False, None objects, []) to hide missing data - raise exceptions with clear error messages instead to enable fail-fast debugging."
The explicit validation approach surfaces instrumentation gaps immediately (e.g., if a handler forgets to call
complete_step()/fail_step()), which is exactly what you want for debugging.Static analysis notes (informational only, no action needed):
- TRY003 warnings about long exception messages are acceptable here—the contextual details (step name, field name) are necessary for debugging.
- G004 warning about f-strings in logging is a false positive—per project learnings, f-strings are the established style for log interpolation in this codebase.
webhook_server/tests/test_log_viewer.py (1)
1-1217: LGTM! Comprehensive test coverage with past review items addressed.This test suite provides excellent coverage of JSON log streaming, log retrieval, export, workflow steps, and edge cases. All past review comments have been properly addressed:
✅ Fixture alignment (lines 85-101): Timing schema now uses
completed_atandduration_ms, matching production structure
✅ Type annotations: Mock helper functions have complete type hints (e.g.,async def mock_close(code: int, reason: str) -> None)
✅ Async best practices:time.sleepreplaced withawait asyncio.sleepto avoid blocking event loop
✅ Meaningful assertions: No-op checks likelen(entries) >= 0replaced with actual validationsRegarding ASYNC230 warnings (informational, no changes needed):
The static analyzer flags blockingopen()calls in async test methods. These warnings are acceptable because:
- File I/O occurs during test setup (creating fixtures), not during async test execution
- Test data files are tiny and write operations are fast
- The actual async test logic (streaming, iteration) correctly uses async patterns
- Coding guidelines mandate
asyncio.to_thread()for PyGithub calls and production code—test fixture setup is explicitly exempt from this requirementThe test structure is clean, readable, and provides robust validation of the JSON-first log viewer functionality.
webhook_server/tests/test_log_parser.py (1)
1-1379: LGTM! All past review items addressed, excellent test coverage added.This file adds comprehensive JSON log parsing tests while properly addressing all previous review concerns:
✅ Type hints (lines 274-279):
_collect_entrieshelper now has complete annotations:async def _collect_entries( self, async_gen: AsyncIterator[LogEntry], entries_list: list[LogEntry], max_entries: int = 10, ) -> None:✅ UTC-aware datetimes (lines 1269, 1276, 1283, 1302, 1309, 1316): All
datetime.datetime()calls now includetzinfo=datetime.UTC, resolving DTZ001 warnings and ensuring consistency with production logging.✅ Non-blocking I/O in async tests (lines 252-258, 1361-1366): File append operations properly wrapped with
asyncio.to_thread():def _append_entries() -> None: with open(f.name, "a") as append_f: # ... write operations await asyncio.to_thread(_append_entries)New test coverage highlights:
- TestJSONLogParsing (lines 772-1192): 30+ tests validating JSON log entry parsing, including valid/invalid JSON, timestamp handling, PR extraction, error scenarios, and timezone formats
- TestAdditionalCoverageTests (lines 1194-1379): Edge cases for parsing (invalid PR numbers, escaped brackets), filtering (unique values, counts), token spend extraction, and directory monitoring with rotated files
The test suite now provides robust validation of both legacy text log parsing and new JSON-first parsing paths, ensuring backward compatibility while enabling the structured logging migration.
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
Add try-except wrapper to label_pull_request_by_merge_state with ctx.fail_step() to match PR #968 structured logging patterns. Also fix log message format.
Summary
This PR migrates the webhook server from traditional line-based logging to structured JSON logging with comprehensive context tracking and workflow step recording.
Key Features
Architecture Changes
New Components
webhook_server/libs/context.pyWebhookContextdataclass for storing webhook metadata and workflow stepswebhook_contextContextVar for async-safe context propagationwebhook_server/libs/structured_logger.pyStructuredLoggerclass for writing JSON log entriesUpdated Components
webhook_server/app.pyAll handlers (pull_request, issue_comment, check_run, etc.)
context.add_step()webhook_server/libs/log_parser.py&webhook_server/web/log_viewer.pyRemoved Components
logger.step()custom logging methodlogger.success()custom logging methodformat_task_fields()helper functionBenefits
Example Log Entry
{ "context": { "delivery_id": "12345678-1234-1234-1234-123456789abc", "duration": "1.23s", "event": "pull_request", "event_data": {...}, "owner": "myk-org", "pr_number": 123, "repository": "github-webhook-server", "sha": "abc123...", "start_time": "2026-01-05T12:34:56.789012", "steps": [ { "duration": "0.12s", "step": "Fetching PR data", "timestamp": "2026-01-05T12:34:56.901234" }, { "duration": "0.45s", "step": "Running auto-merge checks", "timestamp": "2026-01-05T12:34:57.112345" } ] }, "level": "info", "message": "Webhook processing completed", "timestamp": "2026-01-05T12:34:57.901234" }Testing
All 1058 tests pass with 91.04% coverage:
Migration Notes
Backward compatibility maintained:
Deployment:
Test Plan
Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Removals
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.