Fix some bugs, update backend api#17
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the daemon-stopping logic in ultron/cli/watcher.py to support custom pgrep patterns via an optional extra_patterns parameter. The review feedback recommends using Optional[List[str]] instead of string-quoted union types for consistency, deduplicating patterns to avoid redundant pgrep executions, and adding -- to the pgrep command to prevent potential option injection vulnerabilities.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Ultron CLI to support a two-step OSS upload process, binary file handling via raw bytes, and incremental commits. It also introduces a new list command, refactors remote repository resolution with the --repo argument, and improves path traversal protection. The review feedback highlights three key improvements: distinguishing between empty and deleted files in push_incremental by using None instead of empty bytes, resolving a cross-platform crash on Windows by avoiding direct usage of signal.SIGKILL in stop_daemon, and ensuring accurate file counts in cmd_list by using collect_bytes() instead of collect().
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def push_incremental( | ||
| client: "UltronClient", | ||
| username: str, | ||
| name: str, | ||
| changed: Dict[str, bytes], | ||
| remote_paths: set, | ||
| ) -> None: | ||
| """Incremental push via commit interface. | ||
|
|
||
| Builds create/update/delete actions and commits in one request. | ||
| Raises on failure (caller should NOT update baseline on exception). | ||
| """ | ||
| actions: List[dict] = [] | ||
| for fpath, content in changed.items(): | ||
| if not content: # empty bytes = delete | ||
| actions.append({"action": "delete", "file_path": fpath}) | ||
| else: | ||
| action_type = "update" if fpath in remote_paths else "create" |
There was a problem hiding this comment.
To support distinguishing between empty files (b"") and deleted files (represented as None), update push_incremental to check if content is None for deletions.
| def push_incremental( | |
| client: "UltronClient", | |
| username: str, | |
| name: str, | |
| changed: Dict[str, bytes], | |
| remote_paths: set, | |
| ) -> None: | |
| """Incremental push via commit interface. | |
| Builds create/update/delete actions and commits in one request. | |
| Raises on failure (caller should NOT update baseline on exception). | |
| """ | |
| actions: List[dict] = [] | |
| for fpath, content in changed.items(): | |
| if not content: # empty bytes = delete | |
| actions.append({"action": "delete", "file_path": fpath}) | |
| else: | |
| action_type = "update" if fpath in remote_paths else "create" | |
| def push_incremental( | |
| client: "UltronClient", | |
| username: str, | |
| name: str, | |
| changed: Dict[str, Union[bytes, None]], | |
| remote_paths: set, | |
| ) -> None: | |
| """Incremental push via commit interface. | |
| Builds create/update/delete actions and commits in one request. | |
| Raises on failure (caller should NOT update baseline on exception). | |
| """ | |
| actions: List[dict] = [] | |
| for fpath, content in changed.items(): | |
| if content is None: # None = delete | |
| actions.append({"action": "delete", "file_path": fpath}) | |
| else: | |
| action_type = "update" if fpath in remote_paths else "create" |
| if tracked_pid: | ||
| try: | ||
| os.kill(tracked_pid, 0) # Check if still alive. | ||
| os.kill(tracked_pid, signal.SIGKILL) | ||
| except (ProcessLookupError, PermissionError, OSError): | ||
| pass |
There was a problem hiding this comment.
On Windows, the signal module does not have a SIGKILL attribute. Attempting to access signal.SIGKILL will raise an AttributeError, causing stop_daemon to crash if the tracked process does not exit within 1 second.
To ensure cross-platform compatibility, use getattr(signal, "SIGKILL", signal.SIGTERM) or check if SIGKILL is available on the signal module.
| if tracked_pid: | |
| try: | |
| os.kill(tracked_pid, 0) # Check if still alive. | |
| os.kill(tracked_pid, signal.SIGKILL) | |
| except (ProcessLookupError, PermissionError, OSError): | |
| pass | |
| if tracked_pid: | |
| try: | |
| os.kill(tracked_pid, 0) # Check if still alive. | |
| os.kill(tracked_pid, getattr(signal, "SIGKILL", signal.SIGTERM)) | |
| except (ProcessLookupError, PermissionError, OSError, AttributeError): | |
| pass |
| tmp = _build_allowlist(framework, GLOBAL_AGENT_NAME, getattr(args, 'local_dir', None)) | ||
| else: | ||
| tmp = _build_allowlist(framework, a, getattr(args, 'local_dir', None)) | ||
| count = len(tmp.collect()) |
There was a problem hiding this comment.
The cmd_list command uses tmp.collect() to count the files for each sub-agent. However, collect() only gathers text files and ignores binary files (skipping on UnicodeDecodeError). To ensure the file count is accurate and consistent with cmd_upload (which uses collect_bytes()), use tmp.collect_bytes() instead.
| count = len(tmp.collect()) | |
| count = len(tmp.collect_bytes()) |
PR type
PR information
增量 commit 同步 — watch 不再每次全量 zip 上传,改为
create/update/delete细粒度 commit,带宽占用降低 90%+,支持二进制文件(base64 fallback)Windows 100% 优雅退出 — 引入
watch.stop文件轮询机制替代 Unix 信号,stop 命令跨平台统一行为:写停止文件 → 等待退出 → 超时强杀,Windows 下不再触发TerminateProcess硬杀--name/--repo参数解耦 —--name定位本地 sub-agent,--repo指定远端仓库(支持group/repo格式),语义不再混淆--name自动推断 — upload/watch 省略--name时自动发现:单 agent 直接选中,多 agent 报错提示,无 agent 走 global 模式新增
ultron list命令 — 列出当前 framework 可发现的所有 sub-agent 及其文件数量文件列表分页 —
list_repo_files接口支持翻页(每页 100,上限 5000 文件),解决大仓库文件截断问题collect_bytes()二进制安全 — allowlist 新增 bytes 采集路径,watch/upload 全链路 bytes 透传,彻底修复图片等二进制资源同步时的编码损坏全局模式 (
GLOBAL_AGENT_NAME) — 新增哨兵常量区分"共享文件"与"子 agent 专属文件",allowlist pattern 中含{name}的在全局模式下自动排除watch 日志增强 — 每次同步明确输出每个文件的操作类型(CREATE/DELETE/UPDATE)+ 路径 + 大小,排查问题不再猜
watch_loop信号响应从 120s → <5s — 用threading.Event.wait+ 5s 轮询替代time.sleep(120),修复 PEP 475 导致的 SIGTERM 无法中断 sleep 问题stop 命令孤儿进程清理 — Unix 用
pgrep -f+--选项终止符,Windows 用wmic process扫描 command line,确保无残留file-per-agent框架 watch 安全约束 — 共享文件布局的 framework 禁止 watch 指定单个 sub-agent(避免并发冲突),仅允许 global/default 模式空文件 vs 删除语义修正 —
detect_local_changes返回值从""改为None表示删除,不再误判空文件为删除操作测试覆盖 — 新增/增强 upload dry-run、download 格式转换、watch 增量同步、allowlist 全局模式等测试,总计 633 tests 全绿
影响范围:
cli/全模块 +allowlist.py,+963 / -260 行,13 files changedTest results
Paste your test result here (if needed).