diff --git a/coworker/server/manager.py b/coworker/server/manager.py index e09821ee..6390ecfc 100644 --- a/coworker/server/manager.py +++ b/coworker/server/manager.py @@ -1193,6 +1193,9 @@ class SessionManager: try: conn = await self.mcp.ensure(server) self._mcp_errors.pop(server.name, None) + # Recovery resets the notice dedupe: if this server breaks again + # later, the next session gets a fresh transcript notice. + self._clear_mcp_notified(server.name) except Exception as exc: if mcp_oauth.is_auth_required(exc): # Stored tokens no longer refresh (vendor rotated/expired @@ -1218,9 +1221,19 @@ class SessionManager: logger.warning( "mcp %s failed to connect: %s", server.name, msg[:500] ) - self._mcp_session_failures.setdefault(session_id, []).append( - server.name - ) + # Transcript notice on state CHANGE, not state (owner ruling + # 2026-08-21): a continuously-broken server stamps only the first + # session after it breaks (or breaks differently) — the Connectors + # page carries the standing error. Personas that DECLARE the server + # in their manifest keep the every-session notice: for them the + # missing tools are material every time (the 2026-08-20 drill case). + declared = persona_mcp is not None and server.name in persona_mcp + if declared or self._should_notify_mcp_failure( + server.name, self._mcp_errors.get(server.name, "") + ): + self._mcp_session_failures.setdefault(session_id, []).append( + server.name + ) continue callables = build_callables( server, @@ -1239,6 +1252,21 @@ class SessionManager: out.extend(callables) return out + def _should_notify_mcp_failure(self, name: str, error: str) -> bool: + """True once per failure episode: the first session after `name` starts + failing (or its error text changes) notices; unchanged-broken stays quiet. + Persisted in prefs so an app relaunch doesn't re-stamp the same complaint.""" + notified = self._prefs.setdefault("mcp_notified_errors", {}) + if notified.get(name) == error: + return False + notified[name] = error + self._save_prefs() + return True + + def _clear_mcp_notified(self, name: str) -> None: + if self._prefs.get("mcp_notified_errors", {}).pop(name, None) is not None: + self._save_prefs() + def pop_mcp_failures(self, session_id: str) -> list[tuple[str, Optional[str]]]: """Drain (name, error) for servers that failed while preparing this session's tools — consumed once by the WS handler to append a transcript notice.""" @@ -1337,6 +1365,7 @@ class SessionManager: # survive an app restart, so it lives in prefs, not memory. self._prefs.setdefault("mcp_last_test", {})[name] = int(time.time()) self._save_prefs() + self._clear_mcp_notified(name) return {"ok": True, "tools": len(conn.tools)} except Exception as exc: if ( @@ -1425,6 +1454,7 @@ class SessionManager: self._mcp_auth_hints.discard(name) if self._prefs.get("mcp_last_test", {}).pop(name, None) is not None: self._save_prefs() + self._clear_mcp_notified(name) # Removing a server must not leave its connection running until the next # restart, nor its OAuth tokens + DCR registration in the secret store — # "Remove" is the user saying this server is GONE (owner review 2026-08-21). diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 06f595f5..0148516a 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -496,3 +496,86 @@ def test_delete_mcp_shuts_down_connection_and_forgets_tokens(tmp_path): assert res["ok"] assert conn.shutdown.is_set() assert mgr.secrets.get(mcp_oauth.PROFILE_PREFIX + "gone-srv") is None + + +# -- notice dedupe: state CHANGE, not state (owner ruling 2026-08-21) ----------- + + +@pytest.mark.asyncio +async def test_session_notice_fires_once_per_failure_episode(tmp_path, monkeypatch): + """A continuously-broken server stamps only the FIRST session after it breaks; + a changed error re-notices; recovery then re-breakage re-notices. The + Connectors page carries the standing error in between.""" + from types import SimpleNamespace + + from coworker.server import SessionManager + + mgr = SessionManager(data_dir=tmp_path / "data") + server = SimpleNamespace( + name="flaky", transport="stdio", url=None, auth=None, enabled=True, include_tools=None, exclude_tools=None, requires_approval=True + ) + monkeypatch.setattr( + "coworker.server.manager.load_mcp_servers", lambda *a, **k: [server] + ) + + fail_with: list[str] = ["boom one"] + + async def ensure(s, **kw): + if fail_with: + raise RuntimeError(fail_with[0]) + return SimpleNamespace(tools=[]) + + monkeypatch.setattr(mgr.mcp, "ensure", ensure) + monkeypatch.setattr(mgr.mcp, "last_stderr", lambda n: None) + + await mgr.prepare_mcp_tools("s1", workspace=str(tmp_path)) + assert [n for n, _ in mgr.pop_mcp_failures("s1")] == ["flaky"] + + # Same error, next session: quiet (the Connectors page still shows it). + await mgr.prepare_mcp_tools("s2", workspace=str(tmp_path)) + assert mgr.pop_mcp_failures("s2") == [] + assert mgr._mcp_errors.get("flaky") # standing error intact + + # Different error: notice again. + fail_with[0] = "boom two" + await mgr.prepare_mcp_tools("s3", workspace=str(tmp_path)) + assert [n for n, _ in mgr.pop_mcp_failures("s3")] == ["flaky"] + + # Recovery clears the dedupe; the next breakage notices afresh. + fail_with.clear() + await mgr.prepare_mcp_tools("s4", workspace=str(tmp_path)) + assert mgr.pop_mcp_failures("s4") == [] + fail_with.append("boom three") + await mgr.prepare_mcp_tools("s5", workspace=str(tmp_path)) + assert [n for n, _ in mgr.pop_mcp_failures("s5")] == ["flaky"] + + +@pytest.mark.asyncio +async def test_notice_dedupe_survives_restart(tmp_path, monkeypatch): + """The dedupe is persisted: relaunching the app must not re-stamp the same + unchanged complaint into the first session (the owner-hit annoyance).""" + from types import SimpleNamespace + + server = SimpleNamespace( + name="flaky", transport="stdio", url=None, auth=None, enabled=True, include_tools=None, exclude_tools=None, requires_approval=True + ) + monkeypatch.setattr( + "coworker.server.manager.load_mcp_servers", lambda *a, **k: [server] + ) + + async def ensure(s, **kw): + raise RuntimeError("same boom") + + from coworker.server import SessionManager + + mgr = SessionManager(data_dir=tmp_path / "data") + monkeypatch.setattr(mgr.mcp, "ensure", ensure) + monkeypatch.setattr(mgr.mcp, "last_stderr", lambda n: None) + await mgr.prepare_mcp_tools("s1", workspace=str(tmp_path)) + assert [n for n, _ in mgr.pop_mcp_failures("s1")] == ["flaky"] + + reborn = SessionManager(data_dir=tmp_path / "data") + monkeypatch.setattr(reborn.mcp, "ensure", ensure) + monkeypatch.setattr(reborn.mcp, "last_stderr", lambda n: None) + await reborn.prepare_mcp_tools("s2", workspace=str(tmp_path)) + assert reborn.pop_mcp_failures("s2") == []