refactor: complete project-wide migration to structured logging#52
Open
dkargatzis wants to merge 1 commit intomainfrom
Open
refactor: complete project-wide migration to structured logging#52dkargatzis wants to merge 1 commit intomainfrom
dkargatzis wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR standardizes logging across the codebase by replacing Python's stdlib logging with structlog, converting log calls to structured key/value events. It also migrates several Enum subclasses to StrEnum and updates a few async type hints (lifespan/log_operation) and adds a RuleCreationProcessor init. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
src/rules/acknowledgment.py (1)
113-128:⚠️ Potential issue | 🟠 MajorAvoid logging raw comment content and include required boundary fields.
Line 113 currently logs full
comment_body(external/user text), and Lines 113/122/126/128 omit the required structured boundary fields. This creates privacy risk and inconsistent observability.As per coding guidelines, "Use structured logging at boundaries with fields:
operation,subject_ids,decision,latency_ms" and "Strip secrets/PII ... keep raw reasoning out of logs (store summaries only)".Proposed fix
+from time import perf_counter import re from enum import StrEnum import structlog @@ def extract_acknowledgment_reason(comment_body: str) -> str: @@ - logger.info("extracting_acknowledgment_reason_from", comment_body=comment_body) + started_at = perf_counter() + logger.info( + "acknowledgment_reason_extraction_started", + operation="extract_acknowledgment_reason", + subject_ids=[], + decision="started", + latency_ms=0, + comment_length=len(comment_body), + ) for i, pattern in enumerate(ACKNOWLEDGMENT_PATTERNS): match = re.search(pattern, comment_body, re.IGNORECASE | re.DOTALL) if match: @@ - logger.info("pattern_matched_reason", reason=reason) + logger.info( + "acknowledgment_reason_pattern_matched", + operation="extract_acknowledgment_reason", + subject_ids=[], + decision="matched", + latency_ms=round((perf_counter() - started_at) * 1000, 2), + pattern_index=i, + reason_length=len(reason), + ) if reason: return reason else: - logger.debug("pattern_did_not_match") + logger.debug( + "acknowledgment_reason_pattern_not_matched", + operation="extract_acknowledgment_reason", + subject_ids=[], + decision="not_matched", + latency_ms=round((perf_counter() - started_at) * 1000, 2), + pattern_index=i, + ) - logger.info("no_patterns_matched_for_acknowledgment_reason") + logger.info( + "acknowledgment_reason_not_found", + operation="extract_acknowledgment_reason", + subject_ids=[], + decision="no_match", + latency_ms=round((perf_counter() - started_at) * 1000, 2), + ) return ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/acknowledgment.py` around lines 113 - 128, The code currently logs raw comment_body and omits required structured boundary fields; update all logger calls in this block (the initial logger.info, the logger.info("pattern_matched_reason"), logger.debug, and final logger.info) to stop emitting raw user text and instead log only a sanitized summary/flag (e.g., has_reason: true/false) and include the mandated structured fields: operation (e.g., "extract_ack_reason"), subject_ids (pass the relevant IDs or an empty list), decision (e.g., "ack_extracted" or "no_ack"), and latency_ms (measure elapsed time around the loop); when logging the extracted reason variable from ACKNOWLEDGMENT_PATTERNS, do not log the full reason—log a length-limited/hashed summary or a boolean has_reason and reason_summary—while preserving use of the existing ACKNOWLEDGMENT_PATTERNS and reason variable names so the changes are localized.src/core/utils/caching.py (1)
60-65:⚠️ Potential issue | 🟡 MinorIncomplete migration: Line 62 still uses f-string interpolation.
The expiration log on line 62 wasn't converted to structured format, inconsistent with the PR objective and the adjacent line 65.
🔧 Proposed fix
if age >= self.ttl: del self._cache[key] - logger.debug(f"Cache entry '{key}' expired (age: {age:.2f}s, ttl: {self.ttl}s)") + logger.debug("cache_entry_expired", key=key, age_seconds=round(age, 2), ttl_seconds=self.ttl) return None logger.debug("cache_hit_for", key=key)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utils/caching.py` around lines 60 - 65, The expiration log still uses an f-string; update the logger call in the cache expiration branch (inside the method that checks age vs self.ttl) to a structured log like the adjacent cache hit line by passing a message key and the fields (e.g., key, age, ttl) to logger.debug instead of f-string interpolation so the log is consistent with structured logging for the cache (refer to logger.debug, self.ttl, key, age, and self._cache in the expiration branch).src/rules/loaders/github_loader.py (1)
42-76:⚠️ Potential issue | 🟠 MajorComplete the structured log envelope at boundaries (and remove remaining f-string logs).
Line 42 and Line 70 still use interpolated strings, and boundary logs in
get_rulesare missing required fields (operation,subject_ids,decision,latency_ms).Proposed patch (pattern)
+import time import structlog import yaml @@ async def get_rules(self, repository: str, installation_id: int) -> list[Rule]: + started_at = time.perf_counter() + log_ctx = { + "operation": "github_rule_loader.get_rules", + "subject_ids": {"repository": repository, "installation_id": installation_id}, + } try: @@ - logger.info(f"Fetching rules for repository: {repository} (installation: {installation_id})") + logger.info("fetch_rules_start", **log_ctx, decision="start", latency_ms=0) @@ - logger.warning("no_rulesyaml_file_found_in", repository=repository) + logger.warning( + "no_rulesyaml_file_found_in", + **log_ctx, + decision="missing_rules_file", + latency_ms=int((time.perf_counter() - started_at) * 1000), + ) @@ - logger.info(f"Successfully loaded {len(rules)} rules from {repository}") + logger.info( + "fetch_rules_success", + **log_ctx, + decision="success", + rules_count=len(rules), + latency_ms=int((time.perf_counter() - started_at) * 1000), + ) return rules @@ - logger.error("error_fetching_rules_for", repository=repository, e=e) + logger.error( + "error_fetching_rules_for", + **log_ctx, + decision="error", + error=str(e), + latency_ms=int((time.perf_counter() - started_at) * 1000), + ) raiseAs per coding guidelines:
Use structured logging at boundaries with fields: operation, subject_ids, decision, latency_ms.Also applies to: 92-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/loaders/github_loader.py` around lines 42 - 76, Replace the remaining f-string boundary logs and add the required structured fields in get_rules of GitHubRuleLoader: wrap the fetch with a start timestamp (time.monotonic()) and replace logger.info(f"...") at the start with a structured logger call like logger.info("fetching_rules", operation="get_rules", subject_ids=[repository], decision="pending", latency_ms=None, installation_id=installation_id); on successful return replace logger.info(f"Successfully loaded...") with logger.info("fetched_rules", operation="get_rules", subject_ids=[repository], decision="loaded", latency_ms=calculated_ms, count=len(rules)); update error/exception logs (the rule parse error and the outer exception) to include operation="get_rules", subject_ids=[repository], decision="error", latency_ms=calculated_ms and pass exception info via exc_info=True (or structured field like error=str(e)) instead of using f-strings; remove any remaining f-strings in this method.src/event_processors/check_run.py (1)
39-43:⚠️ Potential issue | 🟠 MajorCheck-run boundary logs are still only partially structured.
This path should emit boundary events with
operation,subject_ids,decision, and completionlatency_ms. Current mixed style makes tracing and policy compliance inconsistent.As per coding guidelines "Use structured logging at boundaries with fields: `operation`, `subject_ids`, `decision`, `latency_ms`".Suggested refactor
- logger.info("processing_check_run_event_for", repo_full_name=task.repo_full_name) - logger.info(f" Name: {check_run.get('name')}") - logger.info(f" Status: {check_run.get('status')}") - logger.info(f" Conclusion: {check_run.get('conclusion')}") + logger.info( + "check_run_processing_started", + operation="process_check_run", + subject_ids={"repo_full_name": task.repo_full_name, "check_run_id": check_run.get("id")}, + decision="accepted", + check_run_name=check_run.get("name"), + status=check_run.get("status"), + conclusion=check_run.get("conclusion"), + ) @@ - logger.error("no_installation_id_found_in_task") + logger.error( + "check_run_processing_rejected", + operation="process_check_run", + subject_ids={"repo_full_name": task.repo_full_name}, + decision="rejected", + reason="missing_installation_id", + )Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/check_run.py` around lines 39 - 43, Replace the mixed, human-friendly logs with a single structured boundary log: remove the four logger.info lines that print repo/name/status/conclusion and instead emit one logger.info call with fields operation="check_run.process" (or "check_run.process_completion" if this is the end-of-flow), subject_ids set to a list including task.repo_full_name and the check_run id (e.g. check_run.get("id")), decision set to check_run.get("conclusion"), and latency_ms computed from the task's start/timestamp (e.g. time.monotonic() - task.start_time or task.created_at) converted to milliseconds; apply the same replacement to the similar log at the later location (the one noted at line 57) so all boundary events use the same structured fields.src/rules/utils/validation.py (1)
20-31:⚠️ Potential issue | 🔴 CriticalRemove
src/rules/utils.pyas dead code; it's replaced bysrc/rules/utils/validation.pywhich is the canonical, properly-exported version.Three copies of
validate_rules_yaml_from_repoexist in the codebase:src/rules/utils.py,src/rules/utils/validation.py, andsrc/integrations/github/rules_service.py. Onlysrc/rules/utils/validation.pyis actually imported (viasrc/rules/utils/__init__.py); the other two are unused.Additionally,
src/integrations/github/rules_service.pyviolates structured logging guidelines at lines 25 and 109 by using f-strings instead of keyword arguments in logger calls.Delete
src/rules/utils.pyand either remove or refactorsrc/integrations/github/rules_service.pyto avoid maintenance burden and ensure compliance with logging standards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/utils/validation.py` around lines 20 - 31, Delete the dead duplicate module src/rules/utils.py and ensure all imports refer to the canonical src/rules/utils/validation.py (which defines validate_rules_yaml_from_repo); then open src/integrations/github/rules_service.py, remove or delegate its duplicate validate_rules_yaml_from_repo implementation to the canonical function, and replace the two logger f-string usages (the calls around the previously noted lines) with structured logging calls using keyword args, e.g. logger.info("message", pr_number=pr_number, repo_full_name=repo_full_name) so all logging conforms to the project's structured logging style.src/rules/conditions/filesystem.py (1)
188-188:⚠️ Potential issue | 🟡 MinorIncomplete migration: f-string log still present.
This log statement still uses f-string interpolation, which contradicts the PR objective to convert all f-string interpolations into kwargs for structured log payloads. The same issue exists at lines 213, 215, and 261.
Proposed fix
- logger.debug(f"MaxFileSizeCondition: All {len(files)} files are within size limit of {max_size_mb}MB") + logger.debug("maxfilesizecondition_all_files_within_limit", file_count=len(files), max_size_mb=max_size_mb)Similar changes needed for lines 213, 215, and 261.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/conditions/filesystem.py` at line 188, The f-string in the logger.debug call inside MaxFileSizeCondition should be converted to a structured log call: replace logger.debug(f"...{len(files)}...{max_size_mb}...") with a non-f-string message and supply the dynamic values as kwargs (e.g., logger.debug("MaxFileSizeCondition: All files are within size limit", files=len(files), max_size_mb=max_size_mb)); apply the same conversion pattern to the analogous logger.debug calls referenced at the other occurrences (lines with the same MaxFileSizeCondition context) so all structured logs use kwargs instead of f-strings.src/rules/utils/contributors.py (1)
72-72:⚠️ Potential issue | 🟡 MinorIncomplete migration: f-string log still present.
This log statement still uses f-string interpolation, inconsistent with the structured logging migration goal of this PR.
Proposed fix
- logger.info(f"Found {len(past_contributors)} past contributors for {repo}") + logger.info("found_past_contributors_for", count=len(past_contributors), repo=repo)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/utils/contributors.py` at line 72, Replace the f-string log in contributors.py (the logger.info line that references past_contributors and repo) with a structured log call: call logger.info with a static message and pass the dynamic values as structured fields (e.g., logger.info("Found past contributors", past_contributors_count=len(past_contributors), repo=repo)) so the message no longer uses f-string interpolation and adheres to the structured logging migration.src/event_processors/push.py (1)
85-85:⚠️ Potential issue | 🟡 MinorIncomplete migration: f-string logs still present.
Several log statements still use f-string interpolation (lines 39, 85, 149, 150), which is inconsistent with the PR's structured logging migration objective.
Proposed fix for line 85
- logger.info(f"📋 Loaded {len(rules)} rules for evaluation") + logger.info("loaded_rules_for_evaluation", count=len(rules))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/push.py` at line 85, The log at src/event_processors/push.py uses an f-string: replace logger.info(f"📋 Loaded {len(rules)} rules for evaluation") with structured logging by calling logger.info("Loaded rules for evaluation", rules_count=len(rules)) or logger.info("Loaded %d rules for evaluation", len(rules)); update the same pattern for the other f-string logs mentioned (lines with logger and variables at lines 39, 149, 150) so all logging uses structured/positional parameters instead of f-strings (look for the logger variable and the rules/related variables to change).src/core/utils/retry.py (1)
65-68:⚠️ Potential issue | 🟡 MinorIncomplete migration: f-string with emoji still present.
This warning log still uses f-string interpolation with emoji, inconsistent with the structured migration applied to the error log at line 61 in the same function.
Proposed fix
- logger.warning( - f"⚠️ {func.__name__} attempt {attempt + 1}/{max_retries} failed, " - f"retrying in {wait_time:.2f}s: {e}" - ) + logger.warning( + "attempt_failed_retrying_in_s", + func_name=func.__name__, + attempt=attempt + 1, + max_retries=max_retries, + wait_time_s=wait_time, + error=str(e), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utils/retry.py` around lines 65 - 68, Replace the f-string emoji log in the retry handler with a structured logging call consistent with the earlier error log change: remove the emoji and stop f-string interpolation in the logger.warning call (the call that references func.__name__, attempt, max_retries, wait_time, and e), and pass those values as separate positional arguments (and/or use exc_info=True if the previous error log used it) so the message becomes a format string plus arguments rather than an f-string; mirror the pattern used for the error log at the other logger call in the same function to keep logging consistent.src/event_processors/deployment_protection_rule.py (1)
136-136:⚠️ Potential issue | 🟡 MinorIncomplete migration: %-format string still present.
This log and similar ones at lines 169, 171, and 218 still use %-style or f-string formatting, which is inconsistent with the PR's structured logging migration objective.
Proposed fix
- logger.info("Found %d applicable rules for deployment", len(deployment_rules)) + logger.info("found_applicable_rules_for_deployment", count=len(deployment_rules))Apply similar changes to lines 169, 171, and 218.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/deployment_protection_rule.py` at line 136, Replace the %-style/inlined-format logs with structured-logging calls: for each occurrence (the logger.info that mentions deployment_rules at lines referencing the variable deployment_rules and the other two similar messages), stop using formatted message strings and instead log a constant message plus structured fields via the logger's extra (e.g. extra={"deployment_rules_count": len(deployment_rules), "rule_ids": [...]}) or by passing a dict/kwargs to the logger; locate and update uses around the variable deployment_rules and any other rule/count variables in this module (deployment_protection_rule.py) so messages like "Found %d applicable rules for deployment" become something like "Found applicable rules for deployment" with the counts and identifiers in extra fields for consistent structured logging across the file.src/agents/engine_agent/agent.py (2)
136-136:⚠️ Potential issue | 🟡 MinorAdditional f-string to convert.
Proposed fix
- logger.info(f"🔧 Rule Engine extracted {len(violations)} violations") + logger.info("rule_engine_violations_extracted", count=len(violations))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/engine_agent/agent.py` at line 136, Replace the f-string usage in the logger call so it uses structured logging instead of an f-string; change the line that calls logger.info(f"🔧 Rule Engine extracted {len(violations)} violations") to use logger.info with a format string and parameters (e.g., logger.info("🔧 Rule Engine extracted %d violations", len(violations))) or logger.info("🔧 Rule Engine extracted {} violations".format(len(violations))); this targets the logger.info call that references the violations variable in the same scope.
99-99:⚠️ Potential issue | 🟡 MinorAdditional f-string to convert.
Proposed fix
- logger.info(f"🔧 Rule Engine starting evaluation for {request.event_type} with {len(request.rules)} rules") + logger.info( + "rule_engine_starting_evaluation", + event_type=request.event_type, + rule_count=len(request.rules), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/engine_agent/agent.py` at line 99, Replace the f-string used in the logger.info call so it follows the project's non-f-string logging style: change the logger.info(f"🔧 Rule Engine starting evaluation for {request.event_type} with {len(request.rules)} rules") to a non-f-string form that passes values as parameters (e.g., logger.info("🔧 Rule Engine starting evaluation for %s with %d rules", request.event_type, len(request.rules))) or use .format on the string; update the call where logger.info is invoked in the Rule Engine evaluation path that references request.event_type and len(request.rules).src/agents/acknowledgment_agent/test_agent.py (1)
90-94:⚠️ Potential issue | 🟡 MinorIncomplete migration: f-strings still used in log statements.
These lines still use f-string interpolation instead of structured kwargs.
Proposed fix
- logger.info(f" Valid: {result.data.get('is_valid', False)}") - logger.info(f" Reasoning: {result.data.get('reasoning', 'No reasoning')}") - logger.info(f" Acknowledged violations: {len(result.data.get('acknowledgable_violations', []))}") - logger.info(f" Require fixes: {len(result.data.get('require_fixes', []))}") - logger.info(f" Confidence: {result.data.get('confidence', 0.0)}") + logger.info( + "acknowledgment_result", + is_valid=result.data.get("is_valid", False), + reasoning=result.data.get("reasoning", "No reasoning"), + acknowledged_count=len(result.data.get("acknowledgable_violations", [])), + require_fixes_count=len(result.data.get("require_fixes", [])), + confidence=result.data.get("confidence", 0.0), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/acknowledgment_agent/test_agent.py` around lines 90 - 94, Replace the f-string log statements in test_agent.py with structured logging calls: use logger.info with a constant message and pass values as separate parameters or as structured kwargs (reference the logger variable and the result.data keys is_valid, reasoning, acknowledgable_violations, require_fixes, confidence) — e.g., change logger.info(f" Valid: {result.data.get('is_valid', False)}") to a structured form that supplies the boolean via arguments or named fields, and do the same for the "Reasoning", "Acknowledged violations", "Require fixes", and "Confidence" lines so no f-strings remain.src/agents/engine_agent/nodes.py (1)
37-48:⚠️ Potential issue | 🟡 MinorIncomplete migration: f-strings still used in multiple log statements.
These log statements still use f-string interpolation instead of structured kwargs. This is inconsistent with the PR objective to convert f-strings to kwargs.
Proposed fix for this segment
- logger.info(f"🔍 Analyzing {len(state.rule_descriptions)} rule descriptions") + logger.info("analyzing_rule_descriptions", count=len(state.rule_descriptions)) # Filter rules applicable to this event type applicable_rules = [] for rule_desc in state.rule_descriptions: if state.event_type in rule_desc.event_types: applicable_rules.append(rule_desc) - logger.info(f"🔍 Rule '{rule_desc.description[:50]}...' is applicable to {state.event_type}") + logger.debug("rule_applicable", rule=rule_desc.description[:50], event_type=state.event_type) else: - logger.info( - f"🔍 Rule '{rule_desc.description[:50]}...' is NOT applicable (expects: {rule_desc.event_types})" - ) + logger.debug("rule_not_applicable", rule=rule_desc.description[:50], expected_types=rule_desc.event_types)Note: Similar f-string patterns remain throughout the file (lines 71, 84, 93, 126, 131, 155, 169-170, 182, 226, 251, 259-261, 312, 351, 383, 404).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/engine_agent/nodes.py` around lines 37 - 48, Replace the f-string log calls with structured logging calls: use logger.info with a format string and positional parameters (e.g. logger.info("🔍 Analyzing %d rule descriptions", len(state.rule_descriptions))) or with explicit kwargs if your logger supports them (e.g. logger.info("🔍 Rule '%s...' is applicable to %s", rule_desc.description[:50], state.event_type) or logger.info("🔍 Rule applicable", rule_description=rule_desc.description[:50], event_type=state.event_type)); update every occurrence in this file where logger.info is called using f-strings (e.g. the block iterating state.rule_descriptions referencing logger, state.rule_descriptions, rule_desc, and state.event_type) and apply the same pattern to the other listed lines to ensure structured kwargs/parameters are used instead of f-strings.
🟡 Minor comments (15)
src/event_processors/deployment_status.py-52-52 (1)
52-52:⚠️ Potential issue | 🟡 MinorInconsistent: f-string still used instead of structured kwargs.
This log statement still uses f-string interpolation, which contradicts the PR objective to convert all f-string interpolations to kwargs for structured log payloads. Per coding guidelines, boundary logs should include fields like
operationandlatency_ms.Proposed fix
- logger.info(f"🏁 DEPLOYMENT_STATUS processing completed in {int((time.time() - start_time) * 1000)}ms") + logger.info( + "deployment_status_processing_completed", + operation="deployment_status", + latency_ms=int((time.time() - start_time) * 1000), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/deployment_status.py` at line 52, Replace the f-string in the logger call so it uses structured kwargs instead of interpolation: compute latency_ms = int((time.time() - start_time) * 1000) and call logger.info with a static message (e.g. "🏁 DEPLOYMENT_STATUS processing completed") plus keyword fields operation="DEPLOYMENT_STATUS" and latency_ms=latency_ms (and include any existing context if present), updating the logger.info invocation used in deployment_status.py to pass these kwargs rather than an f-string.src/rules/utils/codeowners.py-201-203 (1)
201-203:⚠️ Potential issue | 🟡 MinorImprove exception logging for better serialization.
Passing the raw exception object as
e=emay not serialize cleanly in all structlog configurations. Use explicit string conversion and consider adding stack trace context:except Exception as e: - logger.error("error_loading_codeowners_file", e=e) + logger.error("error_loading_codeowners_file", operation="load_codeowners", error=str(e), exc_info=True) return NoneThis ensures:
error=str(e)provides a serializable error messageexc_info=Truecaptures the stack trace for debuggingoperationfield aligns with boundary logging requirements🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/utils/codeowners.py` around lines 201 - 203, The exception logging in the except block currently passes the raw exception object (logger.error("error_loading_codeowners_file", e=e)); change it to log a serializable string and include the stack trace and an operation field: call logger.error with a descriptive message key, error=str(e), exc_info=True and operation="load_codeowners_file" (or similar) so the error text is serializable and the stack trace is captured for debugging.src/core/utils/caching.py-178-182 (1)
178-182:⚠️ Potential issue | 🟡 MinorAvoid using
__name__as a log field key.Using dunder names (
__name__) as keyword arguments is unconventional and may cause confusion with Python's built-in module attribute. Use a clearer field name.🔧 Proposed fix
if cached_value is not None: - logger.debug("cache_hit_for_with_key", __name__=func.__name__, cache_key=cache_key) + logger.debug("cache_hit", func_name=func.__name__, cache_key=cache_key) return cached_value # Cache miss - execute function - logger.debug("cache_miss_for_with_key", __name__=func.__name__, cache_key=cache_key) + logger.debug("cache_miss", func_name=func.__name__, cache_key=cache_key) result = await func(*args, **kwargs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utils/caching.py` around lines 178 - 182, The log calls in caching.py use the dunder keyword "__name__" as a log field which is confusing; change both logger.debug invocations that pass __name__=func.__name__ (the cache hit and cache miss logs) to use a clear field name such as "func_name" or "function" (e.g., func_name=func.__name__) and keep cache_key as-is so logs show func_name and cache_key consistently for cache_hit_for_with_key and cache_miss_for_with_key.src/agents/feasibility_agent/nodes.py-125-126 (1)
125-126:⚠️ Potential issue | 🟡 MinorIncomplete migration: f-string log on line 126.
Line 126 still uses f-string interpolation instead of structured kwargs.
Proposed fix
logger.info("yaml_configuration_generated_for_rule_type", rule_type=state.rule_type) - logger.info(f"🔧 Generated YAML length: {len(state.yaml_content)} characters") + logger.info("yaml_generated", yaml_length=len(state.yaml_content))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/feasibility_agent/nodes.py` around lines 125 - 126, Replace the f-string logging on the YAML length with structured logging: locate the logger.info call near logger.info("yaml_configuration_generated_for_rule_type", rule_type=state.rule_type) in nodes.py (the place that references state.yaml_content) and change the f-string logger.info(f"🔧 Generated YAML length: {len(state.yaml_content)} characters") to a structured call that passes the length as a kwarg (e.g., logger.info("generated_yaml_length", length=len(state.yaml_content)) or similar) so no f-string interpolation is used.src/agents/feasibility_agent/nodes.py-58-61 (1)
58-61:⚠️ Potential issue | 🟡 MinorIncomplete migration: f-string log on line 61.
Line 61 still uses f-string interpolation, which contradicts the PR objective to convert all f-strings into kwargs. Additionally, per coding guidelines, structured logs at boundaries should include fields like
operationandlatency_ms.Proposed fix
logger.info("rule_feasibility_analysis_completed", is_feasible=state.is_feasible) logger.info("rule_type_identified", rule_type=state.rule_type) logger.info("confidence_score", confidence_score=state.confidence_score) - logger.info(f"🔍 Analysis steps: {len(state.analysis_steps)} steps") + logger.info("analysis_steps_completed", step_count=len(state.analysis_steps))Based on learnings: "Use structured logging at boundaries with fields:
operation,subject_ids,decision,latency_ms"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/feasibility_agent/nodes.py` around lines 58 - 61, Replace the f-string logger call that logs analysis steps with a structured logger.info call: compute steps_count = len(state.analysis_steps) and call logger.info("rule_analysis_steps", steps_count=steps_count, operation="rule_feasibility_analysis", subject_ids=getattr(state, "subject_ids", None), decision=state.is_feasible, latency_ms=getattr(state, "latency_ms", None)); update the nearby logging lines (logger.info("rule_feasibility_analysis_completed", ...), logger.info("rule_type_identified", ...), logger.info("confidence_score", ...)) to follow the same structured pattern if needed so all boundary logs use kwargs rather than f-strings and include operation/subject_ids/decision/latency_ms.src/agents/feasibility_agent/nodes.py-122-122 (1)
122-122:⚠️ Potential issue | 🟡 MinorTruncated log message and missing context.
The log key
generated_yaml_seems_too_short_mayappears truncated ("may" at the end is incomplete). Include the actual length for debugging context.Proposed fix
- logger.warning("generated_yaml_seems_too_short_may") + logger.warning("generated_yaml_too_short", yaml_length=len(state.yaml_content))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/feasibility_agent/nodes.py` at line 122, The log message key logger.warning("generated_yaml_seems_too_short_may") is truncated and lacks context; update the call to use a complete message (e.g., "generated_yaml_seems_too_short_maybe" or "generated_yaml_too_short") and include the actual length of the YAML string for debugging by passing the length (e.g., len(generated_yaml) or the variable holding the YAML) in the log message so the warning shows both the descriptive text and the numeric length.src/event_processors/violation_acknowledgment.py-60-64 (1)
60-64:⚠️ Potential issue | 🟡 MinorIncomplete migration and potential PII in logs.
Several issues in this logging block:
- Lines 60, 64 use visual separators (
"=" * 80) which don't add value in structured logging systems- Line 63 still uses f-string interpolation instead of structured kwargs
- Line 63 logs user comment content which may contain PII
Per coding guidelines, strip PII from logs and use structured fields like
operation,subject_ids.♻️ Suggested refactor
- logger.info("=" * 80) - logger.info("processing_violation_acknowledgment_for", repo=repo, pr_number=pr_number) - logger.info("commenter", commenter=commenter) - logger.info(f" Comment: {comment_body[:100]}...") - logger.info("=" * 80) + logger.info( + "processing_violation_acknowledgment", + operation="violation_acknowledgment", + repo=repo, + pr_number=pr_number, + commenter=commenter, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/violation_acknowledgment.py` around lines 60 - 64, Remove the decorative separator logs and stop logging raw comment text; replace the f-string logger call that mentions comment_body with a structured logger call that uses kwargs (e.g., operation="violation_acknowledgment", subject_ids={"repo": repo, "pr_number": pr_number}, actor=commenter) and either omit comment_body entirely or log a redacted/hashed placeholder (e.g., comment_present=True or comment_hash) to avoid PII; update the three logger calls that currently use "="*80, the "processing_violation_acknowledgment_for" call, the "commenter" call, and the f-string line so all use structured fields and include operation/subject_ids as shown.src/rules/conditions/pull_request.py-142-142 (1)
142-142:⚠️ Potential issue | 🟡 MinorFormatter drift remains in updated logger calls.
Pre-commit is currently failing on formatting. Please run
pre-commit run --all-files(orruff format) and commit the result.Also applies to: 210-210, 245-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/conditions/pull_request.py` at line 142, Formatting drift in the updated logger.debug calls (e.g., the logger.debug(...) call in src/rules/conditions/pull_request.py and the similar occurrences noted) is causing pre-commit failures; run the formatter (pre-commit run --all-files or ruff format) and commit the changes so the logger calls conform to project style, and verify the three affected logger.debug lines (the one shown plus the two at the other commented locations) are updated consistently by the formatter before committing.src/event_processors/rule_creation.py-63-63 (1)
63-63:⚠️ Potential issue | 🟡 MinorEvent key embeds value - use a field instead.
The event key
api_calls_made_1embeds the value in the key name. This loses the benefit of structured logging where values should be in fields for querying and aggregation.Proposed fix
- logger.info("api_calls_made_1") + logger.info("api_calls_made", count=1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/rule_creation.py` at line 63, The log call in rule_creation.py currently embeds the value in the event key ("api_calls_made_1"); change the logger.info invocation (the logger.info call at that location) to emit a stable event name/message (e.g., "api_calls_made") and pass the numeric value as a structured field (e.g., api_calls_made: 1) so querying/aggregation can use the field instead of the key. Ensure you use the logger's structured logging API (keyword args or an "extra"/"fields" dict depending on your logger) and remove the hard-coded value from the message.src/event_processors/push.py-43-43 (1)
43-43:⚠️ Potential issue | 🟡 MinorInconsistent event key naming.
The event key
pushskippeddeletedoremptylacks underscores, making it inconsistent with other keys likepush_processing_completed_in_ms. Use a consistent naming convention.Proposed fix
- logger.info("pushskippeddeletedorempty") + logger.info("push_skipped_deleted_or_empty")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/push.py` at line 43, Replace the inconsistent event key string "pushskippeddeletedorempty" with a snake_case key to match the project's convention (e.g., "push_skipped_deleted_or_empty") wherever it's logged; update the call to logger.info in src/event_processors/push.py (the logger.info(...) that emits "pushskippeddeletedorempty") so it uses the new snake_case key and ensure it aligns stylistically with existing keys like "push_processing_completed_in_ms".src/agents/acknowledgment_agent/agent.py-179-184 (1)
179-184:⚠️ Potential issue | 🟡 MinorMixed logging styles and raw reasoning in logs.
- Lines 182-183 still use f-strings instead of structured kwargs
- Line 181 logs full LLM reasoning text, which may violate the guideline to "keep raw reasoning out of logs (store summaries only)"
Proposed fix
logger.info("intelligent_evaluation_completed") logger.info("valid", is_valid=structured_result.is_valid) - logger.info("reasoning", reasoning=structured_result.reasoning) - logger.info(f" Acknowledged violations: {len(acknowledgable_violations)}") - logger.info(f" Require fixes: {len(require_fixes)}") + logger.info("evaluation_summary", acknowledged_count=len(acknowledgable_violations), require_fixes_count=len(require_fixes)) logger.info("confidence", confidence=structured_result.confidence)Based on learnings: "Strip secrets/PII from agent prompts; scope tools; keep raw reasoning out of logs (store summaries only)"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/acknowledgment_agent/agent.py` around lines 179 - 184, The logging block mixes f-strings and structured kwargs and also logs raw LLM reasoning text; update the logger calls in the intelligent evaluation completion section to use structured kwargs consistently (e.g., logger.info("valid", is_valid=structured_result.is_valid) and logger.info("acknowledgement_summary", acknowledged_count=len(acknowledgable_violations), require_fixes_count=len(require_fixes))) and remove or replace the raw reasoning log (logger.info("reasoning", reasoning=...)) with a sanitized/short summary field (e.g., structured_result.reasoning_summary or a truncated/scrubbed summary) to avoid storing full LLM reasoning or PII. Ensure you reference the same symbols: logger, structured_result, acknowledgable_violations, require_fixes, and intelligent_evaluation_completed.src/agents/engine_agent/agent.py-51-53 (1)
51-53:⚠️ Potential issue | 🟡 MinorIncomplete migration: f-string on line 52.
Line 52 still uses f-string interpolation while lines 51 and 53 use structured keys.
Proposed fix
logger.info("rule_engine_agent_initializing") - logger.info(f"🔧 Available validators: {len(AVAILABLE_CONDITIONS)}") + logger.info("available_validators", count=len(AVAILABLE_CONDITIONS)) logger.info("validation_strategy_hybrid_validators_llm_fallback")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/engine_agent/agent.py` around lines 51 - 53, Replace the f-string logging on the rule engine agent init with a structured log entry consistent with the other lines: instead of logger.info(f"🔧 Available validators: {len(AVAILABLE_CONDITIONS)}") use logger.info with a message key and a numeric field (e.g., "available_validators") and pass len(AVAILABLE_CONDITIONS) as the value; update the call in agent.py near the existing logger.info("rule_engine_agent_initializing") and logger.info("validation_strategy_hybrid_validators_llm_fallback") lines so the log uses structured arguments rather than f-string interpolation.src/webhooks/handlers/deployment_status.py-31-33 (1)
31-33:⚠️ Potential issue | 🟡 MinorIncomplete migration: Line 33 still uses f-string interpolation.
Lines 31-32 correctly use structured kwargs, but line 33 still uses an f-string. This should be converted to structured logging to complete the migration.
Proposed fix
logger.info("enqueuing_deployment_status_event_for", repo_full_name=repo_full_name) logger.info("state", state=state) - logger.info(f" Environment: {deployment.get('environment', 'unknown')}") + logger.info("deployment_environment", environment=deployment.get("environment", "unknown"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhooks/handlers/deployment_status.py` around lines 31 - 33, The log call still uses an f-string; update the logger.info that prints the Environment to use structured logging like the other calls: replace the f-string logger.info(f" Environment: {deployment.get('environment', 'unknown')}") with a structured logger.info call that provides a message and passes the environment value via a kwarg (use deployment.get('environment', 'unknown') as the value). Locate this in the deployment status handler where logger.info is used alongside repo_full_name and state.src/main.py-63-63 (1)
63-63:⚠️ Potential issue | 🟡 MinorInconsistent logging: still using standard
logginginstead ofstructlog.This line uses
logging.info()while the rest of the lifespan function (lines 87, 91, 97) uses the structloglogger.info(). This appears to be missed during the migration.Proposed fix
- logging.info("Watchflow application starting up...") + logger.info("watchflow_application_starting_up")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.py` at line 63, Replace the stray standard library call with the structlog logger: change the logging.info("Watchflow application starting up...") call to use the module-level structlog instance (logger.info(...)) so it matches the other messages in the lifespan function (see logger and lifespan). Also remove or adjust the unused standard logging import if it becomes unused after this change.src/agents/acknowledgment_agent/test_agent.py-100-105 (1)
100-105:⚠️ Potential issue | 🟡 MinorAdditional f-strings to convert.
These lines also use f-string interpolation.
Proposed fix
if result.data.get("acknowledgable_violations"): logger.info("n_acknowledged_violations") for violation in result.data["acknowledgable_violations"]: - logger.info(f" • {violation.get('rule_name')} - {violation.get('reason')}") + logger.info("acknowledged_violation", rule_name=violation.get("rule_name"), reason=violation.get("reason")) if result.data.get("require_fixes"): logger.info("n_violations_requiring_fixes") for violation in result.data["require_fixes"]: - logger.info(f" • {violation.get('rule_name')} - {violation.get('reason')}") + logger.info("violation_requiring_fix", rule_name=violation.get("rule_name"), reason=violation.get("reason"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/acknowledgment_agent/test_agent.py` around lines 100 - 105, Replace the remaining f-string logger calls with parameterized logging calls: change lines using logger.info(f" • {violation.get('rule_name')} - {violation.get('reason')}") to logger.info(" • %s - %s", violation.get("rule_name"), violation.get("reason")) for both the initial violations loop and the loop over result.data["require_fixes"]; keep the same message text but use logger.info formatting rather than f-strings so the calls in test_agent.py (the logger.info calls referencing violation and result.data["require_fixes"]) no longer use f-string interpolation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
src/agents/acknowledgment_agent/agent.pysrc/agents/acknowledgment_agent/test_agent.pysrc/agents/base.pysrc/agents/engine_agent/agent.pysrc/agents/engine_agent/models.pysrc/agents/engine_agent/nodes.pysrc/agents/factory.pysrc/agents/feasibility_agent/agent.pysrc/agents/feasibility_agent/nodes.pysrc/api/dependencies.pysrc/core/models.pysrc/core/utils/caching.pysrc/core/utils/logging.pysrc/core/utils/metrics.pysrc/core/utils/retry.pysrc/core/utils/timeout.pysrc/event_processors/base.pysrc/event_processors/check_run.pysrc/event_processors/deployment.pysrc/event_processors/deployment_protection_rule.pysrc/event_processors/deployment_review.pysrc/event_processors/deployment_status.pysrc/event_processors/pull_request/enricher.pysrc/event_processors/pull_request/processor.pysrc/event_processors/push.pysrc/event_processors/rule_creation.pysrc/event_processors/violation_acknowledgment.pysrc/integrations/github/check_runs.pysrc/main.pysrc/presentation/github_formatter.pysrc/rules/acknowledgment.pysrc/rules/conditions/base.pysrc/rules/conditions/filesystem.pysrc/rules/conditions/pull_request.pysrc/rules/loaders/github_loader.pysrc/rules/models.pysrc/rules/registry.pysrc/rules/utils.pysrc/rules/utils/codeowners.pysrc/rules/utils/contributors.pysrc/rules/utils/validation.pysrc/webhooks/handlers/deployment_review.pysrc/webhooks/handlers/deployment_status.pysrc/webhooks/handlers/issue_comment.py
- Migrate all remaining standard logging calls to structlog - Convert f-string interpolations into kwargs for structured log payloads - Remove unused variables and fix enum types in core models - Ensure 100% test coverage stability post-refactor Signed-off-by: Dimitris Kargatzis <dkargatzis@gmail.com>
dkargatzis
force-pushed
the
fix/code-quality-improvements
branch
from
4a6c671 to
d03f5d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/event_processors/rule_creation.py (1)
33-70: 🛠️ Refactor suggestion | 🟠 MajorStructured logging does not follow prescribed field conventions.
The logging should use consistent fields:
operation,subject_ids,decision,latency_msas per the coding guidelines. Current implementation has several issues:
- Line 63 -
logger.info("api_calls_made_1")has no structured fields.- Lines 66-68 - Conditional logs lack context fields.
- Decorative separators (lines 33, 35, 60, 70) add noise without semantic value.
- Field naming -
processing_timeshould belatency_msfor consistency.♻️ Suggested refactor for structured logging
- logger.info("=" * 80) - logger.info("processing_rule_creation_command_for", repo_full_name=task.repo_full_name) - logger.info("=" * 80) + logger.info( + "rule_creation_processing_started", + operation="rule_creation", + subject_ids={"repo": task.repo_full_name}, + )- # Summary - logger.info("=" * 80) - logger.info("rule_creation_processing_completed_in_ms", processing_time=processing_time) - logger.info("feasible", success=feasibility_result.success) - logger.info("api_calls_made_1") - - if feasibility_result.success: - logger.info("rule_is_feasible_yaml_provided") - else: - logger.info("rule_is_not_feasible_feedback_provided") - - logger.info("=" * 80) + logger.info( + "rule_creation_processing_completed", + operation="rule_creation", + subject_ids={"repo": task.repo_full_name}, + decision="feasible" if feasibility_result.success else "not_feasible", + latency_ms=processing_time, + api_calls_made=1, + )Based on learnings: "Use structured logging at boundaries with fields:
operation,subject_ids,decision,latency_ms".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/rule_creation.py` around lines 33 - 70, The logs in rule_creation.py use ad-hoc messages and separators; update all logger.info calls in the rule creation flow (around _extract_rule_description, after feasibility_agent.execute and in _post_result_to_comment handling) to use structured fields: operation="process_rule_creation", subject_ids=[task.repo_full_name], decision=(feasibility_result.success ? "feasible" : "not_feasible"), latency_ms=processing_time (rename processing_time to latency_ms), and include api_calls_made as an integer field instead of logger.info("api_calls_made_1"); remove decorative separator logs and replace conditional messages (rule_is_feasible_yaml_provided / rule_is_not_feasible_feedback_provided) with a single structured log that sets decision and any extra metadata.src/event_processors/check_run.py (1)
38-43:⚠️ Potential issue | 🟠 MajorComplete structured logging migration in check-run processing block.
This block still emits separator/f-string style logs, so the migrated event at Line 39 is not enough for consistent structured observability.
Suggested change
- logger.info("=" * 80) - logger.info("processing_check_run_event_for", repo_full_name=task.repo_full_name) - logger.info(f" Name: {check_run.get('name')}") - logger.info(f" Status: {check_run.get('status')}") - logger.info(f" Conclusion: {check_run.get('conclusion')}") - logger.info("=" * 80) + logger.info( + "processing_check_run_event", + operation="check_run", + subject_ids=[task.repo_full_name], + decision="processing", + latency_ms=0, + check_run_name=check_run.get("name"), + status=check_run.get("status"), + conclusion=check_run.get("conclusion"), + )As per coding guidelines "Use structured logging at boundaries with fields:
operation,subject_ids,decision,latency_ms".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/check_run.py` around lines 38 - 43, Replace the separator/f-string logs in the check-run block with a single structured logger.info call: remove the "=" lines and the f-strings and log via logger.info with fields operation="processing_check_run_event", subject_ids including task.repo_full_name and check_run.get("id"), name=check_run.get("name"), status=check_run.get("status"), decision=check_run.get("conclusion"), and latency_ms using task.latency_ms or None if not present; update the code around the existing logger usage and references to logger, task, and check_run to emit these structured fields.src/agents/acknowledgment_agent/agent.py (1)
186-197:⚠️ Potential issue | 🟠 MajorConfidence threshold policy is not enforced before returning success.
The function returns
success=Trueregardless ofstructured_result.confidence. It should reject or route to human review when confidence is below 0.5.As per coding guidelines: "Implement confidence policy: reject or route to human-in-the-loop when confidence < 0.5".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/acknowledgment_agent/agent.py` around lines 186 - 197, The current return always sets success=True regardless of structured_result.confidence; update the logic in the function that builds the AgentResult (the block returning AgentResult with keys is_valid/reasoning/acknowledgable_violations/require_fixes/confidence/recommendations) to enforce the confidence policy: if structured_result.confidence < 0.5, set success to False (or add a route_to_human/human_review flag in the returned data) and change the message to indicate low confidence and human review required, otherwise return success=True as before; ensure you reference structured_result.confidence when deciding the outcome so low-confidence cases are rejected/routed.src/main.py (1)
64-65:⚠️ Potential issue | 🟠 MajorStartup path still bypasses structlog.
Line 64 still uses
logging.info(...)instead oflogger.info(...). This bypasses the structured pipeline you introduced in this module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.py` around lines 64 - 65, The startup message is using the root logging call logging.info(...) which bypasses the structlog pipeline; replace that call with the module's structured logger (logger.info(...)) so the startup message flows through your configured structlog pipeline and include any contextual data if needed; verify that the variable logger (the structlog logger created earlier in this module) is in scope and used instead of the root logging module.src/agents/acknowledgment_agent/test_agent.py (1)
78-117:⚠️ Potential issue | 🔴 CriticalThis test can pass even when the agent fails.
At Line 113 and Line 116, failure paths only log errors. The test never fails on unsuccessful execution or exceptions, which can mask regressions.
Proposed fix
try: # Test evaluation result = await agent.evaluate_acknowledgment( @@ - if result.success: + assert result.success, f"Acknowledgment evaluation failed: {result.message}" + if result.success: logger.info("acknowledgment_evaluation_completed_successfully") @@ - else: - logger.error("acknowledgment_evaluation_failed", message=result.message) + else: + pytest.fail(f"Acknowledgment evaluation failed: {result.message}") except Exception as e: - logger.error("test_failed_with_error", e=e) + logger.error("test_failed_with_error", error=str(e)) + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/acknowledgment_agent/test_agent.py` around lines 78 - 117, The test currently only logs errors when agent.evaluate_acknowledgment returns a failing result or raises an exception; change it so failures cause the test to fail: after calling agent.evaluate_acknowledgment, if result.success is False raise an AssertionError (or call pytest.fail) including result.message and any key diagnostic fields (e.g., result.data.get('reasoning')), and in the except Exception as e block re-raise the exception (or call pytest.fail with str(e)) instead of just logging; update references to agent.evaluate_acknowledgment, result, and the except block so the test will not pass silently on failures.src/core/utils/logging.py (1)
52-69:⚠️ Potential issue | 🟠 MajorCore logging utility still emits interpolated strings in critical paths.
log_operation(Line 52-69) and wrapper error logs (Line 104 and Line 122) still use interpolated messages instead of canonical event+kwargs fields. This leaves the migration incomplete in the shared logging layer and breaks consistency for downstream logs.As per coding guidelines: "Use structured logging at boundaries with fields:
operation,subject_ids,decision,latency_ms".Also applies to: 103-106, 121-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utils/logging.py` around lines 52 - 69, The log_operation context manager is emitting interpolated f-strings; change its logger.info and logger.error calls to use structured logging fields instead of embedding the operation in the message: emit a short constant message (e.g., "operation.started", "operation.completed", "operation.failed") and pass operation, latency_ms and any error into extra (or structured kwargs) so fields are present as operation, subject_ids, decision, latency_ms; do the same fix for the wrapper error log sites in this module (the wrapper/error logging blocks referenced in the file) ensuring exc_info is preserved on errors and that logger calls use the structured fields rather than f-strings.
🧹 Nitpick comments (8)
src/agents/engine_agent/nodes.py (4)
57-57: Consider addingerror_typeto structured error logs.The structured error log is an improvement, but per guidelines, domain errors should include
error_typefor better categorization in monitoring systems.- logger.error("error_in_rule_analysis", error=str(e)) + logger.error("error_in_rule_analysis", error=str(e), error_type=type(e).__name__)Same applies to lines 139, 208, and 276.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/engine_agent/nodes.py` at line 57, Add an error_type field to the structured logger.error calls mentioned (the call at logger.error("error_in_rule_analysis", error=str(e)) and the similar calls at the other occurrences) so logs include a category for monitoring; update each call in nodes.py (the logger.error invocations at the "error_in_rule_analysis" site and the similar logger.error uses around lines referenced) to pass error_type=str(type(e).__name__) alongside the existing message and error details to preserve current behavior and add classification.
90-90: Event name lacks word separation.The event name
all_rules_mapped_to_validatorsconditions_skippingappears to have missing underscores, reducing readability in log queries.- logger.info("all_rules_mapped_to_validatorsconditions_skipping") + logger.info("all_rules_mapped_to_validator_conditions_skipping")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/engine_agent/nodes.py` at line 90, The log event string passed to logger.info in the logger.info("all_rules_mapped_to_validatorsconditions_skipping") call has missing word separation; update that literal to a properly separated event name (for example "all_rules_mapped_to_validators_conditions_skipping" or another agreed-upon snake_case variant) wherever used, keeping the same logger.info call and ensuring any other code or tests that match this event name are updated to the new string.
53-54: Boundary logs missing required structured fields.Per coding guidelines, boundary logs should include
operation,subject_ids,decision, andlatency_ms. Currently onlylatency_msis provided. Consider adding the missing fields for full observability compliance.elapsed_ms = int((time.time() - start_time) * 1000) - logger.info("rule_analysis_completed", latency_ms=elapsed_ms) + logger.info( + "rule_analysis_completed", + operation="analyze_rule_descriptions", + subject_ids=[rd.rule_id for rd in state.rule_descriptions], + decision=f"found_{len(state.rule_descriptions)}_applicable_rules", + latency_ms=elapsed_ms, + )The same pattern applies to lines 135-136, 204-205, and 272-273.
As per coding guidelines: "Use structured logging at boundaries with fields:
operation,subject_ids,decision,latency_ms"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/engine_agent/nodes.py` around lines 53 - 54, Update the boundary structured logs to include the required fields by adding operation, subject_ids, decision alongside latency_ms on the logger.info calls (e.g., the call that currently logs "rule_analysis_completed" should become logger.info("rule_analysis_completed", operation="rule_analysis", subject_ids=[...], decision="...", latency_ms=elapsed_ms)); find and update the other similar boundary log statements in this file that currently only emit latency_ms (the other logger.info calls noted in the review) to follow the same pattern so all boundary logs include operation, subject_ids, decision, and latency_ms for full observability compliance.
37-48: Incomplete migration: f-string logs should use kwargs for structured payloads.The PR objective states "Convert f-string interpolations into kwargs for structured log payloads," but these log calls still use f-strings. This prevents log aggregation tools from parsing and querying the structured fields.
Suggested refactor pattern
- logger.info(f"🔍 Analyzing {len(state.rule_descriptions)} rule descriptions") + logger.info("analyzing_rule_descriptions", rule_count=len(state.rule_descriptions)) # Filter rules applicable to this event type applicable_rules = [] for rule_desc in state.rule_descriptions: if state.event_type in rule_desc.event_types: applicable_rules.append(rule_desc) - logger.info(f"🔍 Rule '{rule_desc.description[:50]}...' is applicable to {state.event_type}") + logger.debug( + "rule_applicable", + rule_id=rule_desc.rule_id, + event_type=state.event_type, + ) else: - logger.info( - f"🔍 Rule '{rule_desc.description[:50]}...' is NOT applicable (expects: {rule_desc.event_types})" - ) + logger.debug( + "rule_not_applicable", + rule_id=rule_desc.rule_id, + expected_event_types=rule_desc.event_types, + actual_event_type=state.event_type, + )Similar f-string patterns exist at lines 71, 84, 93, 126, 131, 155, 169-170, 182, 226, 251, 259-261, 312, 351, and 383.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/engine_agent/nodes.py` around lines 37 - 48, Logs in nodes.py are still using f-strings (e.g., logger.info(f"🔍 Analyzing {len(state.rule_descriptions)} rule descriptions") and the per-rule messages that reference rule_desc.description and state.event_type); replace these f-strings with structured-logging kwargs so fields are queryable (for example: logger.info("Analyzing rule descriptions", count=len(state.rule_descriptions)) and for each rule use a single message with named fields like logger.info("Rule applicability", rule_description=rule_desc.description[:50], event_type=state.event_type, expected_event_types=rule_desc.event_types, applicable=True/False) so you keep the same semantics but emit structured keys; apply the same refactor to the other occurrences mentioned (lines noted in the comment) ensuring unique field names (e.g., rule_description, event_type, expected_event_types, applicable, count) to keep logs consistent.src/event_processors/rule_creation.py (2)
74-82: Use standard exception logging pattern.The exception is logged as
e=ewhich is non-descriptive. Useexc_info=Trueto capture the full traceback or use a standard field name.♻️ Suggested fix for exception logging
- logger.error("error_processing_rule_creation", e=e) + logger.error( + "rule_creation_processing_failed", + operation="rule_creation", + subject_ids={"repo": task.repo_full_name}, + exc_info=True, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/rule_creation.py` around lines 74 - 82, The log call inside the exception handler for rule creation currently passes the exception as a non-descriptive field (logger.error("error_processing_rule_creation", e=e)); update this to use the standard exception logging pattern by calling logger.error or logger.exception with exc_info=True (or use logger.exception("error_processing_rule_creation") if you want stacktrace and default error level) so the full traceback is captured; keep the surrounding return of ProcessingResult (success=False, error=str(e), etc.) unchanged and only replace the logger call in the exception block in rule_creation.py.
109-126: Add context to warning and error logs.Line 110's warning provides no context for debugging. Lines 123 and 126 should include more structured fields for observability.
♻️ Suggested improvements
if not issue_number or not task.installation_id: - logger.warning("no_issue_number_or_installationid_found") + logger.warning( + "missing_required_identifiers", + operation="post_feasibility_reply", + has_issue_number=bool(issue_number), + has_installation_id=bool(task.installation_id), + subject_ids={"repo": task.repo_full_name}, + ) returnelse: - logger.error("failed_to_post_feasibility_reply_to", issue_number=issue_number) + logger.error( + "failed_to_post_feasibility_reply", + operation="post_feasibility_reply", + subject_ids={"repo": task.repo_full_name, "issue": issue_number}, + ) except Exception as e: - logger.error("error_posting_feasibility_reply", e=e) + logger.error( + "error_posting_feasibility_reply", + operation="post_feasibility_reply", + subject_ids={"repo": task.repo_full_name}, + exc_info=True, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event_processors/rule_creation.py` around lines 109 - 126, The warning and error logs lack contextual fields—update the logging around the feasibility reply block (where _format_feasibility_reply is called and github_client.create_issue_comment is invoked) to include relevant identifiers: add issue_number, task.repo_full_name, task.installation_id (and task.id if available) to the warning when issue_number or installation_id is missing; when create_issue_comment returns, include those same structured fields in the logger.info and logger.error calls; and in the exception handler, log the exception alongside the structured identifiers (issue_number, repo_full_name, installation_id) instead of passing the exception as a lone positional arg so observability tools get both the error and context.src/rules/conditions/pull_request.py (1)
73-74: Useexc_info=Trueorlogger.exception()instead of passing exception as a fieldAt lines 73 and 97, logging the exception as
e=ebypasses structlog's exception processing pipeline. Per structlog best practices, pass the exception viaexc_info=True(orexc_info=eto pass the caught exception) and let processors render it consistently. Preferred approach: uselogger.exception()inside the except block, which setsexc_info=Trueautomatically.# Before logger.error("titlepatterncondition_invalid_regex_pattern", pattern=pattern, e=e) # After (either) logger.exception("titlepatterncondition_invalid_regex_pattern", pattern=pattern) # or logger.error("titlepatterncondition_invalid_regex_pattern", pattern=pattern, exc_info=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rules/conditions/pull_request.py` around lines 73 - 74, Replace the logger.error calls that pass the caught exception as a field with structlog-compatible exception logging: inside the exception handlers in TitlePatternCondition (the block that logs "titlepatterncondition_invalid_regex_pattern" when regex compilation or matching fails), call logger.exception(...) or logger.error(..., exc_info=True) instead of logger.error(..., e=e) so the exception is passed via exc_info and processed by structlog correctly.src/webhooks/handlers/issue_comment.py (1)
128-129: Normalize event keys for observability consistency.Several event keys are truncated/concatenated (
posted_feasibility_evaluation_result_as_a,comment_does_not_match_any_known,extractingacknowledgmentreason). Consistent snake_case keys improve searchability and dashboard filters.Also applies to: 158-159, 169-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhooks/handlers/issue_comment.py` around lines 128 - 129, Several logger event keys in issue_comment.py are concatenated/truncated; update the logger.info calls to use consistent snake_case event keys (e.g., change "extractingacknowledgmentreason" to "extracting_acknowledgment_reason" and ensure similar snake_case for "posted_feasibility_evaluation_result_as_a" and "comment_does_not_match_any_known"). Locate the logger.info invocations (the ones returning WebhookResponse and the two other occurrences around the mentioned lines) and replace the current string keys with clear snake_case variants so observability/searchability is consistent across events.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agents/acknowledgment_agent/agent.py`:
- Around line 89-91: Logging currently emits raw user input and internal
reasoning (e.g., acknowledgment_reason, full reasoning_text, and full
traceback_text). Replace direct logger.info calls that log
acknowledgment_reason, reasoning_text, and traceback_text with sanitized
summaries: redact PII/secrets, log a short hash or length and a redaction flag
(e.g., "reason_redacted", "reason_length", "traceback_summary") and/or a safe
metadata tuple (count of violations, categories) instead of full content;
implement or call an existing sanitize/redact helper before logging and ensure
logger.info("violations_to_evaluate", count=len(violations)) remains but that
any strings passed (acknowledgment_reason, reasoning_text, traceback_text) are
replaced by their sanitized summaries in the same function/agent (agent.py)
where these variables are constructed/used.
In `@src/agents/engine_agent/agent.py`:
- Around line 121-128: The completion log after running the hybrid graph (where
result = await self._execute_with_timeout(self.graph.ainvoke(initial_state)))
currently only logs latency_ms; update the final structured logger call (the
logger.info near "rule_engine_evaluation_completed") to include the required
fields operation (e.g., "rule_engine_evaluation_completed"), subject_ids
(derived from initial_state or result), decision (the computed decision from
result), and latency_ms; do the same for the other boundary log near lines
167-170 so both logger.info calls emit a single structured event containing
operation, subject_ids, decision, and latency_ms, using the existing symbols
logger, _execute_with_timeout, graph.ainvoke, initial_state, and result to
populate fields.
In `@src/core/utils/caching.py`:
- Line 178: The current logger.debug call logs the raw cache_key (derived from
function args) which can leak secrets/PII; update the logging in the cache hit
path (the logger.debug call that uses func.__name__ and cache_key) to never emit
the raw cache_key—instead compute and log a non-reversible fingerprint (e.g.,
SHA-256 or HMAC of cache_key) or a redacted marker, and replace cache_key in the
log with that fingerprint/redaction; ensure all similar calls (including the
other occurrence around the same block) are changed so no raw cache_key is
written to logs.
- Line 65: Structured cache log calls like logger.debug("cache_hit_for",
key=key) are missing required boundary fields; update all
logger.debug/logger.info calls in this module (the occurrences around the cache
hit/miss branches identified by key and the cache functions such as
cache_get/cache_set or similar) to include operation (e.g.,
"cache_get"/"cache_set"), subject_ids (use [key] or the relevant id list),
decision ("hit" or "miss"), and latency_ms (compute elapsed ms from the entry
point timing variable or capture start time around the cache operation). Ensure
each structured event includes these four fields alongside existing fields so
they conform to the logging contract; apply the same change to the other listed
logger lines (near lines 82, 89, 95, 106, 178, 182).
In `@src/core/utils/timeout.py`:
- Line 45: Replace the generic boundary log logger.error("event", msg=msg) with
a semantic, structured timeout event that includes the required fields: use a
clear event name (e.g., "timeout" or "operation_timeout") and pass operation,
subject_ids, decision, and latency_ms as explicit fields; locate the
logger.error call in timeout.py and ensure latency_ms is computed from the
existing duration/timing variables (convert to milliseconds) and include subject
identifiers and the decision value so the log becomes
logger.error("operation_timeout", operation=..., subject_ids=..., decision=...,
latency_ms=..., msg=msg) or the equivalent for your structured-logger API.
In `@src/event_processors/deployment_protection_rule.py`:
- Line 79: The decision log call logger.info("no_rules_found_for_repository")
(and the other similar logger.info calls in this module) must be changed to
structured logging with the boundary fields required by our guidelines: include
operation (e.g., "deployment_protection_rule_evaluation" or the existing
operation name used in this module), subject_ids (the repo or deployment
identifiers available in the function), decision (e.g.,
"no_rules_found"/"allow"/"deny"), and latency_ms (compute using the start time
used in this processor). Update the logger.info invocation(s) in
deployment_protection_rule.py (the lines emitting
"no_rules_found_for_repository" and the other two info logs) to pass a dict or
keyword fields with keys operation, subject_ids, decision, and latency_ms
instead of a plain message so they become structured boundary logs.
In `@src/event_processors/pull_request/processor.py`:
- Around line 67-71: Replace the separator and f-string prints with structured
start/finish log events: emit logger.info with fields operation (e.g.
"process_pr_event.start" and "process_pr_event.finish"), subject_ids containing
repo_full_name and pr_number, decision (None at start, actual decision at
finish), and latency_ms (compute by capturing a start_time in the PR processing
entry point and subtracting it at the end). Update the logger calls that
currently print "=" * 80, "processing_pr_event_for", f" Action:
{task.payload.get('action')}", and "pr_number" (the block around logger.info in
the processing function) and mirror the same structured pattern for the finish
logs referenced around the other logger calls (lines ~182-183) so all boundary
logs use the required fields.
In `@src/event_processors/push.py`:
- Around line 36-40: The current logging in push event processing mixes
banner/f-string and unstructured messages (e.g., logger.info("=" * 80),
logger.info("processing_push_event_for", repo_full_name=task.repo_full_name),
logger.info("ref", ref=ref), logger.info(f" Commits: {len(commits)}")) which
violates the structured logging contract; replace these with single structured
boundary logs that include the required fields operation (e.g.,
"push_event.processing.start"/".complete"/".skip"/".error"), subject_ids (e.g.,
task.repo_full_name and ref or commit ids), decision (e.g.,
"started"/"skipped"/"error"/"completed"), and latency_ms (compute for
completion/error paths), and apply the same change for the other logger.info
calls referenced (the other start/skip/error/completion sites in this file).
Locate usages of logger.info in the push processing function and consolidate
into event-style structured logs providing those four fields and any contextual
fields like commits count without using f-strings or banner separators.
In `@src/event_processors/rule_creation.py`:
- Line 49: The current logger.info call logs the full user-provided
rule_description which may contain PII/secrets; update the logging in
src/event_processors/rule_creation.py so that logger.info does not emit the raw
rule_description but instead logs only safe metadata or a sanitized/truncated
version (e.g., first N characters + "[TRUNCATED]" or a hash/summary) and/or a
redaction marker; change the call that uses logger.info("rule_description",
rule_description=rule_description) to produce and log a
sanitized_rule_description derived from rule_description and keep the original
text out of logs while preserving traceability (include function/variable names:
logger.info, rule_description, sanitized_rule_description).
In `@src/integrations/github/check_runs.py`:
- Around line 43-44: The boundary logs at the check-run early-returns (the
logger.warning/logger.info calls in this module) are missing required structured
fields; update each of the log calls (the ones that currently call
logger.warning("cannot_create_check_run_for_sha", repo=repo) and the similar
calls around lines ~62, ~89, ~119) to include operation (e.g.,
operation="create_check_run"), subject_ids (an array identifying the
repo/sha/installation id used by the flow), decision (a short string like
"failed" or "skipped" that reflects why we returned), and latency_ms (either
compute elapsed ms from the function entry timestamp if available or set to
None/0). Ensure you add those fields to the same logger call (not a separate
log) so the messages conform to the boundary schema and reference the existing
logger invocation sites (the logger.warning/logger.info calls in this file).
- Around line 65-66: The logger calls pass raw exception objects (e=e) which
JSONRenderer can't serialize; update both logger.error invocations to pass a
serializable error string and include exc_info for stack traces—replace the call
that logs "error_creating_check_run" (around the check run creation code) with
logger.error(..., error=str(e), exc_info=True) and likewise replace the
"error_creating_check_run_with_acknowledgment" call with logger.error(...,
error=str(e), exc_info=True); locate these by the log message keys to apply the
change.
In `@src/main.py`:
- Around line 88-89: The lifecycle boundary logs in src/main.py (the logger.info
call that currently reads "Event handlers registered, background workers
started, and deployment scheduler started." and the similar startup/shutdown
messages around lines 92-93 and 98-99) must be changed to emit the canonical
structured fields: include operation (e.g., "startup" or "shutdown"),
subject_ids (array or empty list identifying affected components), decision
(e.g., "succeeded" or "failed"), and latency_ms (numeric latency), by passing
those fields into your structured logger call (e.g., via logger.info with an
attributes/extra dict or the project’s logger-binding API) alongside the
existing message so each lifecycle boundary log has operation, subject_ids,
decision, and latency_ms populated.
In `@src/rules/conditions/pull_request.py`:
- Around line 60-61: The boundary logs (e.g., the logger.debug call with message
"titlepatterncondition_title_matches_pattern" and other decision/debug sites
inside the TitlePatternCondition class methods evaluate/validate) must include
the required structured fields: operation, subject_ids, decision, and
latency_ms; capture a start timestamp at the beginning of each evaluate/validate
invocation, compute latency_ms before each boundary log, set operation to a
clear string like "title_pattern_condition.evaluate" or
"title_pattern_condition.validate", populate subject_ids from the pull request
identifier(s) (e.g., pr.id or pr.number), and include the boolean decision value
(match/validate result) in the decision field—update all similar logger.debug
calls noted in the review (the messages around lines 60, 94-95, 143-147,
177-183, 220-226, 260-267, 357-358) to pass these extra fields.
In `@src/rules/loaders/github_loader.py`:
- Around line 42-43: Replace the interpolated logger.info calls around the
get_file_content call and the later log at lines ~70-71 with structured logging
using event keys and kwargs; for the first call that currently logs f"Fetching
rules for repository: {repository} (installation: {installation_id})", call
logger.info with operation="fetch_rules", subject_ids={"repository": repository,
"installation_id": installation_id}, and any additional fields (e.g.,
"rules_file_path") instead of an f-string, then do the same for the logger call
near the get_file_content result (around lines 70-71) — use a clear operation
value (e.g., "rules_lookup" or "rules_not_found"), include subject_ids with
repository and installation_id, and include latency_ms/decision fields as
appropriate to match the file's structured logging schema.
In `@src/rules/utils.py`:
- Line 24: The log call at logger.info("posted_validation_result_to_pr_in", ...)
must include the required boundary fields; update the logger.info invocation in
src/rules/utils.py (the "posted_validation_result_to_pr_in" event) to pass
operation="posted_validation_result_to_pr_in", subject_ids=[pr_number] (or a
list of relevant IDs), decision=<decision> (use the existing decision
variable/result), and latency_ms=<computed integer> (compute as milliseconds
from the request start timestamp, e.g. int((time.time() - start_ts)*1000) or use
the existing latency variable if present); ensure the decision and
start_ts/latency variables are available or computed before calling logger.info.
In `@src/rules/utils/codeowners.py`:
- Line 42: The structured log call logger.warning("invalid_codeowners_line",
line_num=line_num, line=line) is missing required boundary fields; update every
such call that emits the "invalid_codeowners_line" event (and similar warnings
in this module) to include operation (e.g., "parse_codeowners"), subject_ids (an
array identifying the resource, e.g., [repo_id or filename] or [] if
unavailable), decision (e.g., "failure" or "invalid"), and latency_ms (numeric,
0 if not measured) so the event conforms to the logging contract; modify the
logger.warning invocations in this file (the ones with message
"invalid_codeowners_line") to pass those four named fields alongside line_num
and line.
In `@src/rules/utils/contributors.py`:
- Around line 46-50: The logging calls in src/rules/utils/contributors.py must
be converted to structured boundary logs that include the mandatory fields
operation, subject_ids, decision, and latency_ms and remove any f-string
messages; update the early cache branch
(logger.debug("using_cached_past_contributors_for", repo=repo)) and the
subsequent fetch/info calls (logger.info("fetching_past_contributors_for",
repo=repo) and the interpolated message later in get_past_contributors) to use
logger.<level>(message, operation="get_past_contributors", subject_ids=[repo],
decision="cache_hit" or "cache_miss"/"allowed"/"denied" as appropriate,
latency_ms=elapsed_ms) or similar values, and replace the f-string log at the
later call with the same structured call so all boundary events in
get_past_contributors emit the required fields.
In `@src/webhooks/handlers/deployment_status.py`:
- Around line 31-33: The current code mixes structured logger calls and an
f-string (logger.info + f-string) in the deployment enqueue path; replace the
three separate logger.info calls with a single structured boundary log using
logger.info and the required fields (operation, subject_ids, decision,
latency_ms) plus the environment and repo identifier—e.g., build one logger.info
call that sets operation="enqueuing_deployment_status_event", subject_ids or
repo_full_name=repo_full_name, decision=state,
environment=deployment.get("environment","unknown") and include latency_ms when
available; update the code that currently calls
logger.info("enqueuing_deployment_status_event_for", ...), logger.info("state",
...), and logger.info(f" Environment: {deployment.get('environment',
'unknown')}") to this single structured call so the boundary event is emitted
consistently.
In `@src/webhooks/handlers/issue_comment.py`:
- Around line 183-184: The code logs user-supplied `reason` verbatim via
`logger.info("pattern_matched_reason", reason=reason)` — change this to avoid
PII/secrets by removing the raw `reason` from logs and instead log redacted
metadata such as `reason_length` and a non-reversible checksum (e.g., sha256 or
truncated hash) and/or a short redaction flag; keep the defensive `if reason:`
check in the same place (in the same function where `logger` and `reason` are
used) but replace the logged payload so no raw `reason` text is emitted while
preserving ability to correlate events via the checksum/length metadata.
---
Outside diff comments:
In `@src/agents/acknowledgment_agent/agent.py`:
- Around line 186-197: The current return always sets success=True regardless of
structured_result.confidence; update the logic in the function that builds the
AgentResult (the block returning AgentResult with keys
is_valid/reasoning/acknowledgable_violations/require_fixes/confidence/recommendations)
to enforce the confidence policy: if structured_result.confidence < 0.5, set
success to False (or add a route_to_human/human_review flag in the returned
data) and change the message to indicate low confidence and human review
required, otherwise return success=True as before; ensure you reference
structured_result.confidence when deciding the outcome so low-confidence cases
are rejected/routed.
In `@src/agents/acknowledgment_agent/test_agent.py`:
- Around line 78-117: The test currently only logs errors when
agent.evaluate_acknowledgment returns a failing result or raises an exception;
change it so failures cause the test to fail: after calling
agent.evaluate_acknowledgment, if result.success is False raise an
AssertionError (or call pytest.fail) including result.message and any key
diagnostic fields (e.g., result.data.get('reasoning')), and in the except
Exception as e block re-raise the exception (or call pytest.fail with str(e))
instead of just logging; update references to agent.evaluate_acknowledgment,
result, and the except block so the test will not pass silently on failures.
In `@src/core/utils/logging.py`:
- Around line 52-69: The log_operation context manager is emitting interpolated
f-strings; change its logger.info and logger.error calls to use structured
logging fields instead of embedding the operation in the message: emit a short
constant message (e.g., "operation.started", "operation.completed",
"operation.failed") and pass operation, latency_ms and any error into extra (or
structured kwargs) so fields are present as operation, subject_ids, decision,
latency_ms; do the same fix for the wrapper error log sites in this module (the
wrapper/error logging blocks referenced in the file) ensuring exc_info is
preserved on errors and that logger calls use the structured fields rather than
f-strings.
In `@src/event_processors/check_run.py`:
- Around line 38-43: Replace the separator/f-string logs in the check-run block
with a single structured logger.info call: remove the "=" lines and the
f-strings and log via logger.info with fields
operation="processing_check_run_event", subject_ids including
task.repo_full_name and check_run.get("id"), name=check_run.get("name"),
status=check_run.get("status"), decision=check_run.get("conclusion"), and
latency_ms using task.latency_ms or None if not present; update the code around
the existing logger usage and references to logger, task, and check_run to emit
these structured fields.
In `@src/event_processors/rule_creation.py`:
- Around line 33-70: The logs in rule_creation.py use ad-hoc messages and
separators; update all logger.info calls in the rule creation flow (around
_extract_rule_description, after feasibility_agent.execute and in
_post_result_to_comment handling) to use structured fields:
operation="process_rule_creation", subject_ids=[task.repo_full_name],
decision=(feasibility_result.success ? "feasible" : "not_feasible"),
latency_ms=processing_time (rename processing_time to latency_ms), and include
api_calls_made as an integer field instead of logger.info("api_calls_made_1");
remove decorative separator logs and replace conditional messages
(rule_is_feasible_yaml_provided / rule_is_not_feasible_feedback_provided) with a
single structured log that sets decision and any extra metadata.
In `@src/main.py`:
- Around line 64-65: The startup message is using the root logging call
logging.info(...) which bypasses the structlog pipeline; replace that call with
the module's structured logger (logger.info(...)) so the startup message flows
through your configured structlog pipeline and include any contextual data if
needed; verify that the variable logger (the structlog logger created earlier in
this module) is in scope and used instead of the root logging module.
---
Nitpick comments:
In `@src/agents/engine_agent/nodes.py`:
- Line 57: Add an error_type field to the structured logger.error calls
mentioned (the call at logger.error("error_in_rule_analysis", error=str(e)) and
the similar calls at the other occurrences) so logs include a category for
monitoring; update each call in nodes.py (the logger.error invocations at the
"error_in_rule_analysis" site and the similar logger.error uses around lines
referenced) to pass error_type=str(type(e).__name__) alongside the existing
message and error details to preserve current behavior and add classification.
- Line 90: The log event string passed to logger.info in the
logger.info("all_rules_mapped_to_validatorsconditions_skipping") call has
missing word separation; update that literal to a properly separated event name
(for example "all_rules_mapped_to_validators_conditions_skipping" or another
agreed-upon snake_case variant) wherever used, keeping the same logger.info call
and ensuring any other code or tests that match this event name are updated to
the new string.
- Around line 53-54: Update the boundary structured logs to include the required
fields by adding operation, subject_ids, decision alongside latency_ms on the
logger.info calls (e.g., the call that currently logs "rule_analysis_completed"
should become logger.info("rule_analysis_completed", operation="rule_analysis",
subject_ids=[...], decision="...", latency_ms=elapsed_ms)); find and update the
other similar boundary log statements in this file that currently only emit
latency_ms (the other logger.info calls noted in the review) to follow the same
pattern so all boundary logs include operation, subject_ids, decision, and
latency_ms for full observability compliance.
- Around line 37-48: Logs in nodes.py are still using f-strings (e.g.,
logger.info(f"🔍 Analyzing {len(state.rule_descriptions)} rule descriptions")
and the per-rule messages that reference rule_desc.description and
state.event_type); replace these f-strings with structured-logging kwargs so
fields are queryable (for example: logger.info("Analyzing rule descriptions",
count=len(state.rule_descriptions)) and for each rule use a single message with
named fields like logger.info("Rule applicability",
rule_description=rule_desc.description[:50], event_type=state.event_type,
expected_event_types=rule_desc.event_types, applicable=True/False) so you keep
the same semantics but emit structured keys; apply the same refactor to the
other occurrences mentioned (lines noted in the comment) ensuring unique field
names (e.g., rule_description, event_type, expected_event_types, applicable,
count) to keep logs consistent.
In `@src/event_processors/rule_creation.py`:
- Around line 74-82: The log call inside the exception handler for rule creation
currently passes the exception as a non-descriptive field
(logger.error("error_processing_rule_creation", e=e)); update this to use the
standard exception logging pattern by calling logger.error or logger.exception
with exc_info=True (or use logger.exception("error_processing_rule_creation") if
you want stacktrace and default error level) so the full traceback is captured;
keep the surrounding return of ProcessingResult (success=False, error=str(e),
etc.) unchanged and only replace the logger call in the exception block in
rule_creation.py.
- Around line 109-126: The warning and error logs lack contextual fields—update
the logging around the feasibility reply block (where _format_feasibility_reply
is called and github_client.create_issue_comment is invoked) to include relevant
identifiers: add issue_number, task.repo_full_name, task.installation_id (and
task.id if available) to the warning when issue_number or installation_id is
missing; when create_issue_comment returns, include those same structured fields
in the logger.info and logger.error calls; and in the exception handler, log the
exception alongside the structured identifiers (issue_number, repo_full_name,
installation_id) instead of passing the exception as a lone positional arg so
observability tools get both the error and context.
In `@src/rules/conditions/pull_request.py`:
- Around line 73-74: Replace the logger.error calls that pass the caught
exception as a field with structlog-compatible exception logging: inside the
exception handlers in TitlePatternCondition (the block that logs
"titlepatterncondition_invalid_regex_pattern" when regex compilation or matching
fails), call logger.exception(...) or logger.error(..., exc_info=True) instead
of logger.error(..., e=e) so the exception is passed via exc_info and processed
by structlog correctly.
In `@src/webhooks/handlers/issue_comment.py`:
- Around line 128-129: Several logger event keys in issue_comment.py are
concatenated/truncated; update the logger.info calls to use consistent
snake_case event keys (e.g., change "extractingacknowledgmentreason" to
"extracting_acknowledgment_reason" and ensure similar snake_case for
"posted_feasibility_evaluation_result_as_a" and
"comment_does_not_match_any_known"). Locate the logger.info invocations (the
ones returning WebhookResponse and the two other occurrences around the
mentioned lines) and replace the current string keys with clear snake_case
variants so observability/searchability is consistent across events.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
src/agents/acknowledgment_agent/agent.pysrc/agents/acknowledgment_agent/test_agent.pysrc/agents/base.pysrc/agents/engine_agent/agent.pysrc/agents/engine_agent/models.pysrc/agents/engine_agent/nodes.pysrc/agents/factory.pysrc/agents/feasibility_agent/agent.pysrc/agents/feasibility_agent/nodes.pysrc/api/dependencies.pysrc/core/models.pysrc/core/utils/caching.pysrc/core/utils/logging.pysrc/core/utils/metrics.pysrc/core/utils/retry.pysrc/core/utils/timeout.pysrc/event_processors/base.pysrc/event_processors/check_run.pysrc/event_processors/deployment.pysrc/event_processors/deployment_protection_rule.pysrc/event_processors/deployment_review.pysrc/event_processors/deployment_status.pysrc/event_processors/pull_request/enricher.pysrc/event_processors/pull_request/processor.pysrc/event_processors/push.pysrc/event_processors/rule_creation.pysrc/event_processors/violation_acknowledgment.pysrc/integrations/github/check_runs.pysrc/main.pysrc/presentation/github_formatter.pysrc/rules/acknowledgment.pysrc/rules/conditions/base.pysrc/rules/conditions/filesystem.pysrc/rules/conditions/pull_request.pysrc/rules/loaders/github_loader.pysrc/rules/models.pysrc/rules/registry.pysrc/rules/utils.pysrc/rules/utils/codeowners.pysrc/rules/utils/contributors.pysrc/rules/utils/validation.pysrc/webhooks/handlers/deployment_review.pysrc/webhooks/handlers/deployment_status.pysrc/webhooks/handlers/issue_comment.py
🚧 Files skipped from review as they are similar to previous changes (21)
- src/presentation/github_formatter.py
- src/event_processors/deployment.py
- src/agents/feasibility_agent/agent.py
- src/agents/base.py
- src/rules/acknowledgment.py
- src/event_processors/deployment_status.py
- src/event_processors/deployment_review.py
- src/rules/registry.py
- src/agents/engine_agent/models.py
- src/core/utils/metrics.py
- src/agents/factory.py
- src/event_processors/violation_acknowledgment.py
- src/rules/conditions/base.py
- src/agents/feasibility_agent/nodes.py
- src/rules/models.py
- src/rules/utils/validation.py
- src/event_processors/pull_request/enricher.py
- src/core/utils/retry.py
- src/rules/conditions/filesystem.py
- src/event_processors/base.py
- src/webhooks/handlers/deployment_review.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.py: Use modern typing only:dict[str, Any],list[str],str | None(noDict,List,Optional)
GitHub/HTTP/DB calls must beasync def; avoid blocking calls (time.sleep, sync HTTP) in async paths
All agent outputs and external payloads must use validatedBaseModelfrom Pydantic
Use dataclasses for internal immutable state where appropriate
Use structured logging at boundaries with fields:operation,subject_ids,decision,latency_ms
Implement Agent pattern: single-responsibility agents with typed inputs/outputs
Use Decorator pattern for retries, metrics, caching as cross-cutting concerns
Agent outputs must include:decision,confidence (0..1), shortreasoning,recommendations,strategy_used
Implement confidence policy: reject or route to human-in-the-loop when confidence < 0.5
Use minimal, step-driven prompts; provide Chain-of-Thought only for complexity > 0.7 or ambiguity > 0.6
Strip secrets/PII from agent prompts; scope tools; keep raw reasoning out of logs (store summaries only)
Cache idempotent lookups; lazy-import heavy dependencies; bound fan-out withasyncio.Semaphore
Avoid redundant LLM calls; memoize per event when safe
Use domain errors (e.g.,AgentError) witherror_type,message,context,timestamp,retry_count
Use exponential backoff for transient failures; circuit-break noisy integrations when needed
Fail closed for risky decisions; provide actionable remediation in error paths
Validate all external inputs; verify webhook signatures
Implement prompt-injection hardening; sanitize repository content passed to LLMs
Performance targets: Static validation ~<100ms typical, hybrid decisions sub-second when cache warm, budget LLM paths thoughtfully
Reject old typing syntax (Dict,List,Optional) in code review
Reject blocking calls in async code; reject bareexcept:clauses; reject swallowed errors
Reject LLM calls for trivial/deterministic checks
Reject unvalidated agent outputs and missing confidenc...
Files:
src/webhooks/handlers/deployment_status.pysrc/agents/acknowledgment_agent/agent.pysrc/core/utils/timeout.pysrc/event_processors/pull_request/processor.pysrc/rules/conditions/pull_request.pysrc/rules/utils/codeowners.pysrc/event_processors/check_run.pysrc/api/dependencies.pysrc/agents/acknowledgment_agent/test_agent.pysrc/main.pysrc/event_processors/push.pysrc/agents/engine_agent/nodes.pysrc/event_processors/rule_creation.pysrc/rules/utils/contributors.pysrc/rules/loaders/github_loader.pysrc/core/models.pysrc/webhooks/handlers/issue_comment.pysrc/agents/engine_agent/agent.pysrc/event_processors/deployment_protection_rule.pysrc/integrations/github/check_runs.pysrc/core/utils/caching.pysrc/rules/utils.pysrc/core/utils/logging.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: warestack/watchflow PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2026-01-31T19:35:22.504Z
Learning: Applies to **/*.py : Use structured logging at boundaries with fields: `operation`, `subject_ids`, `decision`, `latency_ms`
📚 Learning: 2026-01-31T19:35:22.504Z
Learnt from: CR
Repo: warestack/watchflow PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2026-01-31T19:35:22.504Z
Learning: Applies to **/*.py : Strip secrets/PII from agent prompts; scope tools; keep raw reasoning out of logs (store summaries only)
Applied to files:
src/agents/acknowledgment_agent/agent.pysrc/agents/acknowledgment_agent/test_agent.pysrc/agents/engine_agent/agent.py
📚 Learning: 2026-01-31T19:35:22.504Z
Learnt from: CR
Repo: warestack/watchflow PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2026-01-31T19:35:22.504Z
Learning: Applies to **/*.py : Use structured logging at boundaries with fields: `operation`, `subject_ids`, `decision`, `latency_ms`
Applied to files:
src/agents/acknowledgment_agent/agent.pysrc/rules/conditions/pull_request.pysrc/rules/utils/codeowners.pysrc/agents/engine_agent/nodes.pysrc/agents/engine_agent/agent.pysrc/core/utils/logging.py
📚 Learning: 2026-01-31T19:35:22.504Z
Learnt from: CR
Repo: warestack/watchflow PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2026-01-31T19:35:22.504Z
Learning: Applies to **/*.py : Reject blocking calls in async code; reject bare `except:` clauses; reject swallowed errors
Applied to files:
src/api/dependencies.py
📚 Learning: 2026-01-31T19:35:22.504Z
Learnt from: CR
Repo: warestack/watchflow PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2026-01-31T19:35:22.504Z
Learning: Applies to tests/**/*.py : Write integration tests for webhook parsing, idempotency, multi-agent coordination, and state persistence
Applied to files:
src/agents/acknowledgment_agent/test_agent.py
🧬 Code graph analysis (12)
src/agents/acknowledgment_agent/agent.py (1)
src/agents/base.py (1)
AgentResult(18-24)
src/event_processors/pull_request/processor.py (3)
src/core/models.py (1)
repo_full_name(137-139)src/tasks/task_queue.py (1)
repo_full_name(33-38)src/core/utils/caching.py (1)
get(44-66)
src/event_processors/check_run.py (4)
src/agents/factory.py (1)
get_agent(21-53)src/event_processors/base.py (2)
BaseEventProcessor(26-61)ProcessingResult(16-23)src/tasks/task_queue.py (2)
Task(19-47)repo_full_name(33-38)src/core/models.py (1)
repo_full_name(137-139)
src/api/dependencies.py (1)
src/core/models.py (1)
User(45-56)
src/main.py (2)
src/webhooks/dispatcher.py (1)
register_handler(25-28)src/core/models.py (1)
EventType(91-105)
src/event_processors/push.py (1)
src/tasks/task_queue.py (2)
repo_full_name(33-38)installation_id(41-47)
src/agents/engine_agent/nodes.py (1)
src/agents/engine_agent/models.py (1)
ValidationStrategy(27-32)
src/event_processors/rule_creation.py (5)
src/agents/factory.py (1)
get_agent(21-53)src/agents/base.py (1)
AgentResult(18-24)src/event_processors/base.py (2)
BaseEventProcessor(26-61)ProcessingResult(16-23)src/tasks/task_queue.py (2)
Task(19-47)repo_full_name(33-38)src/core/models.py (1)
repo_full_name(137-139)
src/rules/utils/contributors.py (1)
src/core/utils/caching.py (1)
AsyncCache(19-115)
src/webhooks/handlers/issue_comment.py (2)
src/core/models.py (3)
EventType(91-105)WebhookEvent(116-144)WebhookResponse(108-113)src/webhooks/handlers/base.py (1)
EventHandler(6-25)
src/agents/engine_agent/agent.py (2)
src/agents/base.py (1)
_execute_with_timeout(94-112)src/integrations/providers/vertex_ai_provider.py (1)
ainvoke(109-131)
src/integrations/github/check_runs.py (1)
src/core/models.py (1)
Violation(20-31)
Comment on lines
+89
to
+91
| logger.info("evaluating_acknowledgment_request_from", commenter=commenter) | ||
| logger.info("acknowledgment_reason", reason=acknowledgment_reason) | ||
| logger.info("violations_to_evaluate", count=len(violations)) |
There was a problem hiding this comment.
Raw user input and raw reasoning are being logged.
Line 90 logs acknowledgment_reason, Line 181 logs full reasoning text, and Line 203 logs full traceback text. These can expose secrets/PII or internal reasoning content; log sanitized summaries/metadata instead.
Based on learnings: "Strip secrets/PII from agent prompts; scope tools; keep raw reasoning out of logs (store summaries only)".
Also applies to: 181-183, 203-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/acknowledgment_agent/agent.py` around lines 89 - 91, Logging
currently emits raw user input and internal reasoning (e.g.,
acknowledgment_reason, full reasoning_text, and full traceback_text). Replace
direct logger.info calls that log acknowledgment_reason, reasoning_text, and
traceback_text with sanitized summaries: redact PII/secrets, log a short hash or
length and a redaction flag (e.g., "reason_redacted", "reason_length",
"traceback_summary") and/or a safe metadata tuple (count of violations,
categories) instead of full content; implement or call an existing
sanitize/redact helper before logging and ensure
logger.info("violations_to_evaluate", count=len(violations)) remains but that
any strings passed (acknowledgment_reason, reasoning_text, traceback_text) are
replaced by their sanitized summaries in the same function/agent (agent.py)
where these variables are constructed/used.
Comment on lines
+121
to
128
| logger.info("rule_engine_initial_state_prepared") | ||
|
|
||
| # Run the hybrid graph with timeout | ||
| result = await self._execute_with_timeout(self.graph.ainvoke(initial_state), timeout=self.timeout) | ||
|
|
||
| execution_time = time.time() - start_time | ||
| logger.info(f"🔧 Rule Engine evaluation completed in {execution_time:.2f}s") | ||
| logger.info("rule_engine_evaluation_completed", latency_ms=int(execution_time * 1000)) | ||
|
|
There was a problem hiding this comment.
Evaluation boundary logging is missing required decision context.
Line 127 logs latency_ms, but completion context still lacks a canonical operation, subject_ids, and decision payload in the same boundary event. Please emit one final structured completion event with all required fields.
As per coding guidelines: "Use structured logging at boundaries with fields: operation, subject_ids, decision, latency_ms".
Also applies to: 167-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/engine_agent/agent.py` around lines 121 - 128, The completion log
after running the hybrid graph (where result = await
self._execute_with_timeout(self.graph.ainvoke(initial_state))) currently only
logs latency_ms; update the final structured logger call (the logger.info near
"rule_engine_evaluation_completed") to include the required fields operation
(e.g., "rule_engine_evaluation_completed"), subject_ids (derived from
initial_state or result), decision (the computed decision from result), and
latency_ms; do the same for the other boundary log near lines 167-170 so both
logger.info calls emit a single structured event containing operation,
subject_ids, decision, and latency_ms, using the existing symbols logger,
_execute_with_timeout, graph.ainvoke, initial_state, and result to populate
fields.
| return None | ||
|
|
||
| logger.debug(f"Cache hit for '{key}'") | ||
| logger.debug("cache_hit_for", key=key) |
There was a problem hiding this comment.
Add the required boundary logging fields to structured events.
These events are structured, but they still miss the required fields (operation, subject_ids, decision, latency_ms), so they don’t conform to the repository logging contract.
Suggested schema-aligned logging shape
-logger.debug("cache_hit_for", key=key)
+logger.debug(
+ "cache_lookup",
+ operation="cache_lookup",
+ subject_ids=[key],
+ decision="hit",
+ latency_ms=0,
+)
-logger.debug("invalidated_cache_entry", key=key)
+logger.debug(
+ "cache_invalidate",
+ operation="cache_invalidate",
+ subject_ids=[key],
+ decision="invalidated",
+ latency_ms=0,
+)
-logger.debug("cache_miss_for_with_key", __name__=func.__name__, cache_key=cache_key)
+logger.debug(
+ "cache_lookup",
+ operation="cache_lookup",
+ subject_ids=[func.__name__],
+ decision="miss",
+ latency_ms=0,
+)Also applies to: 82-82, 89-89, 95-95, 106-106, 178-178, 182-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utils/caching.py` at line 65, Structured cache log calls like
logger.debug("cache_hit_for", key=key) are missing required boundary fields;
update all logger.debug/logger.info calls in this module (the occurrences around
the cache hit/miss branches identified by key and the cache functions such as
cache_get/cache_set or similar) to include operation (e.g.,
"cache_get"/"cache_set"), subject_ids (use [key] or the relevant id list),
decision ("hit" or "miss"), and latency_ms (compute elapsed ms from the entry
point timing variable or capture start time around the cache operation). Ensure
each structured event includes these four fields alongside existing fields so
they conform to the logging contract; apply the same change to the other listed
logger lines (near lines 82, 89, 95, 106, 178, 182).
|
|
||
| if cached_value is not None: | ||
| logger.debug(f"Cache hit for {func.__name__} with key '{cache_key}'") | ||
| logger.debug("cache_hit_for_with_key", __name__=func.__name__, cache_key=cache_key) |
There was a problem hiding this comment.
Do not log raw cache keys derived from function arguments.
cache_key may embed sensitive argument/kwarg values; logging it directly can leak secrets/PII.
Suggested redaction/fingerprinting approach
+import hashlib
...
cache_key = key_func(*args, **kwargs) if key_func else f"{func.__name__}:{args}:{kwargs}"
+cache_key_hash = hashlib.sha256(cache_key.encode("utf-8")).hexdigest()[:12]
-logger.debug("cache_hit_for_with_key", __name__=func.__name__, cache_key=cache_key)
+logger.debug("cache_hit_for_with_key", function_name=func.__name__, cache_key_hash=cache_key_hash)
-logger.debug("cache_miss_for_with_key", __name__=func.__name__, cache_key=cache_key)
+logger.debug("cache_miss_for_with_key", function_name=func.__name__, cache_key_hash=cache_key_hash)Also applies to: 182-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utils/caching.py` at line 178, The current logger.debug call logs
the raw cache_key (derived from function args) which can leak secrets/PII;
update the logging in the cache hit path (the logger.debug call that uses
func.__name__ and cache_key) to never emit the raw cache_key—instead compute and
log a non-reversible fingerprint (e.g., SHA-256 or HMAC of cache_key) or a
redacted marker, and replace cache_key in the log with that
fingerprint/redaction; ensure all similar calls (including the other occurrence
around the same block) are changed so no raw cache_key is written to logs.
| except TimeoutError as err: | ||
| msg = timeout_message or f"Operation timed out after {timeout} seconds" | ||
| logger.error(f"❌ {msg}") | ||
| logger.error("event", msg=msg) |
There was a problem hiding this comment.
Use a semantic timeout event with boundary fields.
Line 45 logs "event" with only msg, which loses actionable context for timeout triage. Include operation, subject_ids, decision, and latency_ms in this boundary log.
Suggested change
- logger.error("event", msg=msg)
+ logger.error(
+ "operation_timeout",
+ operation="execute_with_timeout",
+ subject_ids=[],
+ decision="timeout",
+ latency_ms=int(timeout * 1000),
+ message=msg,
+ )Based on learnings "Applies to **/*.py : Use structured logging at boundaries with fields: operation, subject_ids, decision, latency_ms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utils/timeout.py` at line 45, Replace the generic boundary log
logger.error("event", msg=msg) with a semantic, structured timeout event that
includes the required fields: use a clear event name (e.g., "timeout" or
"operation_timeout") and pass operation, subject_ids, decision, and latency_ms
as explicit fields; locate the logger.error call in timeout.py and ensure
latency_ms is computed from the existing duration/timing variables (convert to
milliseconds) and include subject identifiers and the decision value so the log
becomes logger.error("operation_timeout", operation=..., subject_ids=...,
decision=..., latency_ms=..., msg=msg) or the equivalent for your
structured-logger API.
| installation_id=installation_id, | ||
| ) | ||
| logger.info(f"Posted validation result to PR #{pr_number} in {repo_full_name}") | ||
| logger.info("posted_validation_result_to_pr_in", pr_number=pr_number, repo_full_name=repo_full_name) |
There was a problem hiding this comment.
Add required boundary logging fields to validation-post event.
Line 24 should include operation, subject_ids, decision, and latency_ms to stay consistent with boundary observability standards.
As per coding guidelines "Use structured logging at boundaries with fields: operation, subject_ids, decision, latency_ms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rules/utils.py` at line 24, The log call at
logger.info("posted_validation_result_to_pr_in", ...) must include the required
boundary fields; update the logger.info invocation in src/rules/utils.py (the
"posted_validation_result_to_pr_in" event) to pass
operation="posted_validation_result_to_pr_in", subject_ids=[pr_number] (or a
list of relevant IDs), decision=<decision> (use the existing decision
variable/result), and latency_ms=<computed integer> (compute as milliseconds
from the request start timestamp, e.g. int((time.time() - start_ts)*1000) or use
the existing latency variable if present); ensure the decision and
start_ts/latency variables are available or computed before calling logger.info.
| parts = line.split() | ||
| if len(parts) < 2: | ||
| logger.warning(f"Invalid CODEOWNERS line {line_num}: {line}") | ||
| logger.warning("invalid_codeowners_line", line_num=line_num, line=line) |
There was a problem hiding this comment.
Add required boundary log fields to structured events.
On Line 193 and Line 202 (and for consistency, Line 42 and Line 102), the events omit required fields: operation, subject_ids, decision, latency_ms. This breaks the logging contract and makes cross-service querying/alerting harder.
Suggested patch
import re
+import time
from pathlib import Path
@@
- logger.warning("invalid_codeowners_line", line_num=line_num, line=line)
+ logger.warning(
+ "invalid_codeowners_line",
+ operation="parse_codeowners",
+ subject_ids=[f"line:{line_num}"],
+ decision="skip_invalid_line",
+ latency_ms=0,
+ line_num=line_num,
+ line=line,
+ )
@@
- logger.error("invalid_regex_pattern", regex_pattern=regex_pattern)
+ logger.error(
+ "invalid_regex_pattern",
+ operation="match_codeowners_pattern",
+ subject_ids=[file_path, pattern],
+ decision="ignore_pattern",
+ latency_ms=0,
+ regex_pattern=regex_pattern,
+ )
return False
@@
def load_codeowners(repo_path: str = ".") -> CodeOwnersParser | None:
@@
+ started = time.perf_counter()
codeowners_path = Path(repo_path) / "CODEOWNERS"
@@
- logger.warning("codeowners_file_not_found_at", codeowners_path=codeowners_path)
+ logger.warning(
+ "codeowners_file_not_found_at",
+ operation="load_codeowners",
+ subject_ids=[str(codeowners_path)],
+ decision="not_found",
+ latency_ms=int((time.perf_counter() - started) * 1000),
+ codeowners_path=str(codeowners_path),
+ )
return None
@@
- logger.error("error_loading_codeowners_file", e=e)
+ logger.error(
+ "error_loading_codeowners_file",
+ operation="load_codeowners",
+ subject_ids=[str(codeowners_path)],
+ decision="error",
+ latency_ms=int((time.perf_counter() - started) * 1000),
+ error=str(e),
+ )
return NoneAs per coding guidelines: Use structured logging at boundaries with fields: operation, subject_ids, decision, latency_ms.
Also applies to: 102-102, 193-193, 202-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rules/utils/codeowners.py` at line 42, The structured log call
logger.warning("invalid_codeowners_line", line_num=line_num, line=line) is
missing required boundary fields; update every such call that emits the
"invalid_codeowners_line" event (and similar warnings in this module) to include
operation (e.g., "parse_codeowners"), subject_ids (an array identifying the
resource, e.g., [repo_id or filename] or [] if unavailable), decision (e.g.,
"failure" or "invalid"), and latency_ms (numeric, 0 if not measured) so the
event conforms to the logging contract; modify the logger.warning invocations in
this file (the ones with message "invalid_codeowners_line") to pass those four
named fields alongside line_num and line.
Comment on lines
+46
to
+50
| logger.debug("using_cached_past_contributors_for", repo=repo) | ||
| return set(cached_value) | ||
|
|
||
| try: | ||
| logger.info(f"Fetching past contributors for {repo}") | ||
| logger.info("fetching_past_contributors_for", repo=repo) |
There was a problem hiding this comment.
Add mandatory boundary logging fields and remove the remaining f-string log.
These events still miss the required schema (operation, subject_ids, decision, latency_ms), and Line 72 is still an interpolated message instead of structured payload. This makes logs harder to query/aggregate and violates the project logging contract.
Suggested pattern (example for get_past_contributors)
async def get_past_contributors(
self, repo: str, installation_id: int, min_contributions: int = 5, days_back: int = 365
) -> set[str]:
+ started_at = datetime.now()
cache_key = f"{repo}_{min_contributions}_{days_back}"
cached_value = self._contributors_cache.get(cache_key)
if cached_value is not None:
- logger.debug("using_cached_past_contributors_for", repo=repo)
+ logger.debug(
+ "using_cached_past_contributors",
+ operation="get_past_contributors",
+ subject_ids={"repo": repo},
+ decision="cache_hit",
+ latency_ms=int((datetime.now() - started_at).total_seconds() * 1000),
+ )
return set(cached_value)
try:
- logger.info("fetching_past_contributors_for", repo=repo)
+ logger.info(
+ "fetching_past_contributors",
+ operation="get_past_contributors",
+ subject_ids={"repo": repo},
+ decision="start",
+ latency_ms=0,
+ )
...
- logger.info(f"Found {len(past_contributors)} past contributors for {repo}")
+ logger.info(
+ "found_past_contributors",
+ operation="get_past_contributors",
+ subject_ids={"repo": repo},
+ decision="success",
+ latency_ms=int((datetime.now() - started_at).total_seconds() * 1000),
+ contributor_count=len(past_contributors),
+ )
return past_contributors
except Exception as e:
- logger.error("error_fetching_past_contributors_for", repo=repo, e=e)
+ logger.error(
+ "error_fetching_past_contributors",
+ operation="get_past_contributors",
+ subject_ids={"repo": repo},
+ decision="error",
+ latency_ms=int((datetime.now() - started_at).total_seconds() * 1000),
+ error=str(e),
+ )
return set()As per coding guidelines "Use structured logging at boundaries with fields: operation, subject_ids, decision, latency_ms".
Based on learnings "Use structured logging at boundaries with fields: operation, subject_ids, decision, latency_ms".
Also applies to: 72-72, 76-76, 102-102, 106-106, 168-168, 188-188, 217-217, 232-232, 247-247, 261-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rules/utils/contributors.py` around lines 46 - 50, The logging calls in
src/rules/utils/contributors.py must be converted to structured boundary logs
that include the mandatory fields operation, subject_ids, decision, and
latency_ms and remove any f-string messages; update the early cache branch
(logger.debug("using_cached_past_contributors_for", repo=repo)) and the
subsequent fetch/info calls (logger.info("fetching_past_contributors_for",
repo=repo) and the interpolated message later in get_past_contributors) to use
logger.<level>(message, operation="get_past_contributors", subject_ids=[repo],
decision="cache_hit" or "cache_miss"/"allowed"/"denied" as appropriate,
latency_ms=elapsed_ms) or similar values, and replace the f-string log at the
later call with the same structured call so all boundary events in
get_past_contributors emit the required fields.
Comment on lines
+31
to
33
| logger.info("enqueuing_deployment_status_event_for", repo_full_name=repo_full_name) | ||
| logger.info("state", state=state) | ||
| logger.info(f" Environment: {deployment.get('environment', 'unknown')}") |
There was a problem hiding this comment.
Avoid mixed structured + f-string logs in deployment status enqueue path.
Line 31-Line 33 should emit one structured boundary event instead of mixed event/field + formatted string logs.
As per coding guidelines "Use structured logging at boundaries with fields: operation, subject_ids, decision, latency_ms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/webhooks/handlers/deployment_status.py` around lines 31 - 33, The current
code mixes structured logger calls and an f-string (logger.info + f-string) in
the deployment enqueue path; replace the three separate logger.info calls with a
single structured boundary log using logger.info and the required fields
(operation, subject_ids, decision, latency_ms) plus the environment and repo
identifier—e.g., build one logger.info call that sets
operation="enqueuing_deployment_status_event", subject_ids or
repo_full_name=repo_full_name, decision=state,
environment=deployment.get("environment","unknown") and include latency_ms when
available; update the code that currently calls
logger.info("enqueuing_deployment_status_event_for", ...), logger.info("state",
...), and logger.info(f" Environment: {deployment.get('environment',
'unknown')}") to this single structured call so the boundary event is emitted
consistently.
Comment on lines
+183
to
184
| logger.info("pattern_matched_reason", reason=reason) | ||
| if reason: # Defensive: skip empty reasons—user typo, bot spam. |
There was a problem hiding this comment.
Do not log acknowledgment reasons verbatim.
At Line 183, reason is user-supplied text and can contain secrets/PII. Log redacted metadata (e.g., reason_length, checksum) instead of full content.
Based on learnings: "Strip secrets/PII from agent prompts; scope tools; keep raw reasoning out of logs (store summaries only)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/webhooks/handlers/issue_comment.py` around lines 183 - 184, The code logs
user-supplied `reason` verbatim via `logger.info("pattern_matched_reason",
reason=reason)` — change this to avoid PII/secrets by removing the raw `reason`
from logs and instead log redacted metadata such as `reason_length` and a
non-reversible checksum (e.g., sha256 or truncated hash) and/or a short
redaction flag; keep the defensive `if reason:` check in the same place (in the
same function where `logger` and `reason` are used) but replace the logged
payload so no raw `reason` text is emitted while preserving ability to correlate
events via the checksum/length metadata.
|
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (45.0%) is below the target coverage (80.0%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #52 +/- ##
=======================================
- Coverage 68.7% 68.7% -0.1%
=======================================
Files 154 154
Lines 9709 9708 -1
=======================================
- Hits 6677 6675 -2
- Misses 3032 3033 +1 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit