-
Notifications
You must be signed in to change notification settings - Fork 18
feat: 增加节点循环执行机制 --story=130003551 #585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/template_loop
Are you sure you want to change the base?
feat: 增加节点循环执行机制 --story=130003551 #585
Conversation
# Reviewed, transaction id: 71717
|
v_ghluguo seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔍 代码审查总结
本 PR 实现了节点循环执行机制。审查发现以下关键问题需要处理:
🚨 严重问题
- 索引越界风险:loop.py:39 在分割字符串时未做边界检查,可能导致 IndexError
- 除零风险:validator.py:51 在空列表上调用 min() 会抛出 ValueError
⚠️ 逻辑问题
- 异常处理宽泛:decorators.py:48 捕获所有异常但仅记录日志,可能掩盖严重错误
- 状态判断逻辑:operations.py:496 的状态判断仅检查 _result 字段,可能不够健壮
✨ 建议
- 建议添加单元测试覆盖边界情况(空列表、索引越界等)
- 考虑为新增的 loop_config 字段添加 schema 验证
审查了 8 个变更文件,标注了 4 个高优先级问题
| def get_value(self): | ||
| # 循环节点因引用 | ||
| if hasattr(self, "inner_loop") and self.inner_loop != -1: | ||
| return self.value.split(",")[self.inner_loop - 1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 索引越界风险:当 inner_loop 超过数组长度时会抛出 IndexError。建议添加边界检查:
values = self.value.split(",")
if self.inner_loop > len(values):
raise ValueError(f"inner_loop {self.inner_loop} exceeds values length {len(values)}")
return values[self.inner_loop - 1]| ) | ||
| record_kwargs = {**kwargs, "func_result": result} | ||
| recorder.record(*args, **record_kwargs) | ||
| except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if not loop_config.get("enable", False): | ||
| continue | ||
| loop_params = loop_config.get("loop_params", []) | ||
| if loop_config["loop_times"] != min([len(param.split(",")) for key, param in loop_params.items()]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 空列表异常:当 loop_params 为空字典时,min() 会抛出 ValueError。建议先检查是否为空:
if not loop_params:
raise exceptions.ParserWebTreeException("loop_params is empty")
lengths = [len(param.split(",")) for key, param in loop_params.items()]
if loop_config["loop_times"] != min(lengths):
raise exceptions.ParserWebTreeException("loop times not matched")|
|
||
| # 重试记录必然是因为失败才重试,设置了循环策略的节点只有成功才能接着循环 | ||
| if node_info.loop_strategy: | ||
| if hist["skip"] or hist["outputs"].get("_result"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if hist["skip"] or (hist["outputs"].get("_result") is True):
Reviewed, transaction id: 71717