feat: 新增下载 cpp-algo 的功能#1171
Conversation
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并留下了一些高层次的反馈:
- 在
_replace_file_with_retry中,针对PermissionError的while True循环在没有任何退避(backoff)的情况下会一直自旋;建议添加一个小的延迟或最大重试次数,以避免在锁从未释放的情况下出现紧密的无限循环。 - 在
find_cpp_algo_binary中,all_hints集合会在_arch_rank中对每个候选项重新计算一次;你可以在嵌套函数外预先计算一次,或者进行缓存,这样在扫描大量文件时可以避免重复工作。 install_cpp_algo中对 DMG 的处理可以使用subprocess.run(..., check=True)加上有针对性的异常处理,而不是手动检查returncode,以简化挂载/卸载错误处理流程,并让失败更明确。
给 AI 代理的提示
请根据这次代码评审中的评论进行修改:
## 总体评论
- 在 `_replace_file_with_retry` 中,针对 `PermissionError` 的 `while True` 循环在没有任何退避(backoff)的情况下会一直自旋;建议添加一个小的延迟或最大重试次数,以避免在锁从未释放的情况下出现紧密的无限循环。
- 在 `find_cpp_algo_binary` 中,`all_hints` 集合会在 `_arch_rank` 中对每个候选项重新计算一次;你可以在嵌套函数外预先计算一次,或者进行缓存,这样在扫描大量文件时可以避免重复工作。
- `install_cpp_algo` 中对 DMG 的处理可以使用 `subprocess.run(..., check=True)` 加上有针对性的异常处理,而不是手动检查 `returncode`,以简化挂载/卸载错误处理流程,并让失败更明确。
## 单独评论
### 评论 1
<location path="tools/setup_workspace.py" line_range="784-793" />
<code_context>
+ return candidates[0]
+
+
+def _replace_file_with_retry(src_path: Path, target_path: Path) -> None:
+ tmp_target = target_path.with_name(f".{target_path.name}.tmp")
+ while True:
+ try:
+ tmp_target.unlink(missing_ok=True)
+ shutil.copy2(src_path, tmp_target)
+ os.replace(tmp_target, target_path)
+ return
+ except PermissionError as e:
+ tmp_target.unlink(missing_ok=True)
+ print(Console.err(t("err_permission_denied", error=e)))
+ cmd = input(t("prompt_retry_or_quit")).strip().lower()
+ if cmd == "q":
+ raise
+ except Exception:
</code_context>
<issue_to_address>
**issue (bug_risk):** 请考虑为权限错误设计一个非交互式或有边界的重试策略。
当前使用 `while True` + `input()` 的循环,在标准输入不是交互式时(例如 CI、输入被重定向的场景)可能会导致无限期挂起。同时也没有对重试次数进行上限限制。请检测是否为非交互模式并在该模式下快速失败而不是提示输入,或者添加最大重试次数,并在超出时以清晰的错误信息中止。
</issue_to_address>帮我变得更有用!请在每条评论上点击 👍 或 👎,我会根据反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
_replace_file_with_retry, thewhile Trueloop onPermissionErrorwill spin without any backoff; consider adding a small delay or a maximum retry count to avoid a tight infinite loop in scenarios where the lock never clears. - In
find_cpp_algo_binary, theall_hintsset is recomputed for every candidate in_arch_rank; you can precompute this once outside the nested function or cache it to avoid repeated work when many files are scanned. - The DMG handling in
install_cpp_algocould usesubprocess.run(..., check=True)plus targeted exception handling instead of manualreturncodechecks to simplify the attach/detach error paths and make failures more explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_replace_file_with_retry`, the `while True` loop on `PermissionError` will spin without any backoff; consider adding a small delay or a maximum retry count to avoid a tight infinite loop in scenarios where the lock never clears.
- In `find_cpp_algo_binary`, the `all_hints` set is recomputed for every candidate in `_arch_rank`; you can precompute this once outside the nested function or cache it to avoid repeated work when many files are scanned.
- The DMG handling in `install_cpp_algo` could use `subprocess.run(..., check=True)` plus targeted exception handling instead of manual `returncode` checks to simplify the attach/detach error paths and make failures more explicit.
## Individual Comments
### Comment 1
<location path="tools/setup_workspace.py" line_range="784-793" />
<code_context>
+ return candidates[0]
+
+
+def _replace_file_with_retry(src_path: Path, target_path: Path) -> None:
+ tmp_target = target_path.with_name(f".{target_path.name}.tmp")
+ while True:
+ try:
+ tmp_target.unlink(missing_ok=True)
+ shutil.copy2(src_path, tmp_target)
+ os.replace(tmp_target, target_path)
+ return
+ except PermissionError as e:
+ tmp_target.unlink(missing_ok=True)
+ print(Console.err(t("err_permission_denied", error=e)))
+ cmd = input(t("prompt_retry_or_quit")).strip().lower()
+ if cmd == "q":
+ raise
+ except Exception:
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider a non-interactive or bounded retry strategy for permission errors.
The current `while True` + `input()` loop can cause indefinite hangs when stdin isn’t interactive (e.g., CI, redirected input). There’s also no upper bound on retries. Please either detect non-interactive mode and fail fast instead of prompting, or add a maximum retry count and abort with a clear error once it’s exceeded.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def _replace_file_with_retry(src_path: Path, target_path: Path) -> None: | ||
| tmp_target = target_path.with_name(f".{target_path.name}.tmp") | ||
| while True: | ||
| try: | ||
| tmp_target.unlink(missing_ok=True) | ||
| shutil.copy2(src_path, tmp_target) | ||
| os.replace(tmp_target, target_path) | ||
| return | ||
| except PermissionError as e: | ||
| tmp_target.unlink(missing_ok=True) |
There was a problem hiding this comment.
issue (bug_risk): 请考虑为权限错误设计一个非交互式或有边界的重试策略。
当前使用 while True + input() 的循环,在标准输入不是交互式时(例如 CI、输入被重定向的场景)可能会导致无限期挂起。同时也没有对重试次数进行上限限制。请检测是否为非交互模式并在该模式下快速失败而不是提示输入,或者添加最大重试次数,并在超出时以清晰的错误信息中止。
Original comment in English
issue (bug_risk): Consider a non-interactive or bounded retry strategy for permission errors.
The current while True + input() loop can cause indefinite hangs when stdin isn’t interactive (e.g., CI, redirected input). There’s also no upper bound on retries. Please either detect non-interactive mode and fail fast instead of prompting, or add a maximum retry count and abort with a clear error once it’s exceeded.
Continues #1008
Summary by Sourcery
在工作区设置工具中,为 cpp-algo 代理二进制文件添加自动安装和更新支持。
新功能:
增强点:
Original summary in English
Summary by Sourcery
Add automated installation and update support for the cpp-algo agent binary in the workspace setup tool.
New Features:
Enhancements: