Skip to content

.2059806565729300:5a47531e9b15134c7667e7e10eda67f2_69e3499ab13256d09ece8088.69e4b19cb13256d09ece844d.69e4b19c7afba9383018b93c:Trae CN.T(2026/4/19 18:42:36)#1276

Closed
Wpc-121 wants to merge 4 commits intoActivityWatch:masterfrom
Wpc-121:bug_rep_06
Closed

.2059806565729300:5a47531e9b15134c7667e7e10eda67f2_69e3499ab13256d09ece8088.69e4b19cb13256d09ece844d.69e4b19c7afba9383018b93c:Trae CN.T(2026/4/19 18:42:36)#1276
Wpc-121 wants to merge 4 commits intoActivityWatch:masterfrom
Wpc-121:bug_rep_06

Conversation

@Wpc-121
Copy link
Copy Markdown
Contributor

@Wpc-121 Wpc-121 commented Apr 19, 2026

添加构建前的环境检查和严格模式验证

在构建流程中添加 doctor 检查步骤,确保环境依赖正确

新增严格模式验证脚本,检查打包内容和版本一致性

优化构建日志输出,增加详细步骤和错误处理

Wpc-121 added 4 commits April 19, 2026 00:11
重构集成测试脚本,增加健康检查轮询机制替代固定等待时间
添加详细的测试帮助文档和环境变量配置说明
改进日志记录和错误检测功能
添加全局和单测超时控制,改进错误分类和诊断信息
添加 pytest-timeout 依赖,支持进程树清理
优化日志输出和错误处理流程
添加 GitHub Actions 集成测试工作流,支持通过输入参数跳过构建步骤直接下载二进制文件
设置全局测试超时和分步超时控制
支持多平台(ubuntu/macos)测试环境
在构建流程中添加 doctor 检查步骤,确保环境依赖正确
新增严格模式验证脚本,检查打包内容和版本一致性
优化构建日志输出,增加详细步骤和错误处理
import json
import signal
import atexit
import sys


GLOBAL_SERVER_PROCESS: Optional['ServerProcess'] = None
GLOBAL_TEST_TIMEOUT: int = 120
for child in children:
try:
child.kill()
except psutil.NoSuchProcess:

try:
parent.kill()
except psutil.NoSuchProcess:
for p in still_alive:
try:
p.kill()
except psutil.NoSuchProcess:
p.kill()
except psutil.NoSuchProcess:
pass
except psutil.NoSuchProcess:
else:
try:
os.killpg(os.getpgid(pid), signal.SIGKILL)
except ProcessLookupError:
except OSError:
try:
os.kill(pid, signal.SIGKILL)
except ProcessLookupError:

start_time = time.time()
poll_count = 0
last_exit_check = 0
if path and os.path.exists(path):
try:
os.remove(path)
except Exception:
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 19, 2026

Greptile Summary

This PR adds environment pre-checks (make doctor), strict packaging verification (PACKAGE_STRICT=true + verify-package.sh), a new integration test module replacing the old single-line pytest call, and a new integration CI job with workflow_dispatch inputs.

  • P1 (verify-package.sh): set -euo pipefail at the top of the script conflicts with helper functions (dir_exists, check_critical_executables) that return 1 on failure and are called without guards in main(). Bash exits before the summary is printed and before the script can reach its documented exit-code paths (0 = pass, 2 = strict failure), breaking the stated error-reporting contract.

Confidence Score: 3/5

Merge blocked by a correctness bug in verify-package.sh that silently bypasses the intended exit-code contract in strict CI mode.

One P1 bug in verify-package.shset -euo pipefail causes premature exits that suppress the verification summary and produce wrong exit codes, undermining the strict-mode CI gate this PR is specifically adding. The remaining findings are P2 style/reliability issues.

scripts/package/verify-package.sh (P1 logic bug); scripts/tests/integration_tests.py (false-positive risk in error detection).

Important Files Changed

Filename Overview
scripts/package/verify-package.sh New verification script with a P1 bug: set -euo pipefail combined with unguarded helper calls that return 1 can exit prematurely before printing the summary or reaching the intended exit-code logic.
scripts/tests/integration_tests.py New integration test module replacing the old single-line pytest call; well-structured with health polling and diagnostics, but GLOBAL_TEST_TIMEOUT is defined and unused, and error-indicator matching is overly broad.
.github/workflows/test.yml Adds workflow_dispatch inputs and a new integration CI job; uses a floating dtolnay/rust-toolchain@master reference which reduces reproducibility.
.github/workflows/build.yml Adds make doctor pre-build health check and enables PACKAGE_STRICT=true for CI packaging; changes are straightforward and low-risk.
Makefile Substantially expands package, test-integration, and adds package-pre-check and test-integration-help targets with detailed logging; logic is sound though verbose.

Reviews (1): Last reviewed commit: "ci(build): 添加构建前的环境检查和严格模式验证" | Re-trigger Greptile

# 2: Verification failed (only with --strict)
#

set -euo pipefail
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 set -euo pipefail causes premature exit before summary is printed

set -euo pipefail at line 28 means any function that returns non-zero exits the script immediately. Several callers in main() invoke check_critical_executables, check_critical_directories, etc. without wrapping them in if or || true. If check_critical_executables hits its early-return path (line 208–210, when dist/activitywatch is missing), Bash exits with code 1 before the summary block or the intended exit-code logic (exit 2 for strict mode) is ever reached. The documented exit-code contract—"0 when not --strict" and "2 on failure with --strict"—is silently violated.

Fix by either removing set -e (and checking return values manually), or guard every top-level check call:

check_critical_executables || true
check_critical_directories || true
check_artifacts             || true
check_version_consistency   || true



GLOBAL_SERVER_PROCESS: Optional['ServerProcess'] = None
GLOBAL_TEST_TIMEOUT: int = 120
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 GLOBAL_TEST_TIMEOUT is defined but never read

GLOBAL_TEST_TIMEOUT = 120 is set as a module-level constant but no code path reads it; the actual timeout is sourced from ServerConfig.test_timeout via the AW_TEST_TIMEOUT environment variable. The unused variable is misleading—it suggests a global timeout is being enforced when it isn't.

return False

def check_for_errors(self) -> Tuple[bool, List[str]]:
error_indicators = ["ERROR", "panic", "thread '", "stack backtrace", "fatal", "Error:"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Overly broad error indicators may produce false positives

check_for_errors scans logs for "thread '", "Error:", and "fatal" as error signals. Rust binaries routinely emit thread 'main' at startup in normal output, and the server may legitimately log URLs containing Error. These indicators could cause test_no_error_indicators_in_logs to fail spuriously on valid server output. Consider anchoring patterns to known bad prefixes (e.g., ERROR: for Python logging, RUST_BACKTRACE style panics) or filtering out known-good lines.

Comment on lines +246 to +247
uses: dtolnay/rust-toolchain@master
with:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Floating @master reference for dtolnay/rust-toolchain

uses: dtolnay/rust-toolchain@master resolves to whatever the action's master branch points to at run time. A breaking update to that action would silently fail all integration jobs that build from source. Pin to a commit SHA or a stable tag to ensure reproducibility.

@BelKed
Copy link
Copy Markdown
Contributor

BelKed commented Apr 19, 2026

@Wpc-121 Please stop spamming PRs with weird names!

@TimeToBuildBob
Copy link
Copy Markdown
Contributor

PR titles need to use plain English that describes the change (e.g. "feat: add Windows artifact verification"). The current title is an internal session ID and is not acceptable.

This PR and #1277-#1279 will be closed unless retitled. See the warning on #1267.

@ErikBjare ErikBjare closed this Apr 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants