From cc90e10b19ad15f087f7a4eb21d6961a16e831ba Mon Sep 17 00:00:00 2001 From: Pnant <73925474+Panniantong@users.noreply.github.com> Date: Thu, 11 Jun 2026 17:53:37 +0800 Subject: [PATCH] fix(review): four findings from adversarial Codex review round 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- agent_reach/channels/twitter.py | 29 +++++++++++++++++------------ agent_reach/cli.py | 7 +++++-- agent_reach/doctor.py | 7 +++++-- tests/test_cli.py | 23 +++++++++++++++++++++++ tests/test_doctor.py | 20 ++++++++++++++++++++ tests/test_twitter_channel.py | 30 ++++++++++++++++++++++++++++++ 6 files changed, 100 insertions(+), 16 deletions(-) diff --git a/agent_reach/channels/twitter.py b/agent_reach/channels/twitter.py index c1aba6d..8c8869b 100644 --- a/agent_reach/channels/twitter.py +++ b/agent_reach/channels/twitter.py @@ -17,9 +17,14 @@ class TwitterChannel(Channel): return "x.com" in d or "twitter.com" in d 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 - failures = [] + findings = [] for backend in self.ordered_backends(config): if backend == "twitter-cli": @@ -32,18 +37,18 @@ class TwitterChannel(Channel): continue if result is None: - continue # 未安装——继续尝试下一个后端 + continue # 未安装——不参与候选 + findings.append((backend, *result)) - status, message = result - if status in ("ok", "warn"): - # 工具本身是活的（含已装但未登录的 warn） - self.active_backend = backend - return status, message - # broken/timeout —— 记下处方，继续尝试下一个后端 - failures.append(message) + for wanted in ("ok", "warn"): + for backend, status, message in findings: + if status == wanted: + self.active_backend = backend + return status, message + + if findings: # 只剩 broken/timeout 候选 + return "error", "\n".join(m for _, _, m in findings) - if failures: - return "error", "\n".join(failures) return "warn", ( "Twitter CLI 未安装。安装方式：\n" " pipx install twitter-cli\n" diff --git a/agent_reach/cli.py b/agent_reach/cli.py index d69760f..e3826d3 100644 --- a/agent_reach/cli.py +++ b/agent_reach/cli.py @@ -1387,7 +1387,10 @@ def _cmd_uninstall(args): print(f"[dry-run] Would remove {platform_name} skill: {skill_path}") else: 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}") removed_any = True except Exception as e: @@ -1762,7 +1765,7 @@ def _cmd_watch(): if not err and resp and resp.status_code == 200: data = resp.json() latest = data.get("tag_name", "").lstrip("v") - if latest and latest != __version__: + if latest and _is_newer_version(latest, __version__): update_available = True new_version = latest release_body = data.get("body", "") diff --git a/agent_reach/doctor.py b/agent_reach/doctor.py index 0d56f24..6060f87 100644 --- a/agent_reach/doctor.py +++ b/agent_reach/doctor.py @@ -19,15 +19,18 @@ def check_all(config: Config) -> Dict[str, dict]: for ch in get_all_channels(): try: status, message = ch.check(config) + active = getattr(ch, "active_backend", None) 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] = { "status": status, "name": ch.description, "message": message, "tier": ch.tier, "backends": ch.backends, - "active_backend": getattr(ch, "active_backend", None), + "active_backend": active, } return results diff --git a/tests/test_cli.py b/tests/test_cli.py index 24ad993..cc420fb 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -199,3 +199,26 @@ class TestVersionCompare: 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("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 diff --git a/tests/test_doctor.py b/tests/test_doctor.py index 8ecdf57..7ce7013 100644 --- a/tests/test_doctor.py +++ b/tests/test_doctor.py @@ -104,3 +104,23 @@ class TestDoctor: assert "1/3 个渠道可用" in plain # Inactive optional channels should be summarized in one line 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 diff --git a/tests/test_twitter_channel.py b/tests/test_twitter_channel.py index 1f8c716..0670a21 100644 --- a/tests/test_twitter_channel.py +++ b/tests/test_twitter_channel.py @@ -152,3 +152,33 @@ def test_check_twitter_cli_broken_falls_back_to_bird(): assert status == "ok" assert "bird" in message 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