src/webhooks/handlers/deployment_status.py (1)

25-45: ⚠️ Potential issue | 🟠 Major

Boundary logs are missing required telemetry fields and one message remains unstructured.

Line 25, Line 33-35, and Line 45 should emit consistent boundary fields (operation, subject_ids, decision, latency_ms). Also, Line 35 is still an interpolated message, which breaks structured log shape.

Suggested patch

+from time import perf_counter
@@
     async def handle(self, event: WebhookEvent) -> dict[str, Any]:
         """Handle deployment_status events."""
+        started_at = perf_counter()
         payload = event.payload
         repo_full_name = event.repo_full_name
         installation_id = payload.get("installation", {}).get("id")
@@
         if not installation_id:
-            logger.error("no_installation_id_found_in_deploymentstatus", repo_full_name=repo_full_name)
+            logger.error(
+                "deployment_status_rejected_missing_installation_id",
+                operation="webhook.deployment_status.handle",
+                subject_ids={"repo_full_name": repo_full_name, "delivery_id": event.delivery_id},
+                decision="reject",
+                latency_ms=int((perf_counter() - started_at) * 1000),
+            )
             return {"status": "error", "message": "Missing installation ID"}
@@
-        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')}")
+        subject_ids = {
+            "repo_full_name": repo_full_name,
+            "installation_id": installation_id,
+            "delivery_id": event.delivery_id,
+        }
+        logger.info(
+            "deployment_status_enqueue_started",
+            operation="webhook.deployment_status.enqueue",
+            subject_ids=subject_ids,
+            decision="enqueue",
+            latency_ms=int((perf_counter() - started_at) * 1000),
+            state=state,
+            environment=deployment.get("environment", "unknown"),
+        )
@@
-        logger.info("deployment_status_event_enqueued_with_task", task_id=task_id)
+        logger.info(
+            "deployment_status_enqueue_completed",
+            operation="webhook.deployment_status.enqueue",
+            subject_ids=subject_ids,
+            decision="enqueued",
+            latency_ms=int((perf_counter() - started_at) * 1000),
+            task_id=task_id,
+        )

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 25 - 45, The
boundary logs in deployment_status handler (the logger.error call and the
logger.info calls that log enqueuing, state, environment, and task_id) must be
converted to structured logs that include the required telemetry fields:
operation (use "deployment_status"), subject_ids (include repo_full_name and
installation_id), decision (use a meaningful value like "accepted" or
"missing_installation"), and latency_ms (set to None or compute if available).
Replace the interpolated f-string logger.info(f"   Environment:...") with a
structured call that passes environment=deployment.get("environment","unknown"),
and ensure the final enqueue log includes task_id alongside those telemetry
fields; for the missing installation error set decision="missing_installation"
and include subject_ids with repo_full_name only.

src/integrations/github_api.py (1)

38-55: ⚠️ Potential issue | 🟠 Major

Boundary logs are missing the required normalized fields.

Updated integration logs still omit operation, subject_ids, decision, and latency_ms, so they won’t satisfy the logging contract consistently for querying/alerts.

As per coding guidelines, "Use structured logging at boundaries with fields: operation, subject_ids, decision, latency_ms".

Also applies to: 81-85, 163-185

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/integrations/github_api.py` around lines 38 - 55, The boundary logs in
github_api.py (the logger.debug call that returns from
self._token_cache[installation_id] and the logger.info call after creating a
token in the installation-token flow) must be converted to structured logs that
include the required normalized fields: operation (e.g.,
"get_installation_token"), subject_ids (e.g., [installation_id]), decision
(e.g., "cache_hit" / "generated"), and latency_ms; capture start time before the
cache check and compute latency_ms on each terminal log path; also apply the
same structured logging pattern to the other boundary log sites referenced (the
calls around the session POST/response handling and in the methods that cover
ranges ~81-85 and ~163-185) so every entry emitted at these boundaries uses
logger.<level>(..., operation="get_installation_token",
subject_ids=[installation_id], decision="cache_hit" or "generated" or "error",
latency_ms=<ms>) consistently.

src/event_processors/rule_creation.py (1)

56-57: ⚠️ Potential issue | 🟠 Major

Reply-post failures are treated as success in the main result path.

If comment posting fails (Line 122/Line 125), process() still returns success=True on Line 71. This masks user-visible delivery failures and prevents proper remediation/retry signaling.

🛠️ Suggested fix

-            await self._post_result_to_comment(task, feasibility_result)
+            posted = await self._post_result_to_comment(task, feasibility_result)
...
-            return ProcessingResult(success=True, violations=[], api_calls_made=1, processing_time_ms=processing_time)
+            return ProcessingResult(
+                success=posted,
+                violations=[],
+                api_calls_made=1,
+                processing_time_ms=processing_time,
+                error=None if posted else "Failed to post feasibility reply",
+            )
...
-    async def _post_result_to_comment(self, task: Task, feasibility_result):
+    async def _post_result_to_comment(self, task: Task, feasibility_result) -> bool:
...
-                return
+                return False
...
             if result:
                 logger.info("successfully_posted_feasibility_reply_to_issuepr", issue_number=issue_number)
+                return True
             else:
                 logger.error("failed_to_post_feasibility_reply_to", issue_number=issue_number)
+                return False
...
         except Exception as e:
             logger.error("error_posting_feasibility_reply", e=e)
+            return False

As per coding guidelines: "Fail closed for risky decisions; provide actionable remediation in error paths".

Also applies to: 71-71, 120-126

🤖 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 56 - 57, The process()
flow currently treats failures from _post_result_to_comment as a success; modify
process() and its callers so that any exception or error returned by
_post_result_to_comment (and the internal reply/post paths) flips the overall
result to failure instead of returning success=True. Concretely, wrap the call
to self._post_result_to_comment(...) in a try/except (or check its returned
status) inside process(), log the error with context, and return or propagate a
Result object with success=False and an informative error message; ensure
similar handling is added where post-to-comment is invoked (e.g., in the
feasibility/result paths) so posting errors are not masked and will trigger
retry/remediation.

src/webhooks/handlers/issue_comment.py (2)

122-133: ⚠️ Potential issue | 🟠 Major

Validation result is posted with the wrong payload type.

_validate_rules_yaml returns a dict-like result, but create_pull_request_comment(..., comment=...) should receive the message string.

Proposed fix

-                    await github_client.create_pull_request_comment(
+                    await github_client.create_pull_request_comment(
                         repo=repo,
                         pr_number=pr_number,
-                        comment=validation_result,
+                        comment=validation_result.get("message", str(validation_result)),
                         installation_id=installation_id,
                     )

🤖 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 122 - 133, The
validation_result returned by _validate_rules_yaml is a dict-like object but
github_client.create_pull_request_comment expects a string; change the code that
calls create_pull_request_comment to pass a string (e.g.,
validation_result.get("message") or serialize the dict with
json.dumps(validation_result, indent=2)) instead of the raw dict; ensure you
handle the case where "message" is missing by falling back to a safe string
(like a joined errors list or the serialized dict) before calling
github_client.create_pull_request_comment.

---

153-169: ⚠️ Potential issue | 🟠 Major

Do not log raw user comment text or acknowledgment reasons.

These fields are user-supplied and can contain secrets/PII. Log safe metadata (length/hash) instead of raw content.

Safer logging example

-        logger.info("extracting_acknowledgment_reason_from", comment_body=comment_body)
+        logger.info("extracting_acknowledgment_reason_from", comment_length=len(comment_body))
...
-                logger.info("pattern_matched_reason", reason=reason)
+                logger.info("pattern_matched_reason", reason_length=len(reason))

As per coding guidelines "Reject secrets in logs, prompts, or test fixtures."

🤖 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 153 - 169, The code
currently logs raw user-supplied comment_body and extracted reason (logger.info
calls referencing comment_body and reason); remove those raw-value logs and
instead log only safe metadata: compute and log the comment length and a
non-reversible fingerprint (e.g., sha256 hex digest) for comment_body, and for
the extracted reason log its length and a hash/fingerprint rather than the text.
Update the logger.info calls around extracting_acknowledgment_reason_from, the
initial logger.info that references comment_body, and the
logger.info("pattern_matched_reason", ...) so they record only safe metadata
(length/hash) and not the raw strings; keep the matching logic and variables
(patterns, match, reason) unchanged.

src/agents/acknowledgment_agent/agent.py (1)

189-203: ⚠️ Potential issue | 🟠 Major

Add mandatory low-confidence gating before returning success.

The flow currently treats low-confidence model output as final. Add a threshold gate (< 0.5) to route to human review (or reject).

Proposed confidence gate

-            return AgentResult(
+            if structured_result.confidence < 0.5:
+                return AgentResult(
+                    success=False,
+                    message="Low-confidence acknowledgment evaluation; human review required",
+                    data={
+                        "is_valid": False,
+                        "confidence": structured_result.confidence,
+                        "recommendations": structured_result.recommendations,
+                    },
+                    metadata={"decision": "route_to_human"},
+                )
+
+            return AgentResult(
                 success=True,
                 message=f"Acknowledgment evaluation completed: {structured_result.reasoning}",
                 data={

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 189 - 203, The code
currently returns a successful AgentResult regardless of model confidence; add a
gating check using structured_result.confidence and if confidence < 0.5 log a
warning (via logger.warn or logger.warning) and return an AgentResult with
success=False (or a specific status indicating "human_review_required") and an
explanatory message (e.g., "Low confidence — route to human review") while still
including the original data payload (confidence, reasoning, is_valid,
acknowledgable_violations, require_fixes, recommendations); place this check
before the current AgentResult return that uses logger.info and the existing
data fields so low-confidence outputs are routed instead of treated as final.

src/webhooks/handlers/push.py (1)

20-25: ⚠️ Potential issue | 🟡 Minor

Capture X-GitHub-Delivery header to enable delivery-level idempotency.

GitHub sends X-GitHub-Delivery in webhook requests, but the router ignores it. WebhookEvent supports delivery_id, yet the router never extracts or passes it (line 38 of src/webhooks/router.py). Additionally, the deduplication infrastructure exists (_create_event_hash, event_hashes) but is explicitly disabled (event_hash=None at line 125 of src/tasks/task_queue.py).

Without capturing and propagating delivery_id, the system has no defense against duplicate webhook delivery from GitHub, risking duplicate task processing.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/webhooks/handlers/push.py` around lines 20 - 25, Extract the
X-GitHub-Delivery header from incoming requests and populate
WebhookEvent.delivery_id, then propagate that delivery_id into
task_queue.enqueue when creating push events (the handler in push.py calls
task_queue.enqueue). Also enable dedupe by computing/using the existing
_create_event_hash/event_hashes machinery instead of passing event_hash=None in
task_queue.enqueue (see task_queue.enqueue and _create_event_hash), e.g., derive
the event_hash from delivery_id (or delivery_id+payload) and pass it through so
the deduplication layer can detect repeated deliveries.

src/webhooks/router.py (1)

38-38: ⚠️ Potential issue | 🟠 Major

Propagate X-GitHub-Delivery into WebhookEvent.

Line 38 creates WebhookEvent without delivery_id; this loses request-level traceability for filtering/dispatch logs and incident correlation.

Proposed fix

-def _create_event_from_request(event_name: str | None, payload: dict) -> WebhookEvent:
+def _create_event_from_request(
+    event_name: str | None,
+    payload: dict[str, Any],
+    delivery_id: str | None,
+) -> WebhookEvent:
@@
-    return WebhookEvent(event_type=event_type, payload=payload)
+    return WebhookEvent(event_type=event_type, payload=payload, delivery_id=delivery_id)
@@
     payload = await request.json()
     event_name = request.headers.get("X-GitHub-Event")
+    delivery_id = request.headers.get("X-GitHub-Delivery")
@@
-        event = _create_event_from_request(event_name, payload)
+        event = _create_event_from_request(event_name, payload, delivery_id)

Also applies to: 58-63

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/webhooks/router.py` at line 38, The WebhookEvent returned at the end of
the webhook parsing functions is missing the request delivery ID; extract the
"X-GitHub-Delivery" header (e.g., via request.headers.get("X-GitHub-Delivery"))
and pass it into the WebhookEvent constructor as the delivery_id argument where
WebhookEvent(...) is created (the occurrence at the line returning
WebhookEvent(event_type=..., payload=...) and the other creation site around
lines 58-63); ensure both code paths set delivery_id so request-level
traceability is preserved.

src/webhooks/handlers/deployment_review.py (1)

20-24: ⚠️ Potential issue | 🟠 Major

Validate installation_id before enqueueing.

Line 23 passes event.installation_id directly; when missing, this fails downstream and turns a client/input problem into a server error path.

Proposed fix

     async def handle(self, event: WebhookEvent):
         """Handle deployment review events by enqueuing them for background processing."""
         logger.info("enqueuing_deployment_review_event_for", repo_full_name=event.repo_full_name)
 
+        if event.installation_id is None:
+            return {"status": "skipped", "reason": "missing installation_id in webhook payload"}
+
         task_id = await task_queue.enqueue(
             event_type="deployment_review",
             repo_full_name=event.repo_full_name,
             installation_id=event.installation_id,
             payload=event.payload,
         )

As per coding guidelines, "Validate all external inputs".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/webhooks/handlers/deployment_review.py` around lines 20 - 24, The handler
calls task_queue.enqueue with event.installation_id without checking it;
validate that installation_id is present and of expected type before calling
task_queue.enqueue (in the deployment_review handler where task_id is assigned),
and if missing/invalid return a 4xx error or reject the request early (e.g.,
raise/return a BadRequest) instead of enqueuing; update the code path around
task_queue.enqueue/event.installation_id to perform the check and use the
validated value when invoking task_queue.enqueue.

src/tasks/task_queue.py (1)

165-170: ⚠️ Potential issue | 🔴 Critical

Prevent duplicate execution of the same task across workers.

Line 165–170 selects a pending task without reserving it atomically, so two workers can process the same task concurrently.

Proposed fix

 class TaskQueue:
@@
     def __init__(self):
         self.tasks: dict[str, Task] = {}
         self.event_hashes: dict[str, str] = {}  # event_hash -> task_id
         self.running = False
         self.workers = []
+        self._task_lock = asyncio.Lock()
@@
-                # Find pending tasks
-                pending_tasks = [task for task in self.tasks.values() if task.status == TaskStatus.PENDING]
-
-                if pending_tasks:
-                    # Process the oldest task
-                    task = min(pending_tasks, key=lambda t: t.created_at)
-                    await self._process_task(task, worker_name)
+                task = None
+                async with self._task_lock:
+                    pending_tasks = [t for t in self.tasks.values() if t.status == TaskStatus.PENDING]
+                    if pending_tasks:
+                        task = min(pending_tasks, key=lambda t: t.created_at)
+                        task.status = TaskStatus.RUNNING
+                        task.started_at = datetime.now()
+
+                if task:
+                    await self._process_task(task, worker_name)
                 else:
                     # No tasks, wait a bit
                     await asyncio.sleep(1)
@@
     async def _process_task(self, task: Task, worker_name: str):
         """Process a single task."""
         try:
-            task.status = TaskStatus.RUNNING
-            task.started_at = datetime.now()
-
             logger.info("worker_processing_task", worker_name=worker_name, id=task.id)

Also applies to: 184-187

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tasks/task_queue.py` around lines 165 - 170, The code is selecting a
pending task then processing it without reserving it atomically, allowing
multiple workers to pick the same task; modify the selection to perform an
atomic reserve under the task-queue lock: iterate self.tasks while holding the
queue-level asyncio.Lock (or the existing lock used by this class), find the
oldest task with task.status == TaskStatus.PENDING, set task.status =
TaskStatus.RUNNING and record worker_name (e.g. task.worker = worker_name or
task.assigned_to) before releasing the lock, then call await
self._process_task(task, worker_name); apply the same atomic reserve pattern to
the other similar block referenced in the comment (the block at ~184-187) so no
two workers can concurrently start the same Task object.

src/event_processors/pull_request.py (1)

143-149: ⚠️ Potential issue | 🟠 Major

Acknowledgment matching uses the wrong key, so prior acknowledgments may never apply.

previous_acknowledgments is keyed by rule_id (from _get_previous_acknowledgments), but this check uses rule_description, which can cause acknowledged violations to be treated as unacknowledged.

✅ Suggested patch

             for violation in violations:
-                rule_description = violation.get("rule_description", "")
-                if rule_description in previous_acknowledgments:
+                rule_id = violation.get("rule_id", "")
+                if rule_id and rule_id in previous_acknowledgments:
                     # This violation was previously acknowledged
-                    logger.info("violation_previously_acknowledged", rule_description=rule_description)
+                    logger.info("violation_previously_acknowledged", rule_id=rule_id)
                     acknowledgable_violations.append(violation)
                 else:
                     # This violation requires acknowledgment
                     require_acknowledgment_violations.append(violation)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/event_processors/pull_request.py` around lines 143 - 149, The check for
prior acknowledgments is using rule_description but previous_acknowledgments is
keyed by rule_id; update the loop in the pull request event processor so it
reads the violation's "rule_id" (e.g., rule_id = violation.get("rule_id")) and
checks if that rule_id is in previous_acknowledgments instead of using
rule_description. Also update the logger call in the same block (logger.info) to
include the rule_id for clarity and leave the rest of the behavior (appending to
acknowledgable_violations) unchanged; touch the loop that iterates over
violations in the pull_request processor (the block referencing
previous_acknowledgments, rule_description, and acknowledgable_violations).

src/event_processors/push.py (1)

173-227: ⚠️ Potential issue | 🟠 Major

Check-run creation failures are swallowed, but processing still reports success.

If create_check_run fails, _create_check_run() logs and returns, and process() still returns successful processing. This hides a user-visible integration failure.

🛠️ Suggested fix

-    async def _create_check_run(self, task: Task, violations: list[dict[str, Any]]):
+    async def _create_check_run(self, task: Task, violations: list[dict[str, Any]]) -> bool:
@@
             if not sha or sha == NULL_SHA:
                 logger.warning(...)
-                return
+                return False
@@
             if result:
                 logger.info(...)
+                return True
             else:
                 logger.error(...)
+                return False
@@
         except Exception as e:
             logger.error(...)
+            return False

Then in process():

         if violations:
-            await self._create_check_run(task, violations)
+            check_run_ok = await self._create_check_run(task, violations)
             api_calls += 1
+            if not check_run_ok:
+                return ProcessingResult(
+                    success=False,
+                    violations=violations,
+                    api_calls_made=api_calls,
+                    processing_time_ms=int((time.time() - start_time) * 1000),
+                    error="Failed to create check run",
+                )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/event_processors/push.py` around lines 173 - 227, The _create_check_run
block currently swallows create_check_run failures
(github_client.create_check_run) and only logs them, causing process() to report
success; change the behavior so failures are surfaced: either raise the caught
exception from _create_check_run (or return a clear failure boolean) instead of
just logging, and update process() to check the _create_check_run result (or
catch the propagated exception) and mark processing as failed/return an error
accordingly; reference _create_check_run, process, and
github_client.create_check_run when making these changes and preserve existing
logging (logger.error/info) while ensuring failures influence the overall
process outcome.

src/event_processors/deployment_protection_rule.py (1)

177-203: ⚠️ Potential issue | 🟠 Major

Approval/rejection API failures are not propagated to process().

_approve_deployment and _reject_deployment only log on failure and return None, so process() can continue and report an outcome that was never applied upstream.

🛠️ Suggested approach

-    async def _approve_deployment(self, callback_url: str, environment: str, comment: str, installation_id: int):
+    async def _approve_deployment(
+        self, callback_url: str, environment: str, comment: str, installation_id: int
+    ) -> bool:
@@
             if result is None:
                 logger.error(...)
+                return False
             else:
                 logger.info(...)
+                return True
         except Exception as e:
             logger.error(...)
+            return False
@@
-    async def _reject_deployment(
+    async def _reject_deployment(
         self, callback_url: str, environment: str, violations: list[dict[str, Any]], installation_id: int
-    ):
+    ) -> bool:
@@
             if result is None:
                 logger.error(...)
+                return False
             else:
                 logger.info(...)
+                return True
         except Exception as e:
             logger.error(...)
+            return False

Then gate ProcessingResult in process() on these boolean outcomes.

Also applies to: 244-273, 274-307

🤖 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` around lines 177 - 203,
The approval/rejection calls (_approve_deployment and _reject_deployment)
swallow errors and return None, so process() continues as if the upstream action
succeeded; update _approve_deployment and _reject_deployment to return a boolean
(True on success, False on failure) or raise on error, then change process() to
check those return values and short-circuit or set ProcessingResult to a failure
when they indicate failure (e.g., after calling _approve_deployment(...,
installation_id) and _reject_deployment(..., installation_id) in process(),
abort further success logging and mark the processing as failed if the call
returned False or raised). Apply the same change pattern to the other
approval/rejection sites noted (the blocks around the other ranges) so the
deployment scheduler branch and final logger lines only run when the API call
succeeded.