Skip to content
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 53 additions & 1 deletion server.py
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,27 @@ async def handle_notification(client: httpx.AsyncClient, note: Dict):
pass
return

# 检查通知类型 - 只处理 issue 和 pull_request 类型
# GitHub 通知的 subject.type 可以是: "Issue", "PullRequest", "Commit", "Discussion" 等
# 但 issue 关闭等事件也会触发通知,我们需要检查 URL 中是否包含事件信息
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

这里的注释有些过时和误导性。第一行说只处理 issuepull_request,但代码实际上还处理了 CommitDiscussion。第三行说要检查 URL,但代码检查的是 subject.type。建议更新注释以匹配代码逻辑,提高可维护性。

Suggested change
# 检查通知类型 - 只处理 issue 和 pull_request 类型
# GitHub 通知的 subject.type 可以是: "Issue", "PullRequest", "Commit", "Discussion" 等
# issue 关闭等事件也会触发通知,我们需要检查 URL 中是否包含事件信息
# 检查通知类型 - 只处理 "Issue", "PullRequest", "Commit", "Discussion"
# GitHub 通知的 subject.type 可以是多种类型,我们只处理这几种核心类型。
# 其它事件(如 issue 关闭)也可能触发 mention 通知,需要后续通过时间戳进一步过滤。

subject_type = note.get("subject", {}).get("type")
logger.info(f"Notification subject type: {subject_type}")

# 检查 URL 中是否包含事件类型(如 /issues/123 可能是 issue 关闭事件)
# 我们需要确保只处理真正包含 @mention 的内容,而不是所有相关事件
# 例如:issue 被关闭时,如果之前被 @mention 过,也会触发通知
# 但这种通知不应该触发 workflow,因为没有新的 @mention
if subject_type and subject_type not in ["Issue", "PullRequest", "Commit", "Discussion"]:
logger.info(f"Ignoring notification with unsupported subject type: {subject_type}")
try:
await client.patch(
f"{REST_API}/notifications/threads/{thread_id}",
headers={"Authorization": f"token {BOT_TOKEN}"}
)
except:
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

使用空的 except: 块是一个危险的实践,因为它会捕获并忽略所有类型的异常,包括系统退出信号(如 SystemExit, KeyboardInterrupt)和 asyncio.CancelledError,这可能导致程序行为异常且难以调试。建议至少捕获 Exception 并记录错误信息。

Suggested change
try:
await client.patch(
f"{REST_API}/notifications/threads/{thread_id}",
headers={"Authorization": f"token {BOT_TOKEN}"}
)
except:
pass
try:
await client.patch(
f"{REST_API}/notifications/threads/{thread_id}",
headers={"Authorization": f"token {BOT_TOKEN}"}
)
except Exception as e:
logger.warning(f"Failed to mark notification {thread_id} as read: {e}")

return

# 1. 获取资源详情
resource_data = await fetch_resource_details(client, raw_url)
if not resource_data:
Expand Down Expand Up @@ -882,6 +903,37 @@ async def handle_notification(client: httpx.AsyncClient, note: Dict):
pass
return

# 5. 验证触发节点的时间戳 - 防止处理旧事件
# 检查触发节点的创建时间是否在通知的更新时间之前(说明是旧的 @mention)
# 这可以防止 issue 关闭等事件触发 workflow(这些事件会发送通知,但没有新的 @mention)
notification_updated_at = note.get("updated_at")
if notification_updated_at and trigger_node.created_at:
try:
# 解析时间戳(ISO 8601 格式)
note_time = datetime.fromisoformat(notification_updated_at.replace("Z", "+00:00"))
trigger_time = datetime.fromisoformat(trigger_node.created_at.replace("Z", "+00:00"))

# 如果触发节点的创建时间比通知更新时间早很多(超过 1 分钟)
# 说明这不是新的 @mention,而是旧事件的后续通知(如 issue 关闭)
time_diff = (note_time - trigger_time).total_seconds()
if time_diff > 60: # 超过 1 分钟
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

代码中直接使用了魔法数字 60。为了提高代码的可读性和可维护性,建议将其定义为一个具名常量,例如 EVENT_TIME_THRESHOLD_SECONDS = 60,并在文件顶部或函数开头声明。这样,当未来需要调整这个阈值时,会更加清晰和安全。

logger.warning(f"Trigger node is too old (created {time_diff:.0f}s before notification update). This might be an event notification (e.g., issue closed) rather than a new @mention.")
logger.warning(f"Notification updated_at: {notification_updated_at}, Trigger created_at: {trigger_node.created_at}")
# 标记为已读但不触发 workflow
try:
await client.patch(
f"{REST_API}/notifications/threads/{thread_id}",
headers={"Authorization": f"token {BOT_TOKEN}"}
)
except:
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

与之前的代码块类似,这里也使用了空的 except: 块。这会隐藏所有潜在的错误,使得问题排查变得困难。建议捕获具体的 Exception 并记录日志,以便了解API调用失败的原因。

Suggested change
try:
await client.patch(
f"{REST_API}/notifications/threads/{thread_id}",
headers={"Authorization": f"token {BOT_TOKEN}"}
)
except:
pass
try:
await client.patch(
f"{REST_API}/notifications/threads/{thread_id}",
headers={"Authorization": f"token {BOT_TOKEN}"}
)
except Exception as e:
logger.warning(f"Failed to mark notification {thread_id} as read: {e}")

return
else:
logger.info(f"Trigger node time check passed (time diff: {time_diff:.0f}s)")
except Exception as e:
logger.warning(f"Could not parse timestamps for validation: {e}")
# 继续处理,但记录警告

# 验证触发消息是否存在
if not trigger_node.body or not trigger_node.body.strip():
logger.error(f"Trigger node {trigger_node.id} has empty body. Cannot proceed with workflow.")
Expand All @@ -899,7 +951,7 @@ async def handle_notification(client: httpx.AsyncClient, note: Dict):
pass
return

# 5. 构建完整上下文(使用修复后的build_rich_context)
# 6. 构建完整上下文(使用修复后的build_rich_context)
context = build_rich_context(resource_data, timeline_items, trigger_node, raw_url, thread_id)

# 6. 获取diff内容(根据触发类型决定)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

这里的注释编号 6. 与前面的步骤 6. 构建完整上下文 重复了。为了保持代码的清晰和一致性,建议将此处的编号更新为 7.。PR描述中提到修复了注释编号问题,但这里引入了新的重复。

Suggested change
# 6. 获取diff内容(根据触发类型决定)
# 7. 获取diff内容(根据触发类型决定)

Expand Down