fix(review): four findings from adversarial Codex review round 1

- twitter: align backend selection with the two-phase findings pattern —
  an unauthenticated twitter-cli (warn) no longer blocks a fully-working
  OpenCLI (ok) further down the candidate list
- watch: use _is_newer_version like check-update (the != comparison kept
  the downgrade prompt this branch fixed elsewhere)
- uninstall: third copy of the skill-dir removal also gets the symlink
  guard (full `agent-reach uninstall` path)
- doctor: a stale active_backend from a previous check on the singleton
  channel no longer leaks into an errored result

+4 regression tests (162 total).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Pnant
2026-06-11 17:53:37 +08:00
parent fc24fb699b
commit cc90e10b19
6 changed files with 100 additions and 16 deletions
+17 -12
View File
@@ -17,9 +17,14 @@ class TwitterChannel(Channel):
return "x.com" in d or "twitter.com" in d return "x.com" in d or "twitter.com" in d
def check(self, config=None): def check(self, config=None):
"""按 backends 顺序真实探测,第一个活着的后端即为 active_backend。""" """Probe candidates in order; first fully-usable backend wins.
与其他多后端渠道同一套两段式:先收集全部候选状态,第一个 ok 获胜;
没有 ok 才轮到第一个 warn——否则「装了但未登录」的 twitter-cli
会把排在后面、完整可用的 OpenCLI 挡在门外。
"""
self.active_backend = None self.active_backend = None
failures = [] findings = []
for backend in self.ordered_backends(config): for backend in self.ordered_backends(config):
if backend == "twitter-cli": if backend == "twitter-cli":
@@ -32,18 +37,18 @@ class TwitterChannel(Channel):
continue continue
if result is None: if result is None:
continue # 未安装——继续尝试下一个后端 continue # 未安装——不参与候选
findings.append((backend, *result))
status, message = result for wanted in ("ok", "warn"):
if status in ("ok", "warn"): for backend, status, message in findings:
# 工具本身是活的(含已装但未登录的 warn) if status == wanted:
self.active_backend = backend self.active_backend = backend
return status, message return status, message
# broken/timeout —— 记下处方,继续尝试下一个后端
failures.append(message) if findings: # 只剩 broken/timeout 候选
return "error", "\n".join(m for _, _, m in findings)
if failures:
return "error", "\n".join(failures)
return "warn", ( return "warn", (
"Twitter CLI 未安装。安装方式:\n" "Twitter CLI 未安装。安装方式:\n"
" pipx install twitter-cli\n" " pipx install twitter-cli\n"
+5 -2
View File
@@ -1387,7 +1387,10 @@ def _cmd_uninstall(args):
print(f"[dry-run] Would remove {platform_name} skill: {skill_path}") print(f"[dry-run] Would remove {platform_name} skill: {skill_path}")
else: else:
try: try:
shutil.rmtree(skill_path) if os.path.islink(skill_path):
os.unlink(skill_path)
else:
shutil.rmtree(skill_path)
print(f" Removed {platform_name} skill: {skill_path}") print(f" Removed {platform_name} skill: {skill_path}")
removed_any = True removed_any = True
except Exception as e: except Exception as e:
@@ -1762,7 +1765,7 @@ def _cmd_watch():
if not err and resp and resp.status_code == 200: if not err and resp and resp.status_code == 200:
data = resp.json() data = resp.json()
latest = data.get("tag_name", "").lstrip("v") latest = data.get("tag_name", "").lstrip("v")
if latest and latest != __version__: if latest and _is_newer_version(latest, __version__):
update_available = True update_available = True
new_version = latest new_version = latest
release_body = data.get("body", "") release_body = data.get("body", "")
+5 -2
View File
@@ -19,15 +19,18 @@ def check_all(config: Config) -> Dict[str, dict]:
for ch in get_all_channels(): for ch in get_all_channels():
try: try:
status, message = ch.check(config) status, message = ch.check(config)
active = getattr(ch, "active_backend", None)
except Exception as e: # noqa: BLE001 — doctor must survive any channel except Exception as e: # noqa: BLE001 — doctor must survive any channel
status, message = "error", f"体检异常:{e}" # Channels are registry singletons: a stale active_backend from a
# previous check must not leak into an errored result.
status, message, active = "error", f"体检异常:{e}", None
results[ch.name] = { results[ch.name] = {
"status": status, "status": status,
"name": ch.description, "name": ch.description,
"message": message, "message": message,
"tier": ch.tier, "tier": ch.tier,
"backends": ch.backends, "backends": ch.backends,
"active_backend": getattr(ch, "active_backend", None), "active_backend": active,
} }
return results return results
+23
View File
@@ -199,3 +199,26 @@ class TestVersionCompare:
def test_unparseable_falls_back_to_inequality(self): def test_unparseable_falls_back_to_inequality(self):
assert cli._is_newer_version("2026.06-beta", "1.5.0") is True assert cli._is_newer_version("2026.06-beta", "1.5.0") is True
assert cli._is_newer_version("1.5.0", "1.5.0-dev") is True assert cli._is_newer_version("1.5.0", "1.5.0-dev") is True
class TestWatchVersionCompare:
def test_watch_does_not_prompt_downgrade(self, monkeypatch, capsys):
"""watch 与 check-update 同语义:本地领先远端 release 时不提示更新。"""
class R:
status_code = 200
headers = {}
@staticmethod
def json():
return {"tag_name": "v1.4.2", "body": ""}
monkeypatch.setattr(cli, "_github_get_with_retry", lambda *a, **k: (R(), None, 1))
monkeypatch.setattr(
"agent_reach.doctor.check_all",
lambda config: {"web": {"status": "ok", "name": "任意网页", "message": "ok",
"tier": 0, "backends": ["Jina Reader"], "active_backend": "Jina Reader"}},
)
cli._cmd_watch()
out = capsys.readouterr().out
assert "新版本可用" not in out
assert "全部正常" in out
+20
View File
@@ -104,3 +104,23 @@ class TestDoctor:
assert "1/3 个渠道可用" in plain assert "1/3 个渠道可用" in plain
# Inactive optional channels should be summarized in one line # Inactive optional channels should be summarized in one line
assert "可选渠道可以解锁" in plain assert "可选渠道可以解锁" in plain
def test_stale_active_backend_does_not_leak_into_errored_result(monkeypatch):
"""渠道单例上一轮的 active_backend 不得泄漏进本轮异常结果(Codex review 发现)。"""
from agent_reach import doctor
class _ExplodingChannel:
name = "boom"
description = "爆炸渠道"
tier = 0
backends = ["a", "b"]
active_backend = "a" # 上一轮成功的残留
def check(self, config=None):
raise RuntimeError("boom")
monkeypatch.setattr(doctor, "get_all_channels", lambda: [_ExplodingChannel()])
results = doctor.check_all(config=None)
assert results["boom"]["status"] == "error"
assert results["boom"]["active_backend"] is None
+30
View File
@@ -152,3 +152,33 @@ def test_check_twitter_cli_broken_falls_back_to_bird():
assert status == "ok" assert status == "ok"
assert "bird" in message assert "bird" in message
assert channel.active_backend == "bird CLI (legacy)" assert channel.active_backend == "bird CLI (legacy)"
def test_unauthenticated_twitter_cli_does_not_block_working_opencli():
"""warn 候选不得屏蔽排在后面的 ok 候选(Codex review 发现)。"""
channel = TwitterChannel()
with patch.object(
TwitterChannel, "_check_twitter_cli",
return_value=("warn", "twitter-cli 已安装但未认证"),
), patch.object(
TwitterChannel, "_check_opencli",
return_value=("ok", "OpenCLI 可用(复用浏览器登录态)"),
), patch.object(TwitterChannel, "_check_bird", return_value=None):
status, msg = channel.check()
assert status == "ok"
assert channel.active_backend == "OpenCLI"
def test_all_warn_falls_back_to_first_warn():
channel = TwitterChannel()
with patch.object(
TwitterChannel, "_check_twitter_cli",
return_value=("warn", "twitter-cli 未认证"),
), patch.object(
TwitterChannel, "_check_opencli",
return_value=("warn", "扩展未连接"),
), patch.object(TwitterChannel, "_check_bird", return_value=None):
status, msg = channel.check()
assert status == "warn"
assert channel.active_backend == "twitter-cli"
assert "未认证" in msg